This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4275

manifest: set appropriate cache entry when clearing _dirlogcache()
ClosedPublic

Authored by indygreg on Aug 13 2018, 12:55 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG1347bcf52d51: manifest: set appropriate cache entry when clearing _dirlogcache()
Summary

manifestrevlog applies to any manifest, not just the root manifest.
Resetting the cache and populating its root entry with self is not
correct when the instance does not refer to the root manifest.

This has no test fallout. So I'm guessing we only ever call
clearcaches() on the root manifest. Or we have no test coverage that
are impacted by a bad cache on a non-root manifestrevlog.

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

indygreg created this revision.Aug 13 2018, 12:55 PM
Closed by commit rHG1347bcf52d51: manifest: set appropriate cache entry when clearing _dirlogcache() (authored by indygreg). · Explain WhyAug 22 2018, 12:37 PM
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.Aug 28 2018, 2:13 PM
martinvonz added inline comments.
mercurial/manifest.py
1328

All the manifestrevlogs share a single cache that is stored on the root manifest (see comment and code around line 1288), so this change should have no effect. And yes, clearcaches() will only be called on the root manifest.

The code here was more consistent (with line 1292) before this patch, but it doesn't bother me enough that I'll send a backout patch :)

Revision Contents
Changeset List

PathPackages
Mmercurial/manifest.py (2 lines)

Diff 10521

mercurial/manifest.py

@property @property
def fulltextcache(self): def fulltextcache(self):
return self._fulltextcache return self._fulltextcache
def clearcaches(self, clear_persisted_data=False): def clearcaches(self, clear_persisted_data=False):
super(manifestrevlog, self).clearcaches() super(manifestrevlog, self).clearcaches()
self._fulltextcache.clear(clear_persisted_data=clear_persisted_data) self._fulltextcache.clear(clear_persisted_data=clear_persisted_data)
self._dirlogcache = {'': self} self._dirlogcache = {self._dir: self}
martinvonzUnsubmitted
Not Done

All the manifestrevlogs share a single cache that is stored on the root manifest (see comment and code around line 1288), so this change should have no effect. And yes, clearcaches() will only be called on the root manifest.

The code here was more consistent (with line 1292) before this patch, but it doesn't bother me enough that I'll send a backout patch :)

martinvonz: All the manifestrevlogs share a single cache that is stored on the root manifest (see comment…
def dirlog(self, d): def dirlog(self, d):
if d: if d:
assert self._treeondisk assert self._treeondisk
if d not in self._dirlogcache: if d not in self._dirlogcache:
mfrevlog = manifestrevlog(self.opener, d, mfrevlog = manifestrevlog(self.opener, d,
self._dirlogcache, self._dirlogcache,
treemanifest=self._treeondisk) treemanifest=self._treeondisk)