Patchwork [02,of,10] rebase: improve error message for empty --rev set

login
register
mail settings
Submitter Julien Cristau
Date April 23, 2014, 1:17 p.m.
Message ID <20140423131740.GA25648@crater1.logilab.fr>
Download mbox | patch
Permalink /patch/4433/
State Deferred
Headers show

Comments

Julien Cristau - April 23, 2014, 1:17 p.m.
On Sun, Jan 12, 2014 at 17:08:00 +0100, Mads Kiilerich wrote:

> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1384634789 18000
> #      Sat Nov 16 15:46:29 2013 -0500
> # Node ID d4c3a8ae89b0e2bf1e8a0bebb3fd6ba49ad1ac62
> # Parent  e3be09b53038b39614eff4e57b47896cca103a31
> rebase: improve error message for empty --rev set
> 
> Before it just said 'nothing to rebase', now it also hints to the reason:
> 'rev revset is empty'.
> 
Hi,

this changeset breaks my expectation (and the documentation) that hg
rebase exits 1 if it has nothing to do.  I don't really understand why
empty --rev set is any more of a hard error than other cases where
there's nothing to rebase.
E.g. the code at
http://hg.logilab.org/master/cubes/vcreview/file/6540bef1775a/test/unittest_rebase.py#l206
relies on the previous behaviour, and explodes on 2.9.

Any chance I can sell you on the patch below?
(I'm not completely sure about the "source set" case, so it's not
included)

# HG changeset patch
# User Julien Cristau <julien.cristau@logilab.fr>
# Date 1398253895 -7200
#      Wed Apr 23 13:51:35 2014 +0200
# Branch stable
# Node ID 8a0d3c3e18434f39633ef165439763447beda7bf
# Parent  dae36d3e1c60465bfb56f210b4cb3b231f46e5dd
rebase: don't abort if we're asked to rebase an empty revset

The documentation says we exit 1 if we have nothing to do, so avoid
breaking that contract when we're passed an empty revset.

This was changed in http://www.selenic.com/hg/rev/a259f7b488ab to
improve the error message; keep the improved message, just not the
abort.


Cheers,
Julien
Mads Kiilerich - April 23, 2014, 1:46 p.m.
On 04/23/2014 03:17 PM, Julien Cristau wrote:
> On Sun, Jan 12, 2014 at 17:08:00 +0100, Mads Kiilerich wrote:
>
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1384634789 18000
>> #      Sat Nov 16 15:46:29 2013 -0500
>> # Node ID d4c3a8ae89b0e2bf1e8a0bebb3fd6ba49ad1ac62
>> # Parent  e3be09b53038b39614eff4e57b47896cca103a31
>> rebase: improve error message for empty --rev set
>>
>> Before it just said 'nothing to rebase', now it also hints to the reason:
>> 'rev revset is empty'.

