This is an archive of the discontinued Mercurial Phabricator instance.

blackbox: fix rotation with chg
ClosedPublic

Authored by quark on Sep 7 2017, 1:32 AM.

Download Raw Diff

Details

Reviewers

durham
yuja

Group Reviewers

hg-reviewers

Commits

rHG4f60720cf0df: blackbox: fix rotation with chg

Summary

The added test will show:

$ $PYTHON showsize.py .hg/blackbox*
.hg/blackbox.log: < 500
.hg/blackbox.log.1: < 500
.hg/blackbox.log.2: < 500
.hg/blackbox.log.3: < 500
.hg/blackbox.log.4: < 500
.hg/blackbox.log.5: >= 500

with previous code.

The issue is caused by blackbox caching file objects *by path*, and the
rotation size check could run on a wrong file object (i.e. it should check
"blackbox.log", but filehandles["blackbox.log"] contains a file object
that has been renamed to "blackbox.log.5").

This patch removes the "filehandlers" global cache added by 45313f5a3a8c to
solve the issue.

Diff Detail

Repository

rHG Mercurial

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Sep 7 2017, 1:32 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptSep 7 2017, 1:32 AM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

quark added a child revision: D649: blackbox: inline _bbwrite.Sep 7 2017, 1:32 AM

quark edited the summary of this revision. (Show Details)Sep 7 2017, 1:39 AM

quark edited the summary of this revision. (Show Details)

This seems like mostly a revert of the original commit. timeless might have to chime in here, since it's not clear exactly what issue the original patch was fixing.

I think the original patch was trying to make different ui objects use a same file object if their blackbox.log path is the same. In theory it could also be problematic in the rotation case. Anyway, that should become unnecessary after D650.

durham accepted this revision.Sep 11 2017, 12:23 PM

I think the original patch was trying to make different ui objects use a same file object if their blackbox.log path is the same. In theory it could also be problematic in the rotation case. Anyway, that should become unnecessary after D650.

Copied this to the commit message and queued the first 5 patches, thanks.

This revision is now accepted and ready to land.Sep 12 2017, 9:56 AM

Closed by commit rHG4f60720cf0df: blackbox: fix rotation with chg (authored by quark). · Explain WhySep 12 2017, 10:13 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext/blackbox.py (43 lines)
M			tests/test-blackbox.t (54 lines)

Status	Author	Revision
Closed	quark	D655 blackbox: set lastui even if ui.log is not called (issue5518)
Closed	quark	D654 blackbox: unindent a try block
Closed	quark	D652 blackbox: simplify ui states
Closed	quark	D651 blackbox: remove _bbvfs state
Closed	quark	D650 blackbox: do not cache file objects
Closed	quark	D649 blackbox: inline _bbwrite
Closed	quark	D648 blackbox: fix rotation with chg
Closed	quark	D647 test-blackbox: make it compatible with chg

Diff 1750

