Patchwork [09,of,10] rebase: use other branch head as default destination instead of branch head (bc)

login
register
mail settings
Submitter Mads Kiilerich
Date Jan. 12, 2014, 4:08 p.m.
Message ID <7731c6d3de0db683cfc4.1389542887@localhost.localdomain>
Download mbox | patch
Permalink /patch/3303/
State Superseded
Headers show

Comments

Mads Kiilerich - Jan. 12, 2014, 4:08 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1385431986 -3600
#      Tue Nov 26 03:13:06 2013 +0100
# Node ID 7731c6d3de0db683cfc4f0f2846556732b790b8e
# Parent  ebe2b7bef0ff9e3b831e5784954bce7a7d76fb21
rebase: use other branch head as default destination instead of branch head (bc)

This change do not change how rebase works in the common and well-defined
cases. In cases where it failed or had unexpected behaviour it will now more
consistently and predictably pick the single other branch head if there is one.

The previous behaviour was in some cases sub-optimal. Sometimes it would fail
to rebase in cases where it was quite obvious what it could have done, and in
other cases it would do something even though it could be considered ambiguous
what it was told to do. The cases were primarily how the topological ordering
mattered ... which is different from how the merge command works. Other
confusion came from how closed branch heads often didn't count as other branch
head.

Rebase is moving heads around anyway so the branch head concept is a moving
target when rebasing and arguably not really helpful here.

Some tests changed because rebase now in some cases tried to rebase to "wrong"
destinations and failed with odd error messages, instead it will now refuse to
rebase to the "wrong" destination.

The changes in test-rebase-named-branches.t show the intended change of
behaviour.

Some tests in test-rebase-parameters.t relied on using tip-most branch head and
get an additional destination parameters.
Pierre-Yves David - Jan. 13, 2014, 8:20 a.m.
On Sun, Jan 12, 2014 at 05:08:07PM +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1385431986 -3600
> #      Tue Nov 26 03:13:06 2013 +0100
> # Node ID 7731c6d3de0db683cfc4f0f2846556732b790b8e
> # Parent  ebe2b7bef0ff9e3b831e5784954bce7a7d76fb21
> rebase: use other branch head as default destination instead of branch head (bc)

Brain faint at: Use "branch head" instead of "branch head".
What about:

rebase: default destination to single other branch head instead of tipmost one

> This change do not change how rebase works in the common and well-defined
> cases. In cases where it failed or had unexpected behaviour it will now more
> consistently and predictably pick the single other branch head if there is one.

I like this change. I'm a bit afraid about he BC break and I expect some script
to be confused by that. However consequence should not be too huge since phases
prevent rebase to do irreparable damages.

This change also "fix" a quite common use case:

  $ hg pull
  $ hg rebase
  abort: uncommited changes
  $ hg commit -m 'reversing polarity'
  $ rebase
  nothing to rebase #tipmost head changed

> The previous behaviour was in some cases sub-optimal. Sometimes it would fail
> to rebase in cases where it was quite obvious what it could have done, and in
> other cases it would do something even though it could be considered ambiguous
> what it was told to do. The cases were primarily how the topological ordering
> mattered ... which is different from how the merge command works.

Your argument about merge is nice!

> Other confusion came from how closed branch heads often didn't count as other
> branch head.

I feel that reasonable. Excluding closed branch from common operation sound the
way to go. I think hgview is even filtering them out by default.

I would prefer better error message:

  $ hg rebase
  no other open head found
  (xxx closed head found, do you want --dest HEAD ?)

> […]

Did not review the code yet, waiting for the semantic discussion to end.

> --- a/tests/test-largefiles.t
> +++ b/tests/test-largefiles.t
> @@ -977,7 +977,7 @@ rebased or not.
>    M sub2/large6
>    saved backup bundle to $TESTTMP/d/.hg/strip-backup/f574fb32bb45-backup.hg (glob)
>    0 largefiles cached
> -  nothing to rebase - 598410d3eb9a is both base and destination
> +  cannot compute rebase destination - branch default has 0 other heads
>    $ [ -f .hg/largefiles/e166e74c7303192238d60af5a9c4ce9bef0b7928 ]
>    $ hg log --template '{rev}:{node|short}  {desc|firstline}\n'
>    9:598410d3eb9a  modify normal file largefile in repo d

This message is quite long and confusing. What about:


1) nothing to rebase - no other head on branch BRANCH

2) ambiguous destination - 42 other head on branch BRANCH
(specify destination with --dest HEAD)
Mads Kiilerich - Jan. 13, 2014, 2:03 p.m.
Thanks for the comments. I will take a closer look. One quick comment:

On 01/13/2014 09:20 AM, Pierre-Yves David wrote:
>> Other confusion came from how closed branch heads often didn't count as other
>> branch head.
> I feel that reasonable. Excluding closed branch from common operation sound the
> way to go. I think hgview is even filtering them out by default.

I would argue for moving in the opposite direction.

The main or initial motivation for this series was this:

   $ hg pull --rebase    # gets a changeset closing the current branch
   nothing to rebase    # ignoring the closed branch head
   $ hg push
   abort: push creates new remote head ...
   $ hg rebase
   nothing to rebase

But just getting more helpful error messages would we a step forward.

/Mads
Pierre-Yves David - Jan. 17, 2014, 11:37 p.m.
On 01/13/2014 06:03 AM, Mads Kiilerich wrote:
> Thanks for the comments. I will take a closer look. One quick comment:
>
> On 01/13/2014 09:20 AM, Pierre-Yves David wrote:
>>> Other confusion came from how closed branch heads often didn't count 
>>> as other
>>> branch head.
>> I feel that reasonable. Excluding closed branch from common operation 
>> sound the
>> way to go. I think hgview is even filtering them out by default.
>
> I would argue for moving in the opposite direction.

`hg merge` refuses to takes closed heads as implicit target. I strongly 
argue for rebase to go in the same direction than merge.

On a general basis we should make closed heads more powerful, not less 
powerful.

>
> The main or initial motivation for this series was this:
>
>   $ hg pull --rebase    # gets a changeset closing the current branch
>   nothing to rebase    # ignoring the closed branch head
>   $ hg push
>   abort: push creates new remote head ...
>   $ hg rebase
>   nothing to rebase
>
> But just getting more helpful error messages would we a step forward.

+1 for improving error message in that case (note that merge is probably 
affected too)

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -91,8 +91,8 @@  def rebase(ui, repo, **opts):
     rebasing published changes. See :hg:`help phases` for details.
 
     If you don't specify a destination changeset (``-d/--dest``),
