Patchwork bisect: improve option validation message

login
register
mail settings
Submitter Brandon McCaig
Date June 10, 2017, 9:43 p.m.
Message ID <20170610214332.GC18212@test-chamber-1.castopulence.org>
Download mbox | patch
Permalink /patch/21322/
State Superseded
Headers show

Comments

Brandon McCaig - June 10, 2017, 9:43 p.m.
On Sat, Jun 10, 2017 at 09:36:25AM -0400, Augie Fackler wrote:
> > +    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?

The comparison was to eliminate matching pairs and duplicates (I
was not aware of permuations or combinations at the time, though
I wonder if this is any slower).

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

This is similar to itertools.combinations() that 'sfink'
suggested in IRC, which I think fits a bit better since we don't
need all permutations.

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

It was a bit to take in at first. I guess the set comprehension
is there to eliminate the redundant permutations, which
combinations() can do for us instead. I don't like how long the
line gets either.

Now without the equality check sorting is necessary if we care
what order the checks are done in. I prefer it this way, but
maybe others don't care? I guess sorting the keys should be as
good as anything. Unpacking the tuple is probably nicer than
indexing the tuple too (though it works out to more characters
here). So how about this:

# HG changeset patch
# User Brandon McCaig <bamccaig@gmail.com>
# Date 1497053559 14400
#      Fri Jun 09 20:12:39 2017 -0400
# Node ID d1f2ee1cefc15f9043139c835487917d0831485c
# Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
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, 10:45 p.m.
> On Jun 10, 2017, at 5:43 PM, Brandon McCaig <bamccaig@gmail.com> wrote:
> 
> # HG changeset patch
> # User Brandon McCaig <bamccaig@gmail.com>
> # Date 1497053559 14400
> #      Fri Jun 09 20:12:39 2017 -0400
> # Node ID d1f2ee1cefc15f9043139c835487917d0831485c
> # Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
> bisect: Make option validation message more consistent with hg

This looks great, queued.

I’m about to wander away from my laptop for supper, so if you’d rather I push the two patches as one folded-together patch let me know. If I don’t hear anything in the next few hours I’ll just push as-is with two commits.

Thanks!

> 
> 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 os
> import re
> import sys
> @@ -770,9 +771,19 @@
>             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 left, right in itertools.combinations(sorted(incompatibles), 2):
> +      if incompatibles[left] and incompatibles[right]:
> +        raise error.Abort(_('%s and %s are incompatible') % (left, right))
> 
>     if reset:
>         hbisect.resetstate(repo)
> diff --git a/tests/test-bisect.t b/tests/test-bisect.t
> --- a/tests/test-bisect.t
> +++ b/tests/test-bisect.t
> @@ -615,47 +615,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'
>
Yuya Nishihara - June 11, 2017, 5:14 a.m.
On Sat, 10 Jun 2017 18:45:45 -0400, Augie Fackler wrote:
> 
> > On Jun 10, 2017, at 5:43 PM, Brandon McCaig <bamccaig@gmail.com> wrote:
> > 
> > # HG changeset patch
> > # User Brandon McCaig <bamccaig@gmail.com>
> > # Date 1497053559 14400
> > #      Fri Jun 09 20:12:39 2017 -0400
> > # Node ID d1f2ee1cefc15f9043139c835487917d0831485c
> > # Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
> > bisect: Make option validation message more consistent with hg
> 
> This looks great, queued.
> 
> I’m about to wander away from my laptop for supper, so if you’d rather I push the two patches as one folded-together patch let me know. If I don’t hear anything in the next few hours I’ll just push as-is with two commits.

Folded them as fbe9c4dcc8a0 "bisect: improve option validation message".

> > +    incompatibles = {
> > +        '--bad': bad,
> > +        '--command': bool(command),
> > +        '--extend': extend,
> > +        '--good': good,
> > +        '--reset': reset,
> > +        '--skip': skip,
> > +    }
> > +
> > +    for left, right in itertools.combinations(sorted(incompatibles), 2):
> > +      if incompatibles[left] and incompatibles[right]:
> > +        raise error.Abort(_('%s and %s are incompatible') % (left, right))

Perhaps we can filter out False elements first and just take the first two?

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 os
 import re
 import sys
@@ -770,9 +771,19 @@ 
             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 left, right in itertools.combinations(sorted(incompatibles), 2):
+      if incompatibles[left] and incompatibles[right]:
+        raise error.Abort(_('%s and %s are incompatible') % (left, right))
 
     if reset:
         hbisect.resetstate(repo)
diff --git a/tests/test-bisect.t b/tests/test-bisect.t
--- a/tests/test-bisect.t
+++ b/tests/test-bisect.t
@@ -615,47 +615,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]