| Message ID | 20230831211716.GB949469@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7fa701106d485f7bf9d042f822d4366d7059f8ba |
| Headers | show |
| Series | more unused parameters in parseopt callbacks | expand |
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é
On Sat, Sep 02, 2023 at 08:20:28AM +0200, René Scharfe wrote: > > 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, ... Ah, I thought we had a "reverse bool" of some kind, but I couldn't find it. NEGBIT was what I was looking for. But yeah, I agree it gets more complicated with the various aliases. I think what I have here is a good stopping point for this series, but if you want to go further on it, be my guest. :) -Peff
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; -} - 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")), OPT_BOOL(0, "summary", &show_diffstat, N_("(synonym to --stat)")),
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(-)