(A link or hash to the actual final revision would be helpful, 
especially when the patch is trimmed. 
http://selenic.com/hg/rev/a259f7b488ab )

> Hi,
>
> this changeset breaks my expectation (and the documentation) that hg
> rebase exits 1 if it has nothing to do.  I don't really understand why
> empty --rev set is any more of a hard error than other cases where
> there's nothing to rebase.

I would say that there is a difference between explicitly telling rebase 
what to rebase and letting it figure it out by itself. Explicitly giving 
rebase an empty set is more of a user error. 1 is for the case where the 
changesets we are considering already has been rebased. If the set is 
empty, we cannot really say that it already has been rebased.

> E.g. the code at
> http://hg.logilab.org/master/cubes/vcreview/file/6540bef1775a/test/unittest_rebase.py#l206
> relies on the previous behaviour, and explodes on 2.9.
>
> Any chance I can sell you on the patch below?

I think an abort is more correct and helpful and fixes an "issue". But 
it is thus also in a way a change of (undefined / incorrect) behaviour. 
Besides that, the patch and returning 1 is fine with me.

/Mads
Matt Mackall - April 29, 2014, 7:24 p.m.
> # HG changeset patch
> # User Julien Cristau <julien.cristau@logilab.fr>
> # Date 1398253895 -7200
> #      Wed Apr 23 13:51:35 2014 +0200
> # Branch stable
> # Node ID 8a0d3c3e18434f39633ef165439763447beda7bf
> # Parent  dae36d3e1c60465bfb56f210b4cb3b231f46e5dd
> rebase: don't abort if we're asked to rebase an empty revset

Queued for stable, thanks.
Mads Kiilerich - May 1, 2014, 1:04 a.m.
On 04/29/2014 09:24 PM, Matt Mackall wrote:
>> # HG changeset patch
>> # User Julien Cristau <julien.cristau@logilab.fr>
>> # Date 1398253895 -7200
>> #      Wed Apr 23 13:51:35 2014 +0200
>> # Branch stable
>> # Node ID 8a0d3c3e18434f39633ef165439763447beda7bf
>> # Parent  dae36d3e1c60465bfb56f210b4cb3b231f46e5dd
>> rebase: don't abort if we're asked to rebase an empty revset
> Queued for stable, thanks.

Just checking: Should it still be an abort error to specify an empty 
revset as source or base? Or should that also be a soft error?

/Mads
Matt Mackall - May 1, 2014, 1:41 p.m.
On Thu, 2014-05-01 at 03:04 +0200, Mads Kiilerich wrote:
> On 04/29/2014 09:24 PM, Matt Mackall wrote:
> >> # HG changeset patch
> >> # User Julien Cristau <julien.cristau@logilab.fr>
> >> # Date 1398253895 -7200
> >> #      Wed Apr 23 13:51:35 2014 +0200
> >> # Branch stable
> >> # Node ID 8a0d3c3e18434f39633ef165439763447beda7bf
> >> # Parent  dae36d3e1c60465bfb56f210b4cb3b231f46e5dd
> >> rebase: don't abort if we're asked to rebase an empty revset
> > Queued for stable, thanks.
> 
> Just checking: Should it still be an abort error to specify an empty 
> revset as source or base? Or should that also be a soft error?

We've documented an exit code of 1 for "nothing to rebase". One obvious
way to use revsets with rebase is "please move all my crap to the head",
with a revset like "not ::@" or "secret()" or "bookmark()". If those
happen to result in nothing to move, that's not user error, it's just a
no-op.
Mads Kiilerich - May 1, 2014, 1:54 p.m.
On 05/01/2014 03:41 PM, Matt Mackall wrote:
> On Thu, 2014-05-01 at 03:04 +0200, Mads Kiilerich wrote:
>> On 04/29/2014 09:24 PM, Matt Mackall wrote:
>>>> # HG changeset patch
>>>> # User Julien Cristau <julien.cristau@logilab.fr>
>>>> # Date 1398253895 -7200
>>>> #      Wed Apr 23 13:51:35 2014 +0200
>>>> # Branch stable
>>>> # Node ID 8a0d3c3e18434f39633ef165439763447beda7bf
>>>> # Parent  dae36d3e1c60465bfb56f210b4cb3b231f46e5dd
>>>> rebase: don't abort if we're asked to rebase an empty revset
>>> Queued for stable, thanks.
>> Just checking: Should it still be an abort error to specify an empty
>> revset as source or base? Or should that also be a soft error?
> We've documented an exit code of 1 for "nothing to rebase". One obvious
> way to use revsets with rebase is "please move all my crap to the head",
> with a revset like "not ::@" or "secret()" or "bookmark()". If those
> happen to result in nothing to move, that's not user error, it's just a
> no-op.

I still think it makes a difference whether there is nothing to rebase 
because the revisions the user care about already has been rebased, or 
if the user just don't care about any revisions. But fine, that is how 
you want it and that is how it is now.

But that was not my question. The question is if empty "source" or 
"base" revsets should be 1="nothing to rebase" or an abort?

/Mads

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -220,12 +220,13 @@  def rebase(ui, repo, **opts):
                 dest = scmutil.revsingle(repo, destf)
 
             if revf:
                 rebaseset = scmutil.revrange(repo, revf)
                 if not rebaseset:
-                    raise util.Abort(_('empty "rev" revision set - '
-                                       'nothing to rebase'))
+                    ui.status(_('empty "rev" revision set - '
+                                'nothing to rebase\n'))
+                    return 1
             elif srcf:
                 src = scmutil.revrange(repo, [srcf])
                 if not src:
                     raise util.Abort(_('empty "source" revision set - '
                                        'nothing to rebase'))
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
@@ -78,12 +78,12 @@  These fail:
   $ hg rebase --base 5 --rev 4
   abort: cannot specify both a revision and a base
   [255]
 
   $ hg rebase --rev '1 & !1'
-  abort: empty "rev" revision set - nothing to rebase
-  [255]
+  empty "rev" revision set - nothing to rebase
+  [1]
 
   $ hg rebase --source '1 & !1'
   abort: empty "source" revision set - nothing to rebase
   [255]