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

[Martin von Zweigbergk]

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 at 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 at fb.com> wrote:
> > > # HG changeset patch
> > > # User Jun Wu <quark at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170721/65a8befc/attachment-0002.html>