threads / patch / 60176

patch, 8 partsmore unused parameters in parseopt callbacks

Subject: [PATCH 0/8] more unused parameters in parseopt callbacks

## tl;dr

47 messages between Aug 31, 2023 and Sep 19, 2023. Diffs are folded; open one to read it.

replies: 46people: 5as markdown or json

Jeff King· Aug 31, 2023, 07:09 UTC · lore

Here are some more patches silencing -Wunused-parameter warnings. I've prepared them on top of the patches queued in jk/unused-post-2.42, but they should apply equally well directly on 'master'.

These ones are a bit more interesting than usual, because there were some actual code changes and cleanups that seemed preferable to just annotating.

I'm adding René to the cc based on our earlier discussion of callbacks which ignore opt->value. See patch 3 in particular. Also some of your recent work gets a nice shout-out in patch 2. :)

  [1/8]: merge: make xopts a strvec
  [2/8]: merge: simplify parsing of "-n" option
  [3/8]: parse-options: prefer opt->value to globals in callbacks
  [4/8]: parse-options: mark unused "opt" parameter in callbacks
  [5/8]: merge: do not pass unused opt->value parameter
  [6/8]: parse-options: add more BUG_ON() annotations
  [7/8]: interpret-trailers: mark unused "unset" parameters in option callbacks
  [8/8]: parse-options: mark unused parameters in noop callback
 builtin/add.c                |  2 ++
 builtin/checkout-index.c     |  2 +-
 builtin/describe.c           |  6 +++--
 builtin/fast-export.c        | 36 +++++++++++++++++-------------
 builtin/fetch.c              |  3 ++-
 builtin/gc.c                 |  2 +-
 builtin/interpret-trailers.c | 18 +++++++--------
 builtin/log.c                | 18 ++++++++++-----
 builtin/merge.c              | 43 +++++++++---------------------------
 builtin/pack-objects.c       | 27 ++++++++++++----------
 builtin/read-tree.c          |  2 +-
 builtin/update-index.c       |  4 ++--
 parse-options-cb.c           |  4 +++-
 13 files changed, 84 insertions(+), 83 deletions(-)
Jeff King· Aug 31, 2023, 07:12 UTC · re: Jeff King · lore

[PATCH 1/8] merge: make xopts a strvec

The "xopts" variable uses a custom array with ALLOC_GROW(). Using a strvec simplifies things a bit. We need fewer variables, and we can also ditch our custom parseopt callback in favor of OPT_STRVEC().

As a bonus, this means that "--no-strategy-option", which was previously a silent noop, now does something useful: like other list-like options, it will clear the list of -X options seen so far.

Signed-off-by: Jeff King <peff@peff.net>
---
I guess you could argue this is a backwards-incompatible change, but the
existing behavior of --no-strategy-option is so dumb that I can't
believe somebody would prefer it (plus revert/cherry-pick already use
OPT_STRVEC for their matching "-X").

I didn't bother adding a test since we're just re-using OPT_STRVEC code that is used elsewhere.

 builtin/merge.c | 26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)
Show changes to builtin/merge.c +7 −19
diff --git a/builtin/merge.c b/builtin/merge.c
index de68910177..53e9fe70d9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -79,8 +79,7 @@ static int overwrite_ignore = 1;
 static struct strbuf merge_msg = STRBUF_INIT;
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+static struct strvec xopts = STRVEC_INIT;
 static const char *branch;
 static char *branch_mergeoptions;
 static int verbosity;
@@ -242,17 +241,6 @@ static int option_parse_strategy(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_x(const struct option *opt,
-			  const char *arg, int unset)
-{
-	if (unset)
-		return 0;
-
-	ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
-	xopts[xopts_nr++] = xstrdup(arg);
-	return 0;
-}
-
 static int option_parse_n(const struct option *opt,
 			  const char *arg, int unset)
 {
@@ -287,8 +275,8 @@ static struct option builtin_merge_options[] = {
 		N_("verify that the named commit has a valid GPG signature")),
 	OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
 		N_("merge strategy to use"), option_parse_strategy),
-	OPT_CALLBACK('X', "strategy-option", &xopts, N_("option=value"),
-		N_("option for selected merge strategy"), option_parse_x),
+	OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+		N_("option for selected merge strategy")),
 	OPT_CALLBACK('m', "message", &merge_msg, N_("message"),
 		N_("merge commit message (for a non-fast-forward merge)"),
 		option_parse_message),
@@ -749,9 +737,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		o.show_rename_progress =
 			show_progress == -1 ? isatty(2) : show_progress;
 
-		for (x = 0; x < xopts_nr; x++)
-			if (parse_merge_opt(&o, xopts[x]))
-				die(_("unknown strategy option: -X%s"), xopts[x]);
+		for (x = 0; x < xopts.nr; x++)
+			if (parse_merge_opt(&o, xopts.v[x]))
+				die(_("unknown strategy option: -X%s"), xopts.v[x]);
 
 		o.branch1 = head_arg;
 		o.branch2 = merge_remote_util(remoteheads->item)->name;
@@ -777,7 +765,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		return clean ? 0 : 1;
 	} else {
 		return try_merge_command(the_repository,
-					 strategy, xopts_nr, xopts,
+					 strategy, xopts.nr, xopts.v,
 					 common, head_arg, remoteheads);
 	}
 }
-- 
2.42.0.561.gaa987ecc69
Jeff King· Aug 31, 2023, 07:22 UTC · re: Jeff King · lore

Re: [PATCH 1/8] merge: make xopts a strvec

On Thu, Aug 31, 2023 at 03:12:30AM -0400, Jeff King wrote:
Show 17 quoted lines
> The "xopts" variable uses a custom array with ALLOC_GROW(). Using a
> strvec simplifies things a bit. We need fewer variables, and we can also
> ditch our custom parseopt callback in favor of OPT_STRVEC().
> 
> As a bonus, this means that "--no-strategy-option", which was previously
> a silent noop, now does something useful: like other list-like options,
> it will clear the list of -X options seen so far.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I guess you could argue this is a backwards-incompatible change, but the
> existing behavior of --no-strategy-option is so dumb that I can't
> believe somebody would prefer it (plus revert/cherry-pick already use
> OPT_STRVEC for their matching "-X").
> 
> I didn't bother adding a test since we're just re-using OPT_STRVEC code
> that is used elsewhere.

I only noticed the "revert/cherry-pick" thing while sending this patch out. But it seems that it was changed recently-ish, and Phillip noted the same behavior change there (along with mentioning merge.c). So now I doubly feel this is the right thing to do. :)

-Peff
Phillip Wood· Aug 31, 2023, 11:18 UTC · re: Jeff King · lore

Re: [PATCH 1/8] merge: make xopts a strvec

Hi Peff
On 31/08/2023 08:22, Jeff King wrote:
Show 5 quoted lines
> On Thu, Aug 31, 2023 at 03:12:30AM -0400, Jeff King wrote:
> I only noticed the "revert/cherry-pick" thing while sending this patch
> out. But it seems that it was changed recently-ish, and Phillip noted
> the same behavior change there (along with mentioning merge.c). So now I
> doubly feel this is the right thing to do. :)

Thanks for doing this, I had intended to follow up the cherry-pick/revert changes by changing merge as well but didn't get around to it. I think the current behavior is unlikely to be useful to anyone.

Best Wishes
Phillip
Junio C Hamano· Aug 31, 2023, 15:46 UTC · re: Jeff King · lore

Re: [PATCH 1/8] merge: make xopts a strvec

Jeff King <peff@peff.net> writes:
Show 17 quoted lines
> The "xopts" variable uses a custom array with ALLOC_GROW(). Using a
> strvec simplifies things a bit. We need fewer variables, and we can also
> ditch our custom parseopt callback in favor of OPT_STRVEC().
>
> As a bonus, this means that "--no-strategy-option", which was previously
> a silent noop, now does something useful: like other list-like options,
> it will clear the list of -X options seen so far.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I guess you could argue this is a backwards-incompatible change, but the
> existing behavior of --no-strategy-option is so dumb that I can't
> believe somebody would prefer it (plus revert/cherry-pick already use
> OPT_STRVEC for their matching "-X").
>
> I didn't bother adding a test since we're just re-using OPT_STRVEC code
> that is used elsewhere.

I do not think of any useful way to have "--no-strategy-option" on the command line (either as an early part of an alias or in a script) that does nothing (it's not like the command requires at least one -X option on the command line), either. Just like fb60b9f3 (sequencer: use struct strvec to store merge strategy options, 2023-04-10), which met no complaints about a possible fallout by the behaviour change, I do not think that this change even deserves an entry in the backward compatibility notes.

We did not have strvec, let alone OPT_STRVEC(), back when the -Xopts was invented around the Git 1.7.0 cycle. The loss of custom xopts code is very nice.

Thanks.
Taylor Blau· Aug 31, 2023, 20:55 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/8] merge: make xopts a strvec

On Thu, Aug 31, 2023 at 08:46:25AM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> > I guess you could argue this is a backwards-incompatible change, but the
> > existing behavior of --no-strategy-option is so dumb that I can't
> > believe somebody would prefer it (plus revert/cherry-pick already use
> > OPT_STRVEC for their matching "-X").
> >
> > I didn't bother adding a test since we're just re-using OPT_STRVEC code
> > that is used elsewhere.
>
> I do not think of any useful way to have "--no-strategy-option" on
> the command line (either as an early part of an alias or in a
> script) that does nothing (it's not like the command requires at
> least one -X option on the command line), either.  Just like
> fb60b9f3 (sequencer: use struct strvec to store merge strategy
> options, 2023-04-10), which met no complaints about a possible
> fallout by the behaviour change, I do not think that this change
> even deserves an entry in the backward compatibility notes.

I concur with both of you. In a project like this one, we should be rather generous with the set of things we expect users to do. But even in a quite generous interpretation, I cannot imagine anybody relying on this behavior, so I think skipping a mention of it in the backwards compatibility section makes sense.

Thanks, Taylor

Jeff King· Aug 31, 2023, 07:14 UTC · re: Jeff King · lore

[PATCH 2/8] merge: simplify parsing of "-n" option

The "-n" option is implemented by an option callback, as it is really a "reverse bool". If given, it sets show_diffstat to 0. In theory, when negated, it would set the same flag to 1. But it's not possible to trigger that, since short options cannot be negated.

So in practice this is really just a SET_INT to 0. Let's use that instead, which shortens the code.

