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

From: Jeff King <peff@peff.net>
Date: Aug 9, 2023, 14:09 UTC
Message-ID: <20230809140902.GA775795@coredump.intra.peff.net>
In-Reply-To: <xmqqzg3156sy.fsf@gitster.g>

On Tue, Aug 08, 2023 at 06:43:41PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

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