| Message ID | 20230831212107.GE949469@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 66e3309294571ada0e45ea78a2cfb649f08b9dd4 |
| Headers | show |
| Series | more unused parameters in parseopt callbacks | expand |
Am 31.08.23 um 23:21 schrieb Jeff King: > 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. > 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. > 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. > 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 > 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,
On Sat, Sep 02, 2023 at 09:34:32AM +0200, René Scharfe wrote: > > 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. > > 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
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,
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(-)