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

login
register
mail settings
Submitter via Mercurial-devel
Date July 21, 2017, 5:25 a.m.
Message ID <CAESOdVCE+hajA17H8p-pmcsCJN_Ti9RDQQvr9JdTJLKfLmhepw@mail.gmail.com>
Download mbox | patch
Permalink /patch/22538/
State Superseded
Headers show

Comments

via Mercurial-devel - July 21, 2017, 5:25 a.m.
On Thu, Jul 20, 2017 at 4:40 PM, Jun Wu <quark@fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1500594002 25200
> #      Thu Jul 20 16:40:02 2017 -0700
> # Branch stable
> # Node ID 3253102e6e5222a08737e7204eb672ab12e80764
> # Parent  a41e0f1c9b69330a0dd8d6590787faa38d11c0ff
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 3253102e6e52
> 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.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -438,4 +438,14 @@ class rebaseruntime(object):
>                                    'to commit\n') % (rev, ctx))
>                          self.skipped.add(rev)
> +                        # Move bookmark to destination. This cannot be handled
> +                        # by scmutil.cleanupnodes since it will treat rev as
> +                        # removed (no successor) and move bookmark backwards.
> +                        destnode = repo[self.dest].node()
> +                        bmchanges = [(name, destnode)
> +                                     for name in repo.nodebookmarks(ctx.node())]
> +                        if bmchanges:
> +                            with repo.transaction('rebase') as tr1:
> +                                repo._bookmarks.applychanges(repo, tr1,
> +                                                             bmchanges)


This feels like the wrong place to me. The other bookmark moves are
done later. It also doesn't work with --keep (bookmarks are not
normally moved with --keep). How about something like this on top:
Jun Wu - July 21, 2017, 10:30 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-07-20 22:25:20 -0700:
> On Thu, Jul 20, 2017 at 4:40 PM, Jun Wu <quark@fb.com> wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1500594002 25200
> > #      Thu Jul 20 16:40:02 2017 -0700
> > # Branch stable
> > # Node ID 3253102e6e5222a08737e7204eb672ab12e80764
> > # Parent  a41e0f1c9b69330a0dd8d6590787faa38d11c0ff
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 3253102e6e52
> > 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.
> >
> > diff --git a/hgext/rebase.py b/hgext/rebase.py
> > --- a/hgext/rebase.py
> > +++ b/hgext/rebase.py
> > @@ -438,4 +438,14 @@ class rebaseruntime(object):
> >                                    'to commit\n') % (rev, ctx))
> >                          self.skipped.add(rev)
> > +                        # Move bookmark to destination. This cannot be handled
> > +                        # by scmutil.cleanupnodes since it will treat rev as
> > +                        # removed (no successor) and move bookmark backwards.
> > +                        destnode = repo[self.dest].node()
> > +                        bmchanges = [(name, destnode)
> > +                                     for name in repo.nodebookmarks(ctx.node())]
> > +                        if bmchanges:
> > +                            with repo.transaction('rebase') as tr1:
> > +                                repo._bookmarks.applychanges(repo, tr1,
> > +                                                             bmchanges)
> 
> 
> This feels like the wrong place to me. The other bookmark moves are
> done later. It also doesn't work with --keep (bookmarks are not
> normally moved with --keep). How about something like this on top:

This is better. Since you have written the code. Could you send a patch?
Otherwise I can shamelessly steal your code in V2.

> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -437,16 +437,6 @@ class rebaseruntime(object):
>                          ui.warn(_('note: rebase of %d:%s created no changes '
>                                    'to commit\n') % (rev, ctx))
>                          self.skipped.add(rev)
> -                        # Move bookmark to destination. This cannot be handled
> -                        # by scmutil.cleanupnodes since it will treat rev as
> -                        # removed (no successor) and move bookmark backwards.
> -                        destnode = repo[self.dest].node()
> -                        bmchanges = [(name, destnode)
> -                                     for name in
> repo.nodebookmarks(ctx.node())]
> -                        if bmchanges:
> -                            with repo.transaction('rebase') as tr1:
> -                                repo._bookmarks.applychanges(repo, tr1,
> -                                                             bmchanges)
>                      self.state[rev] = p1
>                      ui.debug('next revision set to %s\n' % p1)
>              elif self.state[rev] == nullmerge:
> @@ -522,7 +512,8 @@ class rebaseruntime(object):
>              collapsedas = None
>              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)
>          clearcollapsemsg(repo)
> @@ -1311,12 +1302,21 @@ def buildstate(repo, dest, rebaseset, co
>              state[r] = revprecursor
>      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
> 
>      If `collapsedas` is not None, the rebase was a collapse whose result if the
>      `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()):
>          if newrev >= 0 and newrev != rev:
> diff --git a/tests/test-rebase-emptycommit.t b/tests/test-rebase-emptycommit.t
> --- a/tests/test-rebase-emptycommit.t
> +++ b/tests/test-rebase-emptycommit.t
> @@ -27,6 +27,18 @@
>    |/
>    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
via Mercurial-devel - July 22, 2017, 1:04 a.m.
Feel free to take my patch. I can't queue my own patch anyway so it'll be
more efficient if you send it and I queue it.

On Jul 21, 2017 15:30, "Jun Wu" <quark@fb.com> wrote:

> Excerpts from Martin von Zweigbergk's message of 2017-07-20 22:25:20 -0700:
> > On Thu, Jul 20, 2017 at 4:40 PM, Jun Wu <quark@fb.com> wrote:
> > > # HG changeset patch
> > > # User Jun Wu <quark@fb.com>
> > > # Date 1500594002 25200
> > > #      Thu Jul 20 16:40:02 2017 -0700
> > > # Branch stable
> > > # Node ID 3253102e6e5222a08737e7204eb672ab12e80764
> > > # Parent  a41e0f1c9b69330a0dd8d6590787faa38d11c0ff
> > > # Available At https://bitbucket.org/quark-zju/hg-draft
> > > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r
> 3253102e6e52
> > > 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.
> > >
> > > diff --git a/hgext/rebase.py b/hgext/rebase.py
> > > --- a/hgext/rebase.py
> > > +++ b/hgext/rebase.py
> > > @@ -438,4 +438,14 @@ class rebaseruntime(object):
> > >                                    'to commit\n') % (rev, ctx))
> > >                          self.skipped.add(rev)
> > > +                        # Move bookmark to destination. This cannot
> be handled
> > > +                        # by scmutil.cleanupnodes since it will treat
> rev as
> > > +                        # removed (no successor) and move bookmark
> backwards.
> > > +                        destnode = repo[self.dest].node()
> > > +                        bmchanges = [(name, destnode)
> > > +                                     for name in
> repo.nodebookmarks(ctx.node())]
> > > +                        if bmchanges:
> > > +                            with repo.transaction('rebase') as tr1:
> > > +                                repo._bookmarks.applychanges(repo,
> tr1,
> > > +
>  bmchanges)
> >
> >
> > This feels like the wrong place to me. The other bookmark moves are
> > done later. It also doesn't work with --keep (bookmarks are not
> > normally moved with --keep). How about something like this on top:
>
> This is better. Since you have written the code. Could you send a patch?
> Otherwise I can shamelessly steal your code in V2.
>
> > diff --git a/hgext/rebase.py b/hgext/rebase.py
> > --- a/hgext/rebase.py
> > +++ b/hgext/rebase.py
> > @@ -437,16 +437,6 @@ class rebaseruntime(object):
> >                          ui.warn(_('note: rebase of %d:%s created no
> changes '
> >                                    'to commit\n') % (rev, ctx))
> >                          self.skipped.add(rev)
> > -                        # Move bookmark to destination. This cannot be
> handled
> > -                        # by scmutil.cleanupnodes since it will treat
> rev as
> > -                        # removed (no successor) and move bookmark
> backwards.
> > -                        destnode = repo[self.dest].node()
> > -                        bmchanges = [(name, destnode)
> > -                                     for name in
> > repo.nodebookmarks(ctx.node())]
> > -                        if bmchanges:
> > -                            with repo.transaction('rebase') as tr1:
> > -                                repo._bookmarks.applychanges(repo, tr1,
> > -                                                             bmchanges)
> >                      self.state[rev] = p1
> >                      ui.debug('next revision set to %s\n' % p1)
> >              elif self.state[rev] == nullmerge:
> > @@ -522,7 +512,8 @@ class rebaseruntime(object):
> >              collapsedas = None
> >              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)
> >          clearcollapsemsg(repo)
> > @@ -1311,12 +1302,21 @@ def buildstate(repo, dest, rebaseset, co
> >              state[r] = revprecursor
> >      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
> >
> >      If `collapsedas` is not None, the rebase was a collapse whose
> result if the
> >      `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()):
> >          if newrev >= 0 and newrev != rev:
> > diff --git a/tests/test-rebase-emptycommit.t b/tests/test-rebase-
> emptycommit.t
> > --- a/tests/test-rebase-emptycommit.t
> > +++ b/tests/test-rebase-emptycommit.t
> > @@ -27,6 +27,18 @@
> >    |/
> >    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
>

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -437,16 +437,6 @@  class rebaseruntime(object):
                         ui.warn(_('note: rebase of %d:%s created no changes '
                                   'to commit\n') % (rev, ctx))
                         self.skipped.add(rev)
-                        # Move bookmark to destination. This cannot be handled
-                        # by scmutil.cleanupnodes since it will treat rev as
-                        # removed (no successor) and move bookmark backwards.
-                        destnode = repo[self.dest].node()
-                        bmchanges = [(name, destnode)
-                                     for name in
repo.nodebookmarks(ctx.node())]
-                        if bmchanges:
-                            with repo.transaction('rebase') as tr1:
-                                repo._bookmarks.applychanges(repo, tr1,
-                                                             bmchanges)
                     self.state[rev] = p1
                     ui.debug('next revision set to %s\n' % p1)
             elif self.state[rev] == nullmerge:
@@ -522,7 +512,8 @@  class rebaseruntime(object):
             collapsedas = None
             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)
         clearcollapsemsg(repo)
@@ -1311,12 +1302,21 @@  def buildstate(repo, dest, rebaseset, co
             state[r] = revprecursor
     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

     If `collapsedas` is not None, the rebase was a collapse whose result if the
     `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()):
         if newrev >= 0 and newrev != rev:
diff --git a/tests/test-rebase-emptycommit.t b/tests/test-rebase-emptycommit.t
--- a/tests/test-rebase-emptycommit.t
+++ b/tests/test-rebase-emptycommit.t
@@ -27,6 +27,18 @@ 
   |/
   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