Patchwork D7633: clone: extract helper for checking mutually exclusive args

login
register
mail settings
Submitter phabricator
Date Dec. 13, 2019, 7:56 a.m.
Message ID <differential-rev-PHID-DREV-7pst3ev7ep5ef5wnawyt-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43783/
State Superseded
Headers show

Comments

phabricator - Dec. 13, 2019, 7:56 a.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

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

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D7633

AFFECTED FILES
  mercurial/cmdutil.py
  mercurial/commands.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 13, 2019, 8:18 a.m.
dlax added a comment.


  Useful refactoring!

INLINE COMMENTS

> cmdutil.py:263
>  
> +def check_unique_argument(opts, *args):
> +    """abort if more than one of the arguments are in opts"""

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7633/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7633

To: martinvonz, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Dec. 13, 2019, 8:24 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> dlax wrote in 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`?

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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7633/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7633

To: martinvonz, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Dec. 16, 2019, 5:51 a.m.
pulkit added inline comments.

INLINE COMMENTS

> martinvonz wrote in 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.

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7633/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7633

To: martinvonz, #hg-reviewers
Cc: pulkit, dlax, mercurial-devel
phabricator - Dec. 16, 2019, 11:20 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> pulkit wrote in 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 ;)

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7633/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7633

To: martinvonz, #hg-reviewers
Cc: pulkit, dlax, mercurial-devel

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1889,8 +1889,7 @@ 
     Returns 0 on success.
     """
     opts = pycompat.byteskwargs(opts)
-    if opts.get(b'noupdate') and opts.get(b'updaterev'):
-        raise error.Abort(_(b"cannot specify both --noupdate and --updaterev"))
+    cmdutil.check_unique_argument(opts, b'noupdate', b'updaterev')
 
     # --include/--exclude can come from narrow or sparse.
     includepats, excludepats = None, None
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -260,6 +260,18 @@ 
 _linebelow = b"^HG: ------------------------ >8 ------------------------$"
 
 
+def check_unique_argument(opts, *args):
+    """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):
     """modify commit options dict to handle related options