Patchwork [V2] rebase: move bookmark to destination for commits becoming empty (issue5627)

login
register
mail settings
Submitter Jun Wu
Date July 22, 2017, 1:18 a.m.
Message ID <74b3107a9d9a22a38f09.1500686319@x1c>
Download mbox | patch
Permalink /patch/22542/
State Accepted
Headers show

Comments

Jun Wu - July 22, 2017, 1:18 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1500686190 25200
#      Fri Jul 21 18:16:30 2017 -0700
# Branch stable
# Node ID 74b3107a9d9a22a38f09abaa0267d7ddb9165af9
# Parent  20436925e0803a98566c934aa3433fc75b6d2704
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 74b3107a9d9a
rebase: move bookmark to destination for commits becoming empty (issue5627)

When rebasing a changeset X and that changeset becomes empty, we should move
the bookmark on X to rebase destination.

This is a regression caused by scmutil.cleanupnodes refactoring.

Thanks Martin von Zweigbergk for providing improved path that works with
--keep!
via Mercurial-devel - July 22, 2017, 4:15 a.m.
On Fri, Jul 21, 2017 at 6:18 PM, Jun Wu <quark@fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1500686190 25200
> #      Fri Jul 21 18:16:30 2017 -0700
> # Branch stable
> # Node ID 74b3107a9d9a22a38f09abaa0267d7ddb9165af9
> # Parent  20436925e0803a98566c934aa3433fc75b6d2704
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 74b3107a9d9a
> rebase: move bookmark to destination for commits becoming empty (issue5627)
>

Queued, thanks!
via Mercurial-devel - July 24, 2017, 4:29 p.m.
On Fri, Jul 21, 2017 at 9:15 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Fri, Jul 21, 2017 at 6:18 PM, Jun Wu <quark@fb.com> wrote:
>> # HG changeset patch
>> # User Jun Wu <quark@fb.com>
>> # Date 1500686190 25200
>> #      Fri Jul 21 18:16:30 2017 -0700
>> # Branch stable
>> # Node ID 74b3107a9d9a22a38f09abaa0267d7ddb9165af9
>> # Parent  20436925e0803a98566c934aa3433fc75b6d2704
>> # Available At https://bitbucket.org/quark-zju/hg-draft
>> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 74b3107a9d9a
>> rebase: move bookmark to destination for commits becoming empty (issue5627)
>>
>
> Queued, thanks!

I started wondering what should happen with bookmarks that are not on
pruned commits at the bottom om the stack. Consider the following
case:

  o  4 D
  |
  | o  3 D BOOK
  | |
  | o  2 C
  | |
  o |  1 B
  |/
  o  0 A

  $ hg rebase -s 2 -d 4


The resulting graph will be

  o  3 C
  |
  o  2 D
  |
  o  1 B
  |
  o  0 A

