Re: [PATCH] describe: fix --no-exact-match
From: Jeff King <hidden>
Date: 2023-08-08 21:27:25
Possibly related (same subject, not in this thread)
- 2023-08-12 · Re: [PATCH] describe: fix --no-exact-match · René Scharfe <hidden>
- 2023-08-11 · Re: [PATCH] describe: fix --no-exact-match · Jeff King <hidden>
- 2023-08-11 · Re: [PATCH] describe: fix --no-exact-match · René Scharfe <hidden>
- 2023-08-11 · Re: [PATCH] describe: fix --no-exact-match · René Scharfe <hidden>
- 2023-08-11 · Re: [PATCH] describe: fix --no-exact-match · Jeff King <hidden>
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:
quoted hunk ↗ jump to hunk
@@ -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