Re: [PATCH v2 02/10] merge: simplify parsing of "-n" option

["René Scharfe" <l.s.r@web.de>]

Am 31.08.23 um 23:17 schrieb Jeff King:
> The "-n" option is implemented by an option callback, as it is really a
> "reverse bool". If given, it sets show_diffstat to 0. In theory, when
> negated, it would set the same flag to 1. But it's not possible to
> trigger that, since short options cannot be negated.
>
> So in practice this is really just a SET_INT to 0. Let's use that
> instead, which shortens the code.
>
> Note that negation here would do the wrong thing (as with any SET_INT
> with a value of "0"). We could specify PARSE_OPT_NONEG to future-proof
> ourselves against somebody adding a long option name (which would make
> it possible to negate). But there's not much point:
>
>   1. Nobody is going to do that, because the negated form already
>      exists, and is called "--stat" (which is defined separately so that
>      "--no-stat" works).
>
>   2. If they did, the BUG() check added by 3284b93862 (parse-options:
>      disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and
>      that check is smart enough to realize that our short-only option is
>      OK).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/merge.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 53e9fe70d9..21363b7985 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -241,18 +241,9 @@ static int option_parse_strategy(const struct option *opt,
>  	return 0;
>  }
>
> -static int option_parse_n(const struct option *opt,
> -			  const char *arg, int unset)
> -{
> -	BUG_ON_OPT_ARG(arg);
> -	show_diffstat = unset;
> -	return 0;
> -}

Great -- the less custom callbacks, the better.

> -
>  static struct option builtin_merge_options[] = {
> -	OPT_CALLBACK_F('n', NULL, NULL, NULL,
> -		N_("do not show a diffstat at the end of the merge"),
> -		PARSE_OPT_NOARG, option_parse_n),
> +	OPT_SET_INT('n', NULL, &show_diffstat,
> +		N_("do not show a diffstat at the end of the merge"), 0),
>  	OPT_BOOL(0, "stat", &show_diffstat,
>  		N_("show a diffstat at the end of the merge")),

Makes it easier to see that we can replace the two complementary
definitions with a single one:

	OPT_NEGBIT('n', "no-stat",
		N_("do not show a diffstat at the end of the merge"), 1),

Which is a separate topic, of course.  And if we did that, however, ...

>  	OPT_BOOL(0, "summary", &show_diffstat, N_("(synonym to --stat)")),

... we wouldn't be able to use OPT_ALIAS here anymore (which we arguably
should) because it doesn't allow referencing the negated (or
"positivated") form -- but that is yet another separate topic.

René