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

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

martinvonz added a commit: rHGdaed70e95d60: rebase: use cmdutil.check_at_most_one_arg() for --confirm/--dry-run.Dec 18 2019, 1:46 AM

Closed by commit rHGdaed70e95d60: rebase: use cmdutil.check_at_most_one_arg() for --confirm/--dry-run (authored by martinvonz). · Explain Why

This revision was automatically updated to reflect the committed changes.

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.

			Path	Packages
M			hgext/rebase.py (3 lines)
M			mercurial/cmdutil.py (2 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	18676		Dec 13 2019, 2:53 AM	★	★
Diff 2	18710		Dec 13 2019, 5:56 PM	★	★
Diff 3	18767		Dec 16 2019, 6:18 PM	★	★
Diff 4	18819		Dec 17 2019, 1:10 PM	★	★
Diff 5	18852	rHGdaed70e95d604980d295e4b38b59a5cc8b5ab542	Dec 13 2019, 5:49 PM	★	★

	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_unique_argument(opts, b'abort', b'stop', b'continue')			action = cmdutil.check_unique_argument(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_unique_argument(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'):


	def check_unique_argument(opts, *args):			def check_unique_argument(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.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

This is an archive of the discontinued Mercurial Phabricator instance.

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 18710

hgext/rebase.py

mercurial/cmdutil.py