Patchwork [STABLE] rebase: fix rebase aborts when tip^ is public (issue4082)

login
register
mail settings
Submitter Durham Goode
Date Nov. 5, 2013, 4:09 a.m.
Message ID <f3a3d9b9243ccdc3fdcf.1383624552@dev350.prn1.facebook.com>
Download mbox | patch
Permalink /patch/2849/
State Accepted
Headers show

Comments

Durham Goode - Nov. 5, 2013, 4:09 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1383623940 28800
#      Mon Nov 04 19:59:00 2013 -0800
# Branch stable
# Node ID f3a3d9b9243ccdc3fdcf07f76547098863ae83c3
# Parent  7c4cf8367673e3100cda62927b096cdd9497a409
rebase: fix rebase aborts when tip^ is public (issue4082)

When aborting a rebase where tip^ is public, rebase would fail to undo the merge
state. This caused unexpected dirstate parents and also caused unshelve to
become unabortable (since it uses rebase under the hood).

The problem was that rebase uses -2 as a marker rev, and when it checked for
immutableness during the abort, -2 got resolved to the second to last entry in
the phase cache.

Adds a test for the fix. Add exception to phase code to prevent this in the
future.
Siddharth Agarwal - Nov. 5, 2013, 5:49 a.m.
On 11/04/2013 08:09 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1383623940 28800
> #      Mon Nov 04 19:59:00 2013 -0800
> # Branch stable
> # Node ID f3a3d9b9243ccdc3fdcf07f76547098863ae83c3
> # Parent  7c4cf8367673e3100cda62927b096cdd9497a409
> rebase: fix rebase aborts when tip^ is public (issue4082)

I think this would be rev -2 (i.e. rev number 1 less than tip), which 
isn't always tip^.
Martin Geisler - Nov. 5, 2013, 6:40 a.m.
Durham Goode <durham@fb.com> writes:

> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1383623940 28800
> #      Mon Nov 04 19:59:00 2013 -0800
> # Branch stable
> # Node ID f3a3d9b9243ccdc3fdcf07f76547098863ae83c3
> # Parent  7c4cf8367673e3100cda62927b096cdd9497a409
> rebase: fix rebase aborts when tip^ is public (issue4082)
>
> When aborting a rebase where tip^ is public, rebase would fail to undo
> the merge state. This caused unexpected dirstate parents and also
> caused unshelve to become unabortable (since it uses rebase under the
> hood).
>
> The problem was that rebase uses -2 as a marker rev, and when it
> checked for immutableness during the abort, -2 got resolved to the
> second to last entry in the phase cache.
>
> Adds a test for the fix. Add exception to phase code to prevent this
> in the future.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -692,7 +692,7 @@
>  
>  def abort(repo, originalwd, target, state):
>      'Restore the repository to its original state'
> -    dstates = [s for s in state.values() if s != nullrev]
> +    dstates = [s for s in state.values() if s != nullrev and s != nullmerge]
>      immutable = [d for d in dstates if not repo[d].mutable()]
>      cleanup = True
>      if immutable:
> diff --git a/mercurial/phases.py b/mercurial/phases.py
> --- a/mercurial/phases.py
> +++ b/mercurial/phases.py
> @@ -185,6 +185,8 @@
>          # be replaced without us being notified.
>          if rev == nullrev:
>              return public
> +        if rev < nullrev:
> +            raise error.RepoLookupError(_('cannot lookup negative revision'))

Will some earlier part of the code turn -2 into a positive revision
number before we hit this? I would like 'hg phase -r -2' to keep
working.
Pierre-Yves David - Nov. 5, 2013, 9:16 a.m.
On 5 nov. 2013, at 05:09, Durham Goode wrote:

> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1383623940 28800
> #      Mon Nov 04 19:59:00 2013 -0800
> # Branch stable
> # Node ID f3a3d9b9243ccdc3fdcf07f76547098863ae83c3
> # Parent  7c4cf8367673e3100cda62927b096cdd9497a409
> rebase: fix rebase aborts when tip^ is public (issue4082)

-2 is not necessarily tip^,

> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -692,7 +692,7 @@
> 
> def abort(repo, originalwd, target, state):
>     'Restore the repository to its original state'
> -    dstates = [s for s in state.values() if s != nullrev]
> +    dstates = [s for s in state.values() if s != nullrev and s != nullmerge]

elsewhere in rebase we use `s > nullrev`

> --- a/mercurial/phases.py
> +++ b/mercurial/phases.py
> @@ -185,6 +185,8 @@
>         # be replaced without us being notified.
>         if rev == nullrev:
>             return public
> +        if rev < nullrev:
> +            raise error.RepoLookupError(_('cannot lookup negative revision'))
>         if self._phaserevs is None or rev >= len(self._phaserevs):
>             self._phaserevs = self.getphaserevs(repo, rebuild=True)
>         return self._phaserevs[rev]

