On Fri, Jul 21, 2023 at 03:41:33PM +0200, René Scharfe wrote:
> +static int option_parse_exact_match(const struct option *opt, const char *arg,
> + int unset)
> +{
> + BUG_ON_OPT_ARG(arg);
> +
> + max_candidates = unset ? DEFAULT_CANDIDATES : 0;
> + return 0;
> +}
I wanted to call out a style question here. The "opt" parameter is
unused, since it manipulates the "max_candidates" global directly. I can
add an UNUSED annotation to satisfy -Wunused-parameter, but in such
cases I've usually been modifying them like:
int *value = opt->value;
...
*value = unset ? DEFAULT_CANDIDATES : 0;
so that the callback operates on the value passed in the options list.
But I see you converted that value to NULL here:
> @@ -568,8 +578,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
> OPT_BOOL(0, "long", &longformat, N_("always use long format")),
> OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
> OPT__ABBREV(&abbrev),
> - OPT_SET_INT(0, "exact-match", &max_candidates,
> - N_("only output exact matches"), 0),
> + OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
> + N_("only output exact matches"),
> + PARSE_OPT_NOARG, option_parse_exact_match),
so at least the result does not have the subtle gotcha that existed in
other cases I changed. :)
So before I sent a patch (either to switch to using opt->value, or to
add an UNUSED annotation), I wanted to see what you (or others) thought
between the two. I.e., should we have a rule of "try not to operate on
global data via option callbacks" or is that just being too pedantic for
one-off callbacks like this?
-Peff