but where should the bookmark go? Before your cleanupnodes() commit,
it went on 3, but after the cleanupnodes() commit and this patch (the
one I'm replying to), it will go on 2. I think the previous behavior
was better. How do you want to proceed?
Jun Wu - July 24, 2017, 11:47 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-07-24 09:29:09 -0700:
> On Fri, Jul 21, 2017 at 9:15 PM, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
> > On Fri, Jul 21, 2017 at 6:18 PM, Jun Wu <quark@fb.com> wrote:
> >> # HG changeset patch
> >> # User Jun Wu <quark@fb.com>
> >> # Date 1500686190 25200
> >> #      Fri Jul 21 18:16:30 2017 -0700
> >> # Branch stable
> >> # Node ID 74b3107a9d9a22a38f09abaa0267d7ddb9165af9
> >> # Parent  20436925e0803a98566c934aa3433fc75b6d2704
> >> # Available At https://bitbucket.org/quark-zju/hg-draft 
> >> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 74b3107a9d9a
> >> rebase: move bookmark to destination for commits becoming empty (issue5627)
> >>
> >
> > Queued, thanks!
> 
> I started wondering what should happen with bookmarks that are not on
> pruned commits at the bottom om the stack. Consider the following
> case:
> 
>   o  4 D
>   |
>   | o  3 D BOOK
>   | |
>   | o  2 C
>   | |
>   o |  1 B
>   |/
>   o  0 A
> 
>   $ hg rebase -s 2 -d 4
> 
> 
> The resulting graph will be
> 
>   o  3 C
>   |
>   o  2 D
>   |
>   o  1 B
>   |
>   o  0 A
> 
> but where should the bookmark go? Before your cleanupnodes() commit,
> it went on 3, but after the cleanupnodes() commit and this patch (the
> one I'm replying to), it will go on 2. I think the previous behavior
> was better. How do you want to proceed?

Good case.  I agree the previous behavior is better, but the current
behavior also looks okay to me. I think we can reuse `adjusteddest` in D21
to cleanly solve this (and other corner cases).
Jun Wu - July 25, 2017, 1:03 a.m.
Per discussion with Adam, it's breaking user expectation that bookmarks are
used to identify feature branches (not to identify a single commit), and is
not that rare with rebase.rebaseskipobsolete disabled (we do currently). So
I'll send a patch.

We also discovered many subtle behaviors that might be worth to reconsider.
But let's discuss them after freeze.

I saw that this patch is not public yet. So feel free to discard this.

Excerpts from Jun Wu's message of 2017-07-24 16:47:00 -0700:
> Good case.  I agree the previous behavior is better, but the current
> behavior also looks okay to me. I think we can reuse `adjusteddest` in D21
> to cleanly solve this (and other corner cases).
via Mercurial-devel - July 25, 2017, 1:30 a.m.
On Jul 24, 2017 6:03 PM, "Jun Wu" <quark@fb.com> wrote:

Per discussion with Adam, it's breaking user expectation that bookmarks are
used to identify feature branches (not to identify a single commit), and is
not that rare with rebase.rebaseskipobsolete disabled (we do currently). So
I'll send a patch.


That was my impression too, so thanks for sending a patch.


We also discovered many subtle behaviors that might be worth to reconsider.
But let's discuss them after freeze.

I saw that this patch is not public yet. So feel free to discard this.


Yeah, I actually thought I had pushed it. I discovered this morning that I
had not, but then I had also discovered the regression, so I figured I'd
wait.


Excerpts from Jun Wu's message of 2017-07-24 16:47:00 -0700:
> Good case.  I agree the previous behavior is better, but the current
> behavior also looks okay to me. I think we can reuse `adjusteddest` in D21
> to cleanly solve this (and other corner cases).

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -513,5 +513,6 @@  class rebaseruntime(object):
             if self.collapsef:
                 collapsedas = newnode
-            clearrebased(ui, repo, self.state, self.skipped, collapsedas)
+            clearrebased(ui, repo, self.dest, self.state, self.skipped,
+                         collapsedas)
 
         clearstatus(repo)
@@ -1302,5 +1303,5 @@  def buildstate(repo, dest, rebaseset, co
     return originalwd, dest.rev(), state
 
-def clearrebased(ui, repo, state, skipped, collapsedas=None):
+def clearrebased(ui, repo, dest, state, skipped, collapsedas=None):
     """dispose of rebased revision at the end of the rebase
 
@@ -1308,4 +1309,13 @@  def clearrebased(ui, repo, state, skippe
     `collapsedas` node."""
     tonode = repo.changelog.node
+    # Move bookmark of skipped nodes to destination. This cannot be handled
+    # by scmutil.cleanupnodes since it will treat rev as removed (no successor)
+    # and move bookmark backwards.
+    bmchanges = [(name, tonode(dest))
+                 for rev in skipped
+                 for name in repo.nodebookmarks(tonode(rev))]
+    if bmchanges:
+        with repo.transaction('rebase') as tr:
+            repo._bookmarks.applychanges(repo, tr, bmchanges)
     mapping = {}
     for rev, newrev in sorted(state.items()):
diff --git a/tests/test-rebase-emptycommit.t b/tests/test-rebase-emptycommit.t
new file mode 100644
--- /dev/null
+++ b/tests/test-rebase-emptycommit.t
@@ -0,0 +1,52 @@ 
+  $ cat >> $HGRCPATH<<EOF
+  > [extensions]
+  > rebase=
+  > drawdag=$TESTDIR/drawdag.py
+  > EOF
+
+  $ hg init
+  $ hg debugdrawdag<<'EOS'
+  > B C
+  > |/
+  > A
+  > EOS
+
+  $ hg debugdrawdag<<'EOS'
+  > C
+  > |
+  > B
+  > EOS
+
+  $ hg bookmark -r 2 -i BOOK
+  $ hg log -G -T '{rev} {desc} {bookmarks}'
+  o  3 C
+  |
+  | o  2 C BOOK
+  | |
+  o |  1 B
+  |/
+  o  0 A
+  
+  $ hg rebase -r 2 -d 3 --keep
+  rebasing 2:dc0947a82db8 "C" (BOOK)
+  note: rebase of 2:dc0947a82db8 created no changes to commit
+  $ hg log -G -T '{rev} {desc} {bookmarks}'
+  o  3 C
+  |
+  | o  2 C BOOK
+  | |
+  o |  1 B
+  |/
+  o  0 A
+  
+  $ hg rebase -r 2 -d 3
+  rebasing 2:dc0947a82db8 "C" (BOOK)
+  note: rebase of 2:dc0947a82db8 created no changes to commit
+  saved backup bundle to $TESTTMP/.hg/strip-backup/dc0947a82db8-d21b92a4-rebase.hg (glob)
+  $ hg log -G -T '{rev} {desc} {bookmarks}'
+  o  2 C BOOK
+  |
+  o  1 B
+  |
+  o  0 A
+