[PATCH 2 of 6 V2] localrepo: restore dirstate to one before rollbacking if not parent-gone

[Pierre-Yves David]

On 10/13/2015 10:54 AM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
> # Date 1444758556 -32400
> #      Wed Oct 14 02:49:16 2015 +0900
> # Node ID 04f94f9be97fcff64587440aa1e1ec3afb4f7639
> # Parent  b441136da58f3542910d71e9ad3353340ce8fb90
> localrepo: restore dirstate to one before rollbacking if not parent-gone
>
> 'localrepository.rollback()' explicilty restores dirstate, only if at
> least one of current parents of the working directory is removed at
> rollbacking (a.k.a "parent-gone").
>
> After DirstateTransactionPlan, 'dirstate.write()' will cause marking
> '.hg/dirstate' as a file to be restored at rollbacking.
>
>      https://mercurial.selenic.com/wiki/DirstateTransactionPlan
>
> Then, 'transaction.rollback()' restores '.hg/dirstate' regardless of
> parents of the working directory at that time, and this causes
> unexpected dirstate changes if not "parent-gone" (e.g.  "hg update" to
> another branch after "hg commit" or so, then "hg rollback").
>
> To avoid such situation, this patch restores dirstate to one before
> rollbacking if not "parent-gone".
>
>    before:
>      b1. restore dirstate explicitly, if "parent-gone"
>
>    after:
>      a1. save dirstate before actual rollbacking via dirstateguard
>      a2. restore dirstate via 'transaction.rollback()'
>      a3. if "parent-gone"
>          - discard backup (a1)
>          - restore dirstate from 'undo.dirstate'
>      a4. otherwise, restore dirstate from backup (a1)
>
> Even though restoring dirstate at (a3) after (a2) seems redundant,
> this patch keeps this existing code path, because:
>
>    - it isn't ensured that 'dirstate.write()' was invoked at least once
>      while transaction running
>
>      If not, '.hg/dirstate' isn't restored at (a2).
>
>      In addition to it, rude 3rd party extension invoking
>      'dirstate.write()' without 'repo' while transaction running (see
>      subsequent patches for detail) may break consistency of a file
>      backup-ed by transaction.
>
>    - this patch mainly focuses on changes for DirstateTransactionPlan
>
>      Restoring dirstate at (a3) itself should be cheaper enough than
>      rollbacking itself. Redundancy will be removed in next step.
>
> Newly added test is almost meaningless at this point. It will be used
> to detect regression while implementing delayed dirstate write out.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1084,20 +1084,24 @@
>               lock.release()
>
>       def rollback(self, dryrun=False, force=False):
> -        wlock = lock = None
> +        wlock = lock = dsguard = None
>           try:
>               wlock = self.wlock()
>               lock = self.lock()
>               if self.svfs.exists("undo"):
> -                return self._rollback(dryrun, force)
> +                # delay importing avoids cyclic dependency
> +                from cmdutil import dirstateguard

I'm not sure why you need this. I've dropped it and everything seems fine.

-- 
Pierre-Yves David