This is an archive of the discontinued Mercurial Phabricator instance.

Differential D648

blackbox: fix rotation with chg
ClosedPublic

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

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
quark edited the summary of this revision. (Show Details)Sep 7 2017, 1:39 AM
quark edited the summary of this revision. (Show Details)
durham added a subscriber: durham.Sep 7 2017, 8:14 PM

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.

quark added a comment.EditedSep 7 2017, 8:35 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.

durham accepted this revision.Sep 11 2017, 12:23 PM
yuja accepted this revision.Sep 12 2017, 9:56 AM
yuja added a subscriber: yuja.

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

PathPackages
Mhgext/blackbox.py (43 lines)
Mtests/test-blackbox.t (54 lines)

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