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.

Details

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
This revision was automatically updated to reflect the committed changes.