Patchwork manifest: move dirs() to manifest

login
register
mail settings
Submitter Drew Gottlieb
Date March 13, 2015, 7:18 p.m.
Message ID <47f1fdab71a77d88ca51.1426274300@waste.org>
Download mbox | patch
Permalink /patch/8060/
State Changes Requested
Headers show

Comments

Drew Gottlieb - March 13, 2015, 7:18 p.m.
# HG changeset patch
# User Drew Gottlieb <drgott@google.com>
# Date 1426180563 25200
#      Thu Mar 12 10:16:03 2015 -0700
# Node ID 47f1fdab71a77d88ca51d96002a8f3306b10e527
# Parent  2b7ab29627fd93ca7f5cb838403c2f6c728469bd
manifest: move dirs() to manifest

This moves implementation of dirs() from basectx to manifestdict. Moving
knowledge of the manifest's dirs to the manifest itself allows for cleaner
implementation of more optimized manifests. This change also adds a new
hasdir() method to manifest for the same reason, and also adds a hasdir() to
context for convenience -- which calls the manifest's.
Matt Mackall - March 13, 2015, 10:04 p.m.
On Fri, 2015-03-13 at 14:18 -0500, Drew Gottlieb wrote:
> # HG changeset patch
> # User Drew Gottlieb <drgott@google.com>
> # Date 1426180563 25200
> #      Thu Mar 12 10:16:03 2015 -0700
> # Node ID 47f1fdab71a77d88ca51d96002a8f3306b10e527
> # Parent  2b7ab29627fd93ca7f5cb838403c2f6c728469bd
> manifest: move dirs() to manifest

This looks fine, but we're really picky about patch granularity.
Please send multiple patches:

- addition of dirs() to manifest
- change of dirs() in context
- addition of hasdir() in manifest
- addition of hasdir() in context
- users of hasdir()

This matters because if you have change X and Y in one patch and change
Y turns out to be subtly wrong, it's not interleaved with X for purposes
of review/acceptance/testing/debugging/bisecting/backout.

A good rule of thumb is that if you find yourself using "and" or bullet
points to explain what you've done, your patch is too big.

Also:

> -            if fn in self._dirs:
> -                # specified pattern is a directory
> -                continue
> -            match.bad(fn, _('no such file in rev %s') % self)
> +            if not self.hasdir(fn):

It's not a big deal here, because this is an error path, but we're going
from a fast Python built-in to three or four layers of slow Python
method lookups/calls in a loop.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -264,12 +264,11 @@ 
         diffopts = patch.diffopts(self._repo.ui, opts)
         return patch.diff(self._repo, ctx2, self, match=match, opts=diffopts)
 
-    @propertycache
-    def _dirs(self):
-        return scmutil.dirs(self._manifest)
+    def dirs(self):
+        return self._manifest.dirs()
 
-    def dirs(self):
-        return self._dirs
+    def hasdir(self, dir):
+        return self._manifest.hasdir(dir)
 
     def dirty(self, missing=False, merge=True, branch=True):
         return False
@@ -606,10 +605,8 @@ 
             if match(fn):
                 yield fn
         for fn in sorted(fset):
-            if fn in self._dirs:
-                # specified pattern is a directory
-                continue
-            match.bad(fn, _('no such file in rev %s') % self)
+            if not self.hasdir(fn):
+                match.bad(fn, _('no such file in rev %s') % self)
 
     def matches(self, match):
         return self.walk(match)
@@ -1559,7 +1556,7 @@ 
             def bad(f, msg):
                 # 'f' may be a directory pattern from 'match.files()',
                 # so 'f not in ctx1' is not enough
-                if f not in other and f not in other.dirs():
+                if f not in other and not other.hasdir(f):
                     self._repo.ui.warn('%s: %s\n' %
                                        (self._repo.dirstate.pathto(f), msg))
             match.bad = bad
diff --git a/mercurial/manifest.py b/mercurial/manifest.py
--- a/mercurial/manifest.py
+++ b/mercurial/manifest.py
@@ -6,9 +6,10 @@ 
 # GNU General Public License version 2 or any later version.
 
 from i18n import _
-import mdiff, parsers, error, revlog, util
+import mdiff, parsers, error, revlog, util, scmutil
 import array, struct
 
+propertycache = util.propertycache
 
 class _lazymanifest(dict):
     """This is the pure implementation of lazymanifest.
@@ -141,6 +142,16 @@ 
         files.difference_update(m2.iterkeys())
         return files
 
+    @propertycache
+    def _dirs(self):
+        return scmutil.dirs(self)
+
+    def dirs(self):
+        return self._dirs
+
+    def hasdir(self, dir):
+        return dir in self._dirs
+
     def matches(self, match):
         '''generate a new manifest filtered by the match argument'''
         if match.always():