Patchwork [2,of,2] rebase: clear merge when aborting before any rebasing (issue4661)

login
register
mail settings
Submitter Jordi Gutiérrez Hermoso
Date May 10, 2015, 3:35 p.m.
Message ID <fa727819042b3400cf94.1431272157@Iris>
Download mbox | patch
Permalink /patch/9004/
State Accepted
Delegated to: Matt Mackall
Headers show

Comments

Jordi Gutiérrez Hermoso - May 10, 2015, 3:35 p.m.
# HG changeset patch
# User Jordi Gutiérrez Hermoso <jordigh@octave.org>
# Date 1431269844 14400
#      Sun May 10 10:57:24 2015 -0400
# Node ID fa727819042b3400cf94454020b0fb74e37a4b25
# Parent  9829fcd55faa029b4d4773ac3939cbaa05352322
rebase: clear merge when aborting before any rebasing (issue4661)

The check of the inrebase function was not correct, and it failed to
consider the situation in which nothing has been rebased yet, *and*
the working dir had been updated away from the initial revision.

But this is easy to fix. Given the rebase state, we know exactly where
we should be standing: on the first unrebased commit. We check that
instead. I also took the liberty to rename the function, as "inrebase"
doesn't really describe the situation: we could still be in a rebase
state yet the user somehow forcibly updated to a different revision.

We also check that we're in a merge state, since an interrupted merge
is the only "safe" way to interrupt a rebase. If the rebase got
interrupted by power loss or whatever (so there's no merge state),
it's still safer to not blow away the working directory.
Pierre-Yves David - May 11, 2015, 7:49 a.m.
On 05/10/2015 08:35 AM, Jordi Gutiérrez Hermoso wrote:
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1431269844 14400
> #      Sun May 10 10:57:24 2015 -0400
> # Node ID fa727819042b3400cf94454020b0fb74e37a4b25
> # Parent  9829fcd55faa029b4d4773ac3939cbaa05352322
> rebase: clear merge when aborting before any rebasing (issue4661)

The series looks good to me. I'm also super happy to see this bug fixed.

However this series may breach a couple of the rule for the stable branch:

- add another, not so related, test,
- contains a non-strictly necessary change (function rename).

As my vision is a bit blurred by my joy of seeing this fixed, I'll let 
Matt make the call.
Matt Mackall - May 14, 2015, 8:05 p.m.
On Sun, 2015-05-10 at 11:35 -0400, Jordi Gutiérrez Hermoso wrote:
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1431269844 14400
> #      Sun May 10 10:57:24 2015 -0400
> # Node ID fa727819042b3400cf94454020b0fb74e37a4b25
> # Parent  9829fcd55faa029b4d4773ac3939cbaa05352322
> rebase: clear merge when aborting before any rebasing (issue4661)

These are queued for stable, thanks.

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -841,16 +841,21 @@  def restorestatus(repo):
             raise
         raise util.Abort(_('no rebase in progress'))
 
-def inrebase(repo, originalwd, state):
-    '''check whether the working dir is in an interrupted rebase'''
+def needupdate(repo, state):
+    '''check whether we should `update --clean` away from a merge, or if
+    somehow the working dir got forcibly updated, e.g. by older hg'''
     parents = [p.rev() for p in repo.parents()]
-    if originalwd in parents:
+
+    # Are we in a merge state at all?
+    if len(parents) < 2:
+        return False
+
+    # We should be standing on the first as-of-yet unrebased commit.
+    firstunrebased = min([old for old, new in state.iteritems()
+                          if new == nullrev])
+    if firstunrebased in parents:
         return True
 
-    for newrev in state.itervalues():
-        if newrev in parents:
-            return True
-
     return False
 
 def abort(repo, originalwd, target, state, activebookmark=None):
@@ -877,7 +882,7 @@  def abort(repo, originalwd, target, stat
 
     if cleanup:
         # Update away from the rebase if necessary
-        if inrebase(repo, originalwd, state):
+        if needupdate(repo, state):
             merge.update(repo, originalwd, False, True, False)
 
         # Strip from the first rebased revision
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
@@ -288,3 +288,36 @@  user has somehow managed to update to a 
   new
 
   $ cd ..
+
+On the other hand, make sure we *do* clobber changes whenever we
+haven't somehow managed to update the repo to a different revision
+during a rebase (issue4661)
+
+  $ hg ini yesupdate
+  $ cd yesupdate
+  $ echo "initial data" > foo.txt
+  $ hg add
+  adding foo.txt
+  $ hg ci -m "initial checkin"
+  $ echo "change 1" > foo.txt
+  $ hg ci -m "change 1"
+  $ hg up 0
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ echo "conflicting change 1" > foo.txt
+  $ hg ci -m "conflicting 1"
+  created new head
+  $ echo "conflicting change 2" > foo.txt
+  $ hg ci -m "conflicting 2"
+
+  $ hg rebase -d 1 --tool 'internal:fail'
+  rebasing 2:e4ea5cdc9789 "conflicting 1"
+  unresolved conflicts (see hg resolve, then hg rebase --continue)
+  [1]
+  $ hg rebase --abort
+  rebase aborted
+  $ hg summary
+  parent: 3:b16646383533 tip
+   conflicting 2
+  branch: default
+  commit: (clean)
+  update: 1 new changesets, 2 branch heads (merge)