[PATCH 1 of 3] dirstate: add begin/endparentchange to dirstate

[Mads Kiilerich]

On 09/05/2014 09:36 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham at fb.com>
> # Date 1409942069 25200
> #      Fri Sep 05 11:34:29 2014 -0700
> # Node ID 8c6d502c92c8ed8dfa79b8fac4c8a2cc9e6c4153
> # Parent  188b8aa2120b03eead618ba150319074f4e3b42b
> 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.
>
> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
> --- a/mercurial/dirstate.py
> +++ b/mercurial/dirstate.py
> @@ -44,6 +44,17 @@
>           self._lastnormaltime = 0
>           self._ui = ui
>           self._filecache = {}
> +        self._parentwriters = 0
> +
> +    def beginparentchange(self):
> +        self._parentwriters += 1
> +
> +    def endparentchange(self):
> +        if self._parentwriters > 0:
> +            self._parentwriters -= 1

The primary use case for this might be parent changes but it seems like 
it is a general transaction concept that should have a more generic name.

> +
> +    def pendingparentchange(self):
> +        return self._parentwriters > 0
>   
>       @propertycache
>       def _map(self):
> @@ -300,6 +311,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
> @@ -1108,7 +1108,11 @@
>               return l
>   
>           def unlock():
> -            self.dirstate.write()
> +            if self.dirstate.pendingparentchange():
> +                self.dirstate.invalidate()
> +            else:
> +                self.dirstate.write()
> +

Shouldn't this "don't write with pending parent change" live inside 
dirstate? Why expose it?

I also wonder if it would be a good idea to have some built-in 
verification that the count reaches 0 unless we are in an exceptional 
case. That would probably require some way of indicating whether it is a 
normal or exceptional unlock we are doing ... which perhaps could be 
used by itself instead of this semaphore. Hmm ... I don't know where 
this is going. I will keep wondering.

/Mads