Patchwork [STABLE,V2] rebase: fix rebase aborts when 'tip-1' is public (issue4082)

login
register
mail settings
Submitter Durham Goode
Date Nov. 5, 2013, 6:14 p.m.
Message ID <4e5cb8149929ed1b2afa.1383675291@dev350.prn1.facebook.com>
Download mbox | patch
Permalink /patch/2862/
State Accepted
Commit 7d5e7799a29f97b261e08dcd585b83bdff127c6b
Headers show

Comments

Durham Goode - Nov. 5, 2013, 6:14 p.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 4e5cb8149929ed1b2afa1fc44564429a31a9fc51
# Parent  7c4cf8367673e3100cda62927b096cdd9497a409
rebase: fix rebase aborts when 'tip-1' is public (issue4082)

When aborting a rebase where tip-1 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.
Augie Fackler - Nov. 5, 2013, 6:33 p.m.
On Tue, Nov 05, 2013 at 10:14:51AM -0800, 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 4e5cb8149929ed1b2afa1fc44564429a31a9fc51
> # Parent  7c4cf8367673e3100cda62927b096cdd9497a409
> rebase: fix rebase aborts when 'tip-1' is public (issue4082)

Looks superficially reasonable, but I'd like someone else to take a
look at this too.

>
> When aborting a rebase where tip-1 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]
>      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 ValueError(_('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,46 @@
>
>
>    $ cd ..
> +
> +rebase abort should not leave working copy in a merge state if tip-1 is public
> +(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 log -G --template "{rev} {desc} {bookmarks}"
> +  @  3 C foo
> +  |
> +  | o  2 c master
> +  | |
> +  o |  1 b
> +  |/
> +  o  0 a
> +
> +
> +  $ 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} {bookmarks}"
> +  @  3 C foo
> +  |
> +  | o  2 c master
> +  | |
> +  o |  1 b
> +  |/
> +  o  0 a
> +
> +  $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Siddharth Agarwal - Nov. 6, 2013, 3:19 a.m.
On 11/05/2013 10:14 AM, 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 4e5cb8149929ed1b2afa1fc44564429a31a9fc51
> # Parent  7c4cf8367673e3100cda62927b096cdd9497a409
> rebase: fix rebase aborts when 'tip-1' is public (issue4082)

Looks good to me.
Augie Fackler - Nov. 6, 2013, 5:50 p.m.
On Tue, Nov 05, 2013 at 07:19:42PM -0800, Siddharth Agarwal wrote:
> On 11/05/2013 10:14 AM, 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 4e5cb8149929ed1b2afa1fc44564429a31a9fc51
> ># Parent  7c4cf8367673e3100cda62927b096cdd9497a409
> >rebase: fix rebase aborts when 'tip-1' is public (issue4082)
>
> Looks good to me.

Okay, then I'll queue it.

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

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]
     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 ValueError(_('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,46 @@ 
   
 
   $ cd ..
+
+rebase abort should not leave working copy in a merge state if tip-1 is public
+(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 log -G --template "{rev} {desc} {bookmarks}"
+  @  3 C foo
+  |
+  | o  2 c master
+  | |
+  o |  1 b
+  |/
+  o  0 a
+  
+
+  $ 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} {bookmarks}"
+  @  3 C foo
+  |
+  | o  2 c master
+  | |
+  o |  1 b
+  |/
+  o  0 a
+  
+  $ cd ..