hgext/blackbox.py

	default='1 MB',			default='1 MB',
	)			)
	configitem('blackbox', 'logsource',			configitem('blackbox', 'logsource',
	default=False,			default=False,
	)			)

	lastui = None			lastui = None

	filehandles = {}

	def _openlog(vfs):
	path = vfs.join('blackbox.log')
	if path in filehandles:
	return filehandles[path]
	filehandles[path] = fp = vfs('blackbox.log', 'a')
	return fp

	def _closelog(vfs):
	path = vfs.join('blackbox.log')
	fp = filehandles[path]
	del filehandles[path]
	fp.close()

	def wrapui(ui):			def wrapui(ui):
	class blackboxui(ui.__class__):			class blackboxui(ui.__class__):
	def __init__(self, src=None):			def __init__(self, src=None):
	super(blackboxui, self).__init__(src)			super(blackboxui, self).__init__(src)
	if src is None:			if src is None:
	self._partialinit()			self._partialinit()
	else:			else:
	self._bbfp = getattr(src, '_bbfp', None)			self._bbfp = getattr(src, '_bbfp', None)
	try:			try:
	if newpath:			if newpath:
	self._bbvfs.rename(oldpath, newpath)			self._bbvfs.rename(oldpath, newpath)
	except OSError as err:			except OSError as err:
	if err.errno != errno.ENOENT:			if err.errno != errno.ENOENT:
	self.debug("warning: cannot rename '%s' to '%s': %s\n" %			self.debug("warning: cannot rename '%s' to '%s': %s\n" %
	(newpath, oldpath, err.strerror))			(newpath, oldpath, err.strerror))

	fp = _openlog(self._bbvfs)
	maxsize = self.configbytes('blackbox', 'maxsize')			maxsize = self.configbytes('blackbox', 'maxsize')
				name = 'blackbox.log'
	if maxsize > 0:			if maxsize > 0:
	st = self._bbvfs.fstat(fp)			try:
				st = self._bbvfs.stat(name)
				except OSError:
				pass
				else:
	if st.st_size >= maxsize:			if st.st_size >= maxsize:
	path = fp.name			path = self._bbvfs.join(name)
	_closelog(self._bbvfs)
	maxfiles = self.configint('blackbox', 'maxfiles', 7)			maxfiles = self.configint('blackbox', 'maxfiles', 7)
	for i in xrange(maxfiles - 1, 1, -1):			for i in xrange(maxfiles - 1, 1, -1):
	rotate(oldpath='%s.%d' % (path, i - 1),			rotate(oldpath='%s.%d' % (path, i - 1),
	newpath='%s.%d' % (path, i))			newpath='%s.%d' % (path, i))
	rotate(oldpath=path,			rotate(oldpath=path,
	newpath=maxfiles > 0 and path + '.1')			newpath=maxfiles > 0 and path + '.1')
	fp = _openlog(self._bbvfs)			return self._bbvfs(name, 'a')
	return fp

	def _bbwrite(self, fmt, *args):			def _bbwrite(self, fmt, *args):
	self._bbfp.write(fmt % args)			self._bbfp.write(fmt % args)
	self._bbfp.flush()			self._bbfp.flush()

	def log(self, event, msg, *opts):			def log(self, event, msg, *opts):
	global lastui			global lastui
	super(blackboxui, self).log(event, msg, *opts)			super(blackboxui, self).log(event, msg, *opts)

tests/test-blackbox.t

	> def reposetup(ui, repo):			> def reposetup(ui, repo):
	> extensions.wrapfunction(context.basectx, 'status', status)			> extensions.wrapfunction(context.basectx, 'status', status)
	> EOF			> EOF
	$ hg id --config extensions.x=../r.py --config blackbox.dirty=True			$ hg id --config extensions.x=../r.py --config blackbox.dirty=True
	45589e459b2e tip			45589e459b2e tip

	cleanup			cleanup
	$ cd ..			$ cd ..

				#if chg

				when using chg, blackbox.log should get rotated correctly

				$ cat > $TESTTMP/noop.py << EOF
				> from __future__ import absolute_import
				> import time
				> from mercurial import registrar, scmutil
				> cmdtable = {}
				> command = registrar.command(cmdtable)
				> @command('noop')
				> def noop(ui, repo):
				> pass
				> EOF

				$ hg init blackbox-chg
				$ cd blackbox-chg

				$ cat > .hg/hgrc << EOF
				> [blackbox]
				> maxsize = 500B
				> [extensions]
				> # extension change forces chg to restart
				> noop=$TESTTMP/noop.py
				> EOF

				$ $PYTHON -c 'print("a" * 400)' > .hg/blackbox.log
				$ chg noop
				$ chg noop
				$ chg noop
				$ chg noop
				$ chg noop

				$ cat > showsize.py << 'EOF'
				> import os, sys
				> limit = 500
				> for p in sys.argv[1:]:
				> size = os.stat(p).st_size
				> if size >= limit:
				> desc = '>='
				> else:
				> desc = '<'
				> print('%s: %s %d' % (p, desc, limit))
				> EOF

				$ $PYTHON showsize.py .hg/blackbox*
				.hg/blackbox.log: < 500
				.hg/blackbox.log.1: >= 500
				.hg/blackbox.log.2: >= 500

				$ cd ..

				#endif

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	1651		Sep 7 2017, 1:32 AM	★	★
Diff 2	1750	rHG4f60720cf0df68dc455cba1a11c8fc9711222811	Sep 6 2017, 10:27 PM	★	★