git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
René Scharfe <l.s.r@web.de>
Date
Sep 2, 2023, 07:34 UTC
Message-ID
<8f03fa13-11b3-1f9f-1e1f-8f7d8ce74a23@web.de>
In-Reply-To
<20230831212107.GE949469@coredump.intra.peff.net>
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,
Previous: Jeff KingNext: Jeff King
Message 35 of 47 in “more unused parameters in parseopt callbacks”
  1. 0/8 more unused parameters in parseopt callbacksJeff King, Aug 31, 2023
  2. 1/8 merge: make xopts a strvecJeff King, Aug 31, 2023
  3. Jeff KingAug 31, 2023
  4. Phillip WoodAug 31, 2023
  5. Junio C HamanoAug 31, 2023
  6. Taylor BlauAug 31, 2023
  7. 2/8 merge: simplify parsing of "-n" optionJeff King, Aug 31, 2023
  8. Junio C HamanoAug 31, 2023
  9. 3/8 parse-options: prefer opt->value to globals in callbacksJeff King, Aug 31, 2023
  10. Junio C HamanoAug 31, 2023
  11. 4/8 parse-options: mark unused "opt" parameter in callbacksJeff King, Aug 31, 2023
  12. Junio C HamanoAug 31, 2023
  13. Jeff KingAug 31, 2023
  14. Jeff KingAug 31, 2023
  15. 5/8 merge: do not pass unused opt->value parameterJeff King, Aug 31, 2023
  16. Junio C HamanoAug 31, 2023
  17. 6/8 parse-options: add more BUG_ON() annotationsJeff King, Aug 31, 2023
  18. Junio C HamanoAug 31, 2023
  19. 7/8 interpret-trailers: mark unused "unset" parameters in option callbacksJeff King, Aug 31, 2023
  20. Junio C HamanoAug 31, 2023
  21. Jeff KingAug 31, 2023
  22. 8/8 parse-options: mark unused parameters in noop callbackJeff King, Aug 31, 2023
  23. Junio C HamanoAug 31, 2023
  24. 0/10 more unused parameters in parseopt callbacksJeff King, Aug 31, 2023
  25. 01/10 merge: make xopts a strvecJeff King, Aug 31, 2023
  26. 02/10 merge: simplify parsing of "-n" optionJeff King, Aug 31, 2023
  27. René ScharfeSep 2, 2023
  28. Jeff KingSep 5, 2023
  29. 03/10 format-patch: use OPT_STRING_LIST for to/cc optionsJeff King, Aug 31, 2023
  30. 04/10 checkout-index: delay automatic setting of to_tempfileJeff King, Aug 31, 2023
  31. Junio C HamanoAug 31, 2023
  32. René ScharfeSep 2, 2023
  33. 04/10 checkout-index: delay automatic setting of to_tempfileJeff King, Sep 5, 2023
  34. 05/10 parse-options: prefer opt->value to globals in callbacksJeff King, Aug 31, 2023
  35. René ScharfeSep 2, 2023
  36. Jeff KingSep 5, 2023
  37. 06/10 parse-options: mark unused "opt" parameter in callbacksJeff King, Aug 31, 2023
  38. René ScharfeSep 2, 2023
  39. Jeff KingSep 5, 2023
  40. René ScharfeSep 19, 2023
  41. 07/10 merge: do not pass unused opt->value parameterJeff King, Aug 31, 2023
  42. 08/10 parse-options: add more BUG_ON() annotationsJeff King, Aug 31, 2023
  43. 09/10 interpret-trailers: mark unused "unset" parameters in option callbacksJeff King, Aug 31, 2023
  44. 10/10 parse-options: mark unused parameters in noop callbackJeff King, Aug 31, 2023
  45. René ScharfeSep 2, 2023
  46. Jeff KingSep 5, 2023
  47. René ScharfeSep 7, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.