Patchwork bisect: improve option validation message

login
register
mail settings
Submitter Brandon McCaig
Date June 9, 2017, 10:02 p.m.
Message ID <20170609220204.GB18212@test-chamber-1.castopulence.org>
Download mbox | patch
Permalink /patch/21300/
State Changes Requested
Headers show

Comments

Brandon McCaig - June 9, 2017, 10:02 p.m.
On Fri, Jun 09, 2017 at 03:50:14PM -0400, Augie Fackler wrote:
> It's a pareto improvement over what we've got, so I've queued
> it. I'd encourage some painting of the bike shed on the
> particular wording/formatting of the error message to be a bit
> more in line with our other messages as a follow-up.
> 
> Thanks!

Should I combine the patches or leave them separate? How about
this:

# HG changeset patch
# User Brandon McCaig <bamccaig@gmail.com>
# Date 1497044798 14400
#      Fri Jun 09 17:46:38 2017 -0400
# Node ID ef0370461d011cd21101ec2f72fd6fe09da31f90
# Parent  6720094c6deb90192b46569c43e0707a85bc8cca
bisect: Make option validation message more consistent with hg

Augie suggested improving upon the wording/formatting of the validation
messages as a follow up to a previous patch.



Regards,
Augie Fackler - June 10, 2017, 1:36 p.m.
> On Jun 9, 2017, at 6:02 PM, Brandon McCaig <bamccaig@gmail.com> wrote:
> 
> On Fri, Jun 09, 2017 at 03:50:14PM -0400, Augie Fackler wrote:
>> It's a pareto improvement over what we've got, so I've queued
>> it. I'd encourage some painting of the bike shed on the
>> particular wording/formatting of the error message to be a bit
>> more in line with our other messages as a follow-up.
>> 
>> Thanks!
> 
> Should I combine the patches or leave them separate? How about
> this:
> 
> # HG changeset patch
> # User Brandon McCaig <bamccaig@gmail.com>
> # Date 1497044798 14400
> #      Fri Jun 09 17:46:38 2017 -0400
> # Node ID ef0370461d011cd21101ec2f72fd6fe09da31f90
> # Parent  6720094c6deb90192b46569c43e0707a85bc8cca
> bisect: Make option validation message more consistent with hg
> 
> Augie suggested improving upon the wording/formatting of the validation
> messages as a follow up to a previous patch.
> 
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -9,6 +9,7 @@
> 
> import difflib
> import errno
> +import itertools
> import operator
> import os
> import random
> @@ -894,9 +895,20 @@
>             reset = True
>     elif extra:
>         raise error.Abort(_('incompatible arguments'))
> -    elif good + bad + skip + reset + extend + bool(command) > 1:
> -        raise error.Abort(_('-g|--good, -b|--bad, -s|--skip, -r|--reset, '
> -                            '-e|--extend, and -c|--command are exclusive'))
> +
> +    incompatibles = {
> +        '--bad': bad,
> +        '--command': bool(command),
> +        '--extend': extend,
> +        '--good': good,
> +        '--reset': reset,
> +        '--skip': skip,
> +    }
> +
> +    for pair in itertools.product(incompatibles.keys(), repeat=2):
> +      if pair[0] < pair[1]:

I’m not quite sure what this inequality is doing - can you elaborate? How about this construct?

for left, right in {tuple(sorted(pair)) for pair in itertools.permutations(incompatibles, 2)}:
  if incompatibles[left] and incompatibles[right]:
    raise error.Abort(…)

(Mine may also be way too clever - the set comprehension and the tuple(sorted(pair)) dance seems clear to me, but I wrote it.)


