Patchwork [2,of,4] match: express anypats(), not prefix(), in terms of the others

login
register
mail settings
Submitter via Mercurial-devel
Date July 10, 2017, 5:27 p.m.
Message ID <25d89317826e99474cde.1499707662@martinvonz.svl.corp.google.com>
Download mbox | patch
Permalink /patch/22208/
State Accepted
Headers show

Comments

via Mercurial-devel - July 10, 2017, 5:27 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1499644929 25200
#      Sun Jul 09 17:02:09 2017 -0700
# Node ID 25d89317826e99474cde4b97899903191ef1ba27
# Parent  0ce91c7d311a808b253c3b4d406e137168c95130
match: express anypats(), not prefix(), in terms of the others

When I added prefix() in 9789b4a7c595 (match: introduce boolean
prefix() method, 2014-10-28), we already had always(), isexact(), and
anypats(), so it made sense to write it in terms of them (a prefix
matcher is one that isn't any of the other types). It's only now that
I realize that it's much more natural to define prefix() explicitly
(it's one that uses path: patterns, roughly speaking) and let
anypats() be defined in terms of the others. Remember that these
methods are all used for determining which fast paths are
possible. anypats() simply means that no fast paths are possible (it
could be called complex() instead). Further evidence is that
rootfilesin:some/dir does not have any patterns, but it's still
considered to be an anypats() matcher. That's because anypats() really
just means that it's not a prefix() matcher (and not always() and not
isexact()).

This patch thus changes prefix() to return False by default and
anypats() to return True only if the other three are False. Having
anypats() be True by default also seems like a good thing, because it
means forgetting to override it will lead only to performance bugs,
not correctness bugs.

Since the base class's implementation changes, we're also forced to
update the subclasses. That change exposed and fixed a bug in the
differencematcher: for example when both its two input matchers were
prefix matchers, we would say that the result was also a prefix
matcher, which is incorrect, because e.g "path:dir - path:dir/foo" no
longer matches everything under "dir" (which is what prefix() means).

Patch

diff --git a/mercurial/match.py b/mercurial/match.py
--- a/mercurial/match.py
+++ b/mercurial/match.py
@@ -307,20 +307,25 @@ 
         '''
         return False
 
-    def anypats(self):
-        '''Matcher uses patterns or include/exclude.'''
-        return False
-
     def always(self):
-        '''Matcher will match everything and .files() will be empty
-        - optimization might be possible and necessary.'''
+        '''Matcher will match everything and .files() will be empty --
+        optimization might be possible.'''
         return False
 
     def isexact(self):
+        '''Matcher will match exactly the list of files in .files() --
+        optimization might be possible.'''
         return False
 
     def prefix(self):
-        return not self.always() and not self.isexact() and not self.anypats()
+        '''Matcher will match the paths in .files() recursively --
+        optimization might be possible.'''
+        return False
+
+    def anypats(self):
+        '''None of .always(), .isexact(), and .prefix() is true --
+        optimizations will be difficult.'''
+        return not self.always() and not self.isexact() and not self.prefix()
 
 class alwaysmatcher(basematcher):
     '''Matches everything.'''
@@ -385,8 +390,8 @@ 
                 any(parentdir in self._fileset
                     for parentdir in util.finddirs(dir)))
 
-    def anypats(self):
-        return self._anypats
+    def prefix(self):
+        return not self._anypats
 
     def __repr__(self):
         return ('<patternmatcher patterns=%r>' % self._pats)
@@ -416,9 +421,6 @@ 
                 any(parentdir in self._roots
                     for parentdir in util.finddirs(dir)))
 
-    def anypats(self):
-        return True
-
     def __repr__(self):
         return ('<includematcher includes=%r>' % self._pats)
 
@@ -497,9 +499,6 @@ 
     def isexact(self):
         return self._m1.isexact()
 
-    def anypats(self):
-        return self._m1.anypats() or self._m2.anypats()
-
     def __repr__(self):
         return ('<differencematcher m1=%r, m2=%r>' % (self._m1, self._m2))
 
@@ -566,9 +565,6 @@ 
     def isexact(self):
         return self._m1.isexact() or self._m2.isexact()
 
-    def anypats(self):
-        return self._m1.anypats() or self._m2.anypats()
-
     def __repr__(self):
         return ('<intersectionmatcher m1=%r, m2=%r>' % (self._m1, self._m2))
 
@@ -645,8 +641,8 @@ 
     def always(self):
         return self._always
 
-    def anypats(self):
-        return self._matcher.anypats()
+    def prefix(self):
+        return self._matcher.prefix() and not self._always
 
     def __repr__(self):
         return ('<subdirmatcher path=%r, matcher=%r>' %
@@ -662,12 +658,6 @@ 
     def __call__(self, value):
         return value in self._includes or self._matcher(value)
 
-    def anypats(self):
-        return True
-
-    def prefix(self):
-        return False
-
     def __repr__(self):
         return ('<forceincludematcher matcher=%r, includes=%r>' %
                 (self._matcher, sorted(self._includes)))
@@ -683,12 +673,6 @@ 
                 return True
         return False
 
-    def anypats(self):
-        return True
-
-    def prefix(self):
-        return False
-
     def __repr__(self):
         return ('<unionmatcher matchers=%r>' % self._matchers)
 
@@ -699,9 +683,6 @@ 
     def __call__(self, value):
         return not self._matcher(value)
 
-    def anypats(self):
-        return True
-
     def __repr__(self):
         return ('<negatematcher matcher=%r>' % self._matcher)