diff mbox series

[v2,1/6] parse-options: recognize abbreviated negated option with arg

Message ID 20240303121944.20627-2-l.s.r@web.de (mailing list archive)
State New, archived
Headers show
Series parse-options: long option parsing fixes and cleanup | expand

Commit Message

René Scharfe March 3, 2024, 12:19 p.m. UTC
Giving an argument to an option that doesn't take one causes Git to
report that error specifically:

   $ git rm --dry-run=bogus
   error: option `dry-run' takes no value

The same is true when the option is negated or abbreviated:

   $ git rm --no-dry-run=bogus
   error: option `no-dry-run' takes no value

   $ git rm --dry=bogus
   error: option `dry-run' takes no value

Not so when doing both, though:

   $ git rm --no-dry=bogus
   error: unknown option `no-dry=bogus'
   usage: git rm [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]

(Rest of the usage message omitted.)

Improve consistency and usefulness of the error message by recognizing
abbreviated negated options even if they have a (most likely bogus)
argument.  With this patch we get:

   $ git rm --no-dry=bogus
   error: option `no-dry-run' takes no value

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 parse-options.c          |  5 +++--
 t/t0040-parse-options.sh | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

--
2.44.0

Comments

Josh Steadmon March 12, 2024, 9:42 p.m. UTC | #1
On 2024.03.03 13:19, René Scharfe wrote:
> Giving an argument to an option that doesn't take one causes Git to
> report that error specifically:
> 
>    $ git rm --dry-run=bogus
>    error: option `dry-run' takes no value
> 
> The same is true when the option is negated or abbreviated:
> 
>    $ git rm --no-dry-run=bogus
>    error: option `no-dry-run' takes no value
> 
>    $ git rm --dry=bogus
>    error: option `dry-run' takes no value
> 
> Not so when doing both, though:
> 
>    $ git rm --no-dry=bogus
>    error: unknown option `no-dry=bogus'
>    usage: git rm [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]
> 
> (Rest of the usage message omitted.)
> 
> Improve consistency and usefulness of the error message by recognizing
> abbreviated negated options even if they have a (most likely bogus)
> argument.  With this patch we get:
> 
>    $ git rm --no-dry=bogus
>    error: option `no-dry-run' takes no value
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  parse-options.c          |  5 +++--
>  t/t0040-parse-options.sh | 16 ++++++++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index 63a99dea6e..e4ce33ea48 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -391,7 +391,7 @@ static enum parse_opt_result parse_long_opt(
>  					ambiguous_option = abbrev_option;
>  					ambiguous_flags = abbrev_flags;
>  				}
> -				if (!(flags & OPT_UNSET) && *arg_end)
> +				if (*arg_end)
>  					p->opt = arg_end + 1;
>  				abbrev_option = options;
>  				abbrev_flags = flags ^ opt_flags;

So this part allows us to recognize when we've attached an option onto a
negated flag (like "--no-foo=bar"). Looks good.


> @@ -412,7 +412,8 @@ static enum parse_opt_result parse_long_opt(
>  			if (!skip_prefix(arg + 3, long_name, &rest)) {
>  				/* abbreviated and negated? */
>  				if (allow_abbrev &&
> -				    starts_with(long_name, arg + 3))
> +				    !strncmp(long_name, arg + 3,
> +					     arg_end - arg - 3))
>  					goto is_abbreviated;
>  				else
>  					continue;

And now we fix the abbreviation match. So if arg="no-foo=bar", (arg+3) =
"foo=bar" and thus never matches a long_name. We fix that by only
comparing up to the first equal sign. Also looks good.

> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index ec974867e4..8bb2a8b453 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -210,6 +210,22 @@ test_expect_success 'superfluous value provided: boolean' '
>  	test_cmp expect actual
>  '
> 
> +test_expect_success 'superfluous value provided: boolean, abbreviated' '
> +	cat >expect <<-\EOF &&
> +	error: option `yes'\'' takes no value
> +	EOF
> +	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
> +	test-tool parse-options --ye=hi 2>actual &&
> +	test_cmp expect actual &&
> +
> +	cat >expect <<-\EOF &&
> +	error: option `no-yes'\'' takes no value
> +	EOF
> +	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
> +	test-tool parse-options --no-ye=hi 2>actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'superfluous value provided: cmdmode' '
>  	cat >expect <<-\EOF &&
>  	error: option `mode1'\'' takes no value
> --
> 2.44.0

Test looks good as well.
diff mbox series

Patch

diff --git a/parse-options.c b/parse-options.c
index 63a99dea6e..e4ce33ea48 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -391,7 +391,7 @@  static enum parse_opt_result parse_long_opt(
 					ambiguous_option = abbrev_option;
 					ambiguous_flags = abbrev_flags;
 				}
-				if (!(flags & OPT_UNSET) && *arg_end)
+				if (*arg_end)
 					p->opt = arg_end + 1;
 				abbrev_option = options;
 				abbrev_flags = flags ^ opt_flags;
@@ -412,7 +412,8 @@  static enum parse_opt_result parse_long_opt(
 			if (!skip_prefix(arg + 3, long_name, &rest)) {
 				/* abbreviated and negated? */
 				if (allow_abbrev &&
-				    starts_with(long_name, arg + 3))
+				    !strncmp(long_name, arg + 3,
+					     arg_end - arg - 3))
 					goto is_abbreviated;
 				else
 					continue;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ec974867e4..8bb2a8b453 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -210,6 +210,22 @@  test_expect_success 'superfluous value provided: boolean' '
 	test_cmp expect actual
 '

+test_expect_success 'superfluous value provided: boolean, abbreviated' '
+	cat >expect <<-\EOF &&
+	error: option `yes'\'' takes no value
+	EOF
+	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test-tool parse-options --ye=hi 2>actual &&
+	test_cmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	error: option `no-yes'\'' takes no value
+	EOF
+	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test-tool parse-options --no-ye=hi 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'superfluous value provided: cmdmode' '
 	cat >expect <<-\EOF &&
 	error: option `mode1'\'' takes no value