I'm ok with such change, but it belong to its own patch. I'm not convinced RepoLookupError is right here. ValueError seems better as rev < -1 are out of the scope (or IndexError maybe).

> diff --git a/tests/test-rebase-abort.t b/tests/test-rebase-abort.t
> --- a/tests/test-rebase-abort.t
> +++ b/tests/test-rebase-abort.t
> @@ -181,3 +181,34 @@
> 
> 
>   $ cd ..
> +
> +rebase abort should leave the working copy as it was (issue4082)

Useful test but sightly related to issue4082. You may be a bit more verbose in your description. In particular about the fact phase matter.

> +
> +  $ hg init abortpublic
> +  $ cd abortpublic
> +  $ echo a > a && hg ci -Aqm a
> +  $ hg book master
> +  $ hg book foo
> +  $ echo b > b && hg ci -Aqm b
> +  $ hg up -q master
> +  $ echo c > c && hg ci -Aqm c
> +  $ hg phase -p -r .
> +  $ hg up -q foo
> +  $ echo C > c && hg ci -Aqm C
> +  $ hg rebase -d master -r foo
> +  merging c
> +  warning: conflicts during merge.
> +  merging c incomplete! (edit conflicts, then use 'hg resolve --mark')
> +  unresolved conflicts (see hg resolve, then hg rebase --continue)
> +  [1]
> +  $ hg rebase --abort
> +  rebase aborted
> +  $ hg log -G --template "{rev} {desc}"
> +  @  3 C
> +  |
> +  | o  2 c
> +  | |
> +  o |  1 b
> +  |/
> +  o  0 a
> +  

You should use a template that includes {bookmarks} as you spend significant effort to have some in your repo.

You should run the same log command before the rebase for easy comparison.
Siddharth Agarwal - Nov. 5, 2013, 4:15 p.m.
On 11/04/2013 10:40 PM, Martin Geisler wrote:
> Will some earlier part of the code turn -2 into a positive revision
> number before we hit this? I would like 'hg phase -r -2' to keep
> working.

http://selenic.com/repo/hg/file/d24ee6d7d167/mercurial/commands.py#l4477 
most likely.
Pierre-Yves David - Nov. 5, 2013, 4:27 p.m.
On 5 nov. 2013, at 07:40, Martin Geisler wrote:

> Durham Goode <durham@fb.com> writes:
> 
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1383623940 28800
>> #      Mon Nov 04 19:59:00 2013 -0800
>> # Branch stable
>> # Node ID f3a3d9b9243ccdc3fdcf07f76547098863ae83c3
>> # Parent  7c4cf8367673e3100cda62927b096cdd9497a409
>> rebase: fix rebase aborts when tip^ is public (issue4082)
>> 
>> When aborting a rebase where tip^ is public, rebase would fail to undo
>> the merge state. This caused unexpected dirstate parents and also
>> caused unshelve to become unabortable (since it uses rebase under the
>> hood).
>> 
>> The problem was that rebase uses -2 as a marker rev, and when it
>> checked for immutableness during the abort, -2 got resolved to the
>> second to last entry in the phase cache.
>> 
>> Adds a test for the fix. Add exception to phase code to prevent this
>> in the future.
>> 
>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>> --- a/hgext/rebase.py
>> +++ b/hgext/rebase.py
>> @@ -692,7 +692,7 @@
>> 
>> def abort(repo, originalwd, target, state):
>>     'Restore the repository to its original state'
>> -    dstates = [s for s in state.values() if s != nullrev]
>> +    dstates = [s for s in state.values() if s != nullrev and s != nullmerge]
>>     immutable = [d for d in dstates if not repo[d].mutable()]
>>     cleanup = True
>>     if immutable:
>> diff --git a/mercurial/phases.py b/mercurial/phases.py
>> --- a/mercurial/phases.py
>> +++ b/mercurial/phases.py
>> @@ -185,6 +185,8 @@
>>         # be replaced without us being notified.
>>         if rev == nullrev:
>>             return public
>> +        if rev < nullrev:
>> +            raise error.RepoLookupError(_('cannot lookup negative revision'))
> 
> Will some earlier part of the code turn -2 into a positive revision
> number before we hit this? I would like 'hg phase -r -2' to keep
> working.

This function already check for -1 so it takes strict revision number only.
Durham Goode - Nov. 5, 2013, 5:49 p.m.
On 11/4/13 10:40 PM, "Martin Geisler" <martin@geisler.net> wrote:

