Patchwork [04,of,10] rebase: improve error message for base revision

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

Comments

Mads Kiilerich - Jan. 12, 2014, 4:08 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1384730518 18000
#      Sun Nov 17 18:21:58 2013 -0500
# Node ID 55325a119cbeea17c2379bbe23f20d6226442a03
# Parent  ef40ccf4914c597e32885227dde08abe40e41ad5
rebase: improve error message for base revision

Before it just said 'nothing to rebase', now it also hints to the reason:
'base revset is empty',
'NODE is both base and destination', or
'base NODE is already an ancestor of destination NODE'
Pierre-Yves David - Jan. 13, 2014, 7:29 a.m.
On Sun, Jan 12, 2014 at 05:08:02PM +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1384730518 18000
> #      Sun Nov 17 18:21:58 2013 -0500
> # Node ID 55325a119cbeea17c2379bbe23f20d6226442a03
> # Parent  ef40ccf4914c597e32885227dde08abe40e41ad5
> rebase: improve error message for base revision
> 
> Before it just said 'nothing to rebase', now it also hints to the reason:

I like the idea of improving rebase error message but this one about base is confusing.
The argument from base is "changeset to compute a base from" not "base changeset".

So I would made the following changes:

> nothing to rebase - NODE is both base and destination

nothing to rebase - NODE is both source and destination

> nothing to rebase - base NODE is already an ancestor of destination NODE

cannot rebase - destination NODE is based on source NODE

> 'base revset is empty',

nothing to rebase - can't compute base from empty revset



Moreover reading this test ouput puzzled me:

>   $ hg rebase
>   nothing to rebase - e7ec4e813ba6 is both base and destination

What about dedicated detection and a more specific message.

nothing to rebase - already on BRANCH tipmost head
(do you want to use to use --dest or --base?)


I starting to thing --base could have been better named --branch (but then
people would think it is about named branch)

> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -234,9 +234,27 @@ def rebase(ui, repo, **opts):
>                  assert rebaseset
>              else:
>                  base = scmutil.revrange(repo, [basef or '.'])
> +                if not base:
> +                    ui.status(_('nothing to rebase - '
> +                                'base revset is empty\n'))
> +                    return 1
> +                if base == [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):
> +                    ui.status(_('nothing to rebase - base %s is already an '
> +                                'ancestor of destination %s\n') %
> +                              ('+'.join(str(repo[r]) for r in base),
> +                               dest))
> +                    return 1
>                  rebaseset = repo.revs(
>                      '(children(ancestor(%ld, %d)) and ::(%ld))::',
>                      base, dest, base)
> +                if not rebaseset: # cannot happen?
> +                    ui.status(_('nothing to rebase from %s to %s\n') %
> +                              ('+'.join(str(repo[r]) for r in base), dest))
> +                    return 1
>              if rebaseset:
>                  root = min(rebaseset)
>              else:
> 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
> +  nothing to rebase - 598410d3eb9a is both base and destination
>    $ [ -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-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
> +  nothing to rebase - e7ec4e813ba6 is both base and destination
>    [1]
>  
>    $ hg up -q 7
>  
>    $ hg rebase --traceback
> -  nothing to rebase
> +  nothing to rebase - base 02de42196ebe is already an ancestor of destination e7ec4e813ba6
>    [1]
>  
>    $ hg rebase -r '1 & !1'
> @@ -97,6 +97,9 @@ These fail:
>    nothing to rebase - source revset is empty
>    [1]
>  
> +  $ hg rebase --base '1 & !1'
> +  nothing to rebase - base revset is empty
> +  [1]
>  
>  These work:
>  
> 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
> +  nothing to rebase - base 783333faa078 is already an ancestor of destination 77ae9631bcca
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>    updating bookmark norebase
>  
> 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
> @@ -235,7 +235,7 @@ These will abort gracefully (using --bas
>  G onto G - rebase onto same changeset:
>  
>    $ hg rebase -b 6 -d 6
> -  nothing to rebase
> +  nothing to rebase - eea13746799a is both base and destination
>    [1]
>  
>  G onto F - rebase onto an ancestor:
> @@ -247,7 +247,7 @@ G onto F - rebase onto an ancestor:
>  F onto G - rebase onto a descendant:
>  
>    $ hg rebase -b 5 -d 6
> -  nothing to rebase
> +  nothing to rebase - base 24b6387c8c8c is already an ancestor of destination eea13746799a
>    [1]
>  
>  C onto A - rebase onto an ancestor:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Mads Kiilerich - Jan. 15, 2014, 2:25 a.m.
would also be confusingOn 01/13/2014 08:29 AM, Pierre-Yves David wrote:
> On Sun, Jan 12, 2014 at 05:08:02PM +0100, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1384730518 18000
>> #      Sun Nov 17 18:21:58 2013 -0500
>> # Node ID 55325a119cbeea17c2379bbe23f20d6226442a03
>> # Parent  ef40ccf4914c597e32885227dde08abe40e41ad5
>> rebase: improve error message for base revision
>>
>> Before it just said 'nothing to rebase', now it also hints to the reason:
> I like the idea of improving rebase error message but this one about base is confusing.
> The argument from base is "changeset to compute a base from" not "base changeset".

"compute a base from" - is that an official description somewhere? What 
is a "base" really? The set it is computing from "base" can not really 
be called a "base".

I will call it "base" and quote it. Reading that and the option name as 
"smurf" makes it more readable than when trying to think of it as some 
kind of base ;-)

And more important: I will avoid mentioning "base" when it working 
directory is used as default.

> So I would made the following changes:
>
>> nothing to rebase - NODE is both base and destination
> nothing to rebase - NODE is both source and destination
>
>> nothing to rebase - base NODE is already an ancestor of destination NODE
> cannot rebase - destination NODE is based on source NODE

I don't think "based on" would be helpful. I think 'ancestor' is better.

It would also be confusing to call the value that the user specified 
with --base for "source".

>
>> 'base revset is empty',
> nothing to rebase - can't compute base from empty revset

But it is not a base it is computing!

> Moreover reading this test ouput puzzled me:
>
>>    $ hg rebase
>>    nothing to rebase - e7ec4e813ba6 is both base and destination
> What about dedicated detection and a more specific message.
>
> nothing to rebase - already on BRANCH tipmost head
> (do you want to use to use --dest or --base?)

That can perhaps be introduced later. I would like to get the dest 
computation under control first.

> I starting to thing --base could have been better named --branch (but then
> people would think it is about named branch)

It is not directly specifying the branch. It only gives meaning relative 
to a dest, and then it is giving a hint, pointing at a subtree, 
identifying a branch, selecting a side. Some of these words can perhaps 
be used.


I have resent the least controversial parts of the queue with some 
changes based on your input. Thanks.

/Mads
Pierre-Yves David - Jan. 17, 2014, 11:31 p.m.
On 01/14/2014 06:25 PM, Mads Kiilerich wrote:
> would also be confusingOn 01/13/2014 08:29 AM, Pierre-Yves David wrote:
>> On Sun, Jan 12, 2014 at 05:08:02PM +0100, Mads Kiilerich wrote:
>>> # HG changeset patch
>>> # User Mads Kiilerich <madski@unity3d.com>
>>> # Date 1384730518 18000
>>> #      Sun Nov 17 18:21:58 2013 -0500
>>> # Node ID 55325a119cbeea17c2379bbe23f20d6226442a03
>>> # Parent  ef40ccf4914c597e32885227dde08abe40e41ad5
>>> rebase: improve error message for base revision
>>>
>>> Before it just said 'nothing to rebase', now it also hints to the 
>>> reason:
>> I like the idea of improving rebase error message but this one about 
>> base is confusing.
>> The argument from base is "changeset to compute a base from" not 
>> "base changeset".
>
> "compute a base from" - is that an official description somewhere? 
> What is a "base" really? The set it is computing from "base" can not 
> really be called a "base".

Well it kind of compute a real -base- to use as in --source to get the 
rebaseset.

The revset is "(children(ancestor(<base>, <dest>)) and ::(<base>))::"

Most of it "(children(ancestor(<base>, <dest>)) and ::(<base>))" is 
about computing a base. It then takes all descendant in the rebase set.

>
> I will call it "base" and quote it. Reading that and the option name 
> as "smurf" makes it more readable than when trying to think of it as 
> some kind of base ;-)

Quoting it improve it a lot

>
> And more important: I will avoid mentioning "base" when it working 
> directory is used as default.

+1

>
>> So I would made the following changes:
>>
>>> nothing to rebase - NODE is both base and destination
>> nothing to rebase - NODE is both source and destination
>>
>>> nothing to rebase - base NODE is already an ancestor of destination 
>>> NODE
>> cannot rebase - destination NODE is based on source NODE
>
> I don't think "based on" would be helpful. I think 'ancestor' is better.

