[PATCH 1 of 2] git: pass a list to pathutil.dirs to indicate that it is a manifest

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1 of 2] git: pass a list to pathutil.dirs to indicate that it is a manifest

Josef 'Jeff' Sipek
# HG changeset patch
# User Josef 'Jeff' Sipek <[hidden email]>
# Date 1585319920 14400
#      Fri Mar 27 10:38:40 2020 -0400
# Node ID 4f4c2622ec748ce806c6577c30d4f1ae8660a0c2
# Parent  5a77ab1704526629c316ebd93ca355d3439eb0b7
git: pass a list to pathutil.dirs to indicate that it is a manifest

The python implementation of pathutil.dirs just uses a for loop which
happens to work the same on both dicts and lists.  The rust implementation
actually figures out which of the two types it is, and directs the execution
to either dirstate or manifest processing.

diff --git a/hgext/git/manifest.py b/hgext/git/manifest.py
--- a/hgext/git/manifest.py
+++ b/hgext/git/manifest.py
@@ -232,7 +232,7 @@ class memgittreemanifestctx(object):
         # just narrow?
         assert not match or isinstance(match, matchmod.alwaysmatcher)
 
-        touched_dirs = pathutil.dirs(self._pending_changes)
+        touched_dirs = pathutil.dirs(list(self._pending_changes))
         trees = {
             b'': self._tree,
         }
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Josef 'Jeff' Sipek
# HG changeset patch
# User Josef 'Jeff' Sipek <[hidden email]>
# 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

diff --git a/mercurial/pathutil.py b/mercurial/pathutil.py
--- a/mercurial/pathutil.py
+++ b/mercurial/pathutil.py
@@ -286,6 +286,9 @@ 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
+        '''
         self._dirs = {}
         addpath = self.addpath
         if isinstance(map, dict) and skip is not None:
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Josef 'Jeff' Sipek
On Fri, Mar 27, 2020 at 10:48:58 -0400, Josef 'Jeff' Sipek wrote:
> # HG changeset patch
> # User Josef 'Jeff' Sipek <[hidden email]>
> # 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?

Thanks,

Jeff.

>
> diff --git a/mercurial/pathutil.py b/mercurial/pathutil.py
> --- a/mercurial/pathutil.py
> +++ b/mercurial/pathutil.py
> @@ -286,6 +286,9 @@ 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
> +        '''
>          self._dirs = {}
>          addpath = self.addpath
>          if isinstance(map, dict) and skip is not None:
> _______________________________________________
> Mercurial-devel mailing list
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

--
You measure democracy by the freedom it gives its dissidents, not the
freedom it gives its assimilated conformists.
                - Abbie Hoffman
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Augie Fackler-2
In reply to this post by Josef 'Jeff' Sipek
queued, thanks

> On Mar 27, 2020, at 10:48, Josef 'Jeff' Sipek <[hidden email]> wrote:
>
> # HG changeset patch
> # User Josef 'Jeff' Sipek <[hidden email]>
> # 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
>
> diff --git a/mercurial/pathutil.py b/mercurial/pathutil.py
> --- a/mercurial/pathutil.py
> +++ b/mercurial/pathutil.py
> @@ -286,6 +286,9 @@ 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
> +        '''
>         self._dirs = {}
>         addpath = self.addpath
>         if isinstance(map, dict) and skip is not None:
> _______________________________________________
> Mercurial-devel mailing list
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Augie Fackler-2
In reply to this post by Josef 'Jeff' Sipek


> On Mar 27, 2020, at 10:56, Josef 'Jeff' Sipek <[hidden email]> wrote:
>
> On Fri, Mar 27, 2020 at 10:48:58 -0400, Josef 'Jeff' Sipek wrote:
>> # HG changeset patch
>> # User Josef 'Jeff' Sipek <[hidden email]>
>> # 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):

def __init__(self, seq: Iterable[bytes]):
  for f in seq:
    addpath(f)

@classmethod
def fromdirstate(cls, dirstate_map: Dict[bytes, T], 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 frommanifest(cls, files: Iterable[bytes]):
  return cls(files)


You could probably even elide the second one, since it's more obvious what's going on. Thoughts?

>
> Thanks,
>
> Jeff.
>
>>
>> diff --git a/mercurial/pathutil.py b/mercurial/pathutil.py
>> --- a/mercurial/pathutil.py
>> +++ b/mercurial/pathutil.py
>> @@ -286,6 +286,9 @@ 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
>> +        '''
>>         self._dirs = {}
>>         addpath = self.addpath
>>         if isinstance(map, dict) and skip is not None:
>> _______________________________________________
>> Mercurial-devel mailing list
>> [hidden email]
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
> --
> You measure democracy by the freedom it gives its dissidents, not the
> freedom it gives its assimilated conformists.
> - Abbie Hoffman
> _______________________________________________
> Mercurial-devel mailing list
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Josef 'Jeff' Sipek
On Fri, Mar 27, 2020 at 12:21:50 -0400, Augie Fackler wrote:

> On Mar 27, 2020, at 10:56, Josef 'Jeff' Sipek <[hidden email]> wrote:
> > On Fri, Mar 27, 2020 at 10:48:58 -0400, Josef 'Jeff' Sipek wrote:
> > > # HG changeset patch
> > > # User Josef 'Jeff' Sipek <[hidden email]>
> > > # 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 <[hidden email]>
# Date 1585358782 14400
#      Fri Mar 27 21:26:22 2020 -0400
# Node ID c594db96ec9d119655d30305b5ac8d22e9d4a3bb
# Parent  f313b33e0341724093d866f0bf5478a28cad092a
WIP: rewrite pathutils.dirs

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')
 
 
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel