Patchwork [4,of,6] rebase: allow destination-free continue and abort (issue5513)

login
register
mail settings
Submitter Ryan McElroy
Date March 28, 2017, 9:31 p.m.
Message ID <ac6634fab80b59ba4895.1490736673@devbig314.prn1.facebook.com>
Download mbox | patch
Permalink /patch/19800/
State Changes Requested
Headers show

Comments

Ryan McElroy - March 28, 2017, 9:31 p.m.
# HG changeset patch
# User Ryan McElroy <rmcelroy@fb.com>
# Date 1490735056 25200
#      Tue Mar 28 14:04:16 2017 -0700
# Node ID ac6634fab80b59ba48958ca03289c4434e9483c4
# Parent  409a87200f6b079c9bfe1e85fd0715822346c402
rebase: allow destination-free continue and abort (issue5513)
via Mercurial-devel - March 29, 2017, 5:15 p.m.
On Tue, Mar 28, 2017 at 2:31 PM, Ryan McElroy <rm@fb.com> wrote:
> # HG changeset patch
> # User Ryan McElroy <rmcelroy@fb.com>
> # Date 1490735056 25200
> #      Tue Mar 28 14:04:16 2017 -0700
> # Node ID ac6634fab80b59ba48958ca03289c4434e9483c4
> # Parent  409a87200f6b079c9bfe1e85fd0715822346c402
> rebase: allow destination-free continue and abort (issue5513)
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -705,7 +705,7 @@ def rebase(ui, repo, **opts):
>              raise error.Abort(msg)
>
>          if ui.configbool('commands', 'rebase.requiredest'):
> -            if not destf:
> +            if not contf and not abortf and not destf:

Instead of saying here that we don't care about the destination being
missing here, why not move it just above the call to _definesets() so
we're checking it only when we do care about? We could even move it
inside _definesets(). I hesitated to suggest that at first, because I
thought it was a lower-level function that should not deal with
command line parsing, but it already does a fair bit of that, so it
seems fine to handle this case there too. What do you think?

>                  raise error.Abort(_('you must specify a destination'),
>                                    hint=_('use: hg rebase -d REV'))
>
> diff --git a/tests/test-rebase-dest.t b/tests/test-rebase-dest.t
> --- a/tests/test-rebase-dest.t
> +++ b/tests/test-rebase-dest.t
> @@ -55,6 +55,5 @@ Requiring dest should not break continue
>    (no more unresolved files)
>    continue: hg rebase --continue
>    $ hg rebase --continue
> -  abort: you must specify a destination
> -  (use: hg rebase -d REV)
> -  [255]
> +  rebasing 3:0537f6b50def "dc" (tip)
> +  saved backup bundle to $TESTTMP/repo/.hg/strip-backup/0537f6b50def-be4c7386-backup.hg (glob)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Ryan McElroy - March 30, 2017, 8:18 a.m.
On 3/29/17 6:15 PM, Martin von Zweigbergk wrote:
> On Tue, Mar 28, 2017 at 2:31 PM, Ryan McElroy <rm@fb.com> wrote:
>> # HG changeset patch
>> # User Ryan McElroy <rmcelroy@fb.com>
>> # Date 1490735056 25200
>> #      Tue Mar 28 14:04:16 2017 -0700
>> # Node ID ac6634fab80b59ba48958ca03289c4434e9483c4
>> # Parent  409a87200f6b079c9bfe1e85fd0715822346c402
>> rebase: allow destination-free continue and abort (issue5513)
>>
>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>> --- a/hgext/rebase.py
>> +++ b/hgext/rebase.py
>> @@ -705,7 +705,7 @@ def rebase(ui, repo, **opts):
>>               raise error.Abort(msg)
>>
>>           if ui.configbool('commands', 'rebase.requiredest'):
>> -            if not destf:
>> +            if not contf and not abortf and not destf:
> Instead of saying here that we don't care about the destination being
> missing here, why not move it just above the call to _definesets() so
> we're checking it only when we do care about? We could even move it
> inside _definesets(). I hesitated to suggest that at first, because I
> thought it was a lower-level function that should not deal with
> command line parsing, but it already does a fair bit of that, so it
> seems fine to handle this case there too. What do you think?

Looking at the code, this suggestion seems great. There are already a 
number of aborts in there for conflicting flags; this is an obvious 
addition there. Thanks for the suggestion! I'll send a v2 out in a bit 
after also playing around with Jun's drawdag suggestion to see if it 
helps me write more understandable tests.

>
>>                   raise error.Abort(_('you must specify a destination'),
>>                                     hint=_('use: hg rebase -d REV'))
>>
>> diff --git a/tests/test-rebase-dest.t b/tests/test-rebase-dest.t
>> --- a/tests/test-rebase-dest.t
>> +++ b/tests/test-rebase-dest.t
>> @@ -55,6 +55,5 @@ Requiring dest should not break continue
>>     (no more unresolved files)
>>     continue: hg rebase --continue
>>     $ hg rebase --continue
>> -  abort: you must specify a destination
>> -  (use: hg rebase -d REV)
>> -  [255]
>> +  rebasing 3:0537f6b50def "dc" (tip)
>> +  saved backup bundle to $TESTTMP/repo/.hg/strip-backup/0537f6b50def-be4c7386-backup.hg (glob)
>>

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -705,7 +705,7 @@  def rebase(ui, repo, **opts):
             raise error.Abort(msg)
 
         if ui.configbool('commands', 'rebase.requiredest'):
-            if not destf:
+            if not contf and not abortf and not destf:
                 raise error.Abort(_('you must specify a destination'),
                                   hint=_('use: hg rebase -d REV'))
 
diff --git a/tests/test-rebase-dest.t b/tests/test-rebase-dest.t
--- a/tests/test-rebase-dest.t
+++ b/tests/test-rebase-dest.t
@@ -55,6 +55,5 @@  Requiring dest should not break continue
   (no more unresolved files)
   continue: hg rebase --continue
   $ hg rebase --continue
-  abort: you must specify a destination
-  (use: hg rebase -d REV)
-  [255]
+  rebasing 3:0537f6b50def "dc" (tip)
+  saved backup bundle to $TESTTMP/repo/.hg/strip-backup/0537f6b50def-be4c7386-backup.hg (glob)