The issue I have from this changeset is "already an ancestors of"

When I use `hg rebase --dest D --base A` I'm trying to make D an 
ancestors of A. reading it the other way around is confusing my brain.

$ hg rebase --dest D --base A
# me: please make D an ancestors of A
# hg: can't A is -already- and ancestors of D
# me: not what I'm asking for idiot!

After writing this down I think dropping the "already" will make it fine:

     nothing to rebase - base NODE is an ancestor of destination NODE

I do not really like the "nothing to rebase". And will probbly would 
like to have an error returned here. But his can be pushed out of this 
series.


> It would also be confusing to call the value that the user specified 
> with --base for "source".

hg help rebase use "source" all over the place. The "source/destination" 
idea apply well to rebase whatever the actual initial option is used.

>
>>
>>> 'base revset is empty',
>> nothing to rebase - can't compute base from empty revset
>
> But it is not a base it is computing!

yes it is (see revset above). you could also use "rebase set". (or use 
your "base" trick, okay)

>
>> Moreover reading this test ouput puzzled me:
>>
>>>    $ hg rebase
>>>    nothing to rebase - e7ec4e813ba6 is both base and destination
>> What about dedicated detection and a more specific message.
>>
>> nothing to rebase - already on BRANCH tipmost head
>> (do you want to use to use --dest or --base?)
>
> That can perhaps be introduced later. I would like to get the dest 
> computation under control first.

Ok with it introduced later (could worth being pointed in a comment or a 
description)

>
>> I starting to thing --base could have been better named --branch (but 
>> then
>> people would think it is about named branch)
>
> It is not directly specifying the branch. It only gives meaning 
> relative to a dest, and then it is giving a hint, pointing at a 
> subtree, identifying a branch, selecting a side. Some of these words 
> can perhaps be used.

--base ask rebase to grab everything "branching" from --dest that 
somehow contains the target of  `--branch`. Its the "grab that branchy 
things" option.

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -234,9 +234,27 @@  def rebase(ui, repo, **opts):
                 assert rebaseset
             else:
                 base = scmutil.revrange(repo, [basef or '.'])
+                if not base:
+                    ui.status(_('nothing to rebase - '
+                                'base revset is empty\n'))
+                    return 1
+                if base == [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):
+                    ui.status(_('nothing to rebase - base %s is already an '
+                                'ancestor of destination %s\n') %
+                              ('+'.join(str(repo[r]) for r in base),
+                               dest))
+                    return 1
                 rebaseset = repo.revs(
                     '(children(ancestor(%ld, %d)) and ::(%ld))::',
                     base, dest, base)
+                if not rebaseset: # cannot happen?
+                    ui.status(_('nothing to rebase from %s to %s\n') %
+                              ('+'.join(str(repo[r]) for r in base), dest))
+                    return 1
             if rebaseset:
                 root = min(rebaseset)
             else:
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
+  nothing to rebase - 598410d3eb9a is both base and destination
   $ [ -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-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
+  nothing to rebase - e7ec4e813ba6 is both base and destination
   [1]
 
   $ hg up -q 7
 
   $ hg rebase --traceback
-  nothing to rebase
+  nothing to rebase - base 02de42196ebe is already an ancestor of destination e7ec4e813ba6
   [1]
 
   $ hg rebase -r '1 & !1'
@@ -97,6 +97,9 @@  These fail:
   nothing to rebase - source revset is empty
   [1]
 
+  $ hg rebase --base '1 & !1'
+  nothing to rebase - base revset is empty
+  [1]
 
 These work:
 
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
+  nothing to rebase - base 783333faa078 is already an ancestor of destination 77ae9631bcca
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   updating bookmark norebase
 
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
@@ -235,7 +235,7 @@  These will abort gracefully (using --bas
 G onto G - rebase onto same changeset:
 
   $ hg rebase -b 6 -d 6
-  nothing to rebase
+  nothing to rebase - eea13746799a is both base and destination
   [1]
 
 G onto F - rebase onto an ancestor:
@@ -247,7 +247,7 @@  G onto F - rebase onto an ancestor:
 F onto G - rebase onto a descendant:
 
   $ hg rebase -b 5 -d 6
-  nothing to rebase
+  nothing to rebase - base 24b6387c8c8c is already an ancestor of destination eea13746799a
   [1]
 
 C onto A - rebase onto an ancestor: