Re: [PATCH] describe: fix --no-exact-match

[Jeff King <peff@peff.net>]

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