> +        if incompatibles[pair[0]] and incompatibles[pair[1]]:
> +          raise error.Abort(_('%s and %s are incompatible') % (pair))
> 
>     cmdutil.checkunfinished(repo)
> 
> diff --git a/tests/test-bisect.t b/tests/test-bisect.t
> --- a/tests/test-bisect.t
> +++ b/tests/test-bisect.t
> @@ -608,47 +608,47 @@
> 
>   $ hg bisect -r
>   $ hg bisect -b -c false
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --bad and --command are incompatible
>   [255]
>   $ hg bisect -b -e
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --bad and --extend are incompatible
>   [255]
>   $ hg bisect -b -g
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --bad and --good are incompatible
>   [255]
>   $ hg bisect -b -r
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --bad and --reset are incompatible
>   [255]
>   $ hg bisect -b -s
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --bad and --skip are incompatible
>   [255]
>   $ hg bisect -c false -e
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --command and --extend are incompatible
>   [255]
>   $ hg bisect -c false -g
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --command and --good are incompatible
>   [255]
>   $ hg bisect -c false -r
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --command and --reset are incompatible
>   [255]
>   $ hg bisect -c false -s
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --command and --skip are incompatible
>   [255]
>   $ hg bisect -e -g
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --extend and --good are incompatible
>   [255]
>   $ hg bisect -e -r
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --extend and --reset are incompatible
>   [255]
>   $ hg bisect -e -s
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --extend and --skip are incompatible
>   [255]
>   $ hg bisect -g -r
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --good and --reset are incompatible
>   [255]
>   $ hg bisect -g -s
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --good and --skip are incompatible
>   [255]
>   $ hg bisect -r -s
> -  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
> +  abort: --reset and --skip are incompatible
>   [255]
> 
> 
> Regards,
> 
> 
> -- 
> Brandon McCaig <bamccaig@gmail.com> <bambams@castopulence.org>
> Castopulence Software <https://www.castopulence.org/>
> Blog <http://www.bambams.ca/>
> perl -E '$_=q{V zrna gur orfg jvgu jung V fnl. }.
> q{Vg qbrfa'\''g nyjnlf fbhaq gung jnl.};
> tr/A-Ma-mN-Zn-z/N-Zn-zA-Ma-m/;say'
>

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -9,6 +9,7 @@ 
 
 import difflib
 import errno
+import itertools
 import operator
 import os
 import random
@@ -894,9 +895,20 @@ 
             reset = True
     elif extra:
         raise error.Abort(_('incompatible arguments'))
-    elif good + bad + skip + reset + extend + bool(command) > 1:
-        raise error.Abort(_('-g|--good, -b|--bad, -s|--skip, -r|--reset, '
-                            '-e|--extend, and -c|--command are exclusive'))
+
+    incompatibles = {
+        '--bad': bad,
+        '--command': bool(command),
+        '--extend': extend,
+        '--good': good,
+        '--reset': reset,
+        '--skip': skip,
+    }
+
+    for pair in itertools.product(incompatibles.keys(), repeat=2):
+      if pair[0] < pair[1]:
+        if incompatibles[pair[0]] and incompatibles[pair[1]]:
+          raise error.Abort(_('%s and %s are incompatible') % (pair))
 
     cmdutil.checkunfinished(repo)
 
diff --git a/tests/test-bisect.t b/tests/test-bisect.t
--- a/tests/test-bisect.t
+++ b/tests/test-bisect.t
@@ -608,47 +608,47 @@ 
 
   $ hg bisect -r
   $ hg bisect -b -c false
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --bad and --command are incompatible
   [255]
   $ hg bisect -b -e
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --bad and --extend are incompatible
   [255]
   $ hg bisect -b -g
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --bad and --good are incompatible
   [255]
   $ hg bisect -b -r
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --bad and --reset are incompatible
   [255]
   $ hg bisect -b -s
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --bad and --skip are incompatible
   [255]
   $ hg bisect -c false -e
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --command and --extend are incompatible
   [255]
   $ hg bisect -c false -g
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --command and --good are incompatible
   [255]
   $ hg bisect -c false -r
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --command and --reset are incompatible
   [255]
   $ hg bisect -c false -s
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --command and --skip are incompatible
   [255]
   $ hg bisect -e -g
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --extend and --good are incompatible
   [255]
   $ hg bisect -e -r
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --extend and --reset are incompatible
   [255]
   $ hg bisect -e -s
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --extend and --skip are incompatible
   [255]
   $ hg bisect -g -r
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --good and --reset are incompatible
   [255]
   $ hg bisect -g -s
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --good and --skip are incompatible
   [255]
   $ hg bisect -r -s
-  abort: -g|--good, -b|--bad, -s|--skip, -r|--reset, -e|--extend, and -c|--command are exclusive
+  abort: --reset and --skip are incompatible
   [255]