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,
