Patchwork [2,of,3] rebase: choose default destination the same way as 'hg merge' (BC)

login
register
mail settings
Submitter Pierre-Yves David
Date Feb. 15, 2016, 10:49 a.m.
Message ID <1b45c38d02ddca7ff763.1455533355@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/13196/
State Superseded
Commit fac3a24be50e7c4c97757d19f725fa1342db156e
Delegated to: Yuya Nishihara
Headers show

Comments

Pierre-Yves David - Feb. 15, 2016, 10:49 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1455456359 0
#      Sun Feb 14 13:25:59 2016 +0000
# Node ID 1b45c38d02ddca7ff763366e732528c658ccb13c
# Parent  dfac70f47ed5ac6802eba6a331f36fb843c81bc7
# EXP-Topic destination
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 1b45c38d02dd
rebase: choose default destination the same way as 'hg merge' (BC)

This changeset finally make 'hg rebase' choose its default destination using the
same logic as 'hg merge'. The previous default was "tipmost changeset on the
current branch", the new default is "the other head if there is only one".  This
change has multiple consequences:

- Multiple tests which were not rebasing anything (rebasing from tipmost head)
  are now rebasing on the other "lower" branch. This is the expected new
  behavior.

- A test is now explicitly aborting when there is too many heads on the branch.
  This is the expected behavior.

- We gained a better detection of the "nothing to rebase" case while performing
  'hg pull --rebase' so the message have been updated. Making clearer than an
  update was performed and why. This is beneficial side-effect.

- Rebasing from an active bookmark will behave the same as 'hg merge' from a
  bookmark.
Yuya Nishihara - Feb. 17, 2016, 3:05 p.m.
On Mon, 15 Feb 2016 10:49:15 +0000, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1455456359 0
> #      Sun Feb 14 13:25:59 2016 +0000
> # Node ID 1b45c38d02ddca7ff763366e732528c658ccb13c
> # Parent  dfac70f47ed5ac6802eba6a331f36fb843c81bc7
> # EXP-Topic destination
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 1b45c38d02dd
> rebase: choose default destination the same way as 'hg merge' (BC)

I like the new behavior even though it is a big BC.

> This changeset finally make 'hg rebase' choose its default destination using the
> same logic as 'hg merge'. The previous default was "tipmost changeset on the
> current branch", the new default is "the other head if there is only one".

"help rebase" still says "the current branch tip as the destination." We'll
have to update it.

>  @revsetpredicate('_destrebase')
>  def _revsetdestrebase(repo, subset, x):
>      # ``_rebasedefaultdest()``
>  
>      # default destination for rebase.
>      # # XXX: Currently private because I expect the signature to change.
> -    # # XXX: - taking rev as arguments,
>      # # XXX: - bailing out in case of ambiguity vs returning all data.
> -    # # XXX: - probably merging with the merge destination.
>      # i18n: "_rebasedefaultdest" is a keyword
> -    revset.getargs(x, 0, 0, _("_rebasedefaultdest takes no arguments"))
> -    return subset & revset.baseset([_destrebase(repo)])
> +    sourceset = None
> +    if x is not None:
> +        sourceset = revset.getset(repo, revset.fullreposet(repo), x)
> +    return subset & revset.baseset([destutil.destmerge(repo, action='rebase',
> +                                                       sourceset=sourceset,
> +                                                       onheadcheck=False)])

If this isn't an intermediate state of the rebase refactoring series, can we
have a wrapper for destutil.destmerge() ? There are 4 copies.