Note that negation here would do the wrong thing (as with any SET_INT with a value of "0"). We could specify PARSE_OPT_NONEG to future-proof ourselves against somebody adding a long option name (which would make it possible to negate). But there's not much point:

  1. Nobody is going to do that, because the negated form already
     exists, and is called "--stat" (which is defined separately so that
     "--no-stat" works).
  2. If they did, the BUG() check added by 3284b93862 (parse-options:
     disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and
     that check is smart enough to realize that our short-only option is
     OK).
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/merge.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)
Show changes to builtin/merge.c +2 −11
diff --git a/builtin/merge.c b/builtin/merge.c
index 53e9fe70d9..21363b7985 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -241,18 +241,9 @@ static int option_parse_strategy(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_n(const struct option *opt,
-			  const char *arg, int unset)
-{
-	BUG_ON_OPT_ARG(arg);
-	show_diffstat = unset;
-	return 0;
-}
-
 static struct option builtin_merge_options[] = {
-	OPT_CALLBACK_F('n', NULL, NULL, NULL,
-		N_("do not show a diffstat at the end of the merge"),
-		PARSE_OPT_NOARG, option_parse_n),
+	OPT_SET_INT('n', NULL, &show_diffstat,
+		N_("do not show a diffstat at the end of the merge"), 0),
 	OPT_BOOL(0, "stat", &show_diffstat,
 		N_("show a diffstat at the end of the merge")),
 	OPT_BOOL(0, "summary", &show_diffstat, N_("(synonym to --stat)")),
-- 
2.42.0.561.gaa987ecc69
Junio C Hamano· Aug 31, 2023, 15:56 UTC · re: Jeff King · lore

Re: [PATCH 2/8] merge: simplify parsing of "-n" option

Jeff King <peff@peff.net> writes:
>   2. If they did, the BUG() check added by 3284b93862 (parse-options:
>      disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and
>      that check is smart enough to realize that our short-only option is
>      OK).
;-).
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/merge.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
More code deletion.  Nice.
Jeff King· Aug 31, 2023, 07:17 UTC · re: Jeff King · lore

[PATCH 3/8] parse-options: prefer opt->value to globals in callbacks

We have several parse-options callbacks that ignore their "opt" parameters entirely. This is a little unusual, as we'd normally put the result of the parsing into opt->value. In the case of these callbacks, though, they directly manipulate global variables instead (and in most cases the caller sets opt->value to NULL in the OPT_CALLBACK declaration).

The immediate symptom we'd like to deal with is that the unused "opt" variables trigger -Wunused-parameter. But how to fix that is debatable. One option is to annotate them with UNUSED. But another is to have the caller pass in the appropriate variable via opt->value, and use it. That has the benefit of making the callbacks reusable (in theory at least), and makes it clear from the OPT_CALLBACK declaration which variables will be affected (doubly so for the cases in builtin/fast-export.c, where we do set opt->value, but it is completely ignored!).

The slight downside is that we lose type safety, since they're now passing through void pointers.

I went with the "just use them" approach here. The loss of type safety is unfortunate, but that is already an issue with most of the other callbacks. If we want to try to address that, we should do so more consistently (and this patch would prepare these callbacks for whatever we choose to do there).

Note that in the cases in builtin/fast-export.c, we are passing anonymous enums. We'll have to give them names so that we can declare the appropriate pointer type within the callbacks.

Signed-off-by: Jeff King <peff@peff.net>
---
I was tempted to push some of these globals into the cmd_foo()
functions, to make sure we don't make the same mistake again. But there
are lots of ripple effects from other functions which want to access the
globals (and passing them makes the code longer for little benefit).
Plus it would make things inconsistent, with some options globals and
some not. So that can be a cleanup for another day if somebody is
interested (and this takes us one tiny step closer).
 builtin/describe.c           |  6 ++++--
 builtin/fast-export.c        | 36 +++++++++++++++++++++---------------
 builtin/interpret-trailers.c | 12 ++++++------
 builtin/pack-objects.c       | 21 ++++++++++++---------
 4 files changed, 43 insertions(+), 32 deletions(-)
Show changes to 4 files +43 −32

builtin/describe.c, builtin/fast-export.c, builtin/interpret-trailers.c, builtin/pack-objects.c

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/fast-export.c b/builtin/fast-export.c
index 56dc69fac1..70aff515ac 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -33,9 +33,9 @@ static const char *fast_export_usage[] = {
 };
 
 static int progress;
-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
-static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
-static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
+static enum signed_tag_mode { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
+static enum tag_of_filtered_mode { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
+static enum reencode_mode { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
 static int fake_missing_tagger;
 static int use_done_feature;
 static int no_data;
@@ -53,16 +53,18 @@ static struct revision_sources revision_sources;
 static int parse_opt_signed_tag_mode(const struct option *opt,
 				     const char *arg, int unset)
 {
+	enum signed_tag_mode *val = opt->value;
+
 	if (unset || !strcmp(arg, "abort"))
-		signed_tag_mode = SIGNED_TAG_ABORT;
+		*val = SIGNED_TAG_ABORT;
 	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
-		signed_tag_mode = VERBATIM;
+		*val = VERBATIM;
 	else if (!strcmp(arg, "warn"))
-		signed_tag_mode = WARN;
+		*val = WARN;
 	else if (!strcmp(arg, "warn-strip"))
-		signed_tag_mode = WARN_STRIP;
+		*val = WARN_STRIP;
 	else if (!strcmp(arg, "strip"))
-		signed_tag_mode = STRIP;
+		*val = STRIP;
 	else
 		return error("Unknown signed-tags mode: %s", arg);
 	return 0;
@@ -71,12 +73,14 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
 static int parse_opt_tag_of_filtered_mode(const struct option *opt,
 					  const char *arg, int unset)
 {
+	enum tag_of_filtered_mode *val = opt->value;
+
 	if (unset || !strcmp(arg, "abort"))
-		tag_of_filtered_mode = TAG_FILTERING_ABORT;
+		*val = TAG_FILTERING_ABORT;
 	else if (!strcmp(arg, "drop"))
-		tag_of_filtered_mode = DROP;
+		*val = DROP;
 	else if (!strcmp(arg, "rewrite"))
-		tag_of_filtered_mode = REWRITE;
+		*val = REWRITE;
 	else
 		return error("Unknown tag-of-filtered mode: %s", arg);
 	return 0;
@@ -85,21 +89,23 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,
 static int parse_opt_reencode_mode(const struct option *opt,
 				   const char *arg, int unset)
 {
+	enum reencode_mode *val = opt->value;
+
 	if (unset) {
-		reencode_mode = REENCODE_ABORT;
+		*val = REENCODE_ABORT;
 		return 0;
 	}
 
 	switch (git_parse_maybe_bool(arg)) {
 	case 0:
-		reencode_mode = REENCODE_NO;
+		*val = REENCODE_NO;
 		break;
 	case 1:
-		reencode_mode = REENCODE_YES;
+		*val = REENCODE_YES;
 		break;
 	default:
 		if (!strcasecmp(arg, "abort"))
-			reencode_mode = REENCODE_ABORT;
+			*val = REENCODE_ABORT;
 		else
 			return error("Unknown reencoding mode: %s", arg);
 	}
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index c5e8345265..6aadce6a1e 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -26,19 +26,19 @@ static enum trailer_if_missing if_missing;
 static int option_parse_where(const struct option *opt,
 			      const char *arg, int unset)
 {
-	return trailer_set_where(&where, arg);
+	return trailer_set_where(opt->value, arg);
 }
 
 static int option_parse_if_exists(const struct option *opt,
 				  const char *arg, int unset)
 {
-	return trailer_set_if_exists(&if_exists, arg);
+	return trailer_set_if_exists(opt->value, arg);
 }
 
 static int option_parse_if_missing(const struct option *opt,
 				   const char *arg, int unset)
 {
-	return trailer_set_if_missing(&if_missing, arg);
+	return trailer_set_if_missing(opt->value, arg);
 }
 
 static void new_trailers_clear(struct list_head *trailers)
@@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 
-		OPT_CALLBACK(0, "where", NULL, N_("action"),
+		OPT_CALLBACK(0, "where", &where, N_("action"),
 			     N_("where to place the new trailer"), option_parse_where),
-		OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
+		OPT_CALLBACK(0, "if-exists", &if_exists, N_("action"),
 			     N_("action if trailer already exists"), option_parse_if_exists),
-		OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
+		OPT_CALLBACK(0, "if-missing", &if_missing, N_("action"),
 			     N_("action if trailer is missing"), option_parse_if_missing),
 
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d2a162d528..492372ee5d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4120,29 +4120,32 @@ 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;
 }
 
 static int option_parse_index_version(const struct option *opt,
 				      const char *arg, int unset)
 {
+	struct pack_idx_option *popts = opt->value;
 	char *c;
 	const char *val = arg;
 
 	BUG_ON_OPT_NEG(unset);
 
-	pack_idx_opts.version = strtoul(val, &c, 10);
-	if (pack_idx_opts.version > 2)
+	popts->version = strtoul(val, &c, 10);
+	if (popts->version > 2)
 		die(_("unsupported index version %s"), val);
 	if (*c == ',' && c[1])
-		pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);
-	if (*c || pack_idx_opts.off32_limit & 0x80000000)
+		popts->off32_limit = strtoul(c+1, &c, 0);
+	if (*c || popts->off32_limit & 0x80000000)
 		die(_("bad index version '%s'"), val);
 	return 0;
 }
@@ -4190,7 +4193,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,
@@ -4200,7 +4203,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "all-progress-implied",
 			 &all_progress_implied,
 			 N_("similar to --all-progress when progress meter is shown")),
-		OPT_CALLBACK_F(0, "index-version", NULL, N_("<version>[,<offset>]"),
+		OPT_CALLBACK_F(0, "index-version", &pack_idx_opts, N_("<version>[,<offset>]"),
 		  N_("write the pack index file in the specified idx format version"),
 		  PARSE_OPT_NONEG, option_parse_index_version),
 		OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit,
-- 
2.42.0.561.gaa987ecc69
Junio C Hamano· Aug 31, 2023, 16:14 UTC · re: Jeff King · lore

Re: [PATCH 3/8] parse-options: prefer opt->value to globals in callbacks

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> ... have the
> caller pass in the appropriate variable via opt->value, and use it. That
> has the benefit of making the callbacks reusable (in theory at least),
> and makes it clear from the OPT_CALLBACK declaration which variables
> will be affected (doubly so for the cases in builtin/fast-export.c,
> where we do set opt->value, but it is completely ignored!).

Thanks for doing this. I vaguely recall in my recent reviews I said that I prefer the style for exactly the second reason above.

Show 19 quoted lines
> I went with the "just use them" approach here. The loss of type safety
> is unfortunate, but that is already an issue with most of the other
> callbacks. If we want to try to address that, we should do so more
> consistently (and this patch would prepare these callbacks for whatever
> we choose to do there).
>
> Note that in the cases in builtin/fast-export.c, we are passing
> anonymous enums. We'll have to give them names so that we can declare
> the appropriate pointer type within the callbacks.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I was tempted to push some of these globals into the cmd_foo()
> functions, to make sure we don't make the same mistake again. But there
> are lots of ripple effects from other functions which want to access the
> globals (and passing them makes the code longer for little benefit).
> Plus it would make things inconsistent, with some options globals and
> some not. So that can be a cleanup for another day if somebody is
> interested (and this takes us one tiny step closer).

Yup, as long as these globals are file-scope static that these cmd_foo() use to decide and prepare arguments to pass to the more library-ish part of the code, I do not mind them not being on stack.

Show 26 quoted lines
> 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,

If we ever choose to libify builtin/describe.c::describe_commit(), max_candidates may want to become a parameter to it, which would allow cmd_describe() to have it on its stack, but until that happens, I agree that it is not a very useful churn.

Show 42 quoted lines
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index c5e8345265..6aadce6a1e 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -26,19 +26,19 @@ static enum trailer_if_missing if_missing;
>  static int option_parse_where(const struct option *opt,
>  			      const char *arg, int unset)
>  {
> -	return trailer_set_where(&where, arg);
> +	return trailer_set_where(opt->value, arg);
>  }
>  
>  static int option_parse_if_exists(const struct option *opt,
>  				  const char *arg, int unset)
>  {
> -	return trailer_set_if_exists(&if_exists, arg);
> +	return trailer_set_if_exists(opt->value, arg);
>  }
>  
>  static int option_parse_if_missing(const struct option *opt,
>  				   const char *arg, int unset)
>  {
> -	return trailer_set_if_missing(&if_missing, arg);
> +	return trailer_set_if_missing(opt->value, arg);
>  }
>  
>  static void new_trailers_clear(struct list_head *trailers)
> @@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
>  		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
>  
> -		OPT_CALLBACK(0, "where", NULL, N_("action"),
> +		OPT_CALLBACK(0, "where", &where, N_("action"),
>  			     N_("where to place the new trailer"), option_parse_where),
> -		OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
> +		OPT_CALLBACK(0, "if-exists", &if_exists, N_("action"),
>  			     N_("action if trailer already exists"), option_parse_if_exists),
> -		OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
> +		OPT_CALLBACK(0, "if-missing", &if_missing, N_("action"),
>  			     N_("action if trailer is missing"), option_parse_if_missing),
>  
>  		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),

OK. These three variables should be fun (but not too difficult) to convert to on-stack variables. option_parse_trailer() needs to learn to take a struct that holds "trailers", in addition to these three variables as its members to lose its dependency on the globals, and then these three callbacks can learn to take the same struct and fill their own members in it.

Of course that is outside the scope of this series.
Jeff King· Aug 31, 2023, 07:18 UTC · re: Jeff King · lore

[PATCH 4/8] parse-options: mark unused "opt" parameter in callbacks

The previous commit argued that parse-options callbacks should try to use opt->value rather than touching globals directly. In some cases, however, that's awkward to do. Some callbacks touch multiple variables, or may even just call into an abstracted function that does so.

In some of these cases we _could_ convert them by stuffing the multiple variables into a single struct and passing the struct pointer through opt->value. But that may make other parts of the code less readable, as the struct relationship has to be mentioned everywhere.

Let's just accept that these cases are special and leave them as-is. But we do need to mark their "opt" parameters to satisfy -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/checkout-index.c |  2 +-
 builtin/fetch.c          |  3 ++-
 builtin/gc.c             |  2 +-
 builtin/log.c            | 16 ++++++++++------
 builtin/merge.c          |  2 +-
 builtin/pack-objects.c   |  6 +++---
 builtin/read-tree.c      |  2 +-
 builtin/update-index.c   |  4 ++--
 8 files changed, 21 insertions(+), 16 deletions(-)
Show changes to 8 files +21 −16

builtin/checkout-index.c, builtin/fetch.c, builtin/gc.c, builtin/log.c, builtin/merge.c, builtin/pack-objects.c, builtin/read-tree.c, builtin/update-index.c

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f62f13f2b5..95b3717dd1 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -190,7 +190,7 @@ static const char * const builtin_checkout_index_usage[] = {
 	NULL
 };
 
-static int option_parse_stage(const struct option *opt,
+static int option_parse_stage(const struct option *opt UNUSED,
 			      const char *arg, int unset)
 {
 	BUG_ON_OPT_NEG(unset);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8f93529505..cd2afe33e5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -168,7 +168,8 @@ static int git_fetch_config(const char *k, const char *v,
 	return git_default_config(k, v, ctx, cb);
 }
 
-static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
+static int parse_refmap_arg(const struct option *opt UNUSED,
+			    const char *arg, int unset)
 {
 	BUG_ON_OPT_NEG(unset);
 
diff --git a/builtin/gc.c b/builtin/gc.c
index 369bd43fb2..b842349d86 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
 	strbuf_release(&config_name);
 }
 
-static int task_option_parse(const struct option *opt,
+static int task_option_parse(const struct option *opt UNUSED,
 			     const char *arg, int unset)
 {
 	int i, num_selected = 0;
diff --git a/builtin/log.c b/builtin/log.c
index db3a88bfe9..87e29c4171 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -118,16 +118,17 @@ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
 static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
 static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
 
-static int clear_decorations_callback(const struct option *opt,
-					    const char *arg, int unset)
+static int clear_decorations_callback(const struct option *opt UNUSED,
+				      const char *arg, int unset)
 {
 	string_list_clear(&decorate_refs_include, 0);
 	string_list_clear(&decorate_refs_exclude, 0);
 	use_default_decoration_filter = 0;
 	return 0;
 }
 
-static int decorate_callback(const struct option *opt, const char *arg, int unset)
+static int decorate_callback(const struct option *opt UNUSED, const char *arg,
+			     int unset)
 {
 	if (unset)
 		decoration_style = 0;
@@ -1555,7 +1556,8 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static int header_callback(const struct option *opt, const char *arg, int unset)
+static int header_callback(const struct option *opt UNUSED, const char *arg,
+			   int unset)
 {
 	if (unset) {
 		string_list_clear(&extra_hdr, 0);
@@ -1567,7 +1569,8 @@ static int header_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static int to_callback(const struct option *opt, const char *arg, int unset)
+static int to_callback(const struct option *opt UNUSED, const char *arg,
+		       int unset)
 {
 	if (unset)
 		string_list_clear(&extra_to, 0);
@@ -1576,7 +1579,8 @@ static int to_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static int cc_callback(const struct option *opt, const char *arg, int unset)
+static int cc_callback(const struct option *opt UNUSED, const char *arg,
+		       int unset)
 {
 	if (unset)
 		string_list_clear(&extra_cc, 0);
diff --git a/builtin/merge.c b/builtin/merge.c
index 21363b7985..0436986dab 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -231,7 +231,7 @@ static void append_strategy(struct strategy *s)
 	use_strategies[use_strategies_nr++] = s;
 }
 
-static int option_parse_strategy(const struct option *opt,
+static int option_parse_strategy(const struct option *opt UNUSED,
 				 const char *name, int unset)
 {
 	if (unset)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 492372ee5d..91b4b7c177 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3739,7 +3739,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
 	show_object(obj, name, data);
 }
 
-static int option_parse_missing_action(const struct option *opt,
+static int option_parse_missing_action(const struct option *opt UNUSED,
 				       const char *arg, int unset)
 {
 	assert(arg);
@@ -4150,7 +4150,7 @@ static int option_parse_index_version(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_unpack_unreachable(const struct option *opt,
+static int option_parse_unpack_unreachable(const struct option *opt UNUSED,
 					   const char *arg, int unset)
 {
 	if (unset) {
@@ -4165,7 +4165,7 @@ static int option_parse_unpack_unreachable(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_cruft_expiration(const struct option *opt,
+static int option_parse_cruft_expiration(const struct option *opt UNUSED,
 					 const char *arg, int unset)
 {
 	if (unset) {
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 1fec702a04..8196ca9dd8 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -49,7 +49,7 @@ static const char * const read_tree_usage[] = {
 	NULL
 };
 
-static int index_output_cb(const struct option *opt, const char *arg,
+static int index_output_cb(const struct option *opt UNUSED, const char *arg,
 				 int unset)
 {
 	BUG_ON_OPT_NEG(unset);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index aee3cb8cbd..59acae3336 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,
 	return 0;
 }
 
-static int resolve_undo_clear_callback(const struct option *opt,
+static int resolve_undo_clear_callback(const struct option *opt UNUSED,
 				const char *arg, int unset)
 {
 	BUG_ON_OPT_NEG(unset);
@@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,
 }
 
 static enum parse_opt_result cacheinfo_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	struct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,
 	const char *arg, int unset)
 {
 	struct object_id oid;
-- 
2.42.0.561.gaa987ecc69
Junio C Hamano· Aug 31, 2023, 16:33 UTC · re: Jeff King · lore

Re: [PATCH 4/8] parse-options: mark unused "opt" parameter in callbacks

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index f62f13f2b5..95b3717dd1 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -190,7 +190,7 @@ static const char * const builtin_checkout_index_usage[] = {
>  	NULL
>  };
>  
> -static int option_parse_stage(const struct option *opt,
> +static int option_parse_stage(const struct option *opt UNUSED,
>  			      const char *arg, int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);
I suspect that the original is buggy; when given
  $ git checkout-index --stage=all --stage=1 path

the first option turns --temp on, but the second one does not turn it off. For now I think being bug-to-bug compatible and annotating the opt as UNUSED is good, but as a follow-up, we could make the caller:

 (1) point &checkout_stage with opt->value;
 (2) make to_tempfile to tristate <unspecified, false, true> by
     initializing it to -1;
 (3) adjust to_tempfile that is still <unspecified> after
     parse_options() returns, according to the value in
     checkout_stage.

and then this can follow the "opt->value points at the variable that is affected" pattern.

Show 13 quoted lines
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 8f93529505..cd2afe33e5 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -168,7 +168,8 @@ static int git_fetch_config(const char *k, const char *v,
>  	return git_default_config(k, v, ctx, cb);
>  }
>  
> -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
> +static int parse_refmap_arg(const struct option *opt UNUSED,
> +			    const char *arg, int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);

Can't this just point opt->value at the global &refmap? Obviously not a huge deal, as we could have taken the "annotate as UNUSED" approach for all the functions in [3/8].

Show 13 quoted lines
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 369bd43fb2..b842349d86 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
>  	strbuf_release(&config_name);
>  }
>  
> -static int task_option_parse(const struct option *opt,
> +static int task_option_parse(const struct option *opt UNUSED,
>  			     const char *arg, int unset)
>  {
>  	int i, num_selected = 0;

The TASK__COUNT constant is very closely tied to the task[] array and it is not worth passing the address of the array in opt->value. The loss of clarity inside the callback funtion is a more serious downside than the value on the caller side to document what array is being modified. So I agree this is a good change.

Show 22 quoted lines
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index aee3cb8cbd..59acae3336 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,
>  	return 0;
>  }
>  
> -static int resolve_undo_clear_callback(const struct option *opt,
> +static int resolve_undo_clear_callback(const struct option *opt UNUSED,
>  				const char *arg, int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);
> @@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,
>  }
>  
>  static enum parse_opt_result cacheinfo_callback(
> -	struct parse_opt_ctx_t *ctx, const struct option *opt,
> +	struct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,
>  	const char *arg, int unset)
>  {
>  	struct object_id oid;

These two refuses to take "arg" and the callback mechanism is used purely to trigger the side effect of the function, not as a part of parsing something for its "value", so I agree that these UNUSED annotations reflect the reality very well.

Thanks.
Jeff King· Aug 31, 2023, 17:50 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/8] parse-options: mark unused "opt" parameter in callbacks

On Thu, Aug 31, 2023 at 09:33:22AM -0700, Junio C Hamano wrote:
Show 26 quoted lines
> > -static int option_parse_stage(const struct option *opt,
> > +static int option_parse_stage(const struct option *opt UNUSED,
> >  			      const char *arg, int unset)
> >  {
> >  	BUG_ON_OPT_NEG(unset);
> 
> I suspect that the original is buggy; when given
> 
>   $ git checkout-index --stage=all --stage=1 path
> 
> the first option turns --temp on, but the second one does not turn
> it off.  For now I think being bug-to-bug compatible and annotating
> the opt as UNUSED is good, but as a follow-up, we could make the
> caller:
> 
>  (1) point &checkout_stage with opt->value;
> 
>  (2) make to_tempfile to tristate <unspecified, false, true> by
>      initializing it to -1;
> 
>  (3) adjust to_tempfile that is still <unspecified> after
>      parse_options() returns, according to the value in
>      checkout_stage.
> 
> and then this can follow the "opt->value points at the variable that
> is affected" pattern.
Yeah, I think that would work, with one extra bit:
  (4) complain when (!to_tempfile && checkout_stage == CHECKOUT_ALL)

I do think it would be better to fix separately, but maybe if I'm re-rolling I can do it as an early patch in the series (it is not much different than the "xopts" fix in scope).

Show 9 quoted lines
> > -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
> > +static int parse_refmap_arg(const struct option *opt UNUSED,
> > +			    const char *arg, int unset)
> >  {
> >  	BUG_ON_OPT_NEG(unset);
> 
> Can't this just point opt->value at the global &refmap?  Obviously
> not a huge deal, as we could have taken the "annotate as UNUSED"
> approach for all the functions in [3/8].

Hmm, yeah. I think I looked at the abstract refspec_append() here and assumed that it might be touching other variables. But it's not. It's operating purely on the &refspec we pass it (and even though it uses ALLOC_GROW, the "nr" and "alloc" are both contained in the struct). So yeah, it really should have been converted in patch 3.

I think that is probably worth a re-roll.
-Peff
Jeff King· Aug 31, 2023, 20:48 UTC · re: Jeff King · lore

Re: [PATCH 4/8] parse-options: mark unused "opt" parameter in callbacks

On Thu, Aug 31, 2023 at 01:50:18PM -0400, Jeff King wrote:
Show 15 quoted lines
> > > -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
> > > +static int parse_refmap_arg(const struct option *opt UNUSED,
> > > +			    const char *arg, int unset)
> > >  {
> > >  	BUG_ON_OPT_NEG(unset);
> > 
> > Can't this just point opt->value at the global &refmap?  Obviously
> > not a huge deal, as we could have taken the "annotate as UNUSED"
> > approach for all the functions in [3/8].
> 
> Hmm, yeah. I think I looked at the abstract refspec_append() here and
> assumed that it might be touching other variables. But it's not. It's
> operating purely on the &refspec we pass it (and even though it uses
> ALLOC_GROW, the "nr" and "alloc" are both contained in the struct). So
> yeah, it really should have been converted in patch 3.

Oh, btw, there are two other cases in this patch that _could_ be converted but I left alone. In log.c, we have a to_callback() and a cc_callback(), both of which take a single string list. But there is also header_callback(), which resets both of those when it sees --no-add-header. So I left them alone as a set.

However, it occurs to me that to_callback() and cc_callback() can just be OPT_STRING_LIST these days. And that is probably a worthwhile cleanup to do.

-Peff
Jeff King· Aug 31, 2023, 07:18 UTC · re: Jeff King · lore

[PATCH 5/8] merge: do not pass unused opt->value parameter

The option_parse_strategy() callback does not look at opt->value; instead it calls append_strategy(), which manipulates the global use_strategies array directly. But the OPT_CALLBACK declaration assigns "&use_strategies" to opt->value.

One could argue this is good, as it tells the reader what we generally expect the callback to do. But it is also bad, because it can mislead you into thinking that swapping out "&use_strategies" there might have any effect. Let's switch it to pass NULL (which is what every other "does not bother to look at opt->value" callback does). If you want to know what the callback does, it's easy to read the function itself.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/merge.c +1 −1
diff --git a/builtin/merge.c b/builtin/merge.c
index 0436986dab..545da0c8a1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -264,7 +264,7 @@ static struct option builtin_merge_options[] = {
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 	OPT_BOOL(0, "verify-signatures", &verify_signatures,
 		N_("verify that the named commit has a valid GPG signature")),
-	OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
+	OPT_CALLBACK('s', "strategy", NULL, N_("strategy"),
 		N_("merge strategy to use"), option_parse_strategy),
 	OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
 		N_("option for selected merge strategy")),
-- 
2.42.0.561.gaa987ecc69
Junio C Hamano· Aug 31, 2023, 16:53 UTC · re: Jeff King · lore

Re: [PATCH 5/8] merge: do not pass unused opt->value parameter

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> The option_parse_strategy() callback does not look at opt->value;
> instead it calls append_strategy(), which manipulates the global
> use_strategies array directly. But the OPT_CALLBACK declaration assigns
> "&use_strategies" to opt->value.
>
> One could argue this is good, as it tells the reader what we generally
> expect the callback to do. But it is also bad, because it can mislead
> you into thinking that swapping out "&use_strategies" there might have
> any effect. Let's switch it to pass NULL (which is what every other
> "does not bother to look at opt->value" callback does). If you want to
> know what the callback does, it's easy to read the function itself.

Good. It is not like we have two options and each of them affect its own list of strategies. There is a single use_strategies list that is populated not just by the parse options callback but by other callers. So I agree that the new way is less confusing.

Show 19 quoted lines
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/merge.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 0436986dab..545da0c8a1 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -264,7 +264,7 @@ static struct option builtin_merge_options[] = {
>  	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
>  	OPT_BOOL(0, "verify-signatures", &verify_signatures,
>  		N_("verify that the named commit has a valid GPG signature")),
> -	OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
> +	OPT_CALLBACK('s', "strategy", NULL, N_("strategy"),
>  		N_("merge strategy to use"), option_parse_strategy),
>  	OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
>  		N_("option for selected merge strategy")),
Jeff King· Aug 31, 2023, 07:19 UTC · re: Jeff King · lore

[PATCH 6/8] parse-options: add more BUG_ON() annotations

These callbacks are similar to the ones touched by 517fe807d6 (assert NOARG/NONEG behavior of parse-options callbacks, 2018-11-05), but were either missed in that commit (the one in add.c) or were added later (the one in log.c).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/add.c | 2 ++
 builtin/log.c | 2 ++
 2 files changed, 4 insertions(+)
Show changes to 2 files +4 −0

builtin/add.c, builtin/log.c

diff --git a/builtin/add.c b/builtin/add.c
index 4b0dd798df..cf59108523 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -232,6 +232,8 @@ static char *chmod_arg;
 
 static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
 {
+	BUG_ON_OPT_ARG(arg);
+
 	/* if we are told to ignore, we are not adding removals */
 	*(int *)opt->value = !unset ? 0 : 1;
 	return 0;
diff --git a/builtin/log.c b/builtin/log.c
index 87e29c4171..80fa642858 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -121,6 +121,8 @@ static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
 static int clear_decorations_callback(const struct option *opt UNUSED,
 				      const char *arg, int unset)
 {
+	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 	string_list_clear(&decorate_refs_include, 0);
 	string_list_clear(&decorate_refs_exclude, 0);
 	use_default_decoration_filter = 0;
-- 
2.42.0.561.gaa987ecc69
Junio C Hamano· Aug 31, 2023, 16:58 UTC · re: Jeff King · lore

Re: [PATCH 6/8] parse-options: add more BUG_ON() annotations

Jeff King <peff@peff.net> writes:
> These callbacks are similar to the ones touched by 517fe807d6 (assert
> NOARG/NONEG behavior of parse-options callbacks, 2018-11-05), but were
> either missed in that commit (the one in add.c) or were added later (the
> one in log.c).
They obviously look good.  Thanks.
Show 33 quoted lines
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/add.c | 2 ++
>  builtin/log.c | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 4b0dd798df..cf59108523 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -232,6 +232,8 @@ static char *chmod_arg;
>  
>  static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
>  {
> +	BUG_ON_OPT_ARG(arg);
> +
>  	/* if we are told to ignore, we are not adding removals */
>  	*(int *)opt->value = !unset ? 0 : 1;
>  	return 0;
> diff --git a/builtin/log.c b/builtin/log.c
> index 87e29c4171..80fa642858 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -121,6 +121,8 @@ static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
>  static int clear_decorations_callback(const struct option *opt UNUSED,
>  				      const char *arg, int unset)
>  {
> +	BUG_ON_OPT_NEG(unset);
> +	BUG_ON_OPT_ARG(arg);
>  	string_list_clear(&decorate_refs_include, 0);
>  	string_list_clear(&decorate_refs_exclude, 0);
>  	use_default_decoration_filter = 0;
Jeff King· Aug 31, 2023, 07:19 UTC · re: Jeff King · lore

[PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks

There are a few parse-option callbacks that do not look at their "unset" parameters, but also do not set PARSE_OPT_NONEG. At first glance this seems like a bug, as we'd ignore "--no-if-exists", etc.

But they do work fine, because when "unset" is true, then "arg" is NULL. And all three functions pass "arg" on to helper functions which do the right thing with the NULL.

Note that this shortcut would not be correct if any callback used PARSE_OPT_NOARG (in which case "arg" would be NULL but "unset" would be false). But none of these do.

So the code is fine as-is. But we'll want to mark the unused "unset" parameters to quiet -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/interpret-trailers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to builtin/interpret-trailers.c +3 −3
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 6aadce6a1e..a5f9adf6b9 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -24,19 +24,19 @@ static enum trailer_if_exists if_exists;
 static enum trailer_if_missing if_missing;
 
 static int option_parse_where(const struct option *opt,
-			      const char *arg, int unset)
+			      const char *arg, int unset UNUSED)
 {
 	return trailer_set_where(opt->value, arg);
 }
 
 static int option_parse_if_exists(const struct option *opt,
-				  const char *arg, int unset)
+				  const char *arg, int unset UNUSED)
 {
 	return trailer_set_if_exists(opt->value, arg);
 }
 
 static int option_parse_if_missing(const struct option *opt,
-				   const char *arg, int unset)
+				   const char *arg, int unset UNUSED)
 {
 	return trailer_set_if_missing(opt->value, arg);
 }
-- 
2.42.0.561.gaa987ecc69
Junio C Hamano· Aug 31, 2023, 17:04 UTC · re: Jeff King · lore

Re: [PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> There are a few parse-option callbacks that do not look at their "unset"
> parameters, but also do not set PARSE_OPT_NONEG. At first glance this
> seems like a bug, as we'd ignore "--no-if-exists", etc.
>
> But they do work fine, because when "unset" is true, then "arg" is NULL.
> And all three functions pass "arg" on to helper functions which do the
> right thing with the NULL.
Yuck.  That is ugly.
> Note that this shortcut would not be correct if any callback used
> PARSE_OPT_NOARG (in which case "arg" would be NULL but "unset" would be
> false). But none of these do.

That is even uglier. Unlike the BUG_ON_OPT_NEG() and BUG_ON_OPT_ARG() that catch discrepancies between options[] flags and the expectation by the callback function, there is no way for us to protect against such mistakes?

> So the code is fine as-is. But we'll want to mark the unused "unset"
> parameters to quiet -Wunused-parameter.
OK.  Thanks.
Show 32 quoted lines
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/interpret-trailers.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 6aadce6a1e..a5f9adf6b9 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -24,19 +24,19 @@ static enum trailer_if_exists if_exists;
>  static enum trailer_if_missing if_missing;
>  
>  static int option_parse_where(const struct option *opt,
> -			      const char *arg, int unset)
> +			      const char *arg, int unset UNUSED)
>  {
>  	return trailer_set_where(opt->value, arg);
>  }
>  
>  static int option_parse_if_exists(const struct option *opt,
> -				  const char *arg, int unset)
> +				  const char *arg, int unset UNUSED)
>  {
>  	return trailer_set_if_exists(opt->value, arg);
>  }
>  
>  static int option_parse_if_missing(const struct option *opt,
> -				   const char *arg, int unset)
> +				   const char *arg, int unset UNUSED)
>  {
>  	return trailer_set_if_missing(opt->value, arg);
>  }
Jeff King· Aug 31, 2023, 17:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks

On Thu, Aug 31, 2023 at 10:04:09AM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > There are a few parse-option callbacks that do not look at their "unset"
> > parameters, but also do not set PARSE_OPT_NONEG. At first glance this
> > seems like a bug, as we'd ignore "--no-if-exists", etc.
> >
> > But they do work fine, because when "unset" is true, then "arg" is NULL.
> > And all three functions pass "arg" on to helper functions which do the
> > right thing with the NULL.
> 
> Yuck.  That is ugly.

Yep. I wondered about adding a comment here warning about the situation, but it felt kind of content-less. Something like:

  /* if unset, arg is NULL and handled below */
  trailer_set_where(opt->value, arg);
Show 8 quoted lines
> > Note that this shortcut would not be correct if any callback used
> > PARSE_OPT_NOARG (in which case "arg" would be NULL but "unset" would be
> > false). But none of these do.
> 
> That is even uglier.  Unlike the BUG_ON_OPT_NEG() and BUG_ON_OPT_ARG()
> that catch discrepancies between options[] flags and the expectation
> by the callback function, there is no way for us to protect against
> such mistakes?
I guess it would be something like:
  if (!unset && !arg)
	BUG("unexpected use of PARSE_OPT_OPTARG");

I think it is less important than those other ones, though, because the mistake here is the OPT_CALLBACK declaration adding a flag that the callback is not prepared to handle. Whereas in the other ones, the bug is that the declaration _forgot_ to use a flag, which is a much more likely bug.

So I dunno. If it were more than this one case (well, three, but they are all of the same form) I'd be more worried.

-Peff
Jeff King· Aug 31, 2023, 07:20 UTC · re: Jeff King · lore

[PATCH 8/8] parse-options: mark unused parameters in noop callback

Unsurprisingly, the noop options callback doesn't bother to look at any of its parameters. Let's mark them so that -Wunused-parameter does not complain.

Another option would be to drop the callback and have parse-options itself recognize OPT_NOOP_NOARG. But that seems like extra work for no real benefit.

Signed-off-by: Jeff King <peff@peff.net>
---
 parse-options-cb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to parse-options-cb.c +3 −1
diff --git a/parse-options-cb.c b/parse-options-cb.c
index a24521dee0..bdc7fae497 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
+int parse_opt_noop_cb(const struct option *opt UNUSED,
+		      const char *arg UNUSED,
+		      int unset UNUSED)
 {
 	return 0;
 }
-- 
2.42.0.561.gaa987ecc69
Junio C Hamano· Aug 31, 2023, 17:05 UTC · re: Jeff King · lore

Re: [PATCH 8/8] parse-options: mark unused parameters in noop callback

Jeff King <peff@peff.net> writes:
> Unsurprisingly, the noop options callback doesn't bother to look at any
> of its parameters. Let's mark them so that -Wunused-parameter does not
> complain.
;-)
Thanks.
Show 24 quoted lines
> Another option would be to drop the callback and have parse-options
> itself recognize OPT_NOOP_NOARG. But that seems like extra work for no
> real benefit.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  parse-options-cb.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index a24521dee0..bdc7fae497 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>  
> -int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
> +int parse_opt_noop_cb(const struct option *opt UNUSED,
> +		      const char *arg UNUSED,
> +		      int unset UNUSED)
>  {
>  	return 0;
>  }
Jeff King· Aug 31, 2023, 21:16 UTC · re: Jeff King · lore

[PATCH v2 0/10] more unused parameters in parseopt callbacks

On Thu, Aug 31, 2023 at 03:09:35AM -0400, Jeff King wrote:
> Here are some more patches silencing -Wunused-parameter warnings. I've
> prepared them on top of the patches queued in jk/unused-post-2.42, but
> they should apply equally well directly on 'master'.
And here's a re-roll based on feedback from Junio:
  [01/10]: merge: make xopts a strvec
  [02/10]: merge: simplify parsing of "-n" option
  [03/10]: format-patch: use OPT_STRING_LIST for to/cc options
  [04/10]: checkout-index: delay automatic setting of to_tempfile
  [05/10]: parse-options: prefer opt->value to globals in callbacks
  [06/10]: parse-options: mark unused "opt" parameter in callbacks
  [07/10]: merge: do not pass unused opt->value parameter
  [08/10]: parse-options: add more BUG_ON() annotations
  [09/10]: interpret-trailers: mark unused "unset" parameters in option callbacks
  [10/10]: parse-options: mark unused parameters in noop callback

Range-diff is below, but the most interesting changes are in the two new patches (3 and 4):

  - patch 3 simplifies away a few string-list callbacks (which can then
    be omitted from patch 6, the "mark unused opt" one).
  - patch 4 fixes some minor bugs with "checkout-index --stage". In
    doing so, it's now eligible for cleanup in patch 5 ("prefer
    opt->value") and its annotation dropped from patch 6
  - patch 5 ("prefer opt->value") converts the missed refmap callback,
    and so its annotation is gone from patch 6 now
  - a few extra clarifying comments
 1:  8750d92016 !  1:  82940b6936 merge: make xopts a strvec
    @@ Commit message
     
         As a bonus, this means that "--no-strategy-option", which was previously
         a silent noop, now does something useful: like other list-like options,
    -    it will clear the list of -X options seen so far.
    +    it will clear the list of -X options seen so far. This matches the
    +    behavior of revert/cherry-pick, which made the same change in fb60b9f37f
    +    (sequencer: use struct strvec to store merge strategy options,
    +    2023-04-10).
     
         Signed-off-by: Jeff King <peff@peff.net>
     
 2:  ffdae69bd6 =  2:  c98b87feb9 merge: simplify parsing of "-n" option
 -:  ---------- >  3:  615fef8f90 format-patch: use OPT_STRING_LIST for to/cc options
 -:  ---------- >  4:  66cf7bcf08 checkout-index: delay automatic setting of to_tempfile
 3:  f00c78cc49 !  5:  a7c2bfecba parse-options: prefer opt->value to globals in callbacks
    @@ Commit message
     
         Signed-off-by: Jeff King <peff@peff.net>
     
    + ## builtin/checkout-index.c ##
    +@@ builtin/checkout-index.c: static const char * const builtin_checkout_index_usage[] = {
    + static int option_parse_stage(const struct option *opt,
    + 			      const char *arg, int unset)
    + {
    ++	int *stage = opt->value;
    ++
    + 	BUG_ON_OPT_NEG(unset);
    + 
    + 	if (!strcmp(arg, "all")) {
    +-		checkout_stage = CHECKOUT_ALL;
    ++		*stage = CHECKOUT_ALL;
    + 	} else {
    + 		int ch = arg[0];
    + 		if ('1' <= ch && ch <= '3')
    +-			checkout_stage = arg[0] - '0';
    ++			*stage = arg[0] - '0';
    + 		else
    + 			die(_("stage should be between 1 and 3 or all"));
    + 	}
    +@@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, const char *prefix)
    + 			N_("write the content to temporary files")),
    + 		OPT_STRING(0, "prefix", &state.base_dir, N_("string"),
    + 			N_("when creating files, prepend <string>")),
    +-		OPT_CALLBACK_F(0, "stage", NULL, "(1|2|3|all)",
    ++		OPT_CALLBACK_F(0, "stage", &checkout_stage, "(1|2|3|all)",
    + 			N_("copy out the files from named stage"),
    + 			PARSE_OPT_NONEG, option_parse_stage),
    + 		OPT_END()
    +
      ## builtin/describe.c ##
     @@ builtin/describe.c: static void describe(const char *arg, int last_one)
      static int option_parse_exact_match(const struct option *opt, const char *arg,
    @@ builtin/fast-export.c: static int parse_opt_tag_of_filtered_mode(const struct op
      			return error("Unknown reencoding mode: %s", arg);
      	}
     
    + ## builtin/fetch.c ##
    +@@ builtin/fetch.c: static int parse_refmap_arg(const struct option *opt, const char *arg, int unset
    + 	 * "git fetch --refmap='' origin foo"
    + 	 * can be used to tell the command not to store anywhere
    + 	 */
    +-	refspec_append(&refmap, arg);
    ++	refspec_append(opt->value, arg);
    + 
    + 	return 0;
    + }
    +@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
    + 			   PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules),
    + 		OPT_BOOL(0, "update-shallow", &update_shallow,
    + 			 N_("accept refs that update .git/shallow")),
    +-		OPT_CALLBACK_F(0, "refmap", NULL, N_("refmap"),
    ++		OPT_CALLBACK_F(0, "refmap", &refmap, N_("refmap"),
    + 			       N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
    + 		OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
    + 		OPT_IPVERSION(&family),
    +
      ## builtin/interpret-trailers.c ##
     @@ builtin/interpret-trailers.c: static enum trailer_if_missing if_missing;
      static int option_parse_where(const struct option *opt,
 4:  19621fc01c !  6:  bd915fe4de parse-options: mark unused "opt" parameter in callbacks
    @@ Commit message
     
         Signed-off-by: Jeff King <peff@peff.net>
     
    - ## builtin/checkout-index.c ##
    -@@ builtin/checkout-index.c: static const char * const builtin_checkout_index_usage[] = {
    - 	NULL
    - };
    - 
    --static int option_parse_stage(const struct option *opt,
    -+static int option_parse_stage(const struct option *opt UNUSED,
    - 			      const char *arg, int unset)
    - {
    - 	BUG_ON_OPT_NEG(unset);
    -
    - ## builtin/fetch.c ##
    -@@ builtin/fetch.c: static int git_fetch_config(const char *k, const char *v,
    - 	return git_default_config(k, v, ctx, cb);
    - }
    - 
    --static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
    -+static int parse_refmap_arg(const struct option *opt UNUSED,
    -+			    const char *arg, int unset)
    - {
    - 	BUG_ON_OPT_NEG(unset);
    - 
    -
      ## builtin/gc.c ##
     @@ builtin/gc.c: static void initialize_task_config(int schedule)
      	strbuf_release(&config_name);
    @@ builtin/log.c: static int inline_callback(const struct option *opt, const char *
      {
      	if (unset) {
      		string_list_clear(&extra_hdr, 0);
    -@@ builtin/log.c: static int header_callback(const struct option *opt, const char *arg, int unset)
    - 	return 0;
    - }
    - 
    --static int to_callback(const struct option *opt, const char *arg, int unset)
    -+static int to_callback(const struct option *opt UNUSED, const char *arg,
    -+		       int unset)
    - {
    - 	if (unset)
    - 		string_list_clear(&extra_to, 0);
    -@@ builtin/log.c: static int to_callback(const struct option *opt, const char *arg, int unset)
    - 	return 0;
    - }
    - 
    --static int cc_callback(const struct option *opt, const char *arg, int unset)
    -+static int cc_callback(const struct option *opt UNUSED, const char *arg,
    -+		       int unset)
    - {
    - 	if (unset)
    - 		string_list_clear(&extra_cc, 0);
     
      ## builtin/merge.c ##
     @@ builtin/merge.c: static void append_strategy(struct strategy *s)
 5:  2d6a3b9e1b =  7:  1720ddebc1 merge: do not pass unused opt->value parameter
 6:  7669758cc7 =  8:  a93f155f21 parse-options: add more BUG_ON() annotations
 7:  b80020daec !  9:  529573e281 interpret-trailers: mark unused "unset" parameters in option callbacks
    @@ Commit message
         false). But none of these do.
     
         So the code is fine as-is. But we'll want to mark the unused "unset"
    -    parameters to quiet -Wunused-parameter.
    +    parameters to quiet -Wunused-parameter. I've also added a comment to
    +    make this rather subtle situation more explicit.
     
         Signed-off-by: Jeff King <peff@peff.net>
     
    @@ builtin/interpret-trailers.c: static enum trailer_if_exists if_exists;
     -			      const char *arg, int unset)
     +			      const char *arg, int unset UNUSED)
      {
    ++	/* unset implies NULL arg, which is handled in our helper */
      	return trailer_set_where(opt->value, arg);
      }
      
      static int option_parse_if_exists(const struct option *opt,
     -				  const char *arg, int unset)
     +				  const char *arg, int unset UNUSED)
      {
    ++	/* unset implies NULL arg, which is handled in our helper */
      	return trailer_set_if_exists(opt->value, arg);
      }
      
      static int option_parse_if_missing(const struct option *opt,
     -				   const char *arg, int unset)
     +				   const char *arg, int unset UNUSED)
      {
    ++	/* unset implies NULL arg, which is handled in our helper */
      	return trailer_set_if_missing(opt->value, arg);
      }
    + 
 8:  f21961ed23 = 10:  943161eaf2 parse-options: mark unused parameters in noop callback
Jeff King· Aug 31, 2023, 21:17 UTC · re: Jeff King · lore

[PATCH v2 01/10] merge: make xopts a strvec

The "xopts" variable uses a custom array with ALLOC_GROW(). Using a strvec simplifies things a bit. We need fewer variables, and we can also ditch our custom parseopt callback in favor of OPT_STRVEC().

As a bonus, this means that "--no-strategy-option", which was previously a silent noop, now does something useful: like other list-like options, it will clear the list of -X options seen so far. This matches the behavior of revert/cherry-pick, which made the same change in fb60b9f37f (sequencer: use struct strvec to store merge strategy options, 2023-04-10).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/merge.c | 26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)
Show changes to builtin/merge.c +7 −19
diff --git a/builtin/merge.c b/builtin/merge.c
index de68910177..53e9fe70d9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -79,8 +79,7 @@ static int overwrite_ignore = 1;
 static struct strbuf merge_msg = STRBUF_INIT;
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+static struct strvec xopts = STRVEC_INIT;
 static const char *branch;
 static char *branch_mergeoptions;
 static int verbosity;
@@ -242,17 +241,6 @@ static int option_parse_strategy(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_x(const struct option *opt,
-			  const char *arg, int unset)
-{
-	if (unset)
-		return 0;
-
-	ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
-	xopts[xopts_nr++] = xstrdup(arg);
-	return 0;
-}
-
 static int option_parse_n(const struct option *opt,
 			  const char *arg, int unset)
 {
@@ -287,8 +275,8 @@ static struct option builtin_merge_options[] = {
 		N_("verify that the named commit has a valid GPG signature")),
 	OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
 		N_("merge strategy to use"), option_parse_strategy),
-	OPT_CALLBACK('X', "strategy-option", &xopts, N_("option=value"),
-		N_("option for selected merge strategy"), option_parse_x),
+	OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+		N_("option for selected merge strategy")),
 	OPT_CALLBACK('m', "message", &merge_msg, N_("message"),
 		N_("merge commit message (for a non-fast-forward merge)"),
 		option_parse_message),
@@ -749,9 +737,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		o.show_rename_progress =
 			show_progress == -1 ? isatty(2) : show_progress;
 
-		for (x = 0; x < xopts_nr; x++)
-			if (parse_merge_opt(&o, xopts[x]))
-				die(_("unknown strategy option: -X%s"), xopts[x]);
+		for (x = 0; x < xopts.nr; x++)
+			if (parse_merge_opt(&o, xopts.v[x]))
+				die(_("unknown strategy option: -X%s"), xopts.v[x]);
 
 		o.branch1 = head_arg;
 		o.branch2 = merge_remote_util(remoteheads->item)->name;
@@ -777,7 +765,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		return clean ? 0 : 1;
 	} else {
 		return try_merge_command(the_repository,
-					 strategy, xopts_nr, xopts,
+					 strategy, xopts.nr, xopts.v,
 					 common, head_arg, remoteheads);
 	}
 }
-- 
2.42.0.561.gaa987ecc69
Jeff King· Aug 31, 2023, 21:17 UTC · re: Jeff King · lore

[PATCH v2 02/10] merge: simplify parsing of "-n" option

The "-n" option is implemented by an option callback, as it is really a "reverse bool". If given, it sets show_diffstat to 0. In theory, when negated, it would set the same flag to 1. But it's not possible to trigger that, since short options cannot be negated.

So in practice this is really just a SET_INT to 0. Let's use that instead, which shortens the code.

Note that negation here would do the wrong thing (as with any SET_INT with a value of "0"). We could specify PARSE_OPT_NONEG to future-proof ourselves against somebody adding a long option name (which would make it possible to negate). But there's not much point:

  1. Nobody is going to do that, because the negated form already
     exists, and is called "--stat" (which is defined separately so that
     "--no-stat" works).
  2. If they did, the BUG() check added by 3284b93862 (parse-options:
     disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and
     that check is smart enough to realize that our short-only option is
     OK).
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/merge.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)
Show changes to builtin/merge.c +2 −11
diff --git a/builtin/merge.c b/builtin/merge.c
index 53e9fe70d9..21363b7985 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -241,18 +241,9 @@ static int option_parse_strategy(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_n(const struct option *opt,
-			  const char *arg, int unset)
-{
-	BUG_ON_OPT_ARG(arg);
-	show_diffstat = unset;
-	return 0;
-}
-
 static struct option builtin_merge_options[] = {
-	OPT_CALLBACK_F('n', NULL, NULL, NULL,
-		N_("do not show a diffstat at the end of the merge"),
-		PARSE_OPT_NOARG, option_parse_n),
+	OPT_SET_INT('n', NULL, &show_diffstat,
+		N_("do not show a diffstat at the end of the merge"), 0),
 	OPT_BOOL(0, "stat", &show_diffstat,
 		N_("show a diffstat at the end of the merge")),
 	OPT_BOOL(0, "summary", &show_diffstat, N_("(synonym to --stat)")),
-- 
2.42.0.561.gaa987ecc69
René Scharfe· Sep 2, 2023, 06:20 UTC · re: Jeff King · lore

Re: [PATCH v2 02/10] merge: simplify parsing of "-n" option

Am 31.08.23 um 23:17 schrieb Jeff King:
Show 42 quoted lines
> The "-n" option is implemented by an option callback, as it is really a
> "reverse bool". If given, it sets show_diffstat to 0. In theory, when
> negated, it would set the same flag to 1. But it's not possible to
> trigger that, since short options cannot be negated.
>
> So in practice this is really just a SET_INT to 0. Let's use that
> instead, which shortens the code.
>
> Note that negation here would do the wrong thing (as with any SET_INT
> with a value of "0"). We could specify PARSE_OPT_NONEG to future-proof
> ourselves against somebody adding a long option name (which would make
> it possible to negate). But there's not much point:
>
>   1. Nobody is going to do that, because the negated form already
>      exists, and is called "--stat" (which is defined separately so that
>      "--no-stat" works).
>
>   2. If they did, the BUG() check added by 3284b93862 (parse-options:
>      disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and
>      that check is smart enough to realize that our short-only option is
>      OK).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/merge.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 53e9fe70d9..21363b7985 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -241,18 +241,9 @@ static int option_parse_strategy(const struct option *opt,
>  	return 0;
>  }
>
> -static int option_parse_n(const struct option *opt,
> -			  const char *arg, int unset)
> -{
> -	BUG_ON_OPT_ARG(arg);
> -	show_diffstat = unset;
> -	return 0;
> -}
Great -- the less custom callbacks, the better.
Show 9 quoted lines
> -
>  static struct option builtin_merge_options[] = {
> -	OPT_CALLBACK_F('n', NULL, NULL, NULL,
> -		N_("do not show a diffstat at the end of the merge"),
> -		PARSE_OPT_NOARG, option_parse_n),
> +	OPT_SET_INT('n', NULL, &show_diffstat,
> +		N_("do not show a diffstat at the end of the merge"), 0),
>  	OPT_BOOL(0, "stat", &show_diffstat,
>  		N_("show a diffstat at the end of the merge")),

Makes it easier to see that we can replace the two complementary definitions with a single one:

	OPT_NEGBIT('n', "no-stat",
		N_("do not show a diffstat at the end of the merge"), 1),
Which is a separate topic, of course.  And if we did that, however, ...
>  	OPT_BOOL(0, "summary", &show_diffstat, N_("(synonym to --stat)")),

... we wouldn't be able to use OPT_ALIAS here anymore (which we arguably should) because it doesn't allow referencing the negated (or "positivated") form -- but that is yet another separate topic.

René
Jeff King· Sep 5, 2023, 06:43 UTC · re: René Scharfe · lore

Re: [PATCH v2 02/10] merge: simplify parsing of "-n" option

On Sat, Sep 02, 2023 at 08:20:28AM +0200, René Scharfe wrote:
Show 16 quoted lines
> >  static struct option builtin_merge_options[] = {
> > -	OPT_CALLBACK_F('n', NULL, NULL, NULL,
> > -		N_("do not show a diffstat at the end of the merge"),
> > -		PARSE_OPT_NOARG, option_parse_n),
> > +	OPT_SET_INT('n', NULL, &show_diffstat,
> > +		N_("do not show a diffstat at the end of the merge"), 0),
> >  	OPT_BOOL(0, "stat", &show_diffstat,
> >  		N_("show a diffstat at the end of the merge")),
> 
> Makes it easier to see that we can replace the two complementary
> definitions with a single one:
> 
> 	OPT_NEGBIT('n', "no-stat",
> 		N_("do not show a diffstat at the end of the merge"), 1),
> 
> Which is a separate topic, of course.  And if we did that, however, ...

Ah, I thought we had a "reverse bool" of some kind, but I couldn't find it. NEGBIT was what I was looking for. But yeah, I agree it gets more complicated with the various aliases. I think what I have here is a good stopping point for this series, but if you want to go further on it, be my guest. :)

-Peff
Jeff King· Aug 31, 2023, 21:17 UTC · re: Jeff King · lore

[PATCH v2 03/10] format-patch: use OPT_STRING_LIST for to/cc options

The to_callback() and cc_callback() functions are identical to the generic parse_opt_string_list() function (except that they don't handle optional arguments, but that's OK because their callers do not use the OPTARG flag).

Let's simplify the code by using OPT_STRING_LIST.
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)
Show changes to builtin/log.c +2 −20
diff --git a/builtin/log.c b/builtin/log.c
index db3a88bfe9..fb90d43717 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1567,24 +1567,6 @@ static int header_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static int to_callback(const struct option *opt, const char *arg, int unset)
-{
-	if (unset)
-		string_list_clear(&extra_to, 0);
-	else
-		string_list_append(&extra_to, arg);
-	return 0;
-}
-
-static int cc_callback(const struct option *opt, const char *arg, int unset)
-{
-	if (unset)
-		string_list_clear(&extra_cc, 0);
-	else
-		string_list_append(&extra_cc, arg);
-	return 0;
-}
-
 static int from_callback(const struct option *opt, const char *arg, int unset)
 {
 	char **from = opt->value;
@@ -1957,8 +1939,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(N_("Messaging")),
 		OPT_CALLBACK(0, "add-header", NULL, N_("header"),
 			    N_("add email header"), header_callback),
-		OPT_CALLBACK(0, "to", NULL, N_("email"), N_("add To: header"), to_callback),
-		OPT_CALLBACK(0, "cc", NULL, N_("email"), N_("add Cc: header"), cc_callback),
+		OPT_STRING_LIST(0, "to", &extra_to, N_("email"), N_("add To: header")),
+		OPT_STRING_LIST(0, "cc", &extra_cc, N_("email"), N_("add Cc: header")),
 		OPT_CALLBACK_F(0, "from", &from, N_("ident"),
 			    N_("set From address to <ident> (or committer ident if absent)"),
 			    PARSE_OPT_OPTARG, from_callback),
-- 
2.42.0.561.gaa987ecc69
Jeff King· Aug 31, 2023, 21:20 UTC · re: Jeff King · lore

[PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile

Using --stage=all requires writing to tempfiles, since we cannot put multiple stages into a single file. So --stage=all implies --temp.

But we do this by setting to_tempfile in the options callback for --stage, rather than after all options have been parsed. This leads to two bugs:

  1. If you run "checkout-index --stage=all --stage=2", this should not
     imply --temp, but it currently does. The callback cannot just unset
     to_tempfile when it sees the "2" value, because it no longer knows
     if its value was from the earlier --stage call, or if the user
     specified --temp explicitly.
  2. If you run "checkout-index --stage=all --no-temp", the --no-temp
     will overwrite the earlier implied --temp. But this mode of
     operation cannot work, and the command will fail with "<path>
     already exists" when trying to write the higher stages.

We can fix both by lazily setting to_tempfile. We'll make it a tristate, with -1 as "not yet given", and have --stage=all enable it only after all options are parsed. Likewise, after all options are parsed we can detect and reject the bogus "--no-temp" case.

Note that this does technically change the behavior for "--stage=all --no-temp" for paths which have only one stage present (which accidentally worked before, but is now forbidden). But this behavior was never intended, and you'd have to go out of your way to try to trigger it.

The new tests cover both cases, as well the general "--stage=all implies --temp", as most of the other tests explicitly say "--temp". Ironically, the test "checkout --temp within subdir" is the only one that _doesn't_ use "--temp", and so was implicitly covering this case. But it seems reasonable to have a more explicit test alongside the other related ones.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
One other bug-let I noticed in this function: it only looks at the first
byte of the stage argument, so "--stage=13" is the same as "--stage=1".
We could tighten this, but I wonder if anybody does something weird like
"--stage=2ours" or something. That seems pretty unlikely, but I'd still
prefer to do it as a separate patch (if at all).
 builtin/checkout-index.c       |  8 ++++++--
 t/t2004-checkout-cache-temp.sh | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)
Show changes to 2 files +26 −2

builtin/checkout-index.c, t/t2004-checkout-cache-temp.sh

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f62f13f2b5..6687a495ff 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -24,7 +24,7 @@
 static int nul_term_line;
 static int checkout_stage; /* default to checkout stage0 */
 static int ignore_skip_worktree; /* default to 0 */
-static int to_tempfile;
+static int to_tempfile = -1;
 static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];
 
 static struct checkout state = CHECKOUT_INIT;
@@ -196,7 +196,6 @@ static int option_parse_stage(const struct option *opt,
 	BUG_ON_OPT_NEG(unset);
 
 	if (!strcmp(arg, "all")) {
-		to_tempfile = 1;
 		checkout_stage = CHECKOUT_ALL;
 	} else {
 		int ch = arg[0];
@@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		state.base_dir = "";
 	state.base_dir_len = strlen(state.base_dir);
 
+	if (to_tempfile < 0)
+		to_tempfile = (checkout_stage == CHECKOUT_ALL);
+	if (!to_tempfile && checkout_stage == CHECKOUT_ALL)
+		die("--stage=all and --no-temp are incompatible");
+
 	/*
 	 * when --prefix is specified we do not want to update cache.
 	 */
diff --git a/t/t2004-checkout-cache-temp.sh b/t/t2004-checkout-cache-temp.sh
index b16d69ca4a..8eb2ef44eb 100755
--- a/t/t2004-checkout-cache-temp.sh
+++ b/t/t2004-checkout-cache-temp.sh
@@ -117,6 +117,26 @@ test_expect_success 'checkout all stages/one file to temporary files' '
 	test $(cat $s3) = tree3path1)
 '
 
+test_expect_success '--stage=all implies --temp' '
+	rm -f path* .merge_* actual &&
+	git checkout-index --stage=all -- path1 &&
+	test_path_is_missing path1
+'
+
+test_expect_success 'overriding --stage=all resets implied --temp' '
+	rm -f path* .merge_* actual &&
+	git checkout-index --stage=all --stage=2 -- path1 &&
+	echo tree2path1 >expect &&
+	test_cmp expect path1
+'
+
+test_expect_success '--stage=all --no-temp is rejected' '
+	rm -f path* .merge_* actual &&
+	test_must_fail git checkout-index --stage=all --no-temp -- path1 2>err &&
+	grep -v "already exists" err &&
+	grep "stage=all and --no-temp are incompatible" err
+'
+
 test_expect_success 'checkout some stages/one file to temporary files' '
 	rm -f path* .merge_* actual &&
 	git checkout-index --stage=all --temp -- path2 >actual &&
-- 
2.42.0.561.gaa987ecc69
Junio C Hamano· Aug 31, 2023, 22:12 UTC · re: Jeff King · lore

Re: [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile

Jeff King <peff@peff.net> writes:
> Using --stage=all requires writing to tempfiles, since we cannot put
> multiple stages into a single file. So --stage=all implies --temp.
This one is new, and it does read really well.  Thanks.
René Scharfe· Sep 2, 2023, 06:20 UTC · re: Jeff King · lore

Re: [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile

Am 31.08.23 um 23:20 schrieb Jeff King:
Show 8 quoted lines
> @@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>  		state.base_dir = "";
>  	state.base_dir_len = strlen(state.base_dir);
>
> +	if (to_tempfile < 0)
> +		to_tempfile = (checkout_stage == CHECKOUT_ALL);
> +	if (!to_tempfile && checkout_stage == CHECKOUT_ALL)
> +		die("--stage=all and --no-temp are incompatible");

How about making this message translatable from the start and following the convention from 12909b6b8a (i18n: turn "options are incompatible" into "cannot be used together", 2022-01-05) to reuse the existing translations?

René
Jeff King· Sep 5, 2023, 07:12 UTC · re: René Scharfe · lore

[PATCH v3 04/10] checkout-index: delay automatic setting of to_tempfile

On Sat, Sep 02, 2023 at 08:20:43AM +0200, René Scharfe wrote:
Show 14 quoted lines
> Am 31.08.23 um 23:20 schrieb Jeff King:
> > @@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> >  		state.base_dir = "";
> >  	state.base_dir_len = strlen(state.base_dir);
> >
> > +	if (to_tempfile < 0)
> > +		to_tempfile = (checkout_stage == CHECKOUT_ALL);
> > +	if (!to_tempfile && checkout_stage == CHECKOUT_ALL)
> > +		die("--stage=all and --no-temp are incompatible");
> 
> How about making this message translatable from the start and following
> the convention from 12909b6b8a (i18n: turn "options are incompatible"
> into "cannot be used together", 2022-01-05) to reuse the existing
> translations?

Good catch. I forgot that we had standardized some of these. The other error messages in the file aren't translated, but I don't think that's an intentional choice (even though it is plumbing). Some of them are obviously quite old and don't match our usual style (like starting with "checkout-index: ").

Rather than re-send the whole series, I _think_ this is the only patch I would change in a re-roll (if you buy me "sure, go ahead and send it on top" evasions in my other responses).

So here's a replacement patch 4 that fixes up the message.
-- >8 --
Subject: checkout-index: delay automatic setting of to_tempfile

Using --stage=all requires writing to tempfiles, since we cannot put multiple stages into a single file. So --stage=all implies --temp.

But we do so by setting to_tempfile in the options callback for --stage, rather than after all options have been parsed. This leads to two bugs:

  1. If you run "checkout-index --stage=all --stage=2", this should not
     imply --temp, but it currently does. The callback cannot just unset
     to_tempfile when it sees the "2" value, because it no longer knows
     if its value was from the earlier --stage call, or if the user
     specified --temp explicitly.
  2. If you run "checkout-index --stage=all --no-temp", the --no-temp
     will overwrite the earlier implied --temp. But this mode of
     operation cannot work, and the command will fail with "<path>
     already exists" when trying to write the higher stages.

We can fix both by lazily setting to_tempfile. We'll make it a tristate, with -1 as "not yet given", and have --stage=all enable it only after all options are parsed. Likewise, after all options are parsed we can detect and reject the bogus "--no-temp" case.

Note that this does technically change the behavior for "--stage=all --no-temp" for paths which have only one stage present (which accidentally worked before, but is now forbidden). But this behavior was never intended, and you'd have to go out of your way to try to trigger it.

The new tests cover both cases, as well the general "--stage=all implies --temp", as most of the other tests explicitly say "--temp". Ironically, the test "checkout --temp within subdir" is the only one that _doesn't_ use "--temp", and so was implicitly covering this case. But it seems reasonable to have a more explicit test alongside the other related ones.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/checkout-index.c       |  9 +++++++--
 t/t2004-checkout-cache-temp.sh | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)
Show changes to 2 files +27 −2

builtin/checkout-index.c, t/t2004-checkout-cache-temp.sh

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f62f13f2b5..526c210bcb 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -24,7 +24,7 @@
 static int nul_term_line;
 static int checkout_stage; /* default to checkout stage0 */
 static int ignore_skip_worktree; /* default to 0 */
-static int to_tempfile;
+static int to_tempfile = -1;
 static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];
 
 static struct checkout state = CHECKOUT_INIT;
@@ -196,7 +196,6 @@ static int option_parse_stage(const struct option *opt,
 	BUG_ON_OPT_NEG(unset);
 
 	if (!strcmp(arg, "all")) {
-		to_tempfile = 1;
 		checkout_stage = CHECKOUT_ALL;
 	} else {
 		int ch = arg[0];
@@ -269,6 +268,12 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		state.base_dir = "";
 	state.base_dir_len = strlen(state.base_dir);
 
+	if (to_tempfile < 0)
+		to_tempfile = (checkout_stage == CHECKOUT_ALL);
+	if (!to_tempfile && checkout_stage == CHECKOUT_ALL)
+		die(_("options '%s' and '%s' cannot be used together"),
+		    "--stage=all", "--no-temp");
+
 	/*
 	 * when --prefix is specified we do not want to update cache.
 	 */
diff --git a/t/t2004-checkout-cache-temp.sh b/t/t2004-checkout-cache-temp.sh
index b16d69ca4a..45dd1bc858 100755
--- a/t/t2004-checkout-cache-temp.sh
+++ b/t/t2004-checkout-cache-temp.sh
@@ -117,6 +117,26 @@ test_expect_success 'checkout all stages/one file to temporary files' '
 	test $(cat $s3) = tree3path1)
 '
 
+test_expect_success '--stage=all implies --temp' '
+	rm -f path* .merge_* actual &&
+	git checkout-index --stage=all -- path1 &&
+	test_path_is_missing path1
+'
+
+test_expect_success 'overriding --stage=all resets implied --temp' '
+	rm -f path* .merge_* actual &&
+	git checkout-index --stage=all --stage=2 -- path1 &&
+	echo tree2path1 >expect &&
+	test_cmp expect path1
+'
+
+test_expect_success '--stage=all --no-temp is rejected' '
+	rm -f path* .merge_* actual &&
+	test_must_fail git checkout-index --stage=all --no-temp -- path1 2>err &&
+	grep -v "already exists" err &&
+	grep "options .--stage=all. and .--no-temp. cannot be used together" err
+'
+
 test_expect_success 'checkout some stages/one file to temporary files' '
 	rm -f path* .merge_* actual &&
 	git checkout-index --stage=all --temp -- path2 >actual &&
-- 
2.42.0.567.gc396a4a104
Jeff King· Aug 31, 2023, 21:21 UTC · re: Jeff King · lore

[PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks

We have several parse-options callbacks that ignore their "opt" parameters entirely. This is a little unusual, as we'd normally put the result of the parsing into opt->value. In the case of these callbacks, though, they directly manipulate global variables instead (and in most cases the caller sets opt->value to NULL in the OPT_CALLBACK declaration).

The immediate symptom we'd like to deal with is that the unused "opt" variables trigger -Wunused-parameter. But how to fix that is debatable. One option is to annotate them with UNUSED. But another is to have the caller pass in the appropriate variable via opt->value, and use it. That has the benefit of making the callbacks reusable (in theory at least), and makes it clear from the OPT_CALLBACK declaration which variables will be affected (doubly so for the cases in builtin/fast-export.c, where we do set opt->value, but it is completely ignored!).

The slight downside is that we lose type safety, since they're now passing through void pointers.

I went with the "just use them" approach here. The loss of type safety is unfortunate, but that is already an issue with most of the other callbacks. If we want to try to address that, we should do so more consistently (and this patch would prepare these callbacks for whatever we choose to do there).

Note that in the cases in builtin/fast-export.c, we are passing anonymous enums. We'll have to give them names so that we can declare the appropriate pointer type within the callbacks.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/checkout-index.c     |  8 +++++---
 builtin/describe.c           |  6 ++++--
 builtin/fast-export.c        | 36 +++++++++++++++++++++---------------
 builtin/fetch.c              |  4 ++--
 builtin/interpret-trailers.c | 12 ++++++------
 builtin/pack-objects.c       | 21 ++++++++++++---------
 6 files changed, 50 insertions(+), 37 deletions(-)
Show changes to 6 files +50 −37

builtin/checkout-index.c, builtin/describe.c, builtin/fast-export.c, builtin/fetch.c, builtin/interpret-trailers.c, builtin/pack-objects.c

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 6687a495ff..6ef6ac4c2e 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -193,14 +193,16 @@ static const char * const builtin_checkout_index_usage[] = {
 static int option_parse_stage(const struct option *opt,
 			      const char *arg, int unset)
 {
+	int *stage = opt->value;
+
 	BUG_ON_OPT_NEG(unset);
 
 	if (!strcmp(arg, "all")) {
-		checkout_stage = CHECKOUT_ALL;
+		*stage = CHECKOUT_ALL;
 	} else {
 		int ch = arg[0];
 		if ('1' <= ch && ch <= '3')
-			checkout_stage = arg[0] - '0';
+			*stage = arg[0] - '0';
 		else
 			die(_("stage should be between 1 and 3 or all"));
 	}
@@ -238,7 +240,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 			N_("write the content to temporary files")),
 		OPT_STRING(0, "prefix", &state.base_dir, N_("string"),
 			N_("when creating files, prepend <string>")),
-		OPT_CALLBACK_F(0, "stage", NULL, "(1|2|3|all)",
+		OPT_CALLBACK_F(0, "stage", &checkout_stage, "(1|2|3|all)",
 			N_("copy out the files from named stage"),
 			PARSE_OPT_NONEG, option_parse_stage),
 		OPT_END()
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/fast-export.c b/builtin/fast-export.c
index 56dc69fac1..70aff515ac 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -33,9 +33,9 @@ static const char *fast_export_usage[] = {
 };
 
 static int progress;
-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
-static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
-static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
+static enum signed_tag_mode { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
+static enum tag_of_filtered_mode { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
+static enum reencode_mode { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
 static int fake_missing_tagger;
 static int use_done_feature;
 static int no_data;
@@ -53,16 +53,18 @@ static struct revision_sources revision_sources;
 static int parse_opt_signed_tag_mode(const struct option *opt,
 				     const char *arg, int unset)
 {
+	enum signed_tag_mode *val = opt->value;
+
 	if (unset || !strcmp(arg, "abort"))
-		signed_tag_mode = SIGNED_TAG_ABORT;
+		*val = SIGNED_TAG_ABORT;
 	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
-		signed_tag_mode = VERBATIM;
+		*val = VERBATIM;
 	else if (!strcmp(arg, "warn"))
-		signed_tag_mode = WARN;
+		*val = WARN;
 	else if (!strcmp(arg, "warn-strip"))
-		signed_tag_mode = WARN_STRIP;
+		*val = WARN_STRIP;
 	else if (!strcmp(arg, "strip"))
-		signed_tag_mode = STRIP;
+		*val = STRIP;
 	else
 		return error("Unknown signed-tags mode: %s", arg);
 	return 0;
@@ -71,12 +73,14 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
 static int parse_opt_tag_of_filtered_mode(const struct option *opt,
 					  const char *arg, int unset)
 {
+	enum tag_of_filtered_mode *val = opt->value;
+
 	if (unset || !strcmp(arg, "abort"))
-		tag_of_filtered_mode = TAG_FILTERING_ABORT;
+		*val = TAG_FILTERING_ABORT;
 	else if (!strcmp(arg, "drop"))
-		tag_of_filtered_mode = DROP;
+		*val = DROP;
 	else if (!strcmp(arg, "rewrite"))
-		tag_of_filtered_mode = REWRITE;
+		*val = REWRITE;
 	else
 		return error("Unknown tag-of-filtered mode: %s", arg);
 	return 0;
@@ -85,21 +89,23 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,
 static int parse_opt_reencode_mode(const struct option *opt,
 				   const char *arg, int unset)
 {
+	enum reencode_mode *val = opt->value;
+
 	if (unset) {
-		reencode_mode = REENCODE_ABORT;
+		*val = REENCODE_ABORT;
 		return 0;
 	}
 
 	switch (git_parse_maybe_bool(arg)) {
 	case 0:
-		reencode_mode = REENCODE_NO;
+		*val = REENCODE_NO;
 		break;
 	case 1:
-		reencode_mode = REENCODE_YES;
+		*val = REENCODE_YES;
 		break;
 	default:
 		if (!strcasecmp(arg, "abort"))
-			reencode_mode = REENCODE_ABORT;
+			*val = REENCODE_ABORT;
 		else
 			return error("Unknown reencoding mode: %s", arg);
 	}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8f93529505..fd134ba74d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -176,7 +176,7 @@ static int parse_refmap_arg(const struct option *opt, const char *arg, int unset
 	 * "git fetch --refmap='' origin foo"
 	 * can be used to tell the command not to store anywhere
 	 */
-	refspec_append(&refmap, arg);
+	refspec_append(opt->value, arg);
 
 	return 0;
 }
@@ -2204,7 +2204,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			   PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules),
 		OPT_BOOL(0, "update-shallow", &update_shallow,
 			 N_("accept refs that update .git/shallow")),
-		OPT_CALLBACK_F(0, "refmap", NULL, N_("refmap"),
+		OPT_CALLBACK_F(0, "refmap", &refmap, N_("refmap"),
 			       N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
 		OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
 		OPT_IPVERSION(&family),
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index c5e8345265..6aadce6a1e 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -26,19 +26,19 @@ static enum trailer_if_missing if_missing;
 static int option_parse_where(const struct option *opt,
 			      const char *arg, int unset)
 {
-	return trailer_set_where(&where, arg);
+	return trailer_set_where(opt->value, arg);
 }
 
 static int option_parse_if_exists(const struct option *opt,
 				  const char *arg, int unset)
 {
-	return trailer_set_if_exists(&if_exists, arg);
+	return trailer_set_if_exists(opt->value, arg);
 }
 
 static int option_parse_if_missing(const struct option *opt,
 				   const char *arg, int unset)
 {
-	return trailer_set_if_missing(&if_missing, arg);
+	return trailer_set_if_missing(opt->value, arg);
 }
 
 static void new_trailers_clear(struct list_head *trailers)
@@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 
-		OPT_CALLBACK(0, "where", NULL, N_("action"),
+		OPT_CALLBACK(0, "where", &where, N_("action"),
 			     N_("where to place the new trailer"), option_parse_where),
-		OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
+		OPT_CALLBACK(0, "if-exists", &if_exists, N_("action"),
 			     N_("action if trailer already exists"), option_parse_if_exists),
-		OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
+		OPT_CALLBACK(0, "if-missing", &if_missing, N_("action"),
 			     N_("action if trailer is missing"), option_parse_if_missing),
 
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d2a162d528..492372ee5d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4120,29 +4120,32 @@ 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;
 }
 
 static int option_parse_index_version(const struct option *opt,
 				      const char *arg, int unset)
 {
+	struct pack_idx_option *popts = opt->value;
 	char *c;
 	const char *val = arg;
 
 	BUG_ON_OPT_NEG(unset);
 
-	pack_idx_opts.version = strtoul(val, &c, 10);
-	if (pack_idx_opts.version > 2)
+	popts->version = strtoul(val, &c, 10);
+	if (popts->version > 2)
 		die(_("unsupported index version %s"), val);
 	if (*c == ',' && c[1])
-		pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);
-	if (*c || pack_idx_opts.off32_limit & 0x80000000)
+		popts->off32_limit = strtoul(c+1, &c, 0);
+	if (*c || popts->off32_limit & 0x80000000)
 		die(_("bad index version '%s'"), val);
 	return 0;
 }
@@ -4190,7 +4193,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,
@@ -4200,7 +4203,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "all-progress-implied",
 			 &all_progress_implied,
 			 N_("similar to --all-progress when progress meter is shown")),
-		OPT_CALLBACK_F(0, "index-version", NULL, N_("<version>[,<offset>]"),
+		OPT_CALLBACK_F(0, "index-version", &pack_idx_opts, N_("<version>[,<offset>]"),
 		  N_("write the pack index file in the specified idx format version"),
 		  PARSE_OPT_NONEG, option_parse_index_version),
 		OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit,
-- 
2.42.0.561.gaa987ecc69
René Scharfe· Sep 2, 2023, 07:34 UTC · re: Jeff King · lore

Re: [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks

Am 31.08.23 um 23:21 schrieb Jeff King:
Show 15 quoted lines
> We have several parse-options callbacks that ignore their "opt"
> parameters entirely. This is a little unusual, as we'd normally put the
> result of the parsing into opt->value. In the case of these callbacks,
> though, they directly manipulate global variables instead (and in
> most cases the caller sets opt->value to NULL in the OPT_CALLBACK
> declaration).
>
> The immediate symptom we'd like to deal with is that the unused "opt"
> variables trigger -Wunused-parameter. But how to fix that is debatable.
> One option is to annotate them with UNUSED. But another is to have the
> caller pass in the appropriate variable via opt->value, and use it. That
> has the benefit of making the callbacks reusable (in theory at least),
> and makes it clear from the OPT_CALLBACK declaration which variables
> will be affected (doubly so for the cases in builtin/fast-export.c,
> where we do set opt->value, but it is completely ignored!).

Which allows turning global variables into local ones later, so unlocks more cleanup potential.

Show 8 quoted lines
> The slight downside is that we lose type safety, since they're now
> passing through void pointers.
>
> I went with the "just use them" approach here. The loss of type safety
> is unfortunate, but that is already an issue with most of the other
> callbacks. If we want to try to address that, we should do so more
> consistently (and this patch would prepare these callbacks for whatever
> we choose to do there).
Makes sense.  The types used below look OK to me.
Show 206 quoted lines
> Note that in the cases in builtin/fast-export.c, we are passing
> anonymous enums. We'll have to give them names so that we can declare
> the appropriate pointer type within the callbacks.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/checkout-index.c     |  8 +++++---
>  builtin/describe.c           |  6 ++++--
>  builtin/fast-export.c        | 36 +++++++++++++++++++++---------------
>  builtin/fetch.c              |  4 ++--
>  builtin/interpret-trailers.c | 12 ++++++------
>  builtin/pack-objects.c       | 21 ++++++++++++---------
>  6 files changed, 50 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 6687a495ff..6ef6ac4c2e 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -193,14 +193,16 @@ static const char * const builtin_checkout_index_usage[] = {
>  static int option_parse_stage(const struct option *opt,
>  			      const char *arg, int unset)
>  {
> +	int *stage = opt->value;
> +
>  	BUG_ON_OPT_NEG(unset);
>
>  	if (!strcmp(arg, "all")) {
> -		checkout_stage = CHECKOUT_ALL;
> +		*stage = CHECKOUT_ALL;
>  	} else {
>  		int ch = arg[0];
>  		if ('1' <= ch && ch <= '3')
> -			checkout_stage = arg[0] - '0';
> +			*stage = arg[0] - '0';
>  		else
>  			die(_("stage should be between 1 and 3 or all"));
>  	}
> @@ -238,7 +240,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>  			N_("write the content to temporary files")),
>  		OPT_STRING(0, "prefix", &state.base_dir, N_("string"),
>  			N_("when creating files, prepend <string>")),
> -		OPT_CALLBACK_F(0, "stage", NULL, "(1|2|3|all)",
> +		OPT_CALLBACK_F(0, "stage", &checkout_stage, "(1|2|3|all)",
>  			N_("copy out the files from named stage"),
>  			PARSE_OPT_NONEG, option_parse_stage),
>  		OPT_END()
> 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/fast-export.c b/builtin/fast-export.c
> index 56dc69fac1..70aff515ac 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -33,9 +33,9 @@ static const char *fast_export_usage[] = {
>  };
>
>  static int progress;
> -static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
> -static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
> -static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
> +static enum signed_tag_mode { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
> +static enum tag_of_filtered_mode { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
> +static enum reencode_mode { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
>  static int fake_missing_tagger;
>  static int use_done_feature;
>  static int no_data;
> @@ -53,16 +53,18 @@ static struct revision_sources revision_sources;
>  static int parse_opt_signed_tag_mode(const struct option *opt,
>  				     const char *arg, int unset)
>  {
> +	enum signed_tag_mode *val = opt->value;
> +
>  	if (unset || !strcmp(arg, "abort"))
> -		signed_tag_mode = SIGNED_TAG_ABORT;
> +		*val = SIGNED_TAG_ABORT;
>  	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
> -		signed_tag_mode = VERBATIM;
> +		*val = VERBATIM;
>  	else if (!strcmp(arg, "warn"))
> -		signed_tag_mode = WARN;
> +		*val = WARN;
>  	else if (!strcmp(arg, "warn-strip"))
> -		signed_tag_mode = WARN_STRIP;
> +		*val = WARN_STRIP;
>  	else if (!strcmp(arg, "strip"))
> -		signed_tag_mode = STRIP;
> +		*val = STRIP;
>  	else
>  		return error("Unknown signed-tags mode: %s", arg);
>  	return 0;
> @@ -71,12 +73,14 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
>  static int parse_opt_tag_of_filtered_mode(const struct option *opt,
>  					  const char *arg, int unset)
>  {
> +	enum tag_of_filtered_mode *val = opt->value;
> +
>  	if (unset || !strcmp(arg, "abort"))
> -		tag_of_filtered_mode = TAG_FILTERING_ABORT;
> +		*val = TAG_FILTERING_ABORT;
>  	else if (!strcmp(arg, "drop"))
> -		tag_of_filtered_mode = DROP;
> +		*val = DROP;
>  	else if (!strcmp(arg, "rewrite"))
> -		tag_of_filtered_mode = REWRITE;
> +		*val = REWRITE;
>  	else
>  		return error("Unknown tag-of-filtered mode: %s", arg);
>  	return 0;
> @@ -85,21 +89,23 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,
>  static int parse_opt_reencode_mode(const struct option *opt,
>  				   const char *arg, int unset)
>  {
> +	enum reencode_mode *val = opt->value;
> +
>  	if (unset) {
> -		reencode_mode = REENCODE_ABORT;
> +		*val = REENCODE_ABORT;
>  		return 0;
>  	}
>
>  	switch (git_parse_maybe_bool(arg)) {
>  	case 0:
> -		reencode_mode = REENCODE_NO;
> +		*val = REENCODE_NO;
>  		break;
>  	case 1:
> -		reencode_mode = REENCODE_YES;
> +		*val = REENCODE_YES;
>  		break;
>  	default:
>  		if (!strcasecmp(arg, "abort"))
> -			reencode_mode = REENCODE_ABORT;
> +			*val = REENCODE_ABORT;
>  		else
>  			return error("Unknown reencoding mode: %s", arg);
>  	}
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 8f93529505..fd134ba74d 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -176,7 +176,7 @@ static int parse_refmap_arg(const struct option *opt, const char *arg, int unset
>  	 * "git fetch --refmap='' origin foo"
>  	 * can be used to tell the command not to store anywhere
>  	 */
> -	refspec_append(&refmap, arg);
> +	refspec_append(opt->value, arg);
>
>  	return 0;
>  }
> @@ -2204,7 +2204,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  			   PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules),
>  		OPT_BOOL(0, "update-shallow", &update_shallow,
>  			 N_("accept refs that update .git/shallow")),
> -		OPT_CALLBACK_F(0, "refmap", NULL, N_("refmap"),
> +		OPT_CALLBACK_F(0, "refmap", &refmap, N_("refmap"),
>  			       N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
>  		OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
>  		OPT_IPVERSION(&family),
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index c5e8345265..6aadce6a1e 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -26,19 +26,19 @@ static enum trailer_if_missing if_missing;
>  static int option_parse_where(const struct option *opt,
>  			      const char *arg, int unset)
>  {
> -	return trailer_set_where(&where, arg);
> +	return trailer_set_where(opt->value, arg);
>  }
>
>  static int option_parse_if_exists(const struct option *opt,
>  				  const char *arg, int unset)
>  {
> -	return trailer_set_if_exists(&if_exists, arg);
> +	return trailer_set_if_exists(opt->value, arg);
>  }
>
>  static int option_parse_if_missing(const struct option *opt,
>  				   const char *arg, int unset)
>  {
> -	return trailer_set_if_missing(&if_missing, arg);
> +	return trailer_set_if_missing(opt->value, arg);
>  }

Not your fault, but these all silently exit if "arg" contains an unrecognized value. Reporting the error would be better.

Show 14 quoted lines
>  static void new_trailers_clear(struct list_head *trailers)
> @@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
>  		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
>
> -		OPT_CALLBACK(0, "where", NULL, N_("action"),
> +		OPT_CALLBACK(0, "where", &where, N_("action"),
>  			     N_("where to place the new trailer"), option_parse_where),
> -		OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
> +		OPT_CALLBACK(0, "if-exists", &if_exists, N_("action"),
>  			     N_("action if trailer already exists"), option_parse_if_exists),
> -		OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
> +		OPT_CALLBACK(0, "if-missing", &if_missing, N_("action"),
>  			     N_("action if trailer is missing"), option_parse_if_missing),

And I wonder if "action" should be replaced by "(after|before|end|start)", "(addIfDifferent|addIfDifferentNeighbor|add|replace|doNothing)" and "(doNothing|add)", respectively. Gets a bit long in the middle, but would be more helpful. #leftoverbits

Show 63 quoted lines
>  		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index d2a162d528..492372ee5d 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4120,29 +4120,32 @@ 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;
>  }
>
>  static int option_parse_index_version(const struct option *opt,
>  				      const char *arg, int unset)
>  {
> +	struct pack_idx_option *popts = opt->value;
>  	char *c;
>  	const char *val = arg;
>
>  	BUG_ON_OPT_NEG(unset);
>
> -	pack_idx_opts.version = strtoul(val, &c, 10);
> -	if (pack_idx_opts.version > 2)
> +	popts->version = strtoul(val, &c, 10);
> +	if (popts->version > 2)
>  		die(_("unsupported index version %s"), val);
>  	if (*c == ',' && c[1])
> -		pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);
> -	if (*c || pack_idx_opts.off32_limit & 0x80000000)
> +		popts->off32_limit = strtoul(c+1, &c, 0);
> +	if (*c || popts->off32_limit & 0x80000000)
>  		die(_("bad index version '%s'"), val);
>  	return 0;
>  }
> @@ -4190,7 +4193,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,
> @@ -4200,7 +4203,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL(0, "all-progress-implied",
>  			 &all_progress_implied,
>  			 N_("similar to --all-progress when progress meter is shown")),
> -		OPT_CALLBACK_F(0, "index-version", NULL, N_("<version>[,<offset>]"),
> +		OPT_CALLBACK_F(0, "index-version", &pack_idx_opts, N_("<version>[,<offset>]"),
>  		  N_("write the pack index file in the specified idx format version"),
>  		  PARSE_OPT_NONEG, option_parse_index_version),
>  		OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit,
Jeff King· Sep 5, 2023, 06:52 UTC · re: René Scharfe · lore

Re: [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks

On Sat, Sep 02, 2023 at 09:34:32AM +0200, René Scharfe wrote:
Show 9 quoted lines
> >  static int option_parse_if_missing(const struct option *opt,
> >  				   const char *arg, int unset)
> >  {
> > -	return trailer_set_if_missing(&if_missing, arg);
> > +	return trailer_set_if_missing(opt->value, arg);
> >  }
> 
> Not your fault, but these all silently exit if "arg" contains an
> unrecognized value.  Reporting the error would be better.

Hmm, yeah. On the config side, git_trailer_default_config() issues a warning after the functions return -1. I'd have guessed we at least printed a usage message or something here, but we don't even do that. You get a silent 129 exit code.

Gross, but it's something I think we should fix separately, as it's orthogonal to this series.

Show 19 quoted lines
> >  static void new_trailers_clear(struct list_head *trailers)
> > @@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
> >  		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
> >  		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
> >
> > -		OPT_CALLBACK(0, "where", NULL, N_("action"),
> > +		OPT_CALLBACK(0, "where", &where, N_("action"),
> >  			     N_("where to place the new trailer"), option_parse_where),
> > -		OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
> > +		OPT_CALLBACK(0, "if-exists", &if_exists, N_("action"),
> >  			     N_("action if trailer already exists"), option_parse_if_exists),
> > -		OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
> > +		OPT_CALLBACK(0, "if-missing", &if_missing, N_("action"),
> >  			     N_("action if trailer is missing"), option_parse_if_missing),
> 
> And I wonder if "action" should be replaced by "(after|before|end|start)",
> "(addIfDifferent|addIfDifferentNeighbor|add|replace|doNothing)" and
> "(doNothing|add)", respectively.  Gets a bit long in the middle, but would
> be more helpful.  #leftoverbits

I don't have a strong opinion. It is sometimes nice to provide more detail to save the user having to look it up separately. But sometimes those details can be overwhelming and make it hard to read, especially if they grow over time. I'm thinking less of "-h" output and more of people like to put:

  git foo [--optionA|--optionB|--optionC]

and so on, until eventually the options block is like 4 lines long. Just saying [options] and then listing them is more friendly. I'm not sure if we're approaching that kind of problem here or not.

-Peff
Jeff King· Aug 31, 2023, 21:21 UTC · re: Jeff King · lore

[PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks

The previous commit argued that parse-options callbacks should try to use opt->value rather than touching globals directly. In some cases, however, that's awkward to do. Some callbacks touch multiple variables, or may even just call into an abstracted function that does so.

In some of these cases we _could_ convert them by stuffing the multiple variables into a single struct and passing the struct pointer through opt->value. But that may make other parts of the code less readable, as the struct relationship has to be mentioned everywhere.

Let's just accept that these cases are special and leave them as-is. But we do need to mark their "opt" parameters to satisfy -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/gc.c           |  2 +-
 builtin/log.c          | 10 ++++++----
 builtin/merge.c        |  2 +-
 builtin/pack-objects.c |  6 +++---
 builtin/read-tree.c    |  2 +-
 builtin/update-index.c |  4 ++--
 6 files changed, 14 insertions(+), 12 deletions(-)
Show changes to 6 files +14 −12

builtin/gc.c, builtin/log.c, builtin/merge.c, builtin/pack-objects.c, builtin/read-tree.c, builtin/update-index.c

diff --git a/builtin/gc.c b/builtin/gc.c
index 369bd43fb2..b842349d86 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
 	strbuf_release(&config_name);
 }
 
-static int task_option_parse(const struct option *opt,
+static int task_option_parse(const struct option *opt UNUSED,
 			     const char *arg, int unset)
 {
 	int i, num_selected = 0;
diff --git a/builtin/log.c b/builtin/log.c
index fb90d43717..3599063554 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -118,16 +118,17 @@ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
 static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
 static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
 
-static int clear_decorations_callback(const struct option *opt,
-					    const char *arg, int unset)
+static int clear_decorations_callback(const struct option *opt UNUSED,
+				      const char *arg, int unset)
 {
 	string_list_clear(&decorate_refs_include, 0);
 	string_list_clear(&decorate_refs_exclude, 0);
 	use_default_decoration_filter = 0;
 	return 0;
 }
 
-static int decorate_callback(const struct option *opt, const char *arg, int unset)
+static int decorate_callback(const struct option *opt UNUSED, const char *arg,
+			     int unset)
 {
 	if (unset)
 		decoration_style = 0;
@@ -1555,7 +1556,8 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static int header_callback(const struct option *opt, const char *arg, int unset)
+static int header_callback(const struct option *opt UNUSED, const char *arg,
+			   int unset)
 {
 	if (unset) {
 		string_list_clear(&extra_hdr, 0);
diff --git a/builtin/merge.c b/builtin/merge.c
index 21363b7985..0436986dab 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -231,7 +231,7 @@ static void append_strategy(struct strategy *s)
 	use_strategies[use_strategies_nr++] = s;
 }
 
-static int option_parse_strategy(const struct option *opt,
+static int option_parse_strategy(const struct option *opt UNUSED,
 				 const char *name, int unset)
 {
 	if (unset)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 492372ee5d..91b4b7c177 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3739,7 +3739,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
 	show_object(obj, name, data);
 }
 
-static int option_parse_missing_action(const struct option *opt,
+static int option_parse_missing_action(const struct option *opt UNUSED,
 				       const char *arg, int unset)
 {
 	assert(arg);
@@ -4150,7 +4150,7 @@ static int option_parse_index_version(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_unpack_unreachable(const struct option *opt,
+static int option_parse_unpack_unreachable(const struct option *opt UNUSED,
 					   const char *arg, int unset)
 {
 	if (unset) {
@@ -4165,7 +4165,7 @@ static int option_parse_unpack_unreachable(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_cruft_expiration(const struct option *opt,
+static int option_parse_cruft_expiration(const struct option *opt UNUSED,
 					 const char *arg, int unset)
 {
 	if (unset) {
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 1fec702a04..8196ca9dd8 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -49,7 +49,7 @@ static const char * const read_tree_usage[] = {
 	NULL
 };
 
-static int index_output_cb(const struct option *opt, const char *arg,
+static int index_output_cb(const struct option *opt UNUSED, const char *arg,
 				 int unset)
 {
 	BUG_ON_OPT_NEG(unset);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index aee3cb8cbd..59acae3336 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,
 	return 0;
 }
 
-static int resolve_undo_clear_callback(const struct option *opt,
+static int resolve_undo_clear_callback(const struct option *opt UNUSED,
 				const char *arg, int unset)
 {
 	BUG_ON_OPT_NEG(unset);
@@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,
 }
 
 static enum parse_opt_result cacheinfo_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	struct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,
 	const char *arg, int unset)
 {
 	struct object_id oid;
-- 
2.42.0.561.gaa987ecc69
René Scharfe· Sep 2, 2023, 10:12 UTC · re: Jeff King · lore

Re: [PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks

Am 31.08.23 um 23:21 schrieb Jeff King:
Show 9 quoted lines
> The previous commit argued that parse-options callbacks should try to
> use opt->value rather than touching globals directly. In some cases,
> however, that's awkward to do. Some callbacks touch multiple variables,
> or may even just call into an abstracted function that does so.
>
> In some of these cases we _could_ convert them by stuffing the multiple
> variables into a single struct and passing the struct pointer through
> opt->value. But that may make other parts of the code less readable,
> as the struct relationship has to be mentioned everywhere.

Does that imply you'd be willing to use other methods? Let's find out below. :)

Show 24 quoted lines
>
> Let's just accept that these cases are special and leave them as-is. But
> we do need to mark their "opt" parameters to satisfy -Wunused-parameter.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/gc.c           |  2 +-
>  builtin/log.c          | 10 ++++++----
>  builtin/merge.c        |  2 +-
>  builtin/pack-objects.c |  6 +++---
>  builtin/read-tree.c    |  2 +-
>  builtin/update-index.c |  4 ++--
>  6 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 369bd43fb2..b842349d86 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
>  	strbuf_release(&config_name);
>  }
>
> -static int task_option_parse(const struct option *opt,
> +static int task_option_parse(const struct option *opt UNUSED,

Only the global variable "tasks" seems to be used in here if you don't count the constant "TASK__COUNT", so you could pass it in. This could also be converted to OPT_STRING_LIST with parsing and duplicate checking done later.

And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION) on error, but that's a different matter.

Show 22 quoted lines
>  			     const char *arg, int unset)
>  {
>  	int i, num_selected = 0;
> diff --git a/builtin/log.c b/builtin/log.c
> index fb90d43717..3599063554 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -118,16 +118,17 @@ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
>  static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
>  static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
>
> -static int clear_decorations_callback(const struct option *opt,
> -					    const char *arg, int unset)
> +static int clear_decorations_callback(const struct option *opt UNUSED,
> +				      const char *arg, int unset)
>  {
>  	string_list_clear(&decorate_refs_include, 0);
>  	string_list_clear(&decorate_refs_exclude, 0);
>  	use_default_decoration_filter = 0;
>  	return 0;
>  }
>
Meta: Why do we get seven lines of context in an -U3 patch here?  Did
you use --inter-hunk-context?

This patch would be better viewed with --function-context to see that the callbacks change multiple variables or do other funky things. Only doubles the line count.

Show 6 quoted lines
> -static int decorate_callback(const struct option *opt, const char *arg, int unset)
> +static int decorate_callback(const struct option *opt UNUSED, const char *arg,
> +			     int unset)
>  {
>  	if (unset)
>  		decoration_style = 0;

Sets decoration_style and decoration_given. Never sets decoration_given to a negative value. This could be used to initialize decoration_style to -1 and set decoration_given after parsing, then you could pass in just decoration_style.

Show 6 quoted lines
> @@ -1555,7 +1556,8 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>
> -static int header_callback(const struct option *opt, const char *arg, int unset)
> +static int header_callback(const struct option *opt UNUSED, const char *arg,

Sorts strings into three string_lists based on prefix. Could become a vanilla OPT_STRING_LIST with parsing and sorting done later, if it wouldn't interact with to_callback() and cc_callback().

(--function-context is not enough to see that, it requires a look at the definition of those other functions as well.)

Show 14 quoted lines
> +			   int unset)
>  {
>  	if (unset) {
>  		string_list_clear(&extra_hdr, 0);
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 21363b7985..0436986dab 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -231,7 +231,7 @@ static void append_strategy(struct strategy *s)
>  	use_strategies[use_strategies_nr++] = s;
>  }
>
> -static int option_parse_strategy(const struct option *opt,
> +static int option_parse_strategy(const struct option *opt UNUSED,

Could be an OPT_STRING_LIST and parsing done later. Except that --no-strategy does nothing, which is weird.

Show 13 quoted lines
>  				 const char *name, int unset)
>  {
>  	if (unset)
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 492372ee5d..91b4b7c177 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3739,7 +3739,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
>  	show_object(obj, name, data);
>  }
>
> -static int option_parse_missing_action(const struct option *opt,
> +static int option_parse_missing_action(const struct option *opt UNUSED,

Sets the enum arg_missing_action, fetch_if_missing and fn_show_object. Could be changed to set only the enum and then you could pass it in.

Show 9 quoted lines
>  				       const char *arg, int unset)
>  {
>  	assert(arg);
> @@ -4150,7 +4150,7 @@ static int option_parse_index_version(const struct option *opt,
>  	return 0;
>  }
>
> -static int option_parse_unpack_unreachable(const struct option *opt,
> +static int option_parse_unpack_unreachable(const struct option *opt UNUSED,

Sets both unpack_unreachable and unpack_unreachable_expiration. Perhaps could set only unpack_unreachable_expiration and use TIME_MAX if no argument is given, then set unpack_unreachable after parsing? Not sure.

Show 9 quoted lines
>  					   const char *arg, int unset)
>  {
>  	if (unset) {
> @@ -4165,7 +4165,7 @@ static int option_parse_unpack_unreachable(const struct option *opt,
>  	return 0;
>  }
>
> -static int option_parse_cruft_expiration(const struct option *opt,
> +static int option_parse_cruft_expiration(const struct option *opt UNUSED,

Does the same as option_parse_unpack_unreachable(), just with two different variables. And interacts with --cruft.

Show 13 quoted lines
>  					 const char *arg, int unset)
>  {
>  	if (unset) {
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index 1fec702a04..8196ca9dd8 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -49,7 +49,7 @@ static const char * const read_tree_usage[] = {
>  	NULL
>  };
>
> -static int index_output_cb(const struct option *opt, const char *arg,
> +static int index_output_cb(const struct option *opt UNUSED, const char *arg,

Calls set_alternate_index_output(). Could become an OPT_STRING_F if we assume that the last value wins and earlier calls have no side effects.

Show 13 quoted lines
>  				 int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index aee3cb8cbd..59acae3336 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,
>  	return 0;
>  }
>
> -static int resolve_undo_clear_callback(const struct option *opt,
> +static int resolve_undo_clear_callback(const struct option *opt UNUSED,

Affects the_index. This option (--clear-resolve-undo) is not mentioned in Documentation/, by the way. Not sure if it interacts with other callbacks, like the one below. Otherwise could be an OPT_BOOL and its action done after parsing. Perhaps pass in &the_index?

Show 9 quoted lines
>  				const char *arg, int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);
> @@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,
>  }
>
>  static enum parse_opt_result cacheinfo_callback(
> -	struct parse_opt_ctx_t *ctx, const struct option *opt,
> +	struct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,

A low-level callback for a change. Affects the_index. Pass it in? add_cacheinfo() would need to grow a parameter to receive it. It would document that this option directly changes the_index.

>  	const char *arg, int unset)
>  {
>  	struct object_id oid;
Phew, looks like that's stuff for a whole new series or two!
René
Jeff King· Sep 5, 2023, 07:05 UTC · re: René Scharfe · lore

Re: [PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks

On Sat, Sep 02, 2023 at 12:12:56PM +0200, René Scharfe wrote:
Show 13 quoted lines
> Am 31.08.23 um 23:21 schrieb Jeff King:
> > The previous commit argued that parse-options callbacks should try to
> > use opt->value rather than touching globals directly. In some cases,
> > however, that's awkward to do. Some callbacks touch multiple variables,
> > or may even just call into an abstracted function that does so.
> >
> > In some of these cases we _could_ convert them by stuffing the multiple
> > variables into a single struct and passing the struct pointer through
> > opt->value. But that may make other parts of the code less readable,
> > as the struct relationship has to be mentioned everywhere.
> 
> Does that imply you'd be willing to use other methods?  Let's find out
> below. :)

Well, I'm not necessarily _opposed_. :) Mostly my cutoff was cases where the end result was not obviously and immediately more readable. It is not that big a deal to add an UNUSED annotation. My main goal was to use the opportunity to check that we aren't papering over an obvious bug.

So for example...
Show 15 quoted lines
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 369bd43fb2..b842349d86 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
> >  	strbuf_release(&config_name);
> >  }
> >
> > -static int task_option_parse(const struct option *opt,
> > +static int task_option_parse(const struct option *opt UNUSED,
> 
> Only the global variable "tasks" seems to be used in here if you don't
> count the constant "TASK__COUNT", so you could pass it in.  This could
> also be converted to OPT_STRING_LIST with parsing and duplicate checking
> done later.

...in many cases things can be simplified by parsing into a string, and then validating or acting on the string later. And sometimes that's even a better strategy (because it lets the arg parsing handle all the "last one wins" logic and we just get the result).

But it can also make the code harder to follow, too, because it's now split it into segments (although one is mostly declarative, which is nice).

So I generally tried to err on the side of not touching working code. Both to avoid breaking it, but also to keep the focus on the goal of the series. And I think that applies to a lot of the other cases you mentioned below (I won't respond to each; I think some of them could be fine, but it also feels like writing and review effort for not much gain. I'm not opposed if you want to dig into them, though).

> And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION)
> on error, but that's a different matter.
Yeah, that doesn't make sense at all.
Show 14 quoted lines
> > -static int clear_decorations_callback(const struct option *opt,
> > -					    const char *arg, int unset)
> > +static int clear_decorations_callback(const struct option *opt UNUSED,
> > +				      const char *arg, int unset)
> >  {
> >  	string_list_clear(&decorate_refs_include, 0);
> >  	string_list_clear(&decorate_refs_exclude, 0);
> >  	use_default_decoration_filter = 0;
> >  	return 0;
> >  }
> >
> 
> Meta: Why do we get seven lines of context in an -U3 patch here?  Did
> you use --inter-hunk-context?

Yes, I set diff.interhunkcontext=1 in my config (and have for many years; I think this is the first time anybody noticed in a patch). My rationale is that with "1" the output is never longer, since you save the hunk header. It is a little funny in this case, since the two changes aren't really connected.

> This patch would be better viewed with --function-context to see that
> the callbacks change multiple variables or do other funky things.  Only
> doubles the line count.

I do sometimes use options like that to make a patch more readable, if I notice the difference. I didn't happen to in this case (though I don't disagree with you). As a reviewer, I typically apply and run "git show" myself if I want to dig more (or just go directly look at the preimage in my local repo, of course).

Show 5 quoted lines
> > -static int option_parse_strategy(const struct option *opt,
> > +static int option_parse_strategy(const struct option *opt UNUSED,
> 
> Could be an OPT_STRING_LIST and parsing done later.  Except that
> --no-strategy does nothing, which is weird.

Yeah, I think the handling of "unset" is a bug (similar to the xopts one fixed earlier).

-Peff
René Scharfe· Sep 19, 2023, 07:42 UTC · re: Jeff King · lore

Re: [PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks

Am 05.09.23 um 09:05 schrieb Jeff King:
Show 6 quoted lines
> On Sat, Sep 02, 2023 at 12:12:56PM +0200, René Scharfe wrote:
>
>> And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION)
>> on error, but that's a different matter.
>
> Yeah, that doesn't make sense at all.

Actually it does: any non-zero return from a callback is interpreted as an error. "return error(...);" would be shorter, but "error(...); return 1;" is not wrong. Sorry for the noise!

René
Jeff King· Aug 31, 2023, 21:21 UTC · re: Jeff King · lore

[PATCH v2 07/10] merge: do not pass unused opt->value parameter

The option_parse_strategy() callback does not look at opt->value; instead it calls append_strategy(), which manipulates the global use_strategies array directly. But the OPT_CALLBACK declaration assigns "&use_strategies" to opt->value.

One could argue this is good, as it tells the reader what we generally expect the callback to do. But it is also bad, because it can mislead you into thinking that swapping out "&use_strategies" there might have any effect. Let's switch it to pass NULL (which is what every other "does not bother to look at opt->value" callback does). If you want to know what the callback does, it's easy to read the function itself.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/merge.c +1 −1
diff --git a/builtin/merge.c b/builtin/merge.c
index 0436986dab..545da0c8a1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -264,7 +264,7 @@ static struct option builtin_merge_options[] = {
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 	OPT_BOOL(0, "verify-signatures", &verify_signatures,
 		N_("verify that the named commit has a valid GPG signature")),
-	OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
+	OPT_CALLBACK('s', "strategy", NULL, N_("strategy"),
 		N_("merge strategy to use"), option_parse_strategy),
 	OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
 		N_("option for selected merge strategy")),
-- 
2.42.0.561.gaa987ecc69
Jeff King· Aug 31, 2023, 21:21 UTC · re: Jeff King · lore

[PATCH v2 08/10] parse-options: add more BUG_ON() annotations

These callbacks are similar to the ones touched by 517fe807d6 (assert NOARG/NONEG behavior of parse-options callbacks, 2018-11-05), but were either missed in that commit (the one in add.c) or were added later (the one in log.c).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/add.c | 2 ++
 builtin/log.c | 2 ++
 2 files changed, 4 insertions(+)
Show changes to 2 files +4 −0

builtin/add.c, builtin/log.c

diff --git a/builtin/add.c b/builtin/add.c
index 4b0dd798df..cf59108523 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -232,6 +232,8 @@ static char *chmod_arg;
 
 static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
 {
+	BUG_ON_OPT_ARG(arg);
+
 	/* if we are told to ignore, we are not adding removals */
 	*(int *)opt->value = !unset ? 0 : 1;
 	return 0;
diff --git a/builtin/log.c b/builtin/log.c
index 3599063554..190e1952e9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -121,6 +121,8 @@ static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
 static int clear_decorations_callback(const struct option *opt UNUSED,
 				      const char *arg, int unset)
 {
+	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 	string_list_clear(&decorate_refs_include, 0);
 	string_list_clear(&decorate_refs_exclude, 0);
 	use_default_decoration_filter = 0;
-- 
2.42.0.561.gaa987ecc69
Jeff King· Aug 31, 2023, 21:22 UTC · re: Jeff King · lore

[PATCH v2 09/10] interpret-trailers: mark unused "unset" parameters in option callbacks

There are a few parse-option callbacks that do not look at their "unset" parameters, but also do not set PARSE_OPT_NONEG. At first glance this seems like a bug, as we'd ignore "--no-if-exists", etc.

But they do work fine, because when "unset" is true, then "arg" is NULL. And all three functions pass "arg" on to helper functions which do the right thing with the NULL.

Note that this shortcut would not be correct if any callback used PARSE_OPT_NOARG (in which case "arg" would be NULL but "unset" would be false). But none of these do.

So the code is fine as-is. But we'll want to mark the unused "unset" parameters to quiet -Wunused-parameter. I've also added a comment to make this rather subtle situation more explicit.

Signed-off-by: Jeff King <peff@peff.net>
---
I looked at another BUG_ON here, but it was just so ugly and one-off
that I preferred just adding the comments.
 builtin/interpret-trailers.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Show changes to builtin/interpret-trailers.c +6 −3
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 6aadce6a1e..a110e69f83 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -24,20 +24,23 @@ static enum trailer_if_exists if_exists;
 static enum trailer_if_missing if_missing;
 
 static int option_parse_where(const struct option *opt,
-			      const char *arg, int unset)
+			      const char *arg, int unset UNUSED)
 {
+	/* unset implies NULL arg, which is handled in our helper */
 	return trailer_set_where(opt->value, arg);
 }
 
 static int option_parse_if_exists(const struct option *opt,
-				  const char *arg, int unset)
+				  const char *arg, int unset UNUSED)
 {
+	/* unset implies NULL arg, which is handled in our helper */
 	return trailer_set_if_exists(opt->value, arg);
 }
 
 static int option_parse_if_missing(const struct option *opt,
-				   const char *arg, int unset)
+				   const char *arg, int unset UNUSED)
 {
+	/* unset implies NULL arg, which is handled in our helper */
 	return trailer_set_if_missing(opt->value, arg);
 }
 
-- 
2.42.0.561.gaa987ecc69
Jeff King· Aug 31, 2023, 21:22 UTC · re: Jeff King · lore

[PATCH v2 10/10] parse-options: mark unused parameters in noop callback

Unsurprisingly, the noop options callback doesn't bother to look at any of its parameters. Let's mark them so that -Wunused-parameter does not complain.

Another option would be to drop the callback and have parse-options itself recognize OPT_NOOP_NOARG. But that seems like extra work for no real benefit.

Signed-off-by: Jeff King <peff@peff.net>
---
 parse-options-cb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to parse-options-cb.c +3 −1
diff --git a/parse-options-cb.c b/parse-options-cb.c
index a24521dee0..bdc7fae497 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
+int parse_opt_noop_cb(const struct option *opt UNUSED,
+		      const char *arg UNUSED,
+		      int unset UNUSED)
 {
 	return 0;
 }
-- 
2.42.0.561.gaa987ecc69
René Scharfe· Sep 2, 2023, 11:37 UTC · re: Jeff King · lore

Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback

Am 31.08.23 um 23:22 schrieb Jeff King:
Show 7 quoted lines
> Unsurprisingly, the noop options callback doesn't bother to look at any
> of its parameters. Let's mark them so that -Wunused-parameter does not
> complain.
>
> Another option would be to drop the callback and have parse-options
> itself recognize OPT_NOOP_NOARG. But that seems like extra work for no
> real benefit.

I'm not sure about that -- we don't need to add special flags or option types to drop parse_opt_noop_cb().

Show 21 quoted lines
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  parse-options-cb.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index a24521dee0..bdc7fae497 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>
> -int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
> +int parse_opt_noop_cb(const struct option *opt UNUSED,
> +		      const char *arg UNUSED,
> +		      int unset UNUSED)
>  {
>  	return 0;
>  }

Your patch is simple and makes sense. The one below is more complicated, but leaves the code in a slightly better state. I'd go with that variant if I'd had to add OPT_NOOP_NOARG today, and I slightly prefer it, but similar to you think that the benefit is low (though non-zero). So I'm unsure if that's enough to be worth it or just bikeshedding with some hindsight..

René
--- >8 ---
Subject: [PATCH] parse-options: let NULL callback be a noop

Allow OPTION_CALLBACK options to have a NULL callback function pointer and do nothing in that case, yet still handle arguments as usual. Use this to replace parse_opt_noop_cb().

We lose the ability to distinguish between a forgotten function pointer and intentional noop, but that will be caught by a compiler warning about an unused function in many cases and having an option do nothing is a benign failure mode.

We also lose the ability to add a warning to the callback, but we haven't done that since it was added by 6acec0380b (parseopt: add OPT_NOOP_NOARG, 2011-09-28), so we probably won't do it soon. We can add a callback back when we want to show a warning.

By letting go of features we don't need this patch simplifies the parse-options API and gets rid of an exported empty function.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 parse-options-cb.c | 5 -----
 parse-options.c    | 7 +++----
 parse-options.h    | 2 --
 3 files changed, 3 insertions(+), 11 deletions(-)
Show changes to 3 files +3 −12

parse-options-cb.c, parse-options.c, parse-options.h

diff --git a/parse-options-cb.c b/parse-options-cb.c
index a24521dee0..e79cd54ec2 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -227,11 +227,6 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
 	return 0;
 }

-int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
-{
-	return 0;
-}
-
 /**
  * Recreates the command-line option in the strbuf.
  */
diff --git a/parse-options.c b/parse-options.c
index 76d2e76b49..5be8de93f5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -201,8 +201,9 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 		}
 		if (opt->callback)
 			return (*opt->callback)(opt, p_arg, p_unset) ? (-1) : 0;
-		else
+		if (opt->ll_callback)
 			return (*opt->ll_callback)(p, opt, p_arg, p_unset);
+		return 0;
 	}
 	case OPTION_INTEGER:
 		if (unset) {
@@ -494,9 +495,7 @@ static void parse_options_check(const struct option *opts)
 				optbug(opts, "should not accept an argument");
 			break;
 		case OPTION_CALLBACK:
-			if (!opts->callback && !opts->ll_callback)
-				optbug(opts, "OPTION_CALLBACK needs one callback");
-			else if (opts->callback && opts->ll_callback)
+			if (opts->callback && opts->ll_callback)
 				optbug(opts, "OPTION_CALLBACK can't have two callbacks");
 			break;
 		case OPTION_LOWLEVEL_CALLBACK:
diff --git a/parse-options.h b/parse-options.h
index 57a7fe9d91..41bb47120d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -348,7 +348,6 @@ struct option {
 	.long_name = (l), \
 	.help = N_("no-op (backward compatibility)"), \
 	.flags = PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, \
-	.callback = parse_opt_noop_cb, \
 }

 #define OPT_ALIAS(s, l, source_long_name) { \
@@ -490,7 +489,6 @@ int parse_opt_commit(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
 int parse_opt_string_list(const struct option *, const char *, int);
 int parse_opt_strvec(const struct option *, const char *, int);
-int parse_opt_noop_cb(const struct option *, const char *, int);
 int parse_opt_passthru(const struct option *, const char *, int);
 int parse_opt_passthru_argv(const struct option *, const char *, int);
 /* value is enum branch_track* */
--
2.42.0
Jeff King· Sep 5, 2023, 07:09 UTC · re: René Scharfe · lore

Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback

On Sat, Sep 02, 2023 at 01:37:08PM +0200, René Scharfe wrote:
Show 11 quoted lines
> --- >8 ---
> Subject: [PATCH] parse-options: let NULL callback be a noop
> 
> Allow OPTION_CALLBACK options to have a NULL callback function pointer
> and do nothing in that case, yet still handle arguments as usual.  Use
> this to replace parse_opt_noop_cb().
> 
> We lose the ability to distinguish between a forgotten function pointer
> and intentional noop, but that will be caught by a compiler warning
> about an unused function in many cases and having an option do nothing
> is a benign failure mode.

Yeah, my concern would be missing an accidental NULL here. I guess that is pretty unlikely in practice, though. I'd guess the worst case would be somebody accidentally putting the function into the "opt->value" slot where the compiler might then think it is used (though I don't know off-hand if it would complain about an implicit conversion of a function pointer into a "void *").

Show 7 quoted lines
> We also lose the ability to add a warning to the callback, but we
> haven't done that since it was added by 6acec0380b (parseopt: add
> OPT_NOOP_NOARG, 2011-09-28), so we probably won't do it soon.  We can
> add a callback back when we want to show a warning.
> 
> By letting go of features we don't need this patch simplifies the
> parse-options API and gets rid of an exported empty function.

Your patch looks correct to me. I probably wouldn't have bothered to write it, but now you did. :) My inclination would be to go with my dumb-simple one for this series, and it looks like you may have collected a few slight-more-risky-but-maybe-worthwhile parseopt cleanups on top that could make their own series.

-Peff
René Scharfe· Sep 7, 2023, 20:20 UTC · re: Jeff King · lore

Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback

Am 05.09.23 um 09:09 schrieb Jeff King:
>
> Your patch looks correct to me. I probably wouldn't have bothered to
> write it, but now you did. :) My inclination would be to go with my
> dumb-simple one for this series,
Fair enough.
> and it looks like you may have
> collected a few slight-more-risky-but-maybe-worthwhile parseopt cleanups
> on top that could make their own series.

Well, my aim is more to simplify and improve type safety, but I get carried away a bit at times.

René

← back to recent threads