Patchwork D1341: dirstate: move management of nonnormal sets into dirstate map

login
register
mail settings
Submitter phabricator
Date Nov. 8, 2017, 5:31 p.m.
Message ID <differential-rev-PHID-DREV-y74zjo5vzva4eow6traj-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25425/
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
  The dirstate map owns the nonnormal sets, and so should be the class to update
  them.  A future implementation of the dirstate will manage these maps
  differently.
  
  The action of clearing ambiguous times is now entirely controlled by the
  dirstate map, so it moves there too.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/dirstate.py

CHANGE DETAILS




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

INLINE COMMENTS

> dirstate.py:1247
>          self._map[f] = dirstatetuple(state, mode, size, mtime)
> +        if state != 'n' or mtime == -1:
> +            self.nonnormalset.add(f)

I would prefer to see all mutations to `self._map` go through a common code path so that we can override this behavior easier in Eden.

As it stands, when this logic is conflated, it makes it much harder for us to safely subclass `dirstatemap` in Eden. For reference, here's what we're doing today:

https://github.com/facebookexperimental/eden-hg/blob/master/eden/hg/eden/eden_dirstate_map.py

REPOSITORY
  rHG Mercurial

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

To: mbthomas, #hg-reviewers
Cc: mbolin, mercurial-devel
phabricator - Nov. 10, 2017, 10:08 p.m.
durin42 added a comment.


  I strongly suspect that (if you agree with my feedback on https://phab.mercurial-scm.org/D1340) this will need some adjustments to document the expanded behavior of dirstatemap WRT nonnormalset (whatever a nonnormalset is)

REPOSITORY
  rHG Mercurial

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

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

INLINE COMMENTS

> mbolin wrote in dirstate.py:1247
> I would prefer to see all mutations to `self._map` go through a common code path so that we can override this behavior easier in Eden.
> 
> As it stands, when this logic is conflated, it makes it much harder for us to safely subclass `dirstatemap` in Eden. For reference, here's what we're doing today:
> 
> https://github.com/facebookexperimental/eden-hg/blob/master/eden/hg/eden/eden_dirstate_map.py

I think what you want is a common tuple constructor hook point - eden_dirstate_map does build a different tuple, but it just sets it as a property of the map in the same way.  I think we want to avoid another layer of indirection if we can.

Arguably `dirstatetuple` is already one of these, as it can be either a native tuple type, or the fast C-based one from parses.c, but maybe that's the wrong place for eden to hook as it's global to the module.  Instead, we can do it by setting `dirstatemap._tuplecls` to `dirstatetuple` in the constructor, so the code here becomes more like:

  self._map[f] = self._tuplecls(state, mode, size, mtime)

In eden you can make your own function and assign that to `self._tuplecls` in `eden_dirstate_map.__init__`.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -415,10 +415,6 @@ 
             self._map.dirs.addpath(f)
         self._dirty = True
         self._updatedfiles.add(f)
-        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):
@@ -490,7 +486,6 @@ 
                 elif entry[0] == 'n' and entry[2] == -2: # other parent
                     size = -2
                     self._map.otherparentset.add(f)
-        self._map.nonnormalset.add(f)
         self._map.removefile(f, size)
         if size == 0:
             self._map.copymap.pop(f, None)
@@ -506,8 +501,6 @@ 
         if self._map.dropfile(f):
             self._dirty = True
             self._droppath(f)
-            if f in self._map.nonnormalset:
-                self._map.nonnormalset.remove(f)
             self._map.copymap.pop(f, None)
 
     def _discoverpath(self, path, normed, ignoremissing, exists, storemap):
@@ -631,12 +624,7 @@ 
 
             # emulate dropping timestamp in 'parsers.pack_dirstate'
             now = _getfsnow(self._opener)
-            dmap = self._map
-            for f in self._updatedfiles:
-                e = dmap.get(f)
-                if e is not None and e[0] == 'n' and e[3] == now:
-                    dmap.addfile(f, e[0], e[1], e[2], -1)
-                    self._map.nonnormalset.add(f)
+            self._map.clearambiguoustimes(self._updatedfiles, now)
 
             # emulate that all 'dirstate.normal' results are written out
             self._lastnormaltime = 0
@@ -1256,6 +1244,10 @@ 
     def addfile(self, f, state, mode, size, mtime):
         """Add a tracked file to the dirstate."""
         self._map[f] = dirstatetuple(state, mode, size, mtime)
+        if state != 'n' or mtime == -1:
+            self.nonnormalset.add(f)
+        if size == -2:
+            self.otherparentset.add(f)
 
     def removefile(self, f, size):
         """
@@ -1266,6 +1258,7 @@ 
         to be more explicit about what that state is.
         """
         self._map[f] = dirstatetuple('r', 0, size, 0)
+        self.nonnormalset.add(f)
 
     def dropfile(self, f):
         """
@@ -1275,8 +1268,16 @@ 
         exists = f in self._map
         if exists:
             del self._map[f]
+        self.nonnormalset.discard(f)
         return exists
 
+    def clearambiguoustimes(self, files, now):
+        for f in files:
+            e = self.get(f)
+            if e is not None and e[0] == 'n' and e[3] == now:
+                self._map[f] = dirstatetuple(e[0], e[1], e[2], -1)
+                self.nonnormalset.add(f)
+
     def nonnormalentries(self):
         '''Compute the nonnormal dirstate entries from the dmap'''
         try: