( )⚙ D5814 match: teach diffmatcher.visitdir() to return 'all' if possible

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5814

match: teach diffmatcher.visitdir() to return 'all' if possible
ClosedPublic

Authored by pulkit on Feb 4 2019, 10:14 AM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHGb7a0efb3c370: match: teach diffmatcher.visitdir() to return 'all' if possible
rHG990aa150fd02: match: teach diffmatcher.visitdir() to return 'all' if possible
Summary

This patch teaches differencematcher.visitdir() to return 'all' when
m1.visitdir() returns 'all' and m2 does not matches.

Before this patch, from a differencematcher.visitdir(), we always returned
either True or False. We never returned 'all' even when we can. This causes
problem when m1 and m2 of a differencematcher are themselves differencematcher.
In that case, we try to check:

if self._m2_.visitdir(dir) == 'all'

which will never be 'all' even though it can be.

This leads to iterating over a lot of sub-directory manifest, even though we
don't want to while extending a narrow clone. I am yet to measure the impact of
this but calculating manifest was taking ~50-60 seconds, so this should
definitely save some of time there.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Feb 4 2019, 10:14 AM
pulkit updated this revision to Diff 13740.Feb 4 2019, 10:16 AM
martinvonz accepted this revision.Feb 4 2019, 11:27 AM
martinvonz added a subscriber: martinvonz.

Thanks for fixing! I don't know why I didn't think of writing it this way from the beginning :P

This revision is now accepted and ready to land.Feb 4 2019, 11:27 AM
Closed by commit rHG990aa150fd02: match: teach diffmatcher.visitdir() to return 'all' if possible (authored by pulkit). · Explain WhyFeb 4 2019, 11:36 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/match.py (3 lines)
Mtests/test-match.py (23 lines)

Diff 13739

mercurial/match.py

# m1 is "path:dir" and m2 is "rootfileins:.", we don't # m1 is "path:dir" and m2 is "rootfileins:.", we don't
# want to remove "dir" from the set even though it would match m2, # want to remove "dir" from the set even though it would match m2,
# because the "dir" in m1 may not be a file. # because the "dir" in m1 may not be a file.
return self._m1.files() return self._m1.files()
def visitdir(self, dir): def visitdir(self, dir):
if self._m2.visitdir(dir) == 'all': if self._m2.visitdir(dir) == 'all':
return False return False
elif not self._m2.visitdir(dir):
# m2 does not match dir, we can return 'all' here if possible
return self._m1.visitdir(dir)
return bool(self._m1.visitdir(dir)) return bool(self._m1.visitdir(dir))
def visitchildrenset(self, dir): def visitchildrenset(self, dir):
m2_set = self._m2.visitchildrenset(dir) m2_set = self._m2.visitchildrenset(dir)
if m2_set == 'all': if m2_set == 'all':
return set() return set()
m1_set = self._m1.visitchildrenset(dir) m1_set = self._m1.visitchildrenset(dir)
# Possible values for m1: 'all', 'this', set(...), set() # Possible values for m1: 'all', 'this', set(...), set()

tests/test-match.py

self.assertEqual(dm.visitchildrenset(b'dir/foo'), set()) self.assertEqual(dm.visitchildrenset(b'dir/foo'), set())
self.assertEqual(dm.visitchildrenset(b'dir/subdir/x'), set()) self.assertEqual(dm.visitchildrenset(b'dir/subdir/x'), set())
self.assertEqual(dm.visitchildrenset(b'folder'), set()) self.assertEqual(dm.visitchildrenset(b'folder'), set())
def testVisitdirM2never(self): def testVisitdirM2never(self):
m1 = matchmod.alwaysmatcher(b'', b'') m1 = matchmod.alwaysmatcher(b'', b'')
m2 = matchmod.nevermatcher(b'', b'') m2 = matchmod.nevermatcher(b'', b'')
dm = matchmod.differencematcher(m1, m2) dm = matchmod.differencematcher(m1, m2)
# dm should be equivalent to a alwaysmatcher. OPT: if m2 is a # dm should be equivalent to a alwaysmatcher.
# nevermatcher, we could return 'all' for these.
# #
# We're testing Equal-to-True instead of just 'assertTrue' since # We're testing Equal-to-True instead of just 'assertTrue' since
# assertTrue does NOT verify that it's a bool, just that it's truthy. # assertTrue does NOT verify that it's a bool, just that it's truthy.
# While we may want to eventually make these return 'all', they should # While we may want to eventually make these return 'all', they should
# not currently do so. # not currently do so.
self.assertEqual(dm.visitdir(b'.'), True) self.assertEqual(dm.visitdir(b'.'), 'all')
self.assertEqual(dm.visitdir(b'dir'), True) self.assertEqual(dm.visitdir(b'dir'), 'all')
self.assertEqual(dm.visitdir(b'dir/subdir'), True) self.assertEqual(dm.visitdir(b'dir/subdir'), 'all')
self.assertEqual(dm.visitdir(b'dir/subdir/z'), True) self.assertEqual(dm.visitdir(b'dir/subdir/z'), 'all')
self.assertEqual(dm.visitdir(b'dir/foo'), True) self.assertEqual(dm.visitdir(b'dir/foo'), 'all')
self.assertEqual(dm.visitdir(b'dir/subdir/x'), True) self.assertEqual(dm.visitdir(b'dir/subdir/x'), 'all')
self.assertEqual(dm.visitdir(b'folder'), True) self.assertEqual(dm.visitdir(b'folder'), 'all')
def testVisitchildrensetM2never(self): def testVisitchildrensetM2never(self):
m1 = matchmod.alwaysmatcher(b'', b'') m1 = matchmod.alwaysmatcher(b'', b'')
m2 = matchmod.nevermatcher(b'', b'') m2 = matchmod.nevermatcher(b'', b'')
dm = matchmod.differencematcher(m1, m2) dm = matchmod.differencematcher(m1, m2)
# dm should be equivalent to a alwaysmatcher. # dm should be equivalent to a alwaysmatcher.
self.assertEqual(dm.visitchildrenset(b'.'), b'all') self.assertEqual(dm.visitchildrenset(b'.'), b'all')
self.assertEqual(dm.visitchildrenset(b'dir'), b'all') self.assertEqual(dm.visitchildrenset(b'dir'), b'all')
self.assertEqual(dm.visitdir(b'dir'), True) self.assertEqual(dm.visitdir(b'dir'), True)
self.assertFalse(dm.visitdir(b'dir/subdir')) self.assertFalse(dm.visitdir(b'dir/subdir'))
# OPT: We should probably return False for these; we don't because # OPT: We should probably return False for these; we don't because
# patternmatcher.visitdir() (our m2) doesn't return 'all' for subdirs of # patternmatcher.visitdir() (our m2) doesn't return 'all' for subdirs of
# an 'all' pattern, just True. # an 'all' pattern, just True.
self.assertEqual(dm.visitdir(b'dir/subdir/z'), True) self.assertEqual(dm.visitdir(b'dir/subdir/z'), True)
self.assertEqual(dm.visitdir(b'dir/subdir/x'), True) self.assertEqual(dm.visitdir(b'dir/subdir/x'), True)
# OPT: We could return 'all' for these. # OPT: We could return 'all' for these.
self.assertEqual(dm.visitdir(b'dir/foo'), True) self.assertEqual(dm.visitdir(b'dir/foo'), 'all')
self.assertEqual(dm.visitdir(b'folder'), True) self.assertEqual(dm.visitdir(b'folder'), 'all')
def testVisitchildrensetM2SubdirPrefix(self): def testVisitchildrensetM2SubdirPrefix(self):
m1 = matchmod.alwaysmatcher(b'', b'') m1 = matchmod.alwaysmatcher(b'', b'')
m2 = matchmod.match(b'', b'', patterns=[b'path:dir/subdir']) m2 = matchmod.match(b'', b'', patterns=[b'path:dir/subdir'])
dm = matchmod.differencematcher(m1, m2) dm = matchmod.differencematcher(m1, m2)
self.assertEqual(dm.visitchildrenset(b'.'), b'this') self.assertEqual(dm.visitchildrenset(b'.'), b'this')
self.assertEqual(dm.visitchildrenset(b'dir'), b'this') self.assertEqual(dm.visitchildrenset(b'dir'), b'this')
self.assertEqual(dm.visitchildrenset(b'dir/subdir'), set()) self.assertEqual(dm.visitchildrenset(b'dir/subdir'), set())
self.assertEqual(dm.visitchildrenset(b'dir/foo'), b'all') self.assertEqual(dm.visitchildrenset(b'dir/foo'), b'all')
self.assertEqual(dm.visitchildrenset(b'folder'), b'all') self.assertEqual(dm.visitchildrenset(b'folder'), b'all')
# OPT: We should probably return set() for these; we don't because # OPT: We should probably return set() for these; we don't because
# patternmatcher.visitdir() (our m2) doesn't return 'all' for subdirs of # patternmatcher.visitdir() (our m2) doesn't return 'all' for subdirs of
# an 'all' pattern, just 'this'. # an 'all' pattern, just 'this'.
self.assertEqual(dm.visitchildrenset(b'dir/subdir/z'), b'this') self.assertEqual(dm.visitchildrenset(b'dir/subdir/z'), b'this')
self.assertEqual(dm.visitchildrenset(b'dir/subdir/x'), b'this') self.assertEqual(dm.visitchildrenset(b'dir/subdir/x'), b'this')
# We're using includematcher instead of patterns because it behaves slightly # We're using includematcher instead of patterns because it behaves slightly
# better (giving narrower results) than patternmatcher. # better (giving narrower results) than patternmatcher.
def testVisitdirIncludeIncludfe(self): def testVisitdirIncludeIncludfe(self):
m1 = matchmod.match(b'', b'', include=[b'path:dir/subdir']) m1 = matchmod.match(b'', b'', include=[b'path:dir/subdir'])
m2 = matchmod.match(b'', b'', include=[b'rootfilesin:dir']) m2 = matchmod.match(b'', b'', include=[b'rootfilesin:dir'])
dm = matchmod.differencematcher(m1, m2) dm = matchmod.differencematcher(m1, m2)
self.assertEqual(dm.visitdir(b'.'), True) self.assertEqual(dm.visitdir(b'.'), True)
self.assertEqual(dm.visitdir(b'dir'), True) self.assertEqual(dm.visitdir(b'dir'), True)
self.assertEqual(dm.visitdir(b'dir/subdir'), True) self.assertEqual(dm.visitdir(b'dir/subdir'), 'all')
self.assertFalse(dm.visitdir(b'dir/foo')) self.assertFalse(dm.visitdir(b'dir/foo'))
self.assertFalse(dm.visitdir(b'folder')) self.assertFalse(dm.visitdir(b'folder'))
# OPT: We should probably return False for these; we don't because # OPT: We should probably return False for these; we don't because
# patternmatcher.visitdir() (our m2) doesn't return 'all' for subdirs of # patternmatcher.visitdir() (our m2) doesn't return 'all' for subdirs of
# an 'all' pattern, just True. # an 'all' pattern, just True.
self.assertEqual(dm.visitdir(b'dir/subdir/z'), True) self.assertEqual(dm.visitdir(b'dir/subdir/z'), True)
self.assertEqual(dm.visitdir(b'dir/subdir/x'), True) self.assertEqual(dm.visitdir(b'dir/subdir/x'), True)