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

["René Scharfe" <l.s.r@web.de>]

Am 09.08.23 um 16:09 schrieb Jeff King:
> On Tue, Aug 08, 2023 at 06:43:41PM -0700, Junio C Hamano wrote:
>
>>> 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?
>>
>> So, that was my preference, but I may be missing some obvious
>> downsides.  I am interested in hearing from René, who often shows
>> better taste than I do in these cases ;-)
>
> Me too. :)

I'm pretty sure I didn't think much about it, copied the style from the
other callback functions in builtin/pack-objects.c and was glad to not
have to deal with void pointers.

And sorry for the unused parameter warning.  Just checked; there are
170+ of those remaining before we can enable it in developer mode.  :-/
Seems worthwhile, though, especially not slapping UNUSED blindly on
them.

> The main downsides, I think, are:
>
>   1. It's a little more ugly.
>
>   2. We lose type safety, as the variable address passes through a void
>      pointer (but that is true of all option callbacks).

An upside is that we can reuse the callback if we are careful to wire
it up to a variable of the correct type.  Another is that we can use
it on local variables.

Hmm, we could make these callbacks type-safe fairly easily by adding
pointers of all relevant types to struct option, like "int *value_int".
How many types would we need?

   $ git grep -h ' = opt->value;' | sed 's/\*.*$//; s/^ *//' | sort -u | wc -l
   37

Oh.  Do we really need all those?  Anyway, if we added at least the
most common ones, we'd be better off already, I'd expect:

   $ % git grep -h ' = opt->value;' | sed 's/\*.*$//; s/^ *//' | sort | uniq -c | sort -nr | head -10
     29 struct diff_options
     12 int
      7 struct grep_opt
      6 struct rebase_options
      6 struct apply_state
      5 struct strbuf
      5 char
      4 struct note_data
      3 struct string_list
      2 struct strvec

Increasing the size of the struct like that would increase the total
memory footprint by a few KB at most -- tolerable.

> Here's what it looks like, for reference.
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index b28a4a1f82..718b5c3073 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)
>  static int option_parse_exact_match(const struct option *opt, const char *arg,
>  				    int unset)
>  {
> +	int *val = opt->value;

This line would assign opt->value_int instead...

> +
>  	BUG_ON_OPT_ARG(arg);
>
> -	max_candidates = unset ? DEFAULT_CANDIDATES : 0;
> +	*val = unset ? DEFAULT_CANDIDATES : 0;
>  	return 0;
>  }
>
> @@ -578,7 +580,7 @@ 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_CALLBACK_F(0, "exact-match", NULL, NULL,
> +		OPT_CALLBACK_F(0, "exact-match", &max_candidates, NULL,

... but the macro OP_CALLBACK_F could no longer be used, because we'd
need to select one of the many typed pointers.

Macros like OPT_BOOL could be changed to use the appropriate typed
pointer.

Thoughts?  Too much churn?

>  			       N_("only output exact matches"),
>  			       PARSE_OPT_NOARG, option_parse_exact_match),
>  		OPT_INTEGER(0, "candidates", &max_candidates,
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index d2a162d528..74c2225620 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4120,12 +4120,14 @@ static void add_extra_kept_packs(const struct string_list *names)
>  static int option_parse_quiet(const struct option *opt, const char *arg,
>  			      int unset)
>  {
> +	int *val = opt->value;
> +
>  	BUG_ON_OPT_ARG(arg);
>
>  	if (!unset)
> -		progress = 0;
> -	else if (!progress)
> -		progress = 1;
> +		*val = 0;
> +	else if (!*val)
> +		*val = 1;
>  	return 0;
>  }
>
> @@ -4190,7 +4192,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  		LIST_OBJECTS_FILTER_INIT;
>
>  	struct option pack_objects_options[] = {
> -		OPT_CALLBACK_F('q', "quiet", NULL, NULL,
> +		OPT_CALLBACK_F('q', "quiet", &progress, NULL,
>  			       N_("do not show progress meter"),
>  			       PARSE_OPT_NOARG, option_parse_quiet),
>  		OPT_SET_INT(0, "progress", &progress,