Re: [PATCH] describe: fix --no-exact-match
From: Jeff King <hidden>
Date: 2023-08-09 14:09:17
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2023-08-10 · Re: [PATCH] describe: fix --no-exact-match · René Scharfe <hidden>
- 2023-08-10 · Re: [PATCH] describe: fix --no-exact-match · Junio C Hamano <hidden>
- 2023-08-10 · Re: [PATCH] describe: fix --no-exact-match · Jeff King <hidden>
- 2023-08-09 · Re: [PATCH] describe: fix --no-exact-match · Junio C Hamano <hidden>
- 2023-08-08 · Re: [PATCH] describe: fix --no-exact-match · Jeff King <hidden>
On Tue, Aug 08, 2023 at 06:43:41PM -0700, Junio C Hamano wrote:
quoted
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. :) 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).
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; + 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, 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,