Patchwork [1,of,4,V3] dirstate: add begin/endparentchange to dirstate

login
register
mail settings
Submitter Durham Goode
Date Sept. 9, 2014, 8:21 p.m.
Message ID <b647743bc6fb059c05c2.1410294087@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/5756/
State Accepted
Headers show

Comments

Durham Goode - Sept. 9, 2014, 8:21 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1409942069 25200
#      Fri Sep 05 11:34:29 2014 -0700
# Node ID b647743bc6fb059c05c2e075b2d71d4bdb2654cf
# Parent  897041f6b025778193c6da5b9795da09a91c866e
dirstate: add begin/endparentchange to dirstate

It's possible for the dirstate to become incoherent (issue4353) if there is an
exception in the middle of the dirstate parent and entries being written (like
if the user ctrl+c's). This change adds begin/endparentchange which a future
patch will require to be set before changing the dirstate parent.  This will
allow us to prevent writing the dirstate in the event of an exception while
changing the parent.
Pierre-Yves David - Sept. 11, 2014, 1:08 p.m.
On 09/09/2014 10:21 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1409942069 25200
> #      Fri Sep 05 11:34:29 2014 -0700
> # Node ID b647743bc6fb059c05c2e075b2d71d4bdb2654cf
> # Parent  897041f6b025778193c6da5b9795da09a91c866e
> dirstate: add begin/endparentchange to dirstate
>
> It's possible for the dirstate to become incoherent (issue4353) if there is an
> exception in the middle of the dirstate parent and entries being written (like
> if the user ctrl+c's). This change adds begin/endparentchange which a future
> patch will require to be set before changing the dirstate parent.  This will
> allow us to prevent writing the dirstate in the event of an exception while
> changing the parent.

This series was bugging me because it linked the dirstate change to the 
lock. Linking such related concept to the lock seems like a mistake.

However reading it again, I realize that the dirstate writing is 
-already- attached to the lock and this series just makes is sightly 
less awful. So +1

We should aims to detach dirstate from the lock at some point.

Patch

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -44,6 +44,30 @@ 
         self._lastnormaltime = 0
         self._ui = ui
         self._filecache = {}
+        self._parentwriters = 0
+
+    def beginparentchange(self):
+        '''Marks the beginning of a set of changes that involve changing
+        the dirstate parents. If there is an exception during this time,
+        the dirstate will not be written when the wlock is released. This
+        prevents writing an incoherent dirstate where the parent doesn't
+        match the contents.
+        '''
+        self._parentwriters += 1
+
+    def endparentchange(self):
+        '''Marks the end of a set of changes that involve changing the
+        dirstate parents. Once all parent changes have been marked done,
+        the wlock will be free to write the dirstate on release.
+        '''
+        if self._parentwriters > 0:
+            self._parentwriters -= 1
+
+    def pendingparentchange(self):
+        '''Returns true if the dirstate is in the middle of a set of changes
+        that modify the dirstate parent.
+        '''
+        return self._parentwriters > 0
 
     @propertycache
     def _map(self):
@@ -300,6 +324,7 @@ 
                 delattr(self, a)
         self._lastnormaltime = 0
         self._dirty = False
+        self._parentwriters = 0
 
     def copy(self, source, dest):
         """Mark dest as a copy of source. Unmark dest if source is None."""
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1102,7 +1102,11 @@ 
             return l
 
         def unlock():
-            self.dirstate.write()
+            if self.dirstate.pendingparentchange():
+                self.dirstate.invalidate()
+            else:
+                self.dirstate.write()
+
             self._filecache['dirstate'].refresh()
 
         l = self._lock(self.vfs, "wlock", wait, unlock,