Re: [PATCH 1/4] parse-options: convert "command mode" to a flag

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 19, 2020, 19:15 UTC
Message-ID: <xmqqzhdee6c6.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20200219161352.13562-2-pbonzini@redhat.com>

pbonzini@redhat.com writes:

Show 7 quoted lines

> From: Paolo Bonzini <pbonzini@redhat.com>
>
> OPTION_CMDMODE is essentially OPTION_SET_INT plus the extra check
> that the variable had not set before.  In order to allow custom
> processing, change it to OPTION_SET_INT plus a new flag that takes
> care of the check.  This works as long as the option value points
> to an int.

It is unclear but I am guessing that the purpose of this change is to make "only one of these" orthgonal to "the value of this option is an int", in preparation to allow options other than SET_INT to also be combined with "only one of these"?

If my reading is not correct, that would be an indication that the above paragraph does not tell what it wants to to readers.

It is unclear at this step what other kind of option the flag wants to be combined, though.

Show 25 quoted lines

> diff --git a/parse-options.c b/parse-options.c
> index a0cef401fc..c6e9e2733b 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -61,7 +61,7 @@ static enum parse_opt_result opt_command_mode_error(
>  	 */
>  	for (that = all_opts; that->type != OPTION_END; that++) {
>  		if (that == opt ||
> -		    that->type != OPTION_CMDMODE ||
> +		    !(that->flags & PARSE_OPT_CMDMODE) ||
>  		    that->value != opt->value ||
>  		    that->defval != *(int *)opt->value)
>  			continue;
> @@ -95,6 +95,14 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>  	if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))
>  		return error(_("%s takes no value"), optname(opt, flags));
>  
> +	/*
> +	 * Giving the same mode option twice, although unnecessary,
> +	 * is not a grave error, so let it pass.
> +	 */
> +	if ((opt->flags & PARSE_OPT_CMDMODE) &&
> +	    *(int *)opt->value && *(int *)opt->value != opt->defval)
> +		return opt_command_mode_error(opt, all_opts, flags);
> +

... and when there is no error, we fall through and process it as a regular SET_INT, which makes sense.

Show 8 quoted lines

> @@ -168,8 +168,8 @@ struct option {
>  #define OPT_BOOL(s, l, v, h)        OPT_BOOL_F(s, l, v, h, 0)
>  #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
>  				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
> -#define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \
> -				      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
> +#define OPT_CMDMODE(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
> +				      (h), PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }

OK.

Show 49 quoted lines

> diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
> index af82db06ac..2051ce57db 100644
> --- a/t/helper/test-parse-options.c
> +++ b/t/helper/test-parse-options.c
> @@ -121,6 +121,8 @@ int cmd__parse_options(int argc, const char **argv)
>  		OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
>  		OPT_MAGNITUDE('m', "magnitude", &magnitude, "get a magnitude"),
>  		OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
> +		OPT_CMDMODE(0, "mode1", &integer, "set integer to 1 (cmdmode option)", 1),
> +		OPT_CMDMODE(0, "mode2", &integer, "set integer to 2 (cmdmode option)", 2),
>  		OPT_CALLBACK('L', "length", &integer, "str",
>  			"get length of <str>", length_callback),
>  		OPT_FILENAME('F', "file", &file, "set file to <file>"),
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index 9d7c7fdaa2..7f4c15a52b 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -23,6 +23,8 @@ usage: test-tool parse-options <options>
>      -j <n>                get a integer, too
>      -m, --magnitude <n>   get a magnitude
>      --set23               set integer to 23
> +    --mode1               set integer to 1 (cmdmode option)
> +    --mode2               set integer to 2 (cmdmode option)
>      -L, --length <str>    get length of <str>
>      -F, --file <file>     set file to <file>
>  
> @@ -324,6 +326,22 @@ test_expect_success 'OPT_NEGBIT() works' '
>  	test-tool parse-options --expect="boolean: 6" -bb --no-neg-or4
>  '
>  
> +test_expect_success 'OPT_CMDMODE() works' '
> +	test-tool parse-options --expect="integer: 1" --mode1
> +'
> +
> +test_expect_success 'OPT_CMDMODE() detects incompatibility' '
> +	test_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&
> +	test_must_be_empty output &&
> +	grep "incompatible with --mode" output.err
> +'
> +
> +test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '
> +	test_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&
> +	test_must_be_empty output &&
> +	grep "incompatible with something else" output.err
> +'
> +
>  test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '
>  	test-tool parse-options --expect="boolean: 6" + + + + + +
>  '

Would the updated test-parse-options.c with these three new tests work the same way with or without changes to parse-options.[ch]? That would be a good indication that the change to the code is "upward compatible".

Thanks.