( )⚙ D7633 clone: extract helper for checking mutually exclusive args

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7633

clone: extract helper for checking mutually exclusive args
ClosedPublic

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

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHGd587937600be: clone: extract helper for checking mutually exclusive args
Summary

We have some duplicated code for aborting if the user provided
mutually exclusive arguments. Extensions surely have more such
code. We also have duplicated translations and inconsistent output in
this area.

This patch introduces a simpler helper for checking if more than one
option among a given set was given on the command line. I've made the
clone code call the function to show that it works.

The function has no good way of checking arguments with hyphens in
them. I'll add that later if necessary. The function still won't be
applicable in all cases, but I think it's still better than nothing.

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
dlax added a subscriber: dlax.Dec 13 2019, 3:18 AM

Useful refactoring!

mercurial/cmdutil.py
263

The function name is a bit misleading; it looks like it would check that an option wasn't specified multiple times. Perhaps check_exclusive_arguments?

martinvonz added inline comments.Dec 13 2019, 3:24 AM
mercurial/cmdutil.py
263

I don't think I'd be able to guess what a function by that name did either. check_at_most_one_argument seems clearer, but maybe too long. I'll wait a bit for other ideas before I change anything.

pulkit added a subscriber: pulkit.Dec 16 2019, 12:51 AM
pulkit added inline comments.
mercurial/cmdutil.py
263

I agree that the current name can be bit misleading. check_at_most_one_argument seems clearer. Maybe argument -> arg will make it bit smaller ;)

martinvonz updated this revision to Diff 18759.Dec 16 2019, 6:18 PM
martinvonz added inline comments.Dec 16 2019, 6:18 PM
mercurial/cmdutil.py
263

Done. It ended up exactly the same length with the arugment -> arg change :)

pulkit accepted this revision.Dec 17 2019, 2:09 AM
This revision is now accepted and ready to land.Dec 17 2019, 2:09 AM

Revision Contents
Changeset List

PathPackages
Mmercurial/cmdutil.py (12 lines)
Mmercurial/commands.py (3 lines)

Diff 18759

mercurial/cmdutil.py

(b'', b'dir', b'', _(b'open directory manifest')), (b'', b'dir', b'', _(b'open directory manifest')),
] ]
# special string such that everything below this line will be ingored in the # special string such that everything below this line will be ingored in the
# editor text # editor text
_linebelow = b"^HG: ------------------------ >8 ------------------------$" _linebelow = b"^HG: ------------------------ >8 ------------------------$"
def check_at_most_one_arg(opts, *args):
dlaxUnsubmitted
Not Done

The function name is a bit misleading; it looks like it would check that an option wasn't specified multiple times. Perhaps check_exclusive_arguments?

dlax: The function name is a bit misleading; it looks like it would check that an option wasn't…
martinvonzAuthorUnsubmitted
Done

I don't think I'd be able to guess what a function by that name did either. check_at_most_one_argument seems clearer, but maybe too long. I'll wait a bit for other ideas before I change anything.

martinvonz: I don't think I'd be able to guess what a function by that name did either.
pulkitUnsubmitted
Not Done

I agree that the current name can be bit misleading. check_at_most_one_argument seems clearer. Maybe argument -> arg will make it bit smaller ;)

pulkit: I agree that the current name can be bit misleading. `check_at_most_one_argument` seems clearer.
martinvonzAuthorUnsubmitted
Done

Done. It ended up exactly the same length with the arugment -> arg change :)

martinvonz: Done. It ended up exactly the same length with the `arugment` -> `arg` change :)
"""abort if more than one of the arguments are in opts"""
previous = None
for x in args:
if opts.get(x):
if previous:
raise error.Abort(
_(b'cannot specify both --%s and --%s') % (previous, x)
)
previous = x
def resolvecommitoptions(ui, opts): def resolvecommitoptions(ui, opts):
"""modify commit options dict to handle related options """modify commit options dict to handle related options
The return value indicates that ``rewrite.update-timestamp`` is the reason The return value indicates that ``rewrite.update-timestamp`` is the reason
the ``date`` option is set. the ``date`` option is set.
""" """
if opts.get(b'date') and opts.get(b'currentdate'): if opts.get(b'date') and opts.get(b'currentdate'):
raise error.Abort(_(b'--date and --currentdate are mutually exclusive')) raise error.Abort(_(b'--date and --currentdate are mutually exclusive'))

mercurial/commands.py

hg clone https://www.mercurial-scm.org/repo/hg/#stable hg clone https://www.mercurial-scm.org/repo/hg/#stable
See :hg:`help urls` for details on specifying URLs. See :hg:`help urls` for details on specifying URLs.
Returns 0 on success. Returns 0 on success.
""" """
opts = pycompat.byteskwargs(opts) opts = pycompat.byteskwargs(opts)
if opts.get(b'noupdate') and opts.get(b'updaterev'): cmdutil.check_at_most_one_arg(opts, b'noupdate', b'updaterev')
raise error.Abort(_(b"cannot specify both --noupdate and --updaterev"))
# --include/--exclude can come from narrow or sparse. # --include/--exclude can come from narrow or sparse.
includepats, excludepats = None, None includepats, excludepats = None, None
# hg.clone() differentiates between None and an empty set. So make sure # hg.clone() differentiates between None and an empty set. So make sure
# patterns are sets if narrow is requested without patterns. # patterns are sets if narrow is requested without patterns.
if opts.get(b'narrow'): if opts.get(b'narrow'):
includepats = set() includepats = set()