'git branch --no-merge' is ambiguous

'git branch --no-merge' is ambiguous

[Johannes Sixt]

Look here:

 $ git branch --merge
 * master
 $ git branch --no-merge
 error: Ambiguous option: no-merge (could be --no-merged or --no-merged)
 usage: ...
     --no-merged <commit>  print only not merged branches
     --merged <commit>     print only merged branches

I tried to debug it, but parse_long_opt() is such awful spaghetti code
that I don't grok it. Please help.

-- Hannes

Re: 'git branch --no-merge' is ambiguous

[Andreas Schwab]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Look here:
>
>  $ git branch --merge
>  * master
>  $ git branch --no-merge
>  error: Ambiguous option: no-merge (could be --no-merged or --no-merged)
>  usage: ...
>      --no-merged <commit>  print only not merged branches
>      --merged <commit>     print only merged branches
>
> I tried to debug it, but parse_long_opt() is such awful spaghetti code
> that I don't grok it. Please help.

parse_long_opt always matches both --opt and --no-opt for any option
"opt", and only get_value checks whether --no-opt is actually valid.
Since the options for git branch contains both "no-merged" and "merged"
there are two matches for --no-merge, but no exact match.  With this
patch the negation of a NONEG option is rejected earlier, but it changes
the error message from "option `no-opt' isn't available" to "unknown
option `no-opt'".

diff --git a/parse-options.c b/parse-options.c
index a64a4d6..f559411 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -230,6 +230,9 @@ is_abbreviated:
 				abbrev_flags = flags;
 				continue;
 			}
+			/* negation allowed? */
+			if (options->flags & PARSE_OPT_NONEG)
+				continue;
 			/* negated and abbreviated very much? */
 			if (!prefixcmp("no-", arg)) {
 				flags |= OPT_UNSET;

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: 'git branch --no-merge' is ambiguous

[Jeff King]

On Fri, Sep 25, 2009 at 08:44:44PM +0200, Andreas Schwab wrote:

> parse_long_opt always matches both --opt and --no-opt for any option
> "opt", and only get_value checks whether --no-opt is actually valid.
> Since the options for git branch contains both "no-merged" and "merged"
> there are two matches for --no-merge, but no exact match.  With this
> patch the negation of a NONEG option is rejected earlier, but it changes
> the error message from "option `no-opt' isn't available" to "unknown
> option `no-opt'".

Thanks. Reading through the code, I came to the same conclusion: we
shouldn't be looking at --no-* at all if we are NONEG. I think the
change in error message is acceptable.

It is a little bit annoying that builtin-branch needs to have this as
two separate options in the first place. But it wants to be able to do
--no-merged with an argument, which is not currently possible with just
a negation of --merged. I don't know if it is worth adding an
OPT_NEGARG.

Below is what I'm going to commit.  Can I get your Signed-off-by?

-- >8 --
From: Andreas Schwab <schwab@linux-m68k.org>
Subject: [PATCH] parse-opt: ignore negation of OPT_NONEG for ambiguity checks

parse_long_opt always matches both --opt and --no-opt for any option
"opt", and only get_value checks whether --no-opt is actually valid.
Since the options for git branch contains both "no-merged" and "merged"
there are two matches for --no-merge, but no exact match.  With this
patch the negation of a NONEG option is rejected earlier, but it changes
the error message from "option `no-opt' isn't available" to "unknown
option `no-opt'".

[jk: added test]

Signed-off-by: Jeff King <peff@peff.net>
---
 parse-options.c          |    3 +++
 t/t0040-parse-options.sh |   20 ++++++++++++++++++++
 test-parse-options.c     |    5 +++++
 3 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index a64a4d6..f559411 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -230,6 +230,9 @@ is_abbreviated:
 				abbrev_flags = flags;
 				continue;
 			}
+			/* negation allowed? */
+			if (options->flags & PARSE_OPT_NONEG)
+				continue;
 			/* negated and abbreviated very much? */
 			if (!prefixcmp("no-", arg)) {
 				flags |= OPT_UNSET;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index bbc821e..3d450ed 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -33,6 +33,8 @@ Magic arguments
     --quux                means --quux
     -NUM                  set integer to NUM
     +                     same as -b
+    --ambiguous           positive ambiguity
+    --no-ambiguous        negative ambiguity
 
 Standard options
     --abbrev[=<n>]        use <n> digits to display SHA-1s
@@ -315,4 +317,22 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '
 	test_cmp expect output
 '
 
+cat >expect <<EOF
+boolean: 0
+integer: 0
+timestamp: 0
+string: (not set)
+abbrev: 7
+verbose: 0
+quiet: no
+dry run: no
+file: (not set)
+EOF
+
+test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
+	test-parse-options --no-ambig >output 2>output.err &&
+	test ! -s output.err &&
+	test_cmp expect output
+'
+
 test_done
diff --git a/test-parse-options.c b/test-parse-options.c
index efa734b..acd1a2b 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -8,6 +8,7 @@ static int abbrev = 7;
 static int verbose = 0, dry_run = 0, quiet = 0;
 static char *string = NULL;
 static char *file = NULL;
+static int ambiguous;
 
 static int length_callback(const struct option *opt, const char *arg, int unset)
 {
@@ -59,6 +60,10 @@ int main(int argc, const char **argv)
 			number_callback),
 		{ OPTION_BOOLEAN, '+', NULL, &boolean, NULL, "same as -b",
 		  PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_NODASH },
+		{ OPTION_BOOLEAN, 0, "ambiguous", &ambiguous, NULL,
+		  "positive ambiguity", PARSE_OPT_NOARG | PARSE_OPT_NONEG },
+		{ OPTION_BOOLEAN, 0, "no-ambiguous", &ambiguous, NULL,
+		  "negative ambiguity", PARSE_OPT_NOARG | PARSE_OPT_NONEG },
 		OPT_GROUP("Standard options"),
 		OPT__ABBREV(&abbrev),
 		OPT__VERBOSE(&verbose),
-- 
1.6.5.rc2.197.g25cf3

Re: 'git branch --no-merge' is ambiguous

[Andreas Schwab]

Jeff King <peff@peff.net> writes:

> -- >8 --
> From: Andreas Schwab <schwab@linux-m68k.org>
> Subject: [PATCH] parse-opt: ignore negation of OPT_NONEG for ambiguity checks
>
> parse_long_opt always matches both --opt and --no-opt for any option
> "opt", and only get_value checks whether --no-opt is actually valid.
> Since the options for git branch contains both "no-merged" and "merged"
> there are two matches for --no-merge, but no exact match.  With this
> patch the negation of a NONEG option is rejected earlier, but it changes
> the error message from "option `no-opt' isn't available" to "unknown
> option `no-opt'".
>
> [jk: added test]
>
> Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."