>  @command('rebase',
>      [('s', 'source', '',
>       _('rebase the specified changeset and descendants'), _('REV')),
>      ('b', 'base', '',
> @@ -279,10 +275,16 @@ def rebase(ui, repo, **opts):
>          else:
>              dest, rebaseset = _definesets(ui, repo, destf, srcf, basef, revf)
>              if dest is None:
>                  return _nothingtorebase()
>  
> +            if not destf:
> +                dest = repo[destutil.destmerge(repo, action='rebase',
> +                                               sourceset=rebaseset,
> +                                               onheadcheck=False)]
> +                destf = str(dest)

Is it necessary to recalculate dest here?
If it is, perhaps _definesets() wouldn't do the right thing.

>  def externalparent(repo, state, targetancestors):
>      """Return the revision that should be used as the second parent
>      when the revisions in state is collapsed on top of targetancestors.
> @@ -1165,13 +1177,18 @@ def pullrebase(orig, ui, repo, *args, **
>                      del opts['rev']
>                  # positional argument from pull conflicts with rebase's own
>                  # --source.
>                  if 'source' in opts:
>                      del opts['source']
> -                if rebase(ui, repo, **opts) == _nothingtorebase():
> +                try:
> +                    rebase(ui, repo, **opts)
> +                except error.NoMergeDestAbort:
>                      rev, _a, _b = destutil.destupdate(repo)
> -                    if rev != repo['.'].rev(): # we could update
> +                    if rev == repo['.'].rev(): # we could update
> +                        ui.status(_('nothing to rebase\n'))
> +                    else:

Nit: we could update on "else:" case

> +                        ui.status(_('nothing to rebase - updating instead\n'))
>                          # not passing argument to get the bare update behavior
>                          # with warning and trumpets
>                          commands.update(ui, repo)
Augie Fackler - Feb. 23, 2016, 7:52 p.m.
On Thu, Feb 18, 2016 at 12:05:26AM +0900, Yuya Nishihara wrote:
> On Mon, 15 Feb 2016 10:49:15 +0000, Pierre-Yves David wrote:
> > # HG changeset patch
> > # User Pierre-Yves David <pierre-yves.david@fb.com>
> > # Date 1455456359 0
> > #      Sun Feb 14 13:25:59 2016 +0000
> > # Node ID 1b45c38d02ddca7ff763366e732528c658ccb13c
> > # Parent  dfac70f47ed5ac6802eba6a331f36fb843c81bc7
> > # EXP-Topic destination
> > # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> > #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 1b45c38d02dd
> > rebase: choose default destination the same way as 'hg merge' (BC)
>
> I like the new behavior even though it is a big BC.
>
> > This changeset finally make 'hg rebase' choose its default destination using the
> > same logic as 'hg merge'. The previous default was "tipmost changeset on the
> > current branch", the new default is "the other head if there is only one".
>
> "help rebase" still says "the current branch tip as the destination." We'll
> have to update it.

+1 - I'd like a v2 that at least has this fix. I don't feel strongly
about the other comments below, but for now I'll await a v2 of the
series (eagerly, as this is a huge win for users everywhere).

>
> >  @revsetpredicate('_destrebase')
> >  def _revsetdestrebase(repo, subset, x):
> >      # ``_rebasedefaultdest()``
> >
> >      # default destination for rebase.
> >      # # XXX: Currently private because I expect the signature to change.
> > -    # # XXX: - taking rev as arguments,
> >      # # XXX: - bailing out in case of ambiguity vs returning all data.
> > -    # # XXX: - probably merging with the merge destination.
> >      # i18n: "_rebasedefaultdest" is a keyword
> > -    revset.getargs(x, 0, 0, _("_rebasedefaultdest takes no arguments"))
> > -    return subset & revset.baseset([_destrebase(repo)])
> > +    sourceset = None
> > +    if x is not None:
> > +        sourceset = revset.getset(repo, revset.fullreposet(repo), x)
> > +    return subset & revset.baseset([destutil.destmerge(repo, action='rebase',
> > +                                                       sourceset=sourceset,
> > +                                                       onheadcheck=False)])
>
> If this isn't an intermediate state of the rebase refactoring series, can we
> have a wrapper for destutil.destmerge() ? There are 4 copies.
>
> >  @command('rebase',
> >      [('s', 'source', '',
> >       _('rebase the specified changeset and descendants'), _('REV')),
> >      ('b', 'base', '',
> > @@ -279,10 +275,16 @@ def rebase(ui, repo, **opts):
> >          else:
> >              dest, rebaseset = _definesets(ui, repo, destf, srcf, basef, revf)
> >              if dest is None:
> >                  return _nothingtorebase()
> >
> > +            if not destf:
> > +                dest = repo[destutil.destmerge(repo, action='rebase',
> > +                                               sourceset=rebaseset,
> > +                                               onheadcheck=False)]
> > +                destf = str(dest)
>
> Is it necessary to recalculate dest here?
> If it is, perhaps _definesets() wouldn't do the right thing.
>
> >  def externalparent(repo, state, targetancestors):
> >      """Return the revision that should be used as the second parent
> >      when the revisions in state is collapsed on top of targetancestors.
> > @@ -1165,13 +1177,18 @@ def pullrebase(orig, ui, repo, *args, **
> >                      del opts['rev']
> >                  # positional argument from pull conflicts with rebase's own
> >                  # --source.
> >                  if 'source' in opts:
> >                      del opts['source']
> > -                if rebase(ui, repo, **opts) == _nothingtorebase():
> > +                try:
> > +                    rebase(ui, repo, **opts)
> > +                except error.NoMergeDestAbort:
> >                      rev, _a, _b = destutil.destupdate(repo)
> > -                    if rev != repo['.'].rev(): # we could update
> > +                    if rev == repo['.'].rev(): # we could update
> > +                        ui.status(_('nothing to rebase\n'))
> > +                    else:
>
> Nit: we could update on "else:" case
>
> > +                        ui.status(_('nothing to rebase - updating instead\n'))
> >                          # not passing argument to get the bare update behavior
> >                          # with warning and trumpets
> >                          commands.update(ui, repo)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Feb. 23, 2016, 8:15 p.m.
On 02/23/2016 08:52 PM, Augie Fackler wrote:
> On Thu, Feb 18, 2016 at 12:05:26AM +0900, Yuya Nishihara wrote:
>> On Mon, 15 Feb 2016 10:49:15 +0000, Pierre-Yves David wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1455456359 0
>>> #      Sun Feb 14 13:25:59 2016 +0000
>>> # Node ID 1b45c38d02ddca7ff763366e732528c658ccb13c
>>> # Parent  dfac70f47ed5ac6802eba6a331f36fb843c81bc7
>>> # EXP-Topic destination
>>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 1b45c38d02dd
>>> rebase: choose default destination the same way as 'hg merge' (BC)
>>
>> I like the new behavior even though it is a big BC.
>>
>>> This changeset finally make 'hg rebase' choose its default destination using the
>>> same logic as 'hg merge'. The previous default was "tipmost changeset on the
>>> current branch", the new default is "the other head if there is only one".
>>
>> "help rebase" still says "the current branch tip as the destination." We'll
>> have to update it.
>
> +1 - I'd like a v2 that at least has this fix. I don't feel strongly
> about the other comments below, but for now I'll await a v2 of the
> series (eagerly, as this is a huge win for users everywhere).

yuya pushed a v2 on the clowncopter earlier today.

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -67,30 +67,26 @@  def _makeextrafn(copiers):
     def extrafn(ctx, extra):
         for c in copiers:
             c(ctx, extra)
     return extrafn
 
-def _destrebase(repo):
-    # Destination defaults to the latest revision in the
-    # current branch
-    branch = repo[None].branch()
-    return repo[branch].rev()
-
 revsetpredicate = revset.extpredicate()
 
 @revsetpredicate('_destrebase')
 def _revsetdestrebase(repo, subset, x):
     # ``_rebasedefaultdest()``
 
     # default destination for rebase.
     # # XXX: Currently private because I expect the signature to change.
-    # # XXX: - taking rev as arguments,
     # # XXX: - bailing out in case of ambiguity vs returning all data.
-    # # XXX: - probably merging with the merge destination.
     # i18n: "_rebasedefaultdest" is a keyword
-    revset.getargs(x, 0, 0, _("_rebasedefaultdest takes no arguments"))
-    return subset & revset.baseset([_destrebase(repo)])
+    sourceset = None
+    if x is not None:
+        sourceset = revset.getset(repo, revset.fullreposet(repo), x)
+    return subset & revset.baseset([destutil.destmerge(repo, action='rebase',
+                                                       sourceset=sourceset,
+                                                       onheadcheck=False)])
 
 @command('rebase',
     [('s', 'source', '',
      _('rebase the specified changeset and descendants'), _('REV')),
     ('b', 'base', '',
@@ -279,10 +275,16 @@  def rebase(ui, repo, **opts):
         else:
             dest, rebaseset = _definesets(ui, repo, destf, srcf, basef, revf)
             if dest is None:
                 return _nothingtorebase()
 
+            if not destf:
+                dest = repo[destutil.destmerge(repo, action='rebase',
+                                               sourceset=rebaseset,
+                                               onheadcheck=False)]
+                destf = str(dest)
+
             allowunstable = obsolete.isenabled(repo, obsolete.allowunstableopt)
             if (not (keepf or allowunstable)
                   and repo.revs('first(children(%ld) - %ld)',
                                 rebaseset, rebaseset)):
                 raise error.Abort(
@@ -535,13 +537,10 @@  def _definesets(ui, repo, destf=None, sr
     cmdutil.checkunfinished(repo)
     cmdutil.bailifchanged(repo)
 
     if destf:
         dest = scmutil.revsingle(repo, destf)
-    else:
-        dest = repo[_destrebase(repo)]
-        destf = str(dest)
 
     if revf:
         rebaseset = scmutil.revrange(repo, revf)
         if not rebaseset:
             ui.status(_('empty "rev" revision set - nothing to rebase\n'))
@@ -557,10 +556,16 @@  def _definesets(ui, repo, destf=None, sr
         base = scmutil.revrange(repo, [basef or '.'])
         if not base:
             ui.status(_('empty "base" revision set - '
                         "can't compute rebase set\n"))
             return None, None
+        if not destf:
+            dest = repo[destutil.destmerge(repo, action='rebase',
+                                           sourceset=base,
+                                           onheadcheck=False)]
+            destf = str(dest)
+
         commonanc = repo.revs('ancestor(%ld, %d)', base, dest).first()
         if commonanc is not None:
             rebaseset = repo.revs('(%d::(%ld) - %d)::',
                                   commonanc, base, commonanc)
         else:
@@ -590,10 +595,17 @@  def _definesets(ui, repo, destf=None, sr
                                 'ancestor of destination %s\n') % dest)
             else: # can it happen?
                 ui.status(_('nothing to rebase from %s to %s\n') %
                           ('+'.join(str(repo[r]) for r in base), dest))
             return None, None
+
+    if not destf:
+        dest = repo[destutil.destmerge(repo, action='rebase',
+                                       sourceset=rebaseset,
+                                       onheadcheck=False)]
+        destf = str(dest)
+
     return dest, rebaseset
 
 def externalparent(repo, state, targetancestors):
     """Return the revision that should be used as the second parent
     when the revisions in state is collapsed on top of targetancestors.
@@ -1165,13 +1177,18 @@  def pullrebase(orig, ui, repo, *args, **
                     del opts['rev']
                 # positional argument from pull conflicts with rebase's own
                 # --source.
                 if 'source' in opts:
                     del opts['source']
-                if rebase(ui, repo, **opts) == _nothingtorebase():
+                try:
+                    rebase(ui, repo, **opts)
+                except error.NoMergeDestAbort:
                     rev, _a, _b = destutil.destupdate(repo)
-                    if rev != repo['.'].rev(): # we could update
+                    if rev == repo['.'].rev(): # we could update
+                        ui.status(_('nothing to rebase\n'))
+                    else:
+                        ui.status(_('nothing to rebase - updating instead\n'))
                         # not passing argument to get the bare update behavior
                         # with warning and trumpets
                         commands.update(ui, repo)
         finally:
             release(lock, wlock)
diff --git a/mercurial/destutil.py b/mercurial/destutil.py
--- a/mercurial/destutil.py
+++ b/mercurial/destutil.py
@@ -139,63 +139,95 @@  msgdestmerge = {
     'toomanybookmarks':
         {'merge':
             (_("multiple matching bookmarks to merge -"
                " please merge with an explicit rev or bookmark"),
              _("run 'hg heads' to see all heads")),
+         'rebase':
+            (_("multiple matching bookmarks to rebase -"
+               " please rebase to an explicit rev or bookmark"),
+             _("run 'hg heads' to see all heads")),
         },
     # no other matching divergent bookmark
     'nootherbookmarks':
         {'merge':
             (_("no matching bookmark to merge - "
                "please merge with an explicit rev or bookmark"),
              _("run 'hg heads' to see all heads")),
+         'rebase':
+            (_("no matching bookmark to rebase - "
+               "please rebase to an explicit rev or bookmark"),
+             _("run 'hg heads' to see all heads")),
         },
     # branch have too many unbookmarked heads, no obvious destination
     'toomanyheads':
         {'merge':
             (_("branch '%s' has %d heads - please merge with an explicit rev"),
              _("run 'hg heads .' to see heads")),
+         'rebase':
+            (_("branch '%s' has %d heads - please rebase to an explicit rev"),
+             _("run 'hg heads .' to see heads")),
         },
     # branch have no other unbookmarked heads
     'bookmarkedheads':
         {'merge':
             (_("heads are bookmarked - please merge with an explicit rev"),
              _("run 'hg heads' to see all heads")),
+         'rebase':
+            (_("heads are bookmarked - please rebase to an explicit rev"),
+             _("run 'hg heads' to see all heads")),
         },
     # branch have just a single heads, but there is other branches
     'nootherbranchheads':
         {'merge':
             (_("branch '%s' has one head - please merge with an explicit rev"),
              _("run 'hg heads' to see all heads")),
+         'rebase':
+            (_("branch '%s' has one head - please rebase to an explicit rev"),
+             _("run 'hg heads' to see all heads")),
         },
     # repository have a single head
     'nootherheads':
-            {'merge':
-                (_('nothing to merge'),
-         None),
+        {'merge':
+            (_('nothing to merge'),
+            None),
+         'rebase':
+            (_('nothing to rebase'),
+            None),
         },
     # repository have a single head and we are not on it
     'nootherheadsbehind':
         {'merge':
             (_('nothing to merge'),
              _("use 'hg update' instead")),
+         'rebase':
+            (_('nothing to rebase'),
+             _("use 'hg update' instead")),
         },
     # We are not on a head
     'notatheads':
         {'merge':
             (_('working directory not at a head revision'),
-             _("use 'hg update' or merge with an explicit revision"))
+             _("use 'hg update' or merge with an explicit revision")),
+         'rebase':
+            (_('working directory not at a head revision'),
+             _("use 'hg update' or rebase to an explicit revision"))
         },
     'emptysourceset':
         {'merge':
             (_('source set is empty'),
-             None)
+             None),
+         'rebase':
+            (_('source set is empty'),
+             None),
         },
     'multiplebranchessourceset':
         {'merge':
             (_('source set is rooted in multiple branches'),
-             None)
+             None),
+         'rebase':
+            (_('rebaseset is rooted in multiple named branches'),
+             _('specify an explicit destination with --dest')),
         },
     }
 
 def _destmergebook(repo, action='merge', sourceset=None):
     """find merge destination in the active bookmark case"""
diff --git a/tests/test-largefiles-misc.t b/tests/test-largefiles-misc.t
--- a/tests/test-largefiles-misc.t
+++ b/tests/test-largefiles-misc.t
@@ -1093,9 +1093,9 @@  Test "pull --rebase" when rebase is enab
   requesting all changes
   adding changesets
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
-  nothing to rebase - working directory parent is already an ancestor of destination bf5e395ced2c
+  nothing to rebase - updating instead
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
   $ cd ..
diff --git a/tests/test-rebase-named-branches.t b/tests/test-rebase-named-branches.t
--- a/tests/test-rebase-named-branches.t
+++ b/tests/test-rebase-named-branches.t
@@ -325,18 +325,17 @@  Set up a case:
 
 rebase 'b2' to another lower branch head
 
   $ hg up -qr 2
   $ hg rebase
-  nothing to rebase - working directory parent is also destination
-  [1]
+  rebasing 2:792845bb77ee "b2"
+  note: rebase of 2:792845bb77ee created no changes to commit
+  saved backup bundle to $TESTTMP/case1/.hg/strip-backup/792845bb77ee-627120ee-backup.hg (glob)
   $ hg tglog
-  o  3: 'c1' c
+  o  2: 'c1' c
   |
-  | @  2: 'b2' b
-  |/
-  | o  1: 'b1' b
+  | @  1: 'b1' b
   |/
   o  0: '0'
   
 
 rebase 'b1' on top of the tip of the branch ('b2') - ignoring the tip branch ('c1')
@@ -371,12 +370,13 @@  rebase 'c1' to the branch head 'c2' that
   o |  1: 'b2' b
   |/
   o  0: '0'
   
   $ hg rebase
-  nothing to rebase - working directory parent is also destination
-  [1]
+  abort: branch 'c' has one head - please rebase to an explicit rev
+  (run 'hg heads' to see all heads)
+  [255]
   $ hg tglog
   _  4: 'c2 closed' c
   |
   o  3: 'b1' b
   |
diff --git a/tests/test-rebase-pull.t b/tests/test-rebase-pull.t
--- a/tests/test-rebase-pull.t
+++ b/tests/test-rebase-pull.t
@@ -83,11 +83,11 @@  Invoke pull --rebase and nothing to reba
   searching for changes
   adding changesets
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
-  nothing to rebase - working directory parent is already an ancestor of destination 77ae9631bcca
+  nothing to rebase - updating instead
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   updating bookmark norebase
 
   $ hg tglog -l 1
   @  2: 'R1'
@@ -283,11 +283,11 @@  pull --rebase update (no rebase) use pro
   searching for changes
   adding changesets
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files (+1 heads)
-  nothing to rebase - working directory parent is already an ancestor of destination 65bc164c1d9b
+  nothing to rebase - updating instead
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   1 other heads for branch "default"
   $ hg tglog
   @  9: 'R6'
   |
diff --git a/tests/test-rebase-scenario-global.t b/tests/test-rebase-scenario-global.t
--- a/tests/test-rebase-scenario-global.t
+++ b/tests/test-rebase-scenario-global.t
@@ -816,15 +816,19 @@  Testing from lower head
   summary:     aaa
   
 
 Testing from upper head
 
+  $ hg log -r '_destrebase(4)'
+  changeset:   3:1910d5ff34ea
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     second source with subdir
+  
   $ hg up 4
   1 files updated, 0 files merged, 2 files removed, 0 files unresolved
   $ hg log -r '_destrebase()'
-  changeset:   4:5f7bc9025ed2
-  tag:         tip
-  parent:      1:58d79cc1cf43
+  changeset:   3:1910d5ff34ea
   user:        test
   date:        Thu Jan 01 00:00:00 1970 +0000
-  summary:     aaa
+  summary:     second source with subdir