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

From: Jeff King <peff@peff.net>
Date: Aug 8, 2023, 21:27 UTC
Message-ID: <20230808212720.GA760752@coredump.intra.peff.net>
In-Reply-To: <4eea7e15-6594-93e2-27b5-3d6e3c0baac6@web.de>

On Fri, Jul 21, 2023 at 03:41:33PM +0200, René Scharfe wrote:

Show 8 quoted lines

> +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:

Show 9 quoted lines

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