>Durham Goode <durham@fb.com> writes:
>
>>--- a/mercurial/phases.py
>> +++ b/mercurial/phases.py
>> @@ -185,6 +185,8 @@
>>          # be replaced without us being notified.
>>          if rev == nullrev:
>>              return public
>> +        if rev < nullrev:
>> +            raise error.RepoLookupError(_('cannot lookup negative
>>revision'))
>
>Will some earlier part of the code turn -2 into a positive revision
>number before we hit this? I would like 'hg phase -r -2' to keep
>working.


hg phase -r -2 will continue to work.  The command line revset resolver
turns that -2 into a positive rev before passing it to the actual command.
 This function in phases.py should never, ever get a negative number
because it's indexing into an array that doesn't have the nullrev in it,
so a negative number will always be off by one and return the wrong phase
in this function.
Durham Goode - Nov. 5, 2013, 5:59 p.m.
On 11/5/13 1:16 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:

>On 5 nov. 2013, at 05:09, Durham Goode wrote:
>
>> --- a/mercurial/phases.py
>> +++ b/mercurial/phases.py
>> @@ -185,6 +185,8 @@
>>         # be replaced without us being notified.
>>         if rev == nullrev:
>>             return public
>> +        if rev < nullrev:
>> +            raise error.RepoLookupError(_('cannot lookup negative
>>revision'))
>>         if self._phaserevs is None or rev >= len(self._phaserevs):
>>             self._phaserevs = self.getphaserevs(repo, rebuild=True)
>>         return self._phaserevs[rev]
>
>I'm ok with such change, but it belong to its own patch. I'm not
>convinced RepoLookupError is right here. ValueError seems better as rev <
>-1 are out of the scope (or IndexError maybe).

What is the benefit of putting it in it's own patch? It doesn't make the
changes any more understandable (they're already tiny), and it's highly
related to the other changes being made, so someone looking back in
history would probably want to see all of them together.  I'm not a fan of
micro-patches for the sake of micro-patches and feel they clutter the
history when done unecessarily.
Martin Geisler - Nov. 6, 2013, 7:52 a.m.
Durham Goode <durham@fb.com> writes:

> On 11/4/13 10:40 PM, "Martin Geisler" <martin@geisler.net> wrote:
>
>>Durham Goode <durham@fb.com> writes:
>>
>>>--- a/mercurial/phases.py
>>> +++ b/mercurial/phases.py
>>> @@ -185,6 +185,8 @@
>>>          # be replaced without us being notified.
>>>          if rev == nullrev:
>>>              return public
>>> +        if rev < nullrev:
>>> +            raise error.RepoLookupError(_('cannot lookup negative
>>>revision'))
>>
>> Will some earlier part of the code turn -2 into a positive revision
>> number before we hit this? I would like 'hg phase -r -2' to keep
>> working.
>
>
> hg phase -r -2 will continue to work. The command line revset resolver
> turns that -2 into a positive rev before passing it to the actual
> command. This function in phases.py should never, ever get a negative
> number because it's indexing into an array that doesn't have the
> nullrev in it, so a negative number will always be off by one and
> return the wrong phase in this function.

Super, thanks for checking that for me!

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -692,7 +692,7 @@ 
 
 def abort(repo, originalwd, target, state):
     'Restore the repository to its original state'
-    dstates = [s for s in state.values() if s != nullrev]
+    dstates = [s for s in state.values() if s != nullrev and s != nullmerge]
     immutable = [d for d in dstates if not repo[d].mutable()]
     cleanup = True
     if immutable:
diff --git a/mercurial/phases.py b/mercurial/phases.py
--- a/mercurial/phases.py
+++ b/mercurial/phases.py
@@ -185,6 +185,8 @@ 
         # be replaced without us being notified.
         if rev == nullrev:
             return public
+        if rev < nullrev:
+            raise error.RepoLookupError(_('cannot lookup negative revision'))
         if self._phaserevs is None or rev >= len(self._phaserevs):
             self._phaserevs = self.getphaserevs(repo, rebuild=True)
         return self._phaserevs[rev]
diff --git a/tests/test-rebase-abort.t b/tests/test-rebase-abort.t
--- a/tests/test-rebase-abort.t
+++ b/tests/test-rebase-abort.t
@@ -181,3 +181,34 @@ 
   
 
   $ cd ..
+
+rebase abort should leave the working copy as it was (issue4082)
+
+  $ hg init abortpublic
+  $ cd abortpublic
+  $ echo a > a && hg ci -Aqm a
+  $ hg book master
+  $ hg book foo
+  $ echo b > b && hg ci -Aqm b
+  $ hg up -q master
+  $ echo c > c && hg ci -Aqm c
+  $ hg phase -p -r .
+  $ hg up -q foo
+  $ echo C > c && hg ci -Aqm C
+  $ hg rebase -d master -r foo
+  merging c
+  warning: conflicts during merge.
+  merging c incomplete! (edit conflicts, then use 'hg resolve --mark')
+  unresolved conflicts (see hg resolve, then hg rebase --continue)
+  [1]
+  $ hg rebase --abort
+  rebase aborted
+  $ hg log -G --template "{rev} {desc}"
+  @  3 C
+  |
+  | o  2 c
+  | |
+  o |  1 b
+  |/
+  o  0 a
+