This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4850

store: pass matcher to store.datafiles()
ClosedPublic

Authored by pulkit on Oct 3 2018, 11:02 AM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG2d45b549392f: store: pass matcher to store.datafiles()
Summary

To get narrow stream clones working, we need a way to filter the storage files
using a matcher. This patch adds matcher as an argument to store.walk() and
store.datafiles() so that we can filter the files returned according to the
matcher.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Oct 3 2018, 11:02 AM
martinvonz added a subscriber: martinvonz.Oct 7 2018, 5:48 PM
martinvonz added inline comments.
mercurial/store.py
414–420

This doesn't seem right to me. Let's say the matcher is rootfilesin:some/dir, then matcher('some/dir/foo') will be True, but matcher('some') (the first-level directory) will not be. That seems to mean that the client will not get all the directories it needs.

Maybe this code needs to be made less generic and start walking the directories like other tree-walking algorithms we have do. In repos that use tree manifests for all their commits, we should be able to walk the directories in .hg/store/meta and look for files in .hg/store/data only for directories found in that walk. However, that only works if all commits use treemanifests. I think it's good enough for now (and maybe forever) to instead pass all the file names into a util.dirs object and then walk those directories using matcher.visitdir(). For each directory found that way, we would look for the manifest revlog in .hg/store/meta and include it if it's found (and ignore it if it's not).

martinvonz requested changes to this revision.Oct 7 2018, 5:48 PM
This revision now requires changes to proceed.Oct 7 2018, 5:48 PM
indygreg added a subscriber: indygreg.Oct 13 2018, 4:00 AM

Another conceptual problem with this is that it assumes data/ and meta/ are used for tracking just filelogs and manifestlogs. In theory, other revlogs / data files could be stored there.

For files / data/ paths, I think we're OK making this assumption. But for manifests / meta/, I would feel better if we built up a set of tree manifest directories and then intersected that with files in meta/ that map to their revlogs.

pulkit added a comment.Oct 14 2018, 9:21 AM
In D4850#75708, @indygreg wrote:

Another conceptual problem with this is that it assumes data/ and meta/ are used for tracking just filelogs and manifestlogs. In theory, other revlogs / data files could be stored there.
For files / data/ paths, I think we're OK making this assumption. But for manifests / meta/, I would feel better if we built up a set of tree manifest directories and then intersected that with files in meta/ that map to their revlogs.

I discussed this with martinvonz on Friday and we decided to use matcher.visitdir() for the meta/ ones.

pulkit retitled this revision from store: pass matcher to store.datafiles() and filter files according to it to store: pass matcher to store.datafiles().Oct 17 2018, 10:53 AM
pulkit added a child revision: D5138: narrow: only send includepats and excludepats if they are not empty.
pulkit updated this revision to Diff 12210.
Closed by commit rHG2d45b549392f: store: pass matcher to store.datafiles() (authored by pulkit). · Explain WhyOct 17 2018, 7:25 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/store.py (16 lines)

Diff 12210

mercurial/store.py

if filefilter(f, kind, st): if filefilter(f, kind, st):
n = util.pconvert(fp[striplen:]) n = util.pconvert(fp[striplen:])
l.append((decodedir(n), n, st.st_size)) l.append((decodedir(n), n, st.st_size))
elif kind == stat.S_IFDIR and recurse: elif kind == stat.S_IFDIR and recurse:
visit.append(fp) visit.append(fp)
l.sort() l.sort()
return l return l
def datafiles(self): def datafiles(self, matcher=None):
return self._walk('data', True) + self._walk('meta', True) return self._walk('data', True) + self._walk('meta', True)
def topfiles(self): def topfiles(self):
# yield manifest before changelog # yield manifest before changelog
return reversed(self._walk('', False)) return reversed(self._walk('', False))
def walk(self): def walk(self, matcher=None):
'''yields (unencoded, encoded, size)''' '''yields (unencoded, encoded, size)
if a matcher is passed, storage files of only those tracked paths
are passed with matches the matcher
'''
# yield data files first # yield data files first
for x in self.datafiles(): for x in self.datafiles(matcher):
yield x yield x
for x in self.topfiles(): for x in self.topfiles():
yield x yield x
def copylist(self): def copylist(self):
return ['requires'] + _data.split() return ['requires'] + _data.split()
def write(self, tr): def write(self, tr):
vfs = vfstype(path + '/store') vfs = vfstype(path + '/store')
self.path = vfs.base self.path = vfs.base
self.createmode = _calcmode(vfs) self.createmode = _calcmode(vfs)
vfs.createmode = self.createmode vfs.createmode = self.createmode
self.rawvfs = vfs self.rawvfs = vfs
self.vfs = vfsmod.filtervfs(vfs, encodefilename) self.vfs = vfsmod.filtervfs(vfs, encodefilename)
self.opener = self.vfs self.opener = self.vfs
def datafiles(self): def datafiles(self, matcher=None):
for a, b, size in super(encodedstore, self).datafiles(): for a, b, size in super(encodedstore, self).datafiles():
try: try:
a = decodefilename(a) a = decodefilename(a)
except KeyError: except KeyError:
a = None a = None
yield a, b, size yield a, b, size
martinvonzUnsubmitted
Not Done

This doesn't seem right to me. Let's say the matcher is rootfilesin:some/dir, then matcher('some/dir/foo') will be True, but matcher('some') (the first-level directory) will not be. That seems to mean that the client will not get all the directories it needs.

Maybe this code needs to be made less generic and start walking the directories like other tree-walking algorithms we have do. In repos that use tree manifests for all their commits, we should be able to walk the directories in .hg/store/meta and look for files in .hg/store/data only for directories found in that walk. However, that only works if all commits use treemanifests. I think it's good enough for now (and maybe forever) to instead pass all the file names into a util.dirs object and then walk those directories using matcher.visitdir(). For each directory found that way, we would look for the manifest revlog in .hg/store/meta and include it if it's found (and ignore it if it's not).

martinvonz: This doesn't seem right to me. Let's say the `matcher` is `rootfilesin:some/dir`, then `matcher…
def join(self, f): def join(self, f):
return self.path + '/' + encodefilename(f) return self.path + '/' + encodefilename(f)
def copylist(self): def copylist(self):
return (['requires', '00changelog.i'] + return (['requires', '00changelog.i'] +
['store/' + f for f in _data.split()]) ['store/' + f for f in _data.split()])
self.opener = self.vfs self.opener = self.vfs
def join(self, f): def join(self, f):
return self.pathsep + self.encode(f) return self.pathsep + self.encode(f)
def getsize(self, path): def getsize(self, path):
return self.rawvfs.stat(path).st_size return self.rawvfs.stat(path).st_size
def datafiles(self): def datafiles(self, matcher=None):
for f in sorted(self.fncache): for f in sorted(self.fncache):
ef = self.encode(f) ef = self.encode(f)
try: try:
yield f, ef, self.getsize(ef) yield f, ef, self.getsize(ef)
except OSError as err: except OSError as err:
if err.errno != errno.ENOENT: if err.errno != errno.ENOENT:
raise raise