Patchwork [2,of,5] match: extract base class for matchers

login
register
mail settings
Submitter via Mercurial-devel
Date May 24, 2017, 12:04 a.m.
Message ID <d87d3fba9d29df0fc136.1495584274@martinvonz.svl.corp.google.com>
Download mbox | patch
Permalink /patch/20876/
State Accepted
Headers show

Comments

via Mercurial-devel - May 24, 2017, 12:04 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1495089913 25200
#      Wed May 17 23:45:13 2017 -0700
# Node ID d87d3fba9d29df0fc13681cbd69d8dc5e0a5ff51
# Parent  0f4af20ceddd58eb37db6d1aa2f0331b29b1137a
match: extract base class for matchers

We will soon start splitting up the current matcher class into more
specialized classes, so we'll want a base class for all the things
that don't vary much between different matchers.
Yuya Nishihara - May 25, 2017, 1:32 p.m.
On Tue, 23 May 2017 17:04:34 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1495089913 25200
> #      Wed May 17 23:45:13 2017 -0700
> # Node ID d87d3fba9d29df0fc13681cbd69d8dc5e0a5ff51
> # Parent  0f4af20ceddd58eb37db6d1aa2f0331b29b1137a
> match: extract base class for matchers

The series looks good to me. Queued, thanks.

A few nits inline.

> diff --git a/mercurial/match.py b/mercurial/match.py
> --- a/mercurial/match.py
> +++ b/mercurial/match.py
> @@ -201,19 +201,107 @@
>          kindpats.append((kind, pat, ''))
>      return kindpats
>  
> -class matcher(object):
> +class basematcher(object):

> +    def abs(self, f):
> +        '''Convert a repo path back to path that is relative to the root of the
> +        matcher.'''
> +        return f
> +
> +    def rel(self, f):
> +        '''Convert repo path back to path that is relative to cwd of matcher.'''
> +        return util.pathto(self._root, self._cwd, f)
> +
> +    def uipath(self, f):
> +        '''Convert repo path to a display path.  If patterns or -I/-X were used
> +        to create this matcher, the display path will be relative to cwd.
> +        Otherwise it is relative to the root of the repo.'''
> +        return self.rel(f)

The default uipath() implementation does not agree with the doc and is unused
now. Perhaps it's better to raise NotImplementedError.

> +    def visitdir(self, dir):
> +        '''Decides whether a directory should be visited based on whether it
> +        has potential matches in it or one of its subdirectories. This is
> +        based on the match's primary, included, and excluded patterns.
> +
> +        Returns the string 'all' if the given directory and all subdirectories
> +        should be visited. Otherwise returns True or False indicating whether
> +        the given directory should be visited.
> +
> +        This function's behavior is undefined if it has returned False for
> +        one of the dir's parent directories.
> +        '''
> +        return False

This visitdir() seems also useless.
via Mercurial-devel - May 25, 2017, 4:35 p.m.
On Thu, May 25, 2017 at 6:32 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Tue, 23 May 2017 17:04:34 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@google.com>
>> # Date 1495089913 25200
>> #      Wed May 17 23:45:13 2017 -0700
>> # Node ID d87d3fba9d29df0fc13681cbd69d8dc5e0a5ff51
>> # Parent  0f4af20ceddd58eb37db6d1aa2f0331b29b1137a
>> match: extract base class for matchers
>
> The series looks good to me. Queued, thanks.
>
> A few nits inline.
>
>> diff --git a/mercurial/match.py b/mercurial/match.py
>> --- a/mercurial/match.py
>> +++ b/mercurial/match.py
>> @@ -201,19 +201,107 @@
>>          kindpats.append((kind, pat, ''))
>>      return kindpats
>>
>> -class matcher(object):
>> +class basematcher(object):
>
>> +    def abs(self, f):
>> +        '''Convert a repo path back to path that is relative to the root of the
>> +        matcher.'''
>> +        return f
>> +
>> +    def rel(self, f):
>> +        '''Convert repo path back to path that is relative to cwd of matcher.'''
>> +        return util.pathto(self._root, self._cwd, f)
>> +
>> +    def uipath(self, f):
>> +        '''Convert repo path to a display path.  If patterns or -I/-X were used
>> +        to create this matcher, the display path will be relative to cwd.
>> +        Otherwise it is relative to the root of the repo.'''
>> +        return self.rel(f)
>
> The default uipath() implementation does not agree with the doc and is unused
> now.

In a way it is accurate, as long as the subclasses that do *not* have
any patterns or -I/-X override it. There will be a single such class
(the "alwaysmatcher") and that does override the method.

> Perhaps it's better to raise NotImplementedError.

I tried to make basematcher behave like a "nevermatcher", which is
also why visitdir() is always overridden so far.

Still, good points. I'm happy to get back to them at the end of the
series. Feel free to remind me if I forget.

>
>> +    def visitdir(self, dir):
>> +        '''Decides whether a directory should be visited based on whether it
>> +        has potential matches in it or one of its subdirectories. This is
>> +        based on the match's primary, included, and excluded patterns.
>> +
>> +        Returns the string 'all' if the given directory and all subdirectories
>> +        should be visited. Otherwise returns True or False indicating whether
>> +        the given directory should be visited.
>> +
>> +        This function's behavior is undefined if it has returned False for
>> +        one of the dir's parent directories.
>> +        '''
>> +        return False
>
> This visitdir() seems also useless.
Yuya Nishihara - May 26, 2017, 2:32 p.m.
On Thu, 25 May 2017 09:35:04 -0700, Martin von Zweigbergk wrote:
> On Thu, May 25, 2017 at 6:32 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Tue, 23 May 2017 17:04:34 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> >> # HG changeset patch
> >> # User Martin von Zweigbergk <martinvonz@google.com>
> >> # Date 1495089913 25200
> >> #      Wed May 17 23:45:13 2017 -0700
> >> # Node ID d87d3fba9d29df0fc13681cbd69d8dc5e0a5ff51
> >> # Parent  0f4af20ceddd58eb37db6d1aa2f0331b29b1137a
> >> match: extract base class for matchers
> >
> > The series looks good to me. Queued, thanks.
> >
> > A few nits inline.
> >
> >> diff --git a/mercurial/match.py b/mercurial/match.py
> >> --- a/mercurial/match.py
> >> +++ b/mercurial/match.py
> >> @@ -201,19 +201,107 @@
> >>          kindpats.append((kind, pat, ''))
> >>      return kindpats
> >>
> >> -class matcher(object):
> >> +class basematcher(object):
> >
> >> +    def abs(self, f):
> >> +        '''Convert a repo path back to path that is relative to the root of the
> >> +        matcher.'''
> >> +        return f
> >> +
> >> +    def rel(self, f):
> >> +        '''Convert repo path back to path that is relative to cwd of matcher.'''
> >> +        return util.pathto(self._root, self._cwd, f)
> >> +
> >> +    def uipath(self, f):
> >> +        '''Convert repo path to a display path.  If patterns or -I/-X were used
> >> +        to create this matcher, the display path will be relative to cwd.
> >> +        Otherwise it is relative to the root of the repo.'''
> >> +        return self.rel(f)
> >
> > The default uipath() implementation does not agree with the doc and is unused
> > now.
> 
> In a way it is accurate, as long as the subclasses that do *not* have
> any patterns or -I/-X override it. There will be a single such class
> (the "alwaysmatcher") and that does override the method.

I don't fully understand all possibilities which could formulate the
basematcher, but I thought uipath() would return abs(f) if a matcher were zero
(i.e. no patters, no -I nor -X.)

Patch

diff --git a/mercurial/match.py b/mercurial/match.py
--- a/mercurial/match.py
+++ b/mercurial/match.py
@@ -201,19 +201,107 @@ 
         kindpats.append((kind, pat, ''))
     return kindpats
 
-class matcher(object):
+class basematcher(object):
+
+    def __init__(self, root, cwd, badfn=None):
+        self._root = root
+        self._cwd = cwd
+        if badfn is not None:
+            self.bad = badfn
+        self._files = [] # exact files and roots of patterns
+        self.matchfn = lambda f: False
+
+    def __call__(self, fn):
+        return self.matchfn(fn)
+    def __iter__(self):
+        for f in self._files:
+            yield f
+    # Callbacks related to how the matcher is used by dirstate.walk.
+    # Subscribers to these events must monkeypatch the matcher object.
+    def bad(self, f, msg):
+        '''Callback from dirstate.walk for each explicit file that can't be
+        found/accessed, with an error message.'''
+        pass
+
+    # If an explicitdir is set, it will be called when an explicitly listed
+    # directory is visited.
+    explicitdir = None
+
+    # If an traversedir is set, it will be called when a directory discovered
+    # by recursive traversal is visited.
+    traversedir = None
+
+    def abs(self, f):
+        '''Convert a repo path back to path that is relative to the root of the
+        matcher.'''
+        return f
+
+    def rel(self, f):
+        '''Convert repo path back to path that is relative to cwd of matcher.'''
+        return util.pathto(self._root, self._cwd, f)
+
+    def uipath(self, f):
+        '''Convert repo path to a display path.  If patterns or -I/-X were used
+        to create this matcher, the display path will be relative to cwd.
+        Otherwise it is relative to the root of the repo.'''
+        return self.rel(f)
+
+    def files(self):
+        '''Explicitly listed files or patterns or roots:
+        if no patterns or .always(): empty list,
+        if exact: list exact files,
+        if not .anypats(): list all files and dirs,
+        else: optimal roots'''
+        return self._files
+
+    @propertycache
+    def _fileset(self):
+        return set(self._files)
+
+    def exact(self, f):
+        '''Returns True if f is in .files().'''
+        return f in self._fileset
+
+    def visitdir(self, dir):
+        '''Decides whether a directory should be visited based on whether it
+        has potential matches in it or one of its subdirectories. This is
+        based on the match's primary, included, and excluded patterns.
+
+        Returns the string 'all' if the given directory and all subdirectories
+        should be visited. Otherwise returns True or False indicating whether
+        the given directory should be visited.
+
+        This function's behavior is undefined if it has returned False for
+        one of the dir's parent directories.
+        '''
+        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.'''
+        return False
+
+    def isexact(self):
+        return False
+
+    def prefix(self):
+        return not self.always() and not self.isexact() and not self.anypats()
+
+class matcher(basematcher):
 
     def __init__(self, root, cwd, normalize, patterns, include=None,
                  exclude=None, default='glob', exact=False, auditor=None,
                  ctx=None, listsubrepos=False, warn=None, badfn=None):
+        super(matcher, self).__init__(root, cwd, badfn)
         if include is None:
             include = []
         if exclude is None:
             exclude = []
 
-        self._root = root
-        self._cwd = cwd
-        self._files = [] # exact files and roots of patterns
         self._anypats = bool(include or exclude)
         self._always = False
         self._pathrestricted = bool(include or exclude or patterns)
@@ -227,9 +315,6 @@ 
         # dirs are directories which are non-recursively included.
         self._includedirs = set()
 
-        if badfn is not None:
-            self.bad = badfn
-
         matchfns = []
         if include:
             kindpats = normalize(include, 'glob', root, cwd, auditor, warn)
@@ -280,70 +365,14 @@ 
 
         self.matchfn = m
 
-    def __call__(self, fn):
-        return self.matchfn(fn)
-    def __iter__(self):
-        for f in self._files:
-            yield f
-
-    # Callbacks related to how the matcher is used by dirstate.walk.
-    # Subscribers to these events must monkeypatch the matcher object.
-    def bad(self, f, msg):
-        '''Callback from dirstate.walk for each explicit file that can't be
-        found/accessed, with an error message.'''
-        pass
-
-    # If an explicitdir is set, it will be called when an explicitly listed
-    # directory is visited.
-    explicitdir = None
-
-    # If an traversedir is set, it will be called when a directory discovered
-    # by recursive traversal is visited.
-    traversedir = None
-
-    def abs(self, f):
-        '''Convert a repo path back to path that is relative to the root of the
-        matcher.'''
-        return f
-
-    def rel(self, f):
-        '''Convert repo path back to path that is relative to cwd of matcher.'''
-        return util.pathto(self._root, self._cwd, f)
-
     def uipath(self, f):
-        '''Convert repo path to a display path.  If patterns or -I/-X were used
-        to create this matcher, the display path will be relative to cwd.
-        Otherwise it is relative to the root of the repo.'''
         return (self._pathrestricted and self.rel(f)) or self.abs(f)
 
-    def files(self):
-        '''Explicitly listed files or patterns or roots:
-        if no patterns or .always(): empty list,
-        if exact: list exact files,
-        if not .anypats(): list all files and dirs,
-        else: optimal roots'''
-        return self._files
-
-    @propertycache
-    def _fileset(self):
-        return set(self._files)
-
     @propertycache
     def _dirs(self):
         return set(util.dirs(self._fileset)) | {'.'}
 
     def visitdir(self, dir):
-        '''Decides whether a directory should be visited based on whether it
-        has potential matches in it or one of its subdirectories. This is
-        based on the match's primary, included, and excluded patterns.
-
-        Returns the string 'all' if the given directory and all subdirectories
-        should be visited. Otherwise returns True or False indicating whether
-        the given directory should be visited.
-
-        This function's behavior is undefined if it has returned False for
-        one of the dir's parent directories.
-        '''
         if self.prefix() and dir in self._fileset:
             return 'all'
         if dir in self._excluderoots:
@@ -362,25 +391,15 @@ 
                 any(parentdir in self._fileset
                     for parentdir in util.finddirs(dir)))
 
-    def exact(self, f):
-        '''Returns True if f is in .files().'''
-        return f in self._fileset
-
     def anypats(self):
-        '''Matcher uses patterns or include/exclude.'''
         return self._anypats
 
     def always(self):
-        '''Matcher will match everything and .files() will be empty
-        - optimization might be possible and necessary.'''
         return self._always
 
     def isexact(self):
         return self.matchfn == self.exact
 
-    def prefix(self):
-        return not self.always() and not self.isexact() and not self.anypats()
-
     def __repr__(self):
         return ('<matcher files=%r, patterns=%r, includes=%r, excludes=%r>' %
                 (self._files, self.patternspat, self.includepat,