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é