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
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
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)