Patchwork D1340: dirstate: add explicit methods for modifying dirstate

login
register
mail settings
Submitter phabricator
Date Nov. 8, 2017, 5:31 p.m.
Message ID <differential-rev-PHID-DREV-spdj6wegpgx5qzznx34h-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25424/
State Superseded
Headers show

Comments

phabricator - Nov. 8, 2017, 5:31 p.m.
mbthomas created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Instead of assigning dirstatetuple objects to entries in the dirstate, move
  responsibility for creating tuples into the dirstatemap.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1340

AFFECTED FILES
  mercurial/dirstate.py

CHANGE DETAILS




To: mbthomas, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 10, 2017, 1:01 a.m.
mbolin added inline comments.

INLINE COMMENTS

> dirstate.py:1275
> +        """
> +        exists = f in self._map
> +        if exists:

To avoid doing two lookups in `self._map`:

  try:
      self._map.pop(f)
      return True
  except KeyError:
      return False

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1340

To: mbthomas, #hg-reviewers
Cc: mbolin, mercurial-devel
phabricator - Nov. 10, 2017, 10:06 p.m.
durin42 requested changes to this revision.
durin42 added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> dirstate.py:130
>      def _map(self):
> -        '''Return the dirstate contents as a map from filename to
> -        (state, mode, size, time).'''
> +        '''Returns the dirstate map.'''
>          self._map = dirstatemap(self._ui, self._opener, self._root)

Why is ~all the interesting content of this docstring removed? Where is the meaning of the dirstate map documented other than this docstring?

> dirstate.py:1196
>  
>  class dirstatemap(object):
>      def __init__(self, ui, opener, root):

Per my comment above, let's insert a patch before this one that documents in more detail the API that dirstatemap is supposed to be providing, probably in a docstring on this class. Sound reasonable?

(Then the above docstring I complained about could point to this docstring.)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1340

To: mbthomas, #hg-reviewers, durin42
Cc: durin42, mbolin, mercurial-devel
phabricator - Nov. 11, 2017, 12:59 p.m.
mbthomas added inline comments.

INLINE COMMENTS

> durin42 wrote in dirstate.py:1196
> Per my comment above, let's insert a patch before this one that documents in more detail the API that dirstatemap is supposed to be providing, probably in a docstring on this class. Sound reasonable?
> 
> (Then the above docstring I complained about could point to this docstring.)

Agreed.  I removed the docstring above because it became a lie (we no longer implement `__setitem__` or `__delitem__`, so it's not really a dict any more).  I will add a docstring here on Monday.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1340

To: mbthomas, #hg-reviewers, durin42
Cc: durin42, mbolin, mercurial-devel
phabricator - Nov. 11, 2017, 1:04 p.m.
mbthomas added inline comments.

INLINE COMMENTS

> mbolin wrote in dirstate.py:1275
> To avoid doing two lookups in `self._map`:
> 
>   try:
>       self._map.pop(f)
>       return True
>   except KeyError:
>       return False

Good idea.  I think `return self._map.pop(f, None) is not None` would avoid two lookups and also the exception.  This function's going to be getting more complex in later diffs, so I'd like to avoid an exception handler if possible.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1340

To: mbthomas, #hg-reviewers, durin42
Cc: durin42, mbolin, mercurial-devel

Patch

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -127,8 +127,7 @@ 
 
     @propertycache
     def _map(self):
-        '''Return the dirstate contents as a map from filename to
-        (state, mode, size, time).'''
+        '''Returns the dirstate map.'''
         self._map = dirstatemap(self._ui, self._opener, self._root)
         return self._map
 
@@ -416,11 +415,11 @@ 
             self._map.dirs.addpath(f)
         self._dirty = True
         self._updatedfiles.add(f)
-        self._map[f] = dirstatetuple(state, mode, size, mtime)
         if state != 'n' or mtime == -1:
             self._map.nonnormalset.add(f)
         if size == -2:
             self._map.otherparentset.add(f)
+        self._map.addfile(f, state, mode, size, mtime)
 
     def normal(self, f):
         '''Mark a file normal and clean.'''
@@ -491,8 +490,8 @@ 
                 elif entry[0] == 'n' and entry[2] == -2: # other parent
                     size = -2
                     self._map.otherparentset.add(f)
-        self._map[f] = dirstatetuple('r', 0, size, 0)
         self._map.nonnormalset.add(f)
+        self._map.removefile(f, size)
         if size == 0:
             self._map.copymap.pop(f, None)
 
@@ -504,10 +503,9 @@ 
 
     def drop(self, f):
         '''Drop a file from the dirstate'''
-        if f in self._map:
+        if self._map.dropfile(f):
             self._dirty = True
             self._droppath(f)
-            del self._map[f]
             if f in self._map.nonnormalset:
                 self._map.nonnormalset.remove(f)
             self._map.copymap.pop(f, None)
@@ -637,7 +635,7 @@ 
             for f in self._updatedfiles:
                 e = dmap.get(f)
                 if e is not None and e[0] == 'n' and e[3] == now:
-                    dmap[f] = dirstatetuple(e[0], e[1], e[2], -1)
+                    dmap.addfile(f, e[0], e[1], e[2], -1)
                     self._map.nonnormalset.add(f)
 
             # emulate that all 'dirstate.normal' results are written out
@@ -1245,22 +1243,40 @@ 
     def __contains__(self, key):
         return key in self._map
 
-    def __setitem__(self, key, value):
-        self._map[key] = value
-
     def __getitem__(self, key):
         return self._map[key]
 
-    def __delitem__(self, key):
-        del self._map[key]
-
     def keys(self):
         return self._map.keys()
 
     def preload(self):
         """Loads the underlying data, if it's not already loaded"""
         self._map
 
+    def addfile(self, f, state, mode, size, mtime):
+        """Add a tracked file to the dirstate."""
+        self._map[f] = dirstatetuple(state, mode, size, mtime)
+
+    def removefile(self, f, size):
+        """
+        Mark a file as removed in the dirstate.
+
+        The `size` parameter is used to store sentinel values that indicate
+        the file's previous state.  In the future, we should refactor this
+        to be more explicit about what that state is.
+        """
+        self._map[f] = dirstatetuple('r', 0, size, 0)
+
+    def dropfile(self, f):
+        """
+        Remove a file from the dirstate.  Returns True if the file was
+        previously recorded.
+        """
+        exists = f in self._map
+        if exists:
+            del self._map[f]
+        return exists
+
     def nonnormalentries(self):
         '''Compute the nonnormal dirstate entries from the dmap'''
         try:
@@ -1390,8 +1406,6 @@ 
         # Avoid excess attribute lookups by fast pathing certain checks
         self.__contains__ = self._map.__contains__
         self.__getitem__ = self._map.__getitem__
-        self.__setitem__ = self._map.__setitem__
-        self.__delitem__ = self._map.__delitem__
         self.get = self._map.get
 
     def write(self, st, now):