Patchwork D7827: rebase: don't use rebased node as dirstate p2 (BC)

login
register
mail settings
Submitter phabricator
Date Jan. 11, 2020, 6:48 a.m.
Message ID <differential-rev-PHID-DREV-vsl2rsoksepxb2ilthkc-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44255/
State Superseded
Headers show

Comments

phabricator - Jan. 11, 2020, 6:48 a.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  When rebasing a node, we currently use the rebased node as p2 in the
  dirstate until just before we commit it (we then change to the desired
  parents). This p2 is visible to the user when the rebase gets
  interrupted because of merge conflicts. That can be useful to the user
  as a reminder of which commit is currently being rebased, but I
  believe it's incorrect for a few reasons:
  
  - I think the dirstate parents should be the ones that will be set when the commit is created.
  - I think having two parents means that you're merging those two commits, but when rebasing, you're generally grafting, not merging.
  - When rebasing a merge commit, we should use the two desired parents as dirstate parents (and we clearly can't have the rebased node as a third dirstate parent).
  - `hg graft` (and `hg update --merge`) sets only one parent and `hg rebase` should be consistent with that.
  
  I realize that this is a somewhat large user-visible change, but I
  think it's worth it because it will simplify things quite a bit.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D7827

AFFECTED FILES
  hgext/rebase.py
  relnotes/next
  tests/test-rebase-abort.t
  tests/test-rebase-collapse.t
  tests/test-rebase-conflicts.t
  tests/test-rebase-interruptions.t
  tests/test-rebase-obsolete.t
  tests/test-rebase-parameters.t
  tests/test-rebase-transaction.t

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - April 1, 2020, 6:08 p.m.
martinvonz added a comment.


  Heads up that we ran into a fun bug that was probably caused by this patch. I think what happened was this:
  
  1. User ran with an older hg, which sets two dirstate parents while rebasing
  2. User ran into a merge conflict and was dropped out to their shell
  3. User upgraded hg to the new version (with this patch)
  4. User ran `hg rebase --continue` with the new version, which respects the dirstate parents and thus creates a merge commit (!)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7827/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7827

To: martinvonz, #hg-reviewers, durin42
Cc: mjpieters, mercurial-devel

Patch

diff --git a/tests/test-rebase-transaction.t b/tests/test-rebase-transaction.t
--- a/tests/test-rebase-transaction.t
+++ b/tests/test-rebase-transaction.t
@@ -114,7 +114,7 @@ 
   |
   | @  4: Z
   | |
-  @ |  3: C
+  o |  3: C
   | |
   | o  2: Y
   | |
@@ -123,9 +123,9 @@ 
   o  0: A
   
   $ hg st
-  M C
   M conflict
   A B
+  A C
   ? conflict.orig
   $ echo resolved > conflict
   $ hg resolve -m
diff --git a/tests/test-rebase-parameters.t b/tests/test-rebase-parameters.t
--- a/tests/test-rebase-parameters.t
+++ b/tests/test-rebase-parameters.t
@@ -473,11 +473,9 @@ 
   $ hg summary
   parent: 1:56daeba07f4b 
    c2
-  parent: 2:e4e3f3546619 tip
-   c2b
   branch: default
-  commit: 1 modified, 1 unresolved (merge)
-  update: (current)
+  commit: 1 unresolved (clean)
+  update: 1 new changesets, 2 branch heads (merge)
   phases: 3 draft
   rebase: 0 rebased, 1 remaining (rebase --continue)
 
diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t
+++ b/tests/test-rebase-obsolete.t
@@ -1795,19 +1795,15 @@ 
   $ hg log -G
   @  2:b18e25de2cf5 D
   |
-  | @  1:2ec65233581b B (pruned using prune)
-  |/
   o  0:426bada5c675 A
   
   $ hg summary
   parent: 2:b18e25de2cf5 tip
    D
-  parent: 1:2ec65233581b  (obsolete)
-   B
   branch: default
-  commit: 2 modified, 1 unknown, 1 unresolved (merge)
+  commit: 1 modified, 1 added, 1 unknown, 1 unresolved
   update: (current)
-  phases: 3 draft
+  phases: 2 draft
   rebase: 0 rebased, 2 remaining (rebase --continue)
 
   $ hg rebase --abort
diff --git a/tests/test-rebase-interruptions.t b/tests/test-rebase-interruptions.t
--- a/tests/test-rebase-interruptions.t
+++ b/tests/test-rebase-interruptions.t
@@ -294,7 +294,7 @@ 
   $ hg tglogp
   @  7: 401ccec5e39f secret 'C'
   |
-  | @  6: a0b2430ebfb8 secret 'F'
+  | o  6: a0b2430ebfb8 secret 'F'
   | |
   o |  5: 45396c49d53b public 'B'
   | |
@@ -345,7 +345,7 @@ 
   $ hg tglogp
   @  7: 401ccec5e39f secret 'C'
   |
-  | @  6: a0b2430ebfb8 secret 'F'
+  | o  6: a0b2430ebfb8 secret 'F'
   | |
   o |  5: 45396c49d53b public 'B'
   | |
@@ -395,7 +395,7 @@ 
   $ hg tglogp
   @  7: 401ccec5e39f secret 'C'
   |
-  | @  6: a0b2430ebfb8 secret 'F'
+  | o  6: a0b2430ebfb8 secret 'F'
   | |
   o |  5: 45396c49d53b public 'B'
   | |
diff --git a/tests/test-rebase-conflicts.t b/tests/test-rebase-conflicts.t
--- a/tests/test-rebase-conflicts.t
+++ b/tests/test-rebase-conflicts.t
@@ -456,16 +456,14 @@ 
   warning: conflicts while merging conflict! (edit, then use 'hg resolve --mark')
   unresolved conflicts (see hg resolve, then hg rebase --continue)
   [1]
-It's weird that the current parents are not 7 and 8 since that's what we're
-merging
   $ hg tglog
   @  8:draft 'E'
   |
-  | o  7:draft 'D'
+  | @  7:draft 'D'
   |/
   o  6:draft 'C'
   |
-  | @    5:draft 'F'
+  | o    5:draft 'F'
   | |\
   | | o  4:draft 'E'
   | | |
diff --git a/tests/test-rebase-collapse.t b/tests/test-rebase-collapse.t
--- a/tests/test-rebase-collapse.t
+++ b/tests/test-rebase-collapse.t
@@ -755,7 +755,7 @@ 
   |
   | @  2: 82b8abf9c185 'D'
   | |
-  @ |  1: f899f3910ce7 'B'
+  o |  1: f899f3910ce7 'B'
   |/
   o  0: 4a2df7238c3b 'A'
   
@@ -779,7 +779,7 @@ 
   unresolved conflicts (see hg resolve, then hg rebase --continue)
   [1]
   $ hg tglog
-  @  3: 63668d570d21 'C'
+  o  3: 63668d570d21 'C'
   |
   | @  2: 82b8abf9c185 'D'
   | |
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
@@ -236,7 +236,7 @@ 
   [1]
 
   $ hg tglog
-  @  4:draft 'C1'
+  o  4:draft 'C1'
   |
   o  3:draft 'B bis'
   |
diff --git a/relnotes/next b/relnotes/next
--- a/relnotes/next
+++ b/relnotes/next
@@ -14,6 +14,8 @@ 
 
 == Backwards Compatibility Changes ==
 
+ * When `hg rebase` pauses for merge conflict resolution, the working copy will
+   no longer have the rebased node as a second parent.
 
 == Internal API Changes ==
 
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -617,6 +617,7 @@ 
                         repo,
                         rev,
                         p1,
+                        p2,
                         base,
                         self.collapsef,
                         dest,
@@ -641,10 +642,6 @@ 
                 newnode = self._concludenode(rev, p1, p2, editor)
             else:
                 # Skip commit if we are collapsing
-                if self.inmemory:
-                    self.wctx.setbase(repo[p1])
-                else:
-                    repo.setparents(repo[p1].node())
                 newnode = None
             # Update the state
             if newnode is not None:
@@ -1468,7 +1465,7 @@ 
         return newnode
 
 
-def rebasenode(repo, rev, p1, base, collapse, dest, wctx):
+def rebasenode(repo, rev, p1, p2, base, collapse, dest, wctx):
     """Rebase a single revision rev on top of p1 using base as merge ancestor"""
     # Merge phase
     # Update to destination and merge it with local
@@ -1499,6 +1496,7 @@ 
         labels=[b'dest', b'source'],
         wc=wctx,
     )
+    wctx.setparents(repo[p1].node(), repo[p2].node())
     if collapse:
         copies.duplicatecopies(repo, wctx, rev, dest)
     else: