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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Aug 31, 2023 at 17:21 UTC 66e3309294571ada0e45ea78a2cfb649f08b9dd4
6 files changed +50 -37
builtin/checkout-index.c
+5 -3
@@ -193,14 +193,16 @@ static const char * const builtin_checkout_index_usage[] = {
193 static int option_parse_stage(const struct option *opt,
194 const char *arg, int unset)
195 {
196 + int *stage = opt->value;
197 +
198 BUG_ON_OPT_NEG(unset);
199
200 if (!strcmp(arg, "all")) {
199 - checkout_stage = CHECKOUT_ALL;
201 + *stage = CHECKOUT_ALL;
202 } else {
203 int ch = arg[0];
204 if ('1' <= ch && ch <= '3')
203 - checkout_stage = arg[0] - '0';
205 + *stage = arg[0] - '0';
206 else
207 die(_("stage should be between 1 and 3 or all"));
208 }
@@ -238,7 +240,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
240 N_("write the content to temporary files")),
241 OPT_STRING(0, "prefix", &state.base_dir, N_("string"),
242 N_("when creating files, prepend <string>")),
241 - OPT_CALLBACK_F(0, "stage", NULL, "(1|2|3|all)",
243 + OPT_CALLBACK_F(0, "stage", &checkout_stage, "(1|2|3|all)",
244 N_("copy out the files from named stage"),
245 PARSE_OPT_NONEG, option_parse_stage),
246 OPT_END()
builtin/describe.c
+4 -2
@@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)
561 static int option_parse_exact_match(const struct option *opt, const char *arg,
562 int unset)
563 {
564 + int *val = opt->value;
565 +
566 BUG_ON_OPT_ARG(arg);
567
566 - max_candidates = unset ? DEFAULT_CANDIDATES : 0;
568 + *val = unset ? DEFAULT_CANDIDATES : 0;
569 return 0;
570 }
571
@@ -578,7 +580,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
580 OPT_BOOL(0, "long", &longformat, N_("always use long format")),
581 OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
582 OPT__ABBREV(&abbrev),
581 - OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
583 + OPT_CALLBACK_F(0, "exact-match", &max_candidates, NULL,
584 N_("only output exact matches"),
585 PARSE_OPT_NOARG, option_parse_exact_match),
586 OPT_INTEGER(0, "candidates", &max_candidates,
builtin/fast-export.c
+21 -15
@@ -33,9 +33,9 @@ static const char *fast_export_usage[] = {
33 };
34
35 static int progress;
36 -static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
37 -static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
38 -static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
36 +static enum signed_tag_mode { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
37 +static enum tag_of_filtered_mode { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
38 +static enum reencode_mode { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
39 static int fake_missing_tagger;
40 static int use_done_feature;
41 static int no_data;
@@ -53,16 +53,18 @@ static struct revision_sources revision_sources;
53 static int parse_opt_signed_tag_mode(const struct option *opt,
54 const char *arg, int unset)
55 {
56 + enum signed_tag_mode *val = opt->value;
57 +
58 if (unset || !strcmp(arg, "abort"))
57 - signed_tag_mode = SIGNED_TAG_ABORT;
59 + *val = SIGNED_TAG_ABORT;
60 else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
59 - signed_tag_mode = VERBATIM;
61 + *val = VERBATIM;
62 else if (!strcmp(arg, "warn"))
61 - signed_tag_mode = WARN;
63 + *val = WARN;
64 else if (!strcmp(arg, "warn-strip"))
63 - signed_tag_mode = WARN_STRIP;
65 + *val = WARN_STRIP;
66 else if (!strcmp(arg, "strip"))
65 - signed_tag_mode = STRIP;
67 + *val = STRIP;
68 else
69 return error("Unknown signed-tags mode: %s", arg);
70 return 0;
@@ -71,12 +73,14 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
73 static int parse_opt_tag_of_filtered_mode(const struct option *opt,
74 const char *arg, int unset)
75 {
76 + enum tag_of_filtered_mode *val = opt->value;
77 +
78 if (unset || !strcmp(arg, "abort"))
75 - tag_of_filtered_mode = TAG_FILTERING_ABORT;
79 + *val = TAG_FILTERING_ABORT;
80 else if (!strcmp(arg, "drop"))
77 - tag_of_filtered_mode = DROP;
81 + *val = DROP;
82 else if (!strcmp(arg, "rewrite"))
79 - tag_of_filtered_mode = REWRITE;
83 + *val = REWRITE;
84 else
85 return error("Unknown tag-of-filtered mode: %s", arg);
86 return 0;
@@ -85,21 +89,23 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,
89 static int parse_opt_reencode_mode(const struct option *opt,
90 const char *arg, int unset)
91 {
92 + enum reencode_mode *val = opt->value;
93 +
94 if (unset) {
89 - reencode_mode = REENCODE_ABORT;
95 + *val = REENCODE_ABORT;
96 return 0;
97 }
98
99 switch (git_parse_maybe_bool(arg)) {
100 case 0:
95 - reencode_mode = REENCODE_NO;
101 + *val = REENCODE_NO;
102 break;
103 case 1:
98 - reencode_mode = REENCODE_YES;
104 + *val = REENCODE_YES;
105 break;
106 default:
107 if (!strcasecmp(arg, "abort"))
102 - reencode_mode = REENCODE_ABORT;
108 + *val = REENCODE_ABORT;
109 else
110 return error("Unknown reencoding mode: %s", arg);
111 }
builtin/fetch.c
+2 -2
@@ -176,7 +176,7 @@ static int parse_refmap_arg(const struct option *opt, const char *arg, int unset
176 * "git fetch --refmap='' origin foo"
177 * can be used to tell the command not to store anywhere
178 */
179 - refspec_append(&refmap, arg);
179 + refspec_append(opt->value, arg);
180
181 return 0;
182 }
@@ -2204,7 +2204,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
2204 PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules),
2205 OPT_BOOL(0, "update-shallow", &update_shallow,
2206 N_("accept refs that update .git/shallow")),
2207 - OPT_CALLBACK_F(0, "refmap", NULL, N_("refmap"),
2207 + OPT_CALLBACK_F(0, "refmap", &refmap, N_("refmap"),
2208 N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
2209 OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
2210 OPT_IPVERSION(&family),
builtin/interpret-trailers.c
+6 -6
@@ -26,19 +26,19 @@ static enum trailer_if_missing if_missing;
26 static int option_parse_where(const struct option *opt,
27 const char *arg, int unset)
28 {
29 - return trailer_set_where(&where, arg);
29 + return trailer_set_where(opt->value, arg);
30 }
31
32 static int option_parse_if_exists(const struct option *opt,
33 const char *arg, int unset)
34 {
35 - return trailer_set_if_exists(&if_exists, arg);
35 + return trailer_set_if_exists(opt->value, arg);
36 }
37
38 static int option_parse_if_missing(const struct option *opt,
39 const char *arg, int unset)
40 {
41 - return trailer_set_if_missing(&if_missing, arg);
41 + return trailer_set_if_missing(opt->value, arg);
42 }
43
44 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)
97 OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
98 OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
99
100 - OPT_CALLBACK(0, "where", NULL, N_("action"),
100 + OPT_CALLBACK(0, "where", &where, N_("action"),
101 N_("where to place the new trailer"), option_parse_where),
102 - OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
102 + OPT_CALLBACK(0, "if-exists", &if_exists, N_("action"),
103 N_("action if trailer already exists"), option_parse_if_exists),
104 - OPT_CALLBACK(0, "if-missing", NULL, N_("action"),
104 + OPT_CALLBACK(0, "if-missing", &if_missing, N_("action"),
105 N_("action if trailer is missing"), option_parse_if_missing),
106
107 OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
builtin/pack-objects.c
+12 -9
@@ -4120,29 +4120,32 @@ static void add_extra_kept_packs(const struct string_list *names)
4120 static int option_parse_quiet(const struct option *opt, const char *arg,
4121 int unset)
4122 {
4123 + int *val = opt->value;
4124 +
4125 BUG_ON_OPT_ARG(arg);
4126
4127 if (!unset)
4126 - progress = 0;
4127 - else if (!progress)
4128 - progress = 1;
4128 + *val = 0;
4129 + else if (!*val)
4130 + *val = 1;
4131 return 0;
4132 }
4133
4134 static int option_parse_index_version(const struct option *opt,
4135 const char *arg, int unset)
4136 {
4137 + struct pack_idx_option *popts = opt->value;
4138 char *c;
4139 const char *val = arg;
4140
4141 BUG_ON_OPT_NEG(unset);
4142
4140 - pack_idx_opts.version = strtoul(val, &c, 10);
4141 - if (pack_idx_opts.version > 2)
4143 + popts->version = strtoul(val, &c, 10);
4144 + if (popts->version > 2)
4145 die(_("unsupported index version %s"), val);
4146 if (*c == ',' && c[1])
4144 - pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);
4145 - if (*c || pack_idx_opts.off32_limit & 0x80000000)
4147 + popts->off32_limit = strtoul(c+1, &c, 0);
4148 + if (*c || popts->off32_limit & 0x80000000)
4149 die(_("bad index version '%s'"), val);
4150 return 0;
4151 }
@@ -4190,7 +4193,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
4193 LIST_OBJECTS_FILTER_INIT;
4194
4195 struct option pack_objects_options[] = {
4193 - OPT_CALLBACK_F('q', "quiet", NULL, NULL,
4196 + OPT_CALLBACK_F('q', "quiet", &progress, NULL,
4197 N_("do not show progress meter"),
4198 PARSE_OPT_NOARG, option_parse_quiet),
4199 OPT_SET_INT(0, "progress", &progress,
@@ -4200,7 +4203,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
4203 OPT_BOOL(0, "all-progress-implied",
4204 &all_progress_implied,
4205 N_("similar to --all-progress when progress meter is shown")),
4203 - OPT_CALLBACK_F(0, "index-version", NULL, N_("<version>[,<offset>]"),
4206 + OPT_CALLBACK_F(0, "index-version", &pack_idx_opts, N_("<version>[,<offset>]"),
4207 N_("write the pack index file in the specified idx format version"),
4208 PARSE_OPT_NONEG, option_parse_index_version),
4209 OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit,