Patchwork D1354: dirstate: change all writes to dirstatemap._map to go through one method.

login
register
mail settings
Submitter phabricator
Date Nov. 10, 2017, 1:33 a.m.
Message ID <differential-rev-PHID-DREV-mahhkecprrkgasw6qj6n-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25452/
State Superseded
Headers show

Comments

phabricator - Nov. 10, 2017, 1:33 a.m.
mbolin created this revision.
mbolin added reviewers: mbthomas, durham.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This separates some concerns that were introduced in
  https://phab.mercurial-scm.org/D1341.
  
  In particular, this makes it easier for `eden_dirstate` to provide its own
  implementation, which incidentally, does not use `dirstatetuple`.

TEST PLAN
  Ran this against a complementary change in Eden and verified that all of Eden's
  integration tests pass.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  mercurial/dirstate.py

CHANGE DETAILS




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


  I'm not really happy with this. Why can't eden be replacing the dirstatemap entirely, and be intercepting things at that layer?

REPOSITORY
  rHG Mercurial

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

To: mbolin, mbthomas, durham, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Nov. 13, 2017, 11:43 p.m.
mbolin added a comment.


  @durin42 The issue is that `dirstatemap` is doing so much "stuff" beyond just storage that Eden prefers to subclass it rather than copy/paste all of the business logic.

REPOSITORY
  rHG Mercurial

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

To: mbolin, mbthomas, durham, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Nov. 14, 2017, 12:02 a.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D1354#22991, @mbolin wrote:
  
  > @durin42 The issue is that `dirstatemap` is doing so much "stuff" beyond just storage that Eden prefers to subclass it rather than copy/paste all of the business logic.
  
  
  That's an argument for further splitting dirstatemap. That said, does https://phab.mercurial-scm.org/D1347 resolve the specific concern that motivated this change?

REPOSITORY
  rHG Mercurial

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

To: mbolin, mbthomas, durham, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Dec. 5, 2017, 10:36 p.m.
durin42 added a comment.


  Is this still relevant? Nobody's answered my question.

REPOSITORY
  rHG Mercurial

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

To: mbolin, mbthomas, durham, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Dec. 7, 2017, 9:16 p.m.
mbolin abandoned this revision.
mbolin added a comment.


  https://phab.mercurial-scm.org/D1347 did not address the issue because we still want to subclass it in Eden. If you're curious, you can see exactly what we're doing in https://github.com/facebookexperimental/eden-hg/blob/master/eden/hg/eden/eden_dirstate_map.py.
  
  Regardless, we'll find another way, so I'll abandon this.

REPOSITORY
  rHG Mercurial

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

To: mbolin, mbthomas, durham, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Dec. 7, 2017, 9:22 p.m.
durin42 added a comment.


  If you want, I'd be happy to make the dirstatetuple factory a class-level attribute on the dirstatemap - I'm not quite following what the extra logic you need is, but I'd very much like to make Eden easier, you just need to help me understand the constraints better: I can't read your mind.

REPOSITORY
  rHG Mercurial

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

To: mbolin, mbthomas, durham, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Dec. 11, 2017, 7:06 p.m.
mbolin added a comment.


  @durin42 Here's the implementation of this method we are using in Eden:
  
    def _insert_tuple(self, filename, state, mode, size, mtime):  # override
        if size != MERGE_STATE_BOTH_PARENTS and size != MERGE_STATE_OTHER_PARENT:
            merge_state = MERGE_STATE_NOT_APPLICABLE
        else:
            merge_state = size
    
        self._map[filename] = (state, mode, merge_state)
  
  As you can see, we want to do some conditional logic before doing the insert into `self._map` (and we end up dropping one of the tuple fields), so it seemed like the most straightforward thing for Eden to do is to intercept the writes. It's true that we could also redefine `dirstatetuple()`, but I think that's more confusing because there are places where we need to convert between Eden's internal tuples stored in `eden_dirstate_map` and the `dirstatetuple` type that Mercurial expects everywhere else, so I'm disinclined to conflate the two things.

REPOSITORY
  rHG Mercurial

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

To: mbolin, mbthomas, durham, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Dec. 11, 2017, 7:11 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D1354#28409, @mbolin wrote:
  
  > @durin42 Here's the implementation of this method we are using in Eden:
  >
  >   def _insert_tuple(self, filename, state, mode, size, mtime):  # override
  >       if size != MERGE_STATE_BOTH_PARENTS and size != MERGE_STATE_OTHER_PARENT:
  >           merge_state = MERGE_STATE_NOT_APPLICABLE
  >       else:
  >           merge_state = size
  >   
  >       self._map[filename] = (state, mode, merge_state)
  >   
  >
  > As you can see, we want to do some conditional logic before doing the insert into `self._map` (and we end up dropping one of the tuple fields), so it seemed like the most straightforward thing for Eden to do is to intercept the writes. It's true that we could also redefine `dirstatetuple()`, but I think that's more confusing because there are places where we need to convert between Eden's internal tuples stored in `eden_dirstate_map` and the `dirstatetuple` type that Mercurial expects everywhere else, so I'm disinclined to conflate the two things.
  
  
  Ahhhh. That was extremely not obvious from your commit message. If it'd still help, I'd be happy to land this with a clearer log message and a comment on `_insert_tuple` that it's been extracted to ease the implementation of hg on FUSE filesystems and other similar use cases (is that a good summary?) so that it makes more sense to readers...

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -1235,7 +1235,7 @@ 
             self._dirs.addpath(f)
         if oldstate == "?" and "_alldirs" in self.__dict__:
             self._alldirs.addpath(f)
-        self._map[f] = dirstatetuple(state, mode, size, mtime)
+        self._insert_tuple(f, state, mode, size, mtime)
         if state != 'n' or mtime == -1:
             self.nonnormalset.add(f)
         if size == -2:
@@ -1256,7 +1256,7 @@ 
         if "filefoldmap" in self.__dict__:
             normed = util.normcase(f)
             self.filefoldmap.pop(normed, None)
-        self._map[f] = dirstatetuple('r', 0, size, 0)
+        self._insert_tuple(f, 'r', 0, size, 0)
         self.nonnormalset.add(f)
 
     def dropfile(self, f, oldstate):
@@ -1281,9 +1281,12 @@ 
         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._insert_tuple(f, e[0], e[1], e[2], -1)
                 self.nonnormalset.add(f)
 
+    def _insert_tuple(self, f, state, mode, size, mtime):
+        self._map[f] = dirstatetuple(state, mode, size, mtime)
+
     def nonnormalentries(self):
         '''Compute the nonnormal dirstate entries from the dmap'''
         try: