This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Details

Summary

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

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

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

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
272

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.

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.

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#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.