Patchwork [stable] rebase: derive node from target rev (issue3802)

login
register
mail settings
Submitter Siddharth Agarwal
Date Feb. 3, 2013, 10:30 p.m.
Message ID <6c45e953b3f4a12ed8de.1359930651@sid0x220>
Download mbox | patch
Permalink /patch/797/
State Accepted
Commit 12de53323e59ce3e5f31472ee781649669d6ca9c
Headers show

Comments

Siddharth Agarwal - Feb. 3, 2013, 10:30 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1359930399 28800
# Node ID 6c45e953b3f4a12ed8deedea3adeac8e80fc04e0
# Parent  d29cfda81aece14f39cd9d3f87d30b08d1c05fc0
rebase: derive node from target rev (issue3802)

dest.rev() is the same as target when a new rebase is run, but dest isn't set
when rebase --continue is run.
Pierre-Yves David - Feb. 3, 2013, 11:40 p.m.
On Sun, Feb 03, 2013 at 02:30:51PM -0800, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1359930399 28800
> # Node ID 6c45e953b3f4a12ed8deedea3adeac8e80fc04e0
> # Parent  d29cfda81aece14f39cd9d3f87d30b08d1c05fc0
> rebase: derive node from target rev (issue3802)

You should mention the changeset that introduced the regression.

> dest.rev() is the same as target when a new rebase is run, but dest isn't set
> when rebase --continue is run.

Patch looks ugly but correct. The rebase code probably need additional cleanup
and documentation. But as you said on IRC the fix for stable should be simpler.

However, It seems like your fix could be smaller.


> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -316,6 +316,9 @@ def rebase(ui, repo, **opts):
>              for k, v in state.iteritems():
>                  if v > nullmerge:
>                      nstate[repo[k].node()] = repo[v].node()
> +            # XXX this is the same as dest.node() for the non-continue path --
> +            # this should probably be cleaned up
> +            targetnode = repo[target].node()

Can't that be just `dest = repo[target]` that would remove needs to update
other code part. making the change smaller and safer.

I just had a look at the "function" you are patching and it size and complexity
are really scary.
Siddharth Agarwal - Feb. 3, 2013, 11:47 p.m.
On 02/03/2013 03:40 PM, Pierre-Yves David wrote:
> Can't that be just `dest = repo[target]` that would remove needs to update
> other code part. making the change smaller and safer.

That was the other way to fix this I had in mind. I mildly prefer this 
because that would have one path derive target from dest and one derive 
dest from target, and I thought that was even more confusing than this.

> I just had a look at the "function" you are patching and it size and complexity
> are really scary.

Absolutely.
Pierre-Yves David - Feb. 4, 2013, 8:15 p.m.
On Sun, Feb 03, 2013 at 03:47:29PM -0800, Siddharth Agarwal wrote:
> On 02/03/2013 03:40 PM, Pierre-Yves David wrote:
> >Can't that be just `dest = repo[target]` that would remove needs to update
> >other code part. making the change smaller and safer.
> 
> That was the other way to fix this I had in mind. I mildly prefer
> this because that would have one path derive target from dest and
> one derive dest from target, and I thought that was even more
> confusing than this.

meh, given the current state of the rebase code I would have prefered a patch
that change as few as possible of existing code.

The regression here is serious enough.

Kevin seems to have crewed it as is anyway.

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -316,6 +316,9 @@  def rebase(ui, repo, **opts):
             for k, v in state.iteritems():
                 if v > nullmerge:
                     nstate[repo[k].node()] = repo[v].node()
+            # XXX this is the same as dest.node() for the non-continue path --
+            # this should probably be cleaned up
+            targetnode = repo[target].node()
 
         if not keepf:
             collapsedas = None
@@ -324,7 +327,7 @@  def rebase(ui, repo, **opts):
             clearrebased(ui, repo, state, skipped, collapsedas)
 
         if currentbookmarks:
-            updatebookmarks(repo, dest, nstate, currentbookmarks)
+            updatebookmarks(repo, targetnode, nstate, currentbookmarks)
 
         clearstatus(repo)
         ui.note(_("rebase completed\n"))
@@ -501,15 +504,14 @@  def updatemq(repo, state, skipped, **opt
         mq.seriesdirty = True
         mq.savedirty()
 
-def updatebookmarks(repo, dest, nstate, originalbookmarks):
+def updatebookmarks(repo, targetnode, nstate, originalbookmarks):
     'Move bookmarks to their correct changesets, and delete divergent ones'
-    destnode = dest.node()
     marks = repo._bookmarks
     for k, v in originalbookmarks.iteritems():
         if v in nstate:
             # update the bookmarks for revs that have moved
             marks[k] = nstate[v]
-            bookmarks.deletedivergent(repo, [destnode], k)
+            bookmarks.deletedivergent(repo, [targetnode], k)
 
     marks.write()
 
diff --git a/tests/test-rebase-bookmarks.t b/tests/test-rebase-bookmarks.t
--- a/tests/test-rebase-bookmarks.t
+++ b/tests/test-rebase-bookmarks.t
@@ -128,4 +128,34 @@  Keep active bookmark on the correct chan
   o  0: 'A' bookmarks:
   
 
-  $ cd ..
+rebase --continue with bookmarks present (issue3802)
+
+  $ hg up 2
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ echo 'C' > c
+  $ hg add c
+  $ hg ci -m 'other C'
+  created new head
+  $ hg up 3
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg rebase
+  merging c
+  warning: conflicts during merge.
+  merging c incomplete! (edit conflicts, then use 'hg resolve --mark')
+  abort: unresolved conflicts (see hg resolve, then hg rebase --continue)
+  [255]
+  $ echo 'c' > c
+  $ hg resolve --mark c
+  $ hg rebase --continue
+  saved backup bundle to $TESTTMP/a3/.hg/strip-backup/3d5fa227f4b5-backup.hg (glob)
+  $ hg tglog
+  @  4: 'C' bookmarks: Y Z
+  |
+  o  3: 'other C' bookmarks:
+  |
+  o  2: 'B' bookmarks: X
+  |
+  o  1: 'D' bookmarks: W
+  |
+  o  0: 'A' bookmarks:
+