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@xxxxxxxx>
> ---
> 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é
