From dee02da8264bf7e47c8f2689663fac98d31ee450 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 Aug 2023 17:17:11 -0400 Subject: [PATCH 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 Signed-off-by: Junio C Hamano --- builtin/merge.c | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) 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); } } From 7fa701106d485f7bf9d042f822d4366d7059f8ba Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 Aug 2023 17:17:16 -0400 Subject: [PATCH 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 Signed-off-by: Junio C Hamano --- 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; -} - 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)")), From 3ccb4c55adc36793ec34f6f9f2235a695df7d0a5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 Aug 2023 17:17:33 -0400 Subject: [PATCH 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 Signed-off-by: Junio C Hamano --- builtin/log.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) 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 (or committer ident if absent)"), PARSE_OPT_OPTARG, from_callback), From 9b40386586aca7364f7518d5d7e7f89ba9e80d85 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2023 03:12:59 -0400 Subject: [PATCH 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 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 " 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/checkout-index.c | 9 +++++++-- t/t2004-checkout-cache-temp.sh | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) 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 && From 66e3309294571ada0e45ea78a2cfb649f08b9dd4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 Aug 2023 17:21:07 -0400 Subject: [PATCH 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 Signed-off-by: Junio C Hamano --- 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 526c210bcb..3b68b47615 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 ")), - 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 eed4a7cdb6..f6b42b3d58 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_("[,]"), + OPT_CALLBACK_F(0, "index-version", &pack_idx_opts, N_("[,]"), 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, From 34bf44f2d50e835a4824a07c139bdececccf4da1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 Aug 2023 17:21:28 -0400 Subject: [PATCH 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 Signed-off-by: Junio C Hamano --- 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 1f53b66c7b..5cfc178c7b 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,8 +118,8 @@ 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); @@ -127,7 +127,8 @@ static int clear_decorations_callback(const struct option *opt, 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; From 62c5358a5e728d332e4a991c87d9eb0d5161a02a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 Aug 2023 17:21:46 -0400 Subject: [PATCH 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 Signed-off-by: Junio C Hamano --- 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")), From abf2952f83f657ff40a731aa4b370350711029f4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 Aug 2023 17:21:49 -0400 Subject: [PATCH 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 Signed-off-by: Junio C Hamano --- 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 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; From d775365db321c9f05c12809438d3c801d7e445af Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 Aug 2023 17:22:15 -0400 Subject: [PATCH 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 Signed-off-by: Junio C Hamano --- builtin/interpret-trailers.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) 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); } From 0058b3d5eedcf5777712e872e01f74bf8d933be7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 Aug 2023 17:22:20 -0400 Subject: [PATCH 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 Signed-off-by: Junio C Hamano --- 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; }