-    rebase uses the current branch tip as the destination. (The
-    destination changeset is not modified by rebasing, but new
+    rebase uses the other branch head as the destination, assuming there only
+    is one. (The destination changeset is not modified by rebasing, but new
     changesets are added as its descendants.)
 
     You can specify which changesets to rebase in two ways: as a
@@ -118,13 +118,6 @@  def rebase(ui, repo, **opts):
     changesets in the source branch (e.g. merges from the destination
     branch) may be dropped if they no longer contribute any change.
 
-    One result of the rules for selecting the destination changeset
-    and source branch is that, unlike ``merge``, rebase will do
-    nothing if you are at the branch tip of a named branch
-    with two heads. You need to explicitly specify source and/or
-    destination (or ``update`` to the other head, if it's the head of
-    the intended source branch).
-
     If a rebase is interrupted to manually resolve a merge, it can be
     continued with --continue/-c or aborted with --abort/-a.
 
@@ -211,14 +204,7 @@  def rebase(ui, repo, **opts):
             cmdutil.checkunfinished(repo)
             cmdutil.bailifchanged(repo)
 
-            if not destf:
-                # Destination defaults to the latest revision in the
-                # current branch
-                branch = repo[None].branch()
-                dest = repo[branch]
-            else:
-                dest = scmutil.revsingle(repo, destf)
-
+            fromancestor = False
             if revf:
                 rebaseset = scmutil.revrange(repo, revf)
                 if not rebaseset:
@@ -233,28 +219,55 @@  def rebase(ui, repo, **opts):
                 rebaseset = repo.revs('(%ld)::', src)
                 assert rebaseset
             else:
-                base = scmutil.revrange(repo, [basef or '.'])
-                if not base:
+                rebaseset = scmutil.revrange(repo, [basef or '.'])
+                if not rebaseset:
                     ui.status(_('nothing to rebase - '
                                 'base revset is empty\n'))
                     return 1
-                if base == [dest.rev()]:
+                fromancestor = True
+
+            if destf:
+                dest = scmutil.revsingle(repo, destf)
+            else:
+                # If rebaseset changesets are all on the same branch, pick the
+                # single head on that branch that not is a descendant of
+                # rebaseset
+                branches = set(repo[r].branch() for r in rebaseset)
+                if len(branches) != 1:
+                    raise util.Abort(
+                        _('cannot find other branch head when rebasing '
+                          'changesets on several branches'),
+                        hint=_('please specify --dest'))
+                branch = branches.pop()
+                otherheads = repo.revs(
+                    'head()&branch(%s)&!(%ld::)', branch, rebaseset)
+                if len(otherheads) != 1:
+                    ui.status(_('cannot compute rebase destination '
+                                '- branch %s has %s other heads\n') %
+                              (branch, len(otherheads)))
+                    return 1 # TODO: should we abort?
+                dest = repo[otherheads[0]]
+
+            if fromancestor:
+                if rebaseset == [dest.rev()]:
                     ui.status(_('nothing to rebase - '
                                 '%s is both base and destination\n') % dest)
                     return 1
-                if not repo.revs('%ld - ::%d', base, dest):
+                if not repo.revs('%ld - ::%d', rebaseset, dest):
                     ui.status(_('nothing to rebase - base %s is already an '
                                 'ancestor of destination %s\n') %
-                              ('+'.join(str(repo[r]) for r in base),
+                              ('+'.join(str(repo[r]) for r in rebaseset),
                                dest))
                     return 1
-                rebaseset = repo.revs(
-                    '(children(ancestor(%ld, %d)) and ::(%ld))::',
-                    base, dest, base)
-                if not rebaseset: # cannot happen?
+                bases = repo.revs(
+                    'children(ancestor(%ld, %d)) and ::%ld',
+                    rebaseset, dest, rebaseset)
+                if not bases: # cannot happen?
                     ui.status(_('nothing to rebase from %s to %s\n') %
-                              ('+'.join(str(repo[r]) for r in base), dest))
+                              ('+'.join(str(repo[r]) for r in rebaseset), dest))
                     return 1
+                rebaseset = repo.revs('%ld::', bases)
+                assert rebaseset
 
             if (not (keepf or obsolete._enabled)
                   and repo.revs('first(children(%ld) - %ld)',
diff --git a/tests/test-largefiles.t b/tests/test-largefiles.t
--- a/tests/test-largefiles.t
+++ b/tests/test-largefiles.t
@@ -977,7 +977,7 @@  rebased or not.
   M sub2/large6
   saved backup bundle to $TESTTMP/d/.hg/strip-backup/f574fb32bb45-backup.hg (glob)
   0 largefiles cached
-  nothing to rebase - 598410d3eb9a is both base and destination
+  cannot compute rebase destination - branch default has 0 other heads
   $ [ -f .hg/largefiles/e166e74c7303192238d60af5a9c4ce9bef0b7928 ]
   $ hg log --template '{rev}:{node|short}  {desc|firstline}\n'
   9:598410d3eb9a  modify normal file largefile in repo d
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
@@ -274,14 +274,11 @@  rebase 'b2' to another lower branch head
 
   $ hg up -qr 2
   $ hg rebase
-  nothing to rebase - 792845bb77ee is both base and destination
-  [1]
+  saved backup bundle to $TESTTMP/case1/.hg/strip-backup/792845bb77ee-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'
   
@@ -319,17 +316,14 @@  rebase 'c1' to the branch head 'c2' that
   o  0: '0'
   
   $ hg rebase
-  nothing to rebase - c062e3ecd6c6 is both base and destination
-  [1]
+  saved backup bundle to $TESTTMP/case2/.hg/strip-backup/c062e3ecd6c6-backup.hg (glob)
   $ hg tglog
-  o  4: 'c2 closed' c
+  @  3: 'c2 closed' c
   |
-  o  3: 'b1' b
+  o  2: 'b1' b
   |
-  | @  2: 'c1' c
-  | |
-  o |  1: 'b2' b
-  |/
+  o  1: 'b2' b
+  |
   o  0: '0'
   
 
diff --git a/tests/test-rebase-parameters.t b/tests/test-rebase-parameters.t
--- a/tests/test-rebase-parameters.t
+++ b/tests/test-rebase-parameters.t
@@ -80,13 +80,13 @@  These fail:
   [255]
 
   $ hg rebase
-  nothing to rebase - e7ec4e813ba6 is both base and destination
+  cannot compute rebase destination - branch default has 2 other heads
   [1]
 
   $ hg up -q 7
 
   $ hg rebase --traceback
-  nothing to rebase - base 02de42196ebe is already an ancestor of destination e7ec4e813ba6
+  cannot compute rebase destination - branch default has 2 other heads
   [1]
 
   $ hg rebase -r '1 & !1'
@@ -112,6 +112,9 @@  Rebase with no arguments (from 3 onto 8)
   $ hg up -q -C 3
 
   $ hg rebase
+  cannot compute rebase destination - branch default has 2 other heads
+  [1]
+  $ hg rebase --dest 8
   saved backup bundle to $TESTTMP/a1/.hg/strip-backup/*-backup.hg (glob)
 
   $ hg tglog
@@ -148,6 +151,9 @@  Rebase with base == '.' => same as no ar
   $ cd a2
 
   $ hg rebase --base .
+  cannot compute rebase destination - branch default has 2 other heads
+  [1]
+  $ hg rebase --base . --dest 8
   saved backup bundle to $TESTTMP/a2/.hg/strip-backup/*-backup.hg (glob)
 
   $ hg tglog
@@ -208,6 +214,9 @@  Specify only source (from 2 onto 8):
   $ cd a4
 
   $ hg rebase --source 'desc("C")'
+  cannot compute rebase destination - branch default has 2 other heads
+  [1]
+  $ hg rebase --source 'desc("C")' --dest 8
   saved backup bundle to $TESTTMP/a4/.hg/strip-backup/*-backup.hg (glob)
 
   $ hg tglog
@@ -268,6 +277,9 @@  Specify only base (from 1 onto 8):
   $ cd a6
 
   $ hg rebase --base 'desc("D")'
+  cannot compute rebase destination - branch default has 2 other heads
+  [1]
+  $ hg rebase --base 'desc("D")' --dest 8
   saved backup bundle to $TESTTMP/a6/.hg/strip-backup/*-backup.hg (glob)
 
   $ hg tglog
@@ -358,6 +370,9 @@  Specify only revs (from 2 onto 8)
   $ cd a9
 
   $ hg rebase --rev 'desc("C")::'
+  cannot compute rebase destination - branch default has 2 other heads
+  [1]
+  $ hg rebase --rev 'desc("C")::' --dest 8
   saved backup bundle to $TESTTMP/a9/.hg/strip-backup/*-backup.hg (glob)
 
   $ hg tglog
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
@@ -84,7 +84,7 @@  Invoke pull --rebase and nothing to reba
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
-  nothing to rebase - base 783333faa078 is already an ancestor of destination 77ae9631bcca
+  cannot compute rebase destination - branch default has 0 other heads
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   updating bookmark norebase