Patchwork [2,of,2] pathutil: document that dirs map type implies manifest/dirstate processing

login
register
mail settings
Submitter Josef 'Jeff' Sipek
Date April 1, 2020, 2:41 p.m.
Message ID <20200401144121.GB1539@meili>
Download mbox | patch
Permalink /patch/45957/
State New
Headers show

Comments

Josef 'Jeff' Sipek - April 1, 2020, 2:41 p.m.
On Fri, Mar 27, 2020 at 12:21:50 -0400, Augie Fackler wrote:
> On Mar 27, 2020, at 10:56, Josef 'Jeff' Sipek <jeffpc@josefsipek.net> wrote:
> > On Fri, Mar 27, 2020 at 10:48:58 -0400, Josef 'Jeff' Sipek wrote:
> > > # HG changeset patch
> > > # User Josef 'Jeff' Sipek <jeffpc@josefsipek.net>
> > > # Date 1585319999 14400
> > > #      Fri Mar 27 10:39:59 2020 -0400
> > > # Node ID f313b33e0341724093d866f0bf5478a28cad092a
> > > # Parent  4f4c2622ec748ce806c6577c30d4f1ae8660a0c2
> > > pathutil: document that dirs map type implies manifest/dirstate processing
> > 
> > FWIW, this "argument type implies manifest or dirstate" seems like a hack.
> > I'm not familiar enough with python to know if I'm wrong here, but I'm open
> > to replacing this patch with somethig that adds a "type" argument.  Then,
> > __init__ can assert/abort/etc. if it gets a mismatch.  I imagine something
> > like:
> > 
> > def __init__(self, map, is_dirstate, skip=None):
> >    if is_dirstate:
> >        assert isinstance(map, dict)
> >    else:
> >        assert isinstance(map, list)
> >    ...code more or less as before...
> > 
> > Would this be a good change?
> 
> I'm conflicted. What we've got is akin to a type-overload in C++, but
> there's (obviously) no function overloading in Python. It might be clearer
> to convert this to three methods, eg (PEP484 hints included for clarity):
...
> Thoughts?

Agreed.

I already shared this on IRC last week, but I should share it here as well.
It is a work-in-progress.  I haven't run the tests, but normal usage seems
to work WITHOUT the C or Rust code.  Those still need to be changed and I
haven't even looked at how to implement class methods in those.

Jeff.


# HG changeset patch
# User Josef 'Jeff' Sipek <jeffpc@josefsipek.net>
# Date 1585358782 14400
#      Fri Mar 27 21:26:22 2020 -0400
# Node ID c594db96ec9d119655d30305b5ac8d22e9d4a3bb
# Parent  f313b33e0341724093d866f0bf5478a28cad092a
WIP: rewrite pathutils.dirs

Patch

diff --git a/hgext/git/manifest.py b/hgext/git/manifest.py
--- a/hgext/git/manifest.py
+++ b/hgext/git/manifest.py
@@ -120,7 +120,7 @@  class gittreemanifest(object):
 
     @util.propertycache
     def _dirs(self):
-        return pathutil.dirs(self)
+        return pathutil.dirs.from_manifest(self)
 
     def hasdir(self, dir):
         return dir in self._dirs
@@ -232,7 +232,7 @@  class memgittreemanifestctx(object):
         # just narrow?
         assert not match or isinstance(match, matchmod.alwaysmatcher)
 
-        touched_dirs = pathutil.dirs(list(self._pending_changes))
+        touched_dirs = pathutil.dirs.from_manifest(self._pending_changes)
         trees = {
             b'': self._tree,
         }
diff --git a/hgext/narrow/narrowcommands.py b/hgext/narrow/narrowcommands.py
--- a/hgext/narrow/narrowcommands.py
+++ b/hgext/narrow/narrowcommands.py
@@ -278,7 +278,7 @@  def _narrow(
                     todelete.append(f)
             elif f.startswith(b'meta/'):
                 dir = f[5:-13]
-                dirs = sorted(pathutil.dirs({dir})) + [dir]
+                dirs = sorted(pathutil.dirs.from_manifest({dir})) + [dir]
                 include = True
                 for d in dirs:
                     visit = newmatch.visitdir(d)
diff --git a/hgext/uncommit.py b/hgext/uncommit.py
--- a/hgext/uncommit.py
+++ b/hgext/uncommit.py
@@ -186,7 +186,7 @@  def uncommit(ui, repo, *pats, **opts):
             # if not everything tracked in that directory can be
             # uncommitted.
             if badfiles:
-                badfiles -= {f for f in pathutil.dirs(eligible)}
+                badfiles -= {f for f in pathutil.dirs.from_manifest(eligible)}
 
             for f in sorted(badfiles):
                 if f in s.clean:
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2823,7 +2823,7 @@  def remove(
     progress.complete()
 
     # warn about failure to delete explicit files/dirs
-    deleteddirs = pathutil.dirs(deleted)
+    deleteddirs = pathutil.dirs.from_manifest(deleted)
     files = m.files()
     progress = ui.makeprogress(
         _(b'deleting'), total=len(files), unit=_(b'files')
diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -1577,11 +1577,11 @@  class dirstatemap(object):
 
     @propertycache
     def _dirs(self):
-        return pathutil.dirs(self._map, b'r')
+        return pathutil.dirs.from_dirstate(self._map, b'r')
 
     @propertycache
     def _alldirs(self):
-        return pathutil.dirs(self._map)
+        return pathutil.dirs.from_dirstate(self._map)
 
     def _opendirstatefile(self):
         fp, mode = txnutil.trypending(self._root, self._opener, self._filename)
diff --git a/mercurial/manifest.py b/mercurial/manifest.py
--- a/mercurial/manifest.py
+++ b/mercurial/manifest.py
@@ -491,7 +491,7 @@  class manifestdict(object):
 
     @propertycache
     def _dirs(self):
-        return pathutil.dirs(self)
+        return pathutil.dirs.from_manifest(self)
 
     def dirs(self):
         return self._dirs
@@ -1103,7 +1103,7 @@  class treemanifest(object):
 
     @propertycache
     def _alldirs(self):
-        return pathutil.dirs(self)
+        return pathutil.dirs.from_manifest(self)
 
     def dirs(self):
         return self._alldirs
diff --git a/mercurial/match.py b/mercurial/match.py
--- a/mercurial/match.py
+++ b/mercurial/match.py
@@ -592,7 +592,7 @@  class patternmatcher(basematcher):
 
     @propertycache
     def _dirs(self):
-        return set(pathutil.dirs(self._fileset))
+        return set(pathutil.dirs.from_manifest(self._fileset))
 
     def visitdir(self, dir):
         if self._prefix and dir in self._fileset:
@@ -763,7 +763,7 @@  class exactmatcher(basematcher):
 
     @propertycache
     def _dirs(self):
-        return set(pathutil.dirs(self._fileset))
+        return set(pathutil.dirs.from_manifest(self._fileset))
 
     def visitdir(self, dir):
         return dir in self._dirs
@@ -1489,8 +1489,8 @@  def _rootsdirsandparents(kindpats):
     p = set()
     # Add the parents as non-recursive/exact directories, since they must be
     # scanned to get to either the roots or the other exact directories.
-    p.update(pathutil.dirs(d))
-    p.update(pathutil.dirs(r))
+    p.update(pathutil.dirs.from_manifest(d))
+    p.update(pathutil.dirs.from_manifest(r))
 
     # FIXME: all uses of this function convert these to sets, do so before
     # returning.
diff --git a/mercurial/pathutil.py b/mercurial/pathutil.py
--- a/mercurial/pathutil.py
+++ b/mercurial/pathutil.py
@@ -285,23 +285,21 @@  def finddirs(path):
 class dirs(object):
     '''a multiset of directory names from a set of file paths'''
 
-    def __init__(self, map, skip=None):
-        '''
-        a dict map indicates a dirstate while a list indicates a manifest
-        '''
+    @classmethod
+    def from_dirstate(cls, dirstate_map, skip=None):
+        if skip is not None:
+            return cls(f for f, s in pycompat.iteritems(dirstate_map) if s[0] != skip)
+        return cls(dirstate)
+
+    @classmethod
+    def from_manifest(cls, files):
+        return cls(list(files))
+
+    def __init__(self, seq):
         self._dirs = {}
         addpath = self.addpath
-        if isinstance(map, dict) and skip is not None:
-            for f, s in pycompat.iteritems(map):
-                if s[0] != skip:
-                    addpath(f)
-        elif skip is not None:
-            raise error.ProgrammingError(
-                b"skip character is only supported with a dict source"
-            )
-        else:
-            for f in map:
-                addpath(f)
+        for f in seq:
+            addpath(f)
 
     def addpath(self, path):
         dirs = self._dirs
diff --git a/mercurial/repair.py b/mercurial/repair.py
--- a/mercurial/repair.py
+++ b/mercurial/repair.py
@@ -479,7 +479,7 @@  def rebuildfncache(ui, repo):
         if b'treemanifest' in repo.requirements:
             # This logic is safe if treemanifest isn't enabled, but also
             # pointless, so we skip it if treemanifest isn't enabled.
-            for dir in pathutil.dirs(seenfiles):
+            for dir in pathutil.dirs.from_manifest(seenfiles):
                 i = b'meta/%s/00manifest.i' % dir
                 d = b'meta/%s/00manifest.d' % dir
 
diff --git a/tests/test-dirs.py b/tests/test-dirs.py
--- a/tests/test-dirs.py
+++ b/tests/test-dirs.py
@@ -13,13 +13,13 @@  class dirstests(unittest.TestCase):
             (b'a/a/a', [b'a', b'a/a', b'']),
             (b'alpha/beta/gamma', [b'', b'alpha', b'alpha/beta']),
         ]:
-            d = pathutil.dirs({})
+            d = pathutil.dirs.from_manifest({})
             d.addpath(case)
             self.assertEqual(sorted(d), sorted(want))
 
     def testinvalid(self):
         with self.assertRaises(ValueError):
-            d = pathutil.dirs({})
+            d = pathutil.dirs.from_manifest({})
             d.addpath(b'a//b')