This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7641

rebase: use cmdutil.check_at_most_one_arg() for --confirm/--dry-run
ClosedPublic

Authored by martinvonz on Dec 13 2019, 2:53 AM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHGdaed70e95d60: rebase: use cmdutil.check_at_most_one_arg() for --confirm/--dry-run
Summary

I've also updated the helper to work with the hyphenated --dry-run
option.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Dec 13 2019, 2:53 AM
martinvonz retitled this revision from rebase: use cmdutil.check_unique_argument() for --confirm/--dry-run to rebase: use cmdutil.check_at_most_one_arg() for --confirm/--dry-run.Dec 16 2019, 6:18 PM
martinvonz removed a child revision: D7642: rebase: inline single-use `dryrun` and `confirm` variables.
martinvonz updated this revision to Diff 18767.
pulkit accepted this revision.Dec 17 2019, 2:15 AM
This revision is now accepted and ready to land.Dec 17 2019, 2:15 AM
pulkit added inline comments.Dec 17 2019, 2:29 AM
mercurial/cmdutil.py
273

Spotted in a later patch, need to replace _ back to - before printing output.

martinvonz added inline comments.Dec 17 2019, 10:16 AM
mercurial/cmdutil.py
273

Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?

yuja added a subscriber: yuja.Dec 17 2019, 10:32 AM

Spotted in a later patch, need to replace _ back to - before printing output.

Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?

Maybe he meant opts[b'dry_run'] vs --dry-run. *args should follow
underscore naming, but the printed option names don't.

pulkit added a comment.Dec 17 2019, 11:02 AM
In D7641#113027, @yuja wrote:

Spotted in a later patch, need to replace _ back to - before printing output.

Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?

Maybe he meant opts[b'dry_run'] vs --dry-run. *args should follow
underscore naming, but the printed option names don't.

Yep what Yuya said. Spotted in D7644, test-rebase-obsolete.t, line 490.

martinvonz added a comment.Dec 17 2019, 12:55 PM
In D7641#113027, @yuja wrote:

Spotted in a later patch, need to replace _ back to - before printing output.

Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?

Maybe he meant opts[b'dry_run'] vs --dry-run. *args should follow
underscore naming, but the printed option names don't.

In D7641#113029, @pulkit wrote:
In D7641#113027, @yuja wrote:

Spotted in a later patch, need to replace _ back to - before printing output.

Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?

Maybe he meant opts[b'dry_run'] vs --dry-run. *args should follow
underscore naming, but the printed option names don't.

Yep what Yuya said. Spotted in D7644, test-rebase-obsolete.t, line 490.

Oh, I would consider that a bug in that patch (I would have used dry-run if I had noticed). However, I can see how you would prefer that we pass in the underscore-separated name (since that's what we do with indexing into opts). I was concerned that there were existing flags that were underscore-separated in the UI, but there doesn't seem to be any (I checked by running the test suite with a hacked fancyopts.py). So I'll switch to using the underscore-separated names in the API.

martinvonz updated this revision to Diff 18819.Dec 17 2019, 1:10 PM
pulkit accepted this revision.Dec 18 2019, 12:55 AM
yuja added a comment.Dec 18 2019, 7:53 AM
  • a/mercurial/cmdutil.py

+++ b/mercurial/cmdutil.py
@@ -268,6 +268,7 @@

previous = None
for x in args:
    if opts.get(x):

+ x = x.replace(b'_', b'-')

if previous:
    raise error.Abort(
        _(b'cannot specify both --%s and --%s') % (previous, x)

Nit: better to return the unique argument in the original format, not
the dash-ed version.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (3 lines)
Mmercurial/cmdutil.py (1 line)

Diff 18852

hgext/rebase.py

inmemory = ui.configbool(b'rebase', b'experimental.inmemory') inmemory = ui.configbool(b'rebase', b'experimental.inmemory')
dryrun = opts.get(b'dry_run') dryrun = opts.get(b'dry_run')
confirm = opts.get(b'confirm') confirm = opts.get(b'confirm')
action = cmdutil.check_at_most_one_arg(opts, b'abort', b'stop', b'continue') action = cmdutil.check_at_most_one_arg(opts, b'abort', b'stop', b'continue')
if dryrun and action: if dryrun and action:
raise error.Abort(_(b'cannot specify both --dry-run and --%s') % action) raise error.Abort(_(b'cannot specify both --dry-run and --%s') % action)
if confirm and action: if confirm and action:
raise error.Abort(_(b'cannot specify both --confirm and --%s') % action) raise error.Abort(_(b'cannot specify both --confirm and --%s') % action)
if dryrun and confirm: cmdutil.check_at_most_one_arg(opts, b'confirm', b'dry_run')
raise error.Abort(_(b'cannot specify both --confirm and --dry-run'))
if action or repo.currenttransaction() is not None: if action or repo.currenttransaction() is not None:
# in-memory rebase is not compatible with resuming rebases. # in-memory rebase is not compatible with resuming rebases.
# (Or if it is run within a transaction, since the restart logic can # (Or if it is run within a transaction, since the restart logic can
# fail the entire transaction.) # fail the entire transaction.)
inmemory = False inmemory = False
if opts.get(b'auto_orphans'): if opts.get(b'auto_orphans'):

mercurial/cmdutil.py

def check_at_most_one_arg(opts, *args): def check_at_most_one_arg(opts, *args):
"""abort if more than one of the arguments are in opts """abort if more than one of the arguments are in opts
Returns the unique argument or None if none of them were specified. Returns the unique argument or None if none of them were specified.
""" """
previous = None previous = None
for x in args: for x in args:
if opts.get(x): if opts.get(x):
x = x.replace(b'_', b'-')
if previous: if previous:
raise error.Abort( raise error.Abort(
pulkitUnsubmitted
Not Done

Spotted in a later patch, need to replace _ back to - before printing output.

pulkit: Spotted in a later patch, need to replace `_` back to `-` before printing output.
martinvonzAuthorUnsubmitted
Done

Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?

martinvonz: Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand…
_(b'cannot specify both --%s and --%s') % (previous, x) _(b'cannot specify both --%s and --%s') % (previous, x)
) )
previous = x previous = x
return previous return previous
def check_incompatible_arguments(opts, first, *others): def check_incompatible_arguments(opts, first, *others):
"""abort if the first argument is given along with any of the others """abort if the first argument is given along with any of the others