{"thread":{"id":"60176","subject":"[PATCH 0/8] more unused parameters in parseopt callbacks","startedAt":"2023-08-31T07:09:40Z","lastAt":"2023-09-19T07:42:23Z","messageCount":47,"participants":["Jeff King","Phillip Wood","Junio C Hamano","Taylor Blau","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"481220","messageId":"20230831070935.GA3197495@coredump.intra.peff.net","threadId":"60176","inReplyTo":null,"subject":"[PATCH 0/8] more unused parameters in parseopt callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T07:09:35Z","receivedAt":"2023-08-31T07:09:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are some more patches silencing -Wunused-parameter warnings. I've\nprepared them on top of the patches queued in jk/unused-post-2.42, but\nthey should apply equally well directly on 'master'.\n\nThese ones are a bit more interesting than usual, because there were\nsome actual code changes and cleanups that seemed preferable to just\nannotating.\n\nI'm adding René to the cc based on our earlier discussion of callbacks\nwhich ignore opt->value. See patch 3 in particular. Also some of your\nrecent work gets a nice shout-out in patch 2. :)\n\n  [1/8]: merge: make xopts a strvec\n  [2/8]: merge: simplify parsing of \"-n\" option\n  [3/8]: parse-options: prefer opt->value to globals in callbacks\n  [4/8]: parse-options: mark unused \"opt\" parameter in callbacks\n  [5/8]: merge: do not pass unused opt->value parameter\n  [6/8]: parse-options: add more BUG_ON() annotations\n  [7/8]: interpret-trailers: mark unused \"unset\" parameters in option callbacks\n  [8/8]: parse-options: mark unused parameters in noop callback\n\n builtin/add.c                |  2 ++\n builtin/checkout-index.c     |  2 +-\n builtin/describe.c           |  6 +++--\n builtin/fast-export.c        | 36 +++++++++++++++++-------------\n builtin/fetch.c              |  3 ++-\n builtin/gc.c                 |  2 +-\n builtin/interpret-trailers.c | 18 +++++++--------\n builtin/log.c                | 18 ++++++++++-----\n builtin/merge.c              | 43 +++++++++---------------------------\n builtin/pack-objects.c       | 27 ++++++++++++----------\n builtin/read-tree.c          |  2 +-\n builtin/update-index.c       |  4 ++--\n parse-options-cb.c           |  4 +++-\n 13 files changed, 84 insertions(+), 83 deletions(-)\n\n"},{"id":"481221","messageId":"20230831071230.GA3197647@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831070935.GA3197495@coredump.intra.peff.net","subject":"[PATCH 1/8] merge: make xopts a strvec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T07:12:30Z","receivedAt":"2023-08-31T07:12:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"xopts\" variable uses a custom array with ALLOC_GROW(). Using a\nstrvec simplifies things a bit. We need fewer variables, and we can also\nditch our custom parseopt callback in favor of OPT_STRVEC().\n\nAs a bonus, this means that \"--no-strategy-option\", which was previously\na silent noop, now does something useful: like other list-like options,\nit will clear the list of -X options seen so far.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI guess you could argue this is a backwards-incompatible change, but the\nexisting behavior of --no-strategy-option is so dumb that I can't\nbelieve somebody would prefer it (plus revert/cherry-pick already use\nOPT_STRVEC for their matching \"-X\").\n\nI didn't bother adding a test since we're just re-using OPT_STRVEC code\nthat is used elsewhere.\n\n builtin/merge.c | 26 +++++++-------------------\n 1 file changed, 7 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex de68910177..53e9fe70d9 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -79,8 +79,7 @@ static int overwrite_ignore = 1;\n static struct strbuf merge_msg = STRBUF_INIT;\n static struct strategy **use_strategies;\n static size_t use_strategies_nr, use_strategies_alloc;\n-static const char **xopts;\n-static size_t xopts_nr, xopts_alloc;\n+static struct strvec xopts = STRVEC_INIT;\n static const char *branch;\n static char *branch_mergeoptions;\n static int verbosity;\n@@ -242,17 +241,6 @@ static int option_parse_strategy(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_x(const struct option *opt,\n-\t\t\t  const char *arg, int unset)\n-{\n-\tif (unset)\n-\t\treturn 0;\n-\n-\tALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);\n-\txopts[xopts_nr++] = xstrdup(arg);\n-\treturn 0;\n-}\n-\n static int option_parse_n(const struct option *opt,\n \t\t\t  const char *arg, int unset)\n {\n@@ -287,8 +275,8 @@ static struct option builtin_merge_options[] = {\n \t\tN_(\"verify that the named commit has a valid GPG signature\")),\n \tOPT_CALLBACK('s', \"strategy\", &use_strategies, N_(\"strategy\"),\n \t\tN_(\"merge strategy to use\"), option_parse_strategy),\n-\tOPT_CALLBACK('X', \"strategy-option\", &xopts, N_(\"option=value\"),\n-\t\tN_(\"option for selected merge strategy\"), option_parse_x),\n+\tOPT_STRVEC('X', \"strategy-option\", &xopts, N_(\"option=value\"),\n+\t\tN_(\"option for selected merge strategy\")),\n \tOPT_CALLBACK('m', \"message\", &merge_msg, N_(\"message\"),\n \t\tN_(\"merge commit message (for a non-fast-forward merge)\"),\n \t\toption_parse_message),\n@@ -749,9 +737,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\to.show_rename_progress =\n \t\t\tshow_progress == -1 ? isatty(2) : show_progress;\n \n-\t\tfor (x = 0; x < xopts_nr; x++)\n-\t\t\tif (parse_merge_opt(&o, xopts[x]))\n-\t\t\t\tdie(_(\"unknown strategy option: -X%s\"), xopts[x]);\n+\t\tfor (x = 0; x < xopts.nr; x++)\n+\t\t\tif (parse_merge_opt(&o, xopts.v[x]))\n+\t\t\t\tdie(_(\"unknown strategy option: -X%s\"), xopts.v[x]);\n \n \t\to.branch1 = head_arg;\n \t\to.branch2 = merge_remote_util(remoteheads->item)->name;\n@@ -777,7 +765,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\treturn clean ? 0 : 1;\n \t} else {\n \t\treturn try_merge_command(the_repository,\n-\t\t\t\t\t strategy, xopts_nr, xopts,\n+\t\t\t\t\t strategy, xopts.nr, xopts.v,\n \t\t\t\t\t common, head_arg, remoteheads);\n \t}\n }\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481222","messageId":"20230831071401.GB3197647@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831070935.GA3197495@coredump.intra.peff.net","subject":"[PATCH 2/8] merge: simplify parsing of \"-n\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T07:14:01Z","receivedAt":"2023-08-31T07:14:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"-n\" option is implemented by an option callback, as it is really a\n\"reverse bool\". If given, it sets show_diffstat to 0. In theory, when\nnegated, it would set the same flag to 1. But it's not possible to\ntrigger that, since short options cannot be negated.\n\nSo in practice this is really just a SET_INT to 0. Let's use that\ninstead, which shortens the code.\n\nNote that negation here would do the wrong thing (as with any SET_INT\nwith a value of \"0\"). We could specify PARSE_OPT_NONEG to future-proof\nourselves against somebody adding a long option name (which would make\nit possible to negate). But there's not much point:\n\n  1. Nobody is going to do that, because the negated form already\n     exists, and is called \"--stat\" (which is defined separately so that\n     \"--no-stat\" works).\n\n  2. If they did, the BUG() check added by 3284b93862 (parse-options:\n     disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and\n     that check is smart enough to realize that our short-only option is\n     OK).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/merge.c | 13 ++-----------\n 1 file changed, 2 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 53e9fe70d9..21363b7985 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -241,18 +241,9 @@ static int option_parse_strategy(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_n(const struct option *opt,\n-\t\t\t  const char *arg, int unset)\n-{\n-\tBUG_ON_OPT_ARG(arg);\n-\tshow_diffstat = unset;\n-\treturn 0;\n-}\n-\n static struct option builtin_merge_options[] = {\n-\tOPT_CALLBACK_F('n', NULL, NULL, NULL,\n-\t\tN_(\"do not show a diffstat at the end of the merge\"),\n-\t\tPARSE_OPT_NOARG, option_parse_n),\n+\tOPT_SET_INT('n', NULL, &show_diffstat,\n+\t\tN_(\"do not show a diffstat at the end of the merge\"), 0),\n \tOPT_BOOL(0, \"stat\", &show_diffstat,\n \t\tN_(\"show a diffstat at the end of the merge\")),\n \tOPT_BOOL(0, \"summary\", &show_diffstat, N_(\"(synonym to --stat)\")),\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481223","messageId":"20230831071743.GA3197751@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831070935.GA3197495@coredump.intra.peff.net","subject":"[PATCH 3/8] parse-options: prefer opt->value to globals in callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T07:17:43Z","receivedAt":"2023-08-31T07:17:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have several parse-options callbacks that ignore their \"opt\"\nparameters entirely. This is a little unusual, as we'd normally put the\nresult of the parsing into opt->value. In the case of these callbacks,\nthough, they directly manipulate global variables instead (and in\nmost cases the caller sets opt->value to NULL in the OPT_CALLBACK\ndeclaration).\n\nThe immediate symptom we'd like to deal with is that the unused \"opt\"\nvariables trigger -Wunused-parameter. But how to fix that is debatable.\nOne option is to annotate them with UNUSED. But another is to have the\ncaller pass in the appropriate variable via opt->value, and use it. That\nhas the benefit of making the callbacks reusable (in theory at least),\nand makes it clear from the OPT_CALLBACK declaration which variables\nwill be affected (doubly so for the cases in builtin/fast-export.c,\nwhere we do set opt->value, but it is completely ignored!).\n\nThe slight downside is that we lose type safety, since they're now\npassing through void pointers.\n\nI went with the \"just use them\" approach here. The loss of type safety\nis unfortunate, but that is already an issue with most of the other\ncallbacks. If we want to try to address that, we should do so more\nconsistently (and this patch would prepare these callbacks for whatever\nwe choose to do there).\n\nNote that in the cases in builtin/fast-export.c, we are passing\nanonymous enums. We'll have to give them names so that we can declare\nthe appropriate pointer type within the callbacks.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI was tempted to push some of these globals into the cmd_foo()\nfunctions, to make sure we don't make the same mistake again. But there\nare lots of ripple effects from other functions which want to access the\nglobals (and passing them makes the code longer for little benefit).\nPlus it would make things inconsistent, with some options globals and\nsome not. So that can be a cleanup for another day if somebody is\ninterested (and this takes us one tiny step closer).\n\n builtin/describe.c           |  6 ++++--\n builtin/fast-export.c        | 36 +++++++++++++++++++++---------------\n builtin/interpret-trailers.c | 12 ++++++------\n builtin/pack-objects.c       | 21 ++++++++++++---------\n 4 files changed, 43 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex b28a4a1f82..718b5c3073 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)\n static int option_parse_exact_match(const struct option *opt, const char *arg,\n \t\t\t\t    int unset)\n {\n+\tint *val = opt->value;\n+\n \tBUG_ON_OPT_ARG(arg);\n \n-\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n+\t*val = unset ? DEFAULT_CANDIDATES : 0;\n \treturn 0;\n }\n \n@@ -578,7 +580,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"long\",       &longformat, N_(\"always use long format\")),\n \t\tOPT_BOOL(0, \"first-parent\", &first_parent, N_(\"only follow first parent\")),\n \t\tOPT__ABBREV(&abbrev),\n-\t\tOPT_CALLBACK_F(0, \"exact-match\", NULL, NULL,\n+\t\tOPT_CALLBACK_F(0, \"exact-match\", &max_candidates, NULL,\n \t\t\t       N_(\"only output exact matches\"),\n \t\t\t       PARSE_OPT_NOARG, option_parse_exact_match),\n \t\tOPT_INTEGER(0, \"candidates\", &max_candidates,\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 56dc69fac1..70aff515ac 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -33,9 +33,9 @@ static const char *fast_export_usage[] = {\n };\n \n static int progress;\n-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;\n-static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;\n-static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;\n+static enum signed_tag_mode { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;\n+static enum tag_of_filtered_mode { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;\n+static enum reencode_mode { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;\n static int fake_missing_tagger;\n static int use_done_feature;\n static int no_data;\n@@ -53,16 +53,18 @@ static struct revision_sources revision_sources;\n static int parse_opt_signed_tag_mode(const struct option *opt,\n \t\t\t\t     const char *arg, int unset)\n {\n+\tenum signed_tag_mode *val = opt->value;\n+\n \tif (unset || !strcmp(arg, \"abort\"))\n-\t\tsigned_tag_mode = SIGNED_TAG_ABORT;\n+\t\t*val = SIGNED_TAG_ABORT;\n \telse if (!strcmp(arg, \"verbatim\") || !strcmp(arg, \"ignore\"))\n-\t\tsigned_tag_mode = VERBATIM;\n+\t\t*val = VERBATIM;\n \telse if (!strcmp(arg, \"warn\"))\n-\t\tsigned_tag_mode = WARN;\n+\t\t*val = WARN;\n \telse if (!strcmp(arg, \"warn-strip\"))\n-\t\tsigned_tag_mode = WARN_STRIP;\n+\t\t*val = WARN_STRIP;\n \telse if (!strcmp(arg, \"strip\"))\n-\t\tsigned_tag_mode = STRIP;\n+\t\t*val = STRIP;\n \telse\n \t\treturn error(\"Unknown signed-tags mode: %s\", arg);\n \treturn 0;\n@@ -71,12 +73,14 @@ static int parse_opt_signed_tag_mode(const struct option *opt,\n static int parse_opt_tag_of_filtered_mode(const struct option *opt,\n \t\t\t\t\t  const char *arg, int unset)\n {\n+\tenum tag_of_filtered_mode *val = opt->value;\n+\n \tif (unset || !strcmp(arg, \"abort\"))\n-\t\ttag_of_filtered_mode = TAG_FILTERING_ABORT;\n+\t\t*val = TAG_FILTERING_ABORT;\n \telse if (!strcmp(arg, \"drop\"))\n-\t\ttag_of_filtered_mode = DROP;\n+\t\t*val = DROP;\n \telse if (!strcmp(arg, \"rewrite\"))\n-\t\ttag_of_filtered_mode = REWRITE;\n+\t\t*val = REWRITE;\n \telse\n \t\treturn error(\"Unknown tag-of-filtered mode: %s\", arg);\n \treturn 0;\n@@ -85,21 +89,23 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,\n static int parse_opt_reencode_mode(const struct option *opt,\n \t\t\t\t   const char *arg, int unset)\n {\n+\tenum reencode_mode *val = opt->value;\n+\n \tif (unset) {\n-\t\treencode_mode = REENCODE_ABORT;\n+\t\t*val = REENCODE_ABORT;\n \t\treturn 0;\n \t}\n \n \tswitch (git_parse_maybe_bool(arg)) {\n \tcase 0:\n-\t\treencode_mode = REENCODE_NO;\n+\t\t*val = REENCODE_NO;\n \t\tbreak;\n \tcase 1:\n-\t\treencode_mode = REENCODE_YES;\n+\t\t*val = REENCODE_YES;\n \t\tbreak;\n \tdefault:\n \t\tif (!strcasecmp(arg, \"abort\"))\n-\t\t\treencode_mode = REENCODE_ABORT;\n+\t\t\t*val = REENCODE_ABORT;\n \t\telse\n \t\t\treturn error(\"Unknown reencoding mode: %s\", arg);\n \t}\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex c5e8345265..6aadce6a1e 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -26,19 +26,19 @@ static enum trailer_if_missing if_missing;\n static int option_parse_where(const struct option *opt,\n \t\t\t      const char *arg, int unset)\n {\n-\treturn trailer_set_where(&where, arg);\n+\treturn trailer_set_where(opt->value, arg);\n }\n \n static int option_parse_if_exists(const struct option *opt,\n \t\t\t\t  const char *arg, int unset)\n {\n-\treturn trailer_set_if_exists(&if_exists, arg);\n+\treturn trailer_set_if_exists(opt->value, arg);\n }\n \n static int option_parse_if_missing(const struct option *opt,\n \t\t\t\t   const char *arg, int unset)\n {\n-\treturn trailer_set_if_missing(&if_missing, arg);\n+\treturn trailer_set_if_missing(opt->value, arg);\n }\n \n static void new_trailers_clear(struct list_head *trailers)\n@@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"in-place\", &opts.in_place, N_(\"edit files in place\")),\n \t\tOPT_BOOL(0, \"trim-empty\", &opts.trim_empty, N_(\"trim empty trailers\")),\n \n-\t\tOPT_CALLBACK(0, \"where\", NULL, N_(\"action\"),\n+\t\tOPT_CALLBACK(0, \"where\", &where, N_(\"action\"),\n \t\t\t     N_(\"where to place the new trailer\"), option_parse_where),\n-\t\tOPT_CALLBACK(0, \"if-exists\", NULL, N_(\"action\"),\n+\t\tOPT_CALLBACK(0, \"if-exists\", &if_exists, N_(\"action\"),\n \t\t\t     N_(\"action if trailer already exists\"), option_parse_if_exists),\n-\t\tOPT_CALLBACK(0, \"if-missing\", NULL, N_(\"action\"),\n+\t\tOPT_CALLBACK(0, \"if-missing\", &if_missing, N_(\"action\"),\n \t\t\t     N_(\"action if trailer is missing\"), option_parse_if_missing),\n \n \t\tOPT_BOOL(0, \"only-trailers\", &opts.only_trailers, N_(\"output only the trailers\")),\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d2a162d528..492372ee5d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4120,29 +4120,32 @@ static void add_extra_kept_packs(const struct string_list *names)\n static int option_parse_quiet(const struct option *opt, const char *arg,\n \t\t\t      int unset)\n {\n+\tint *val = opt->value;\n+\n \tBUG_ON_OPT_ARG(arg);\n \n \tif (!unset)\n-\t\tprogress = 0;\n-\telse if (!progress)\n-\t\tprogress = 1;\n+\t\t*val = 0;\n+\telse if (!*val)\n+\t\t*val = 1;\n \treturn 0;\n }\n \n static int option_parse_index_version(const struct option *opt,\n \t\t\t\t      const char *arg, int unset)\n {\n+\tstruct pack_idx_option *popts = opt->value;\n \tchar *c;\n \tconst char *val = arg;\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tpack_idx_opts.version = strtoul(val, &c, 10);\n-\tif (pack_idx_opts.version > 2)\n+\tpopts->version = strtoul(val, &c, 10);\n+\tif (popts->version > 2)\n \t\tdie(_(\"unsupported index version %s\"), val);\n \tif (*c == ',' && c[1])\n-\t\tpack_idx_opts.off32_limit = strtoul(c+1, &c, 0);\n-\tif (*c || pack_idx_opts.off32_limit & 0x80000000)\n+\t\tpopts->off32_limit = strtoul(c+1, &c, 0);\n+\tif (*c || popts->off32_limit & 0x80000000)\n \t\tdie(_(\"bad index version '%s'\"), val);\n \treturn 0;\n }\n@@ -4190,7 +4193,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tLIST_OBJECTS_FILTER_INIT;\n \n \tstruct option pack_objects_options[] = {\n-\t\tOPT_CALLBACK_F('q', \"quiet\", NULL, NULL,\n+\t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n \t\t\t       N_(\"do not show progress meter\"),\n \t\t\t       PARSE_OPT_NOARG, option_parse_quiet),\n \t\tOPT_SET_INT(0, \"progress\", &progress,\n@@ -4200,7 +4203,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"all-progress-implied\",\n \t\t\t &all_progress_implied,\n \t\t\t N_(\"similar to --all-progress when progress meter is shown\")),\n-\t\tOPT_CALLBACK_F(0, \"index-version\", NULL, N_(\"<version>[,<offset>]\"),\n+\t\tOPT_CALLBACK_F(0, \"index-version\", &pack_idx_opts, N_(\"<version>[,<offset>]\"),\n \t\t  N_(\"write the pack index file in the specified idx format version\"),\n \t\t  PARSE_OPT_NONEG, option_parse_index_version),\n \t\tOPT_MAGNITUDE(0, \"max-pack-size\", &pack_size_limit,\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481224","messageId":"20230831071820.GB3197751@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831070935.GA3197495@coredump.intra.peff.net","subject":"[PATCH 4/8] parse-options: mark unused \"opt\" parameter in callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T07:18:20Z","receivedAt":"2023-08-31T07:18:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit argued that parse-options callbacks should try to\nuse opt->value rather than touching globals directly. In some cases,\nhowever, that's awkward to do. Some callbacks touch multiple variables,\nor may even just call into an abstracted function that does so.\n\nIn some of these cases we _could_ convert them by stuffing the multiple\nvariables into a single struct and passing the struct pointer through\nopt->value. But that may make other parts of the code less readable,\nas the struct relationship has to be mentioned everywhere.\n\nLet's just accept that these cases are special and leave them as-is. But\nwe do need to mark their \"opt\" parameters to satisfy -Wunused-parameter.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout-index.c |  2 +-\n builtin/fetch.c          |  3 ++-\n builtin/gc.c             |  2 +-\n builtin/log.c            | 16 ++++++++++------\n builtin/merge.c          |  2 +-\n builtin/pack-objects.c   |  6 +++---\n builtin/read-tree.c      |  2 +-\n builtin/update-index.c   |  4 ++--\n 8 files changed, 21 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex f62f13f2b5..95b3717dd1 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -190,7 +190,7 @@ static const char * const builtin_checkout_index_usage[] = {\n \tNULL\n };\n \n-static int option_parse_stage(const struct option *opt,\n+static int option_parse_stage(const struct option *opt UNUSED,\n \t\t\t      const char *arg, int unset)\n {\n \tBUG_ON_OPT_NEG(unset);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 8f93529505..cd2afe33e5 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -168,7 +168,8 @@ static int git_fetch_config(const char *k, const char *v,\n \treturn git_default_config(k, v, ctx, cb);\n }\n \n-static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n+static int parse_refmap_arg(const struct option *opt UNUSED,\n+\t\t\t    const char *arg, int unset)\n {\n \tBUG_ON_OPT_NEG(unset);\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 369bd43fb2..b842349d86 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)\n \tstrbuf_release(&config_name);\n }\n \n-static int task_option_parse(const struct option *opt,\n+static int task_option_parse(const struct option *opt UNUSED,\n \t\t\t     const char *arg, int unset)\n {\n \tint i, num_selected = 0;\ndiff --git a/builtin/log.c b/builtin/log.c\nindex db3a88bfe9..87e29c4171 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -118,16 +118,17 @@ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;\n static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;\n static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;\n \n-static int clear_decorations_callback(const struct option *opt,\n-\t\t\t\t\t    const char *arg, int unset)\n+static int clear_decorations_callback(const struct option *opt UNUSED,\n+\t\t\t\t      const char *arg, int unset)\n {\n \tstring_list_clear(&decorate_refs_include, 0);\n \tstring_list_clear(&decorate_refs_exclude, 0);\n \tuse_default_decoration_filter = 0;\n \treturn 0;\n }\n \n-static int decorate_callback(const struct option *opt, const char *arg, int unset)\n+static int decorate_callback(const struct option *opt UNUSED, const char *arg,\n+\t\t\t     int unset)\n {\n \tif (unset)\n \t\tdecoration_style = 0;\n@@ -1555,7 +1556,8 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n-static int header_callback(const struct option *opt, const char *arg, int unset)\n+static int header_callback(const struct option *opt UNUSED, const char *arg,\n+\t\t\t   int unset)\n {\n \tif (unset) {\n \t\tstring_list_clear(&extra_hdr, 0);\n@@ -1567,7 +1569,8 @@ static int header_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n-static int to_callback(const struct option *opt, const char *arg, int unset)\n+static int to_callback(const struct option *opt UNUSED, const char *arg,\n+\t\t       int unset)\n {\n \tif (unset)\n \t\tstring_list_clear(&extra_to, 0);\n@@ -1576,7 +1579,8 @@ static int to_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n-static int cc_callback(const struct option *opt, const char *arg, int unset)\n+static int cc_callback(const struct option *opt UNUSED, const char *arg,\n+\t\t       int unset)\n {\n \tif (unset)\n \t\tstring_list_clear(&extra_cc, 0);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 21363b7985..0436986dab 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -231,7 +231,7 @@ static void append_strategy(struct strategy *s)\n \tuse_strategies[use_strategies_nr++] = s;\n }\n \n-static int option_parse_strategy(const struct option *opt,\n+static int option_parse_strategy(const struct option *opt UNUSED,\n \t\t\t\t const char *name, int unset)\n {\n \tif (unset)\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 492372ee5d..91b4b7c177 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3739,7 +3739,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,\n \tshow_object(obj, name, data);\n }\n \n-static int option_parse_missing_action(const struct option *opt,\n+static int option_parse_missing_action(const struct option *opt UNUSED,\n \t\t\t\t       const char *arg, int unset)\n {\n \tassert(arg);\n@@ -4150,7 +4150,7 @@ static int option_parse_index_version(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_unpack_unreachable(const struct option *opt,\n+static int option_parse_unpack_unreachable(const struct option *opt UNUSED,\n \t\t\t\t\t   const char *arg, int unset)\n {\n \tif (unset) {\n@@ -4165,7 +4165,7 @@ static int option_parse_unpack_unreachable(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_cruft_expiration(const struct option *opt,\n+static int option_parse_cruft_expiration(const struct option *opt UNUSED,\n \t\t\t\t\t const char *arg, int unset)\n {\n \tif (unset) {\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 1fec702a04..8196ca9dd8 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -49,7 +49,7 @@ static const char * const read_tree_usage[] = {\n \tNULL\n };\n \n-static int index_output_cb(const struct option *opt, const char *arg,\n+static int index_output_cb(const struct option *opt UNUSED, const char *arg,\n \t\t\t\t int unset)\n {\n \tBUG_ON_OPT_NEG(unset);\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex aee3cb8cbd..59acae3336 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,\n \treturn 0;\n }\n \n-static int resolve_undo_clear_callback(const struct option *opt,\n+static int resolve_undo_clear_callback(const struct option *opt UNUSED,\n \t\t\t\tconst char *arg, int unset)\n {\n \tBUG_ON_OPT_NEG(unset);\n@@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,\n }\n \n static enum parse_opt_result cacheinfo_callback(\n-\tstruct parse_opt_ctx_t *ctx, const struct option *opt,\n+\tstruct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,\n \tconst char *arg, int unset)\n {\n \tstruct object_id oid;\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481225","messageId":"20230831071845.GC3197751@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831070935.GA3197495@coredump.intra.peff.net","subject":"[PATCH 5/8] merge: do not pass unused opt->value parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T07:18:45Z","receivedAt":"2023-08-31T07:18:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The option_parse_strategy() callback does not look at opt->value;\ninstead it calls append_strategy(), which manipulates the global\nuse_strategies array directly. But the OPT_CALLBACK declaration assigns\n\"&use_strategies\" to opt->value.\n\nOne could argue this is good, as it tells the reader what we generally\nexpect the callback to do. But it is also bad, because it can mislead\nyou into thinking that swapping out \"&use_strategies\" there might have\nany effect. Let's switch it to pass NULL (which is what every other\n\"does not bother to look at opt->value\" callback does). If you want to\nknow what the callback does, it's easy to read the function itself.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/merge.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 0436986dab..545da0c8a1 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -264,7 +264,7 @@ static struct option builtin_merge_options[] = {\n \tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n \tOPT_BOOL(0, \"verify-signatures\", &verify_signatures,\n \t\tN_(\"verify that the named commit has a valid GPG signature\")),\n-\tOPT_CALLBACK('s', \"strategy\", &use_strategies, N_(\"strategy\"),\n+\tOPT_CALLBACK('s', \"strategy\", NULL, N_(\"strategy\"),\n \t\tN_(\"merge strategy to use\"), option_parse_strategy),\n \tOPT_STRVEC('X', \"strategy-option\", &xopts, N_(\"option=value\"),\n \t\tN_(\"option for selected merge strategy\")),\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481226","messageId":"20230831071900.GD3197751@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831070935.GA3197495@coredump.intra.peff.net","subject":"[PATCH 6/8] parse-options: add more BUG_ON() annotations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T07:19:00Z","receivedAt":"2023-08-31T07:19:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These callbacks are similar to the ones touched by 517fe807d6 (assert\nNOARG/NONEG behavior of parse-options callbacks, 2018-11-05), but were\neither missed in that commit (the one in add.c) or were added later (the\none in log.c).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/add.c | 2 ++\n builtin/log.c | 2 ++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 4b0dd798df..cf59108523 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -232,6 +232,8 @@ static char *chmod_arg;\n \n static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)\n {\n+\tBUG_ON_OPT_ARG(arg);\n+\n \t/* if we are told to ignore, we are not adding removals */\n \t*(int *)opt->value = !unset ? 0 : 1;\n \treturn 0;\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 87e29c4171..80fa642858 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -121,6 +121,8 @@ static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;\n static int clear_decorations_callback(const struct option *opt UNUSED,\n \t\t\t\t      const char *arg, int unset)\n {\n+\tBUG_ON_OPT_NEG(unset);\n+\tBUG_ON_OPT_ARG(arg);\n \tstring_list_clear(&decorate_refs_include, 0);\n \tstring_list_clear(&decorate_refs_exclude, 0);\n \tuse_default_decoration_filter = 0;\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481227","messageId":"20230831071945.GE3197751@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831070935.GA3197495@coredump.intra.peff.net","subject":"[PATCH 7/8] interpret-trailers: mark unused \"unset\" parameters in option callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T07:19:45Z","receivedAt":"2023-08-31T07:19:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are a few parse-option callbacks that do not look at their \"unset\"\nparameters, but also do not set PARSE_OPT_NONEG. At first glance this\nseems like a bug, as we'd ignore \"--no-if-exists\", etc.\n\nBut they do work fine, because when \"unset\" is true, then \"arg\" is NULL.\nAnd all three functions pass \"arg\" on to helper functions which do the\nright thing with the NULL.\n\nNote that this shortcut would not be correct if any callback used\nPARSE_OPT_NOARG (in which case \"arg\" would be NULL but \"unset\" would be\nfalse). But none of these do.\n\nSo the code is fine as-is. But we'll want to mark the unused \"unset\"\nparameters to quiet -Wunused-parameter.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/interpret-trailers.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 6aadce6a1e..a5f9adf6b9 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -24,19 +24,19 @@ static enum trailer_if_exists if_exists;\n static enum trailer_if_missing if_missing;\n \n static int option_parse_where(const struct option *opt,\n-\t\t\t      const char *arg, int unset)\n+\t\t\t      const char *arg, int unset UNUSED)\n {\n \treturn trailer_set_where(opt->value, arg);\n }\n \n static int option_parse_if_exists(const struct option *opt,\n-\t\t\t\t  const char *arg, int unset)\n+\t\t\t\t  const char *arg, int unset UNUSED)\n {\n \treturn trailer_set_if_exists(opt->value, arg);\n }\n \n static int option_parse_if_missing(const struct option *opt,\n-\t\t\t\t   const char *arg, int unset)\n+\t\t\t\t   const char *arg, int unset UNUSED)\n {\n \treturn trailer_set_if_missing(opt->value, arg);\n }\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481228","messageId":"20230831072027.GF3197751@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831070935.GA3197495@coredump.intra.peff.net","subject":"[PATCH 8/8] parse-options: mark unused parameters in noop callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T07:20:27Z","receivedAt":"2023-08-31T07:20:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Unsurprisingly, the noop options callback doesn't bother to look at any\nof its parameters. Let's mark them so that -Wunused-parameter does not\ncomplain.\n\nAnother option would be to drop the callback and have parse-options\nitself recognize OPT_NOOP_NOARG. But that seems like extra work for no\nreal benefit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n parse-options-cb.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex a24521dee0..bdc7fae497 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n-int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)\n+int parse_opt_noop_cb(const struct option *opt UNUSED,\n+\t\t      const char *arg UNUSED,\n+\t\t      int unset UNUSED)\n {\n \treturn 0;\n }\n-- \n2.42.0.561.gaa987ecc69\n"},{"id":"481229","messageId":"20230831072216.GA3197833@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831071230.GA3197647@coredump.intra.peff.net","subject":"Re: [PATCH 1/8] merge: make xopts a strvec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T07:22:16Z","receivedAt":"2023-08-31T07:22:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 31, 2023 at 03:12:30AM -0400, Jeff King wrote:\n\n> The \"xopts\" variable uses a custom array with ALLOC_GROW(). Using a\n> strvec simplifies things a bit. We need fewer variables, and we can also\n> ditch our custom parseopt callback in favor of OPT_STRVEC().\n> \n> As a bonus, this means that \"--no-strategy-option\", which was previously\n> a silent noop, now does something useful: like other list-like options,\n> it will clear the list of -X options seen so far.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I guess you could argue this is a backwards-incompatible change, but the\n> existing behavior of --no-strategy-option is so dumb that I can't\n> believe somebody would prefer it (plus revert/cherry-pick already use\n> OPT_STRVEC for their matching \"-X\").\n> \n> I didn't bother adding a test since we're just re-using OPT_STRVEC code\n> that is used elsewhere.\n\nI only noticed the \"revert/cherry-pick\" thing while sending this patch\nout. But it seems that it was changed recently-ish, and Phillip noted\nthe same behavior change there (along with mentioning merge.c). So now I\ndoubly feel this is the right thing to do. :)\n\n-Peff\n"},{"id":"481234","messageId":"32d494db-5f8a-4bb5-8342-571c12499118@gmail.com","threadId":"60176","inReplyTo":"20230831072216.GA3197833@coredump.intra.peff.net","subject":"Re: [PATCH 1/8] merge: make xopts a strvec","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-31T11:18:46Z","receivedAt":"2023-08-31T11:19:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 31/08/2023 08:22, Jeff King wrote:\n> On Thu, Aug 31, 2023 at 03:12:30AM -0400, Jeff King wrote:\n> I only noticed the \"revert/cherry-pick\" thing while sending this patch\n> out. But it seems that it was changed recently-ish, and Phillip noted\n> the same behavior change there (along with mentioning merge.c). So now I\n> doubly feel this is the right thing to do. :)\n\nThanks for doing this, I had intended to follow up the \ncherry-pick/revert changes by changing merge as well but didn't get \naround to it. I think the current behavior is unlikely to be useful to \nanyone.\n\nBest Wishes\n\nPhillip\n"},{"id":"481239","messageId":"xmqqcyz3z032.fsf@gitster.g","threadId":"60176","inReplyTo":"20230831071230.GA3197647@coredump.intra.peff.net","subject":"Re: [PATCH 1/8] merge: make xopts a strvec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T15:46:25Z","receivedAt":"2023-08-31T15:46:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The \"xopts\" variable uses a custom array with ALLOC_GROW(). Using a\n> strvec simplifies things a bit. We need fewer variables, and we can also\n> ditch our custom parseopt callback in favor of OPT_STRVEC().\n>\n> As a bonus, this means that \"--no-strategy-option\", which was previously\n> a silent noop, now does something useful: like other list-like options,\n> it will clear the list of -X options seen so far.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I guess you could argue this is a backwards-incompatible change, but the\n> existing behavior of --no-strategy-option is so dumb that I can't\n> believe somebody would prefer it (plus revert/cherry-pick already use\n> OPT_STRVEC for their matching \"-X\").\n>\n> I didn't bother adding a test since we're just re-using OPT_STRVEC code\n> that is used elsewhere.\n\nI do not think of any useful way to have \"--no-strategy-option\" on\nthe command line (either as an early part of an alias or in a\nscript) that does nothing (it's not like the command requires at\nleast one -X option on the command line), either.  Just like\nfb60b9f3 (sequencer: use struct strvec to store merge strategy\noptions, 2023-04-10), which met no complaints about a possible\nfallout by the behaviour change, I do not think that this change\neven deserves an entry in the backward compatibility notes.\n\nWe did not have strvec, let alone OPT_STRVEC(), back when the -Xopts\nwas invented around the Git 1.7.0 cycle.  The loss of custom xopts\ncode is very nice.\n\nThanks.\n"},{"id":"481240","messageId":"xmqq8r9ryzms.fsf@gitster.g","threadId":"60176","inReplyTo":"20230831071401.GB3197647@coredump.intra.peff.net","subject":"Re: [PATCH 2/8] merge: simplify parsing of \"-n\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T15:56:11Z","receivedAt":"2023-08-31T15:56:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   2. If they did, the BUG() check added by 3284b93862 (parse-options:\n>      disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and\n>      that check is smart enough to realize that our short-only option is\n>      OK).\n\n;-).\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/merge.c | 13 ++-----------\n>  1 file changed, 2 insertions(+), 11 deletions(-)\n\nMore code deletion.  Nice.\n"},{"id":"481241","messageId":"xmqqr0njxk7a.fsf@gitster.g","threadId":"60176","inReplyTo":"20230831071743.GA3197751@coredump.intra.peff.net","subject":"Re: [PATCH 3/8] parse-options: prefer opt->value to globals in callbacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T16:14:49Z","receivedAt":"2023-08-31T16:14:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... have the\n> caller pass in the appropriate variable via opt->value, and use it. That\n> has the benefit of making the callbacks reusable (in theory at least),\n> and makes it clear from the OPT_CALLBACK declaration which variables\n> will be affected (doubly so for the cases in builtin/fast-export.c,\n> where we do set opt->value, but it is completely ignored!).\n\nThanks for doing this.  I vaguely recall in my recent reviews I said\nthat I prefer the style for exactly the second reason above.\n\n> I went with the \"just use them\" approach here. The loss of type safety\n> is unfortunate, but that is already an issue with most of the other\n> callbacks. If we want to try to address that, we should do so more\n> consistently (and this patch would prepare these callbacks for whatever\n> we choose to do there).\n>\n> Note that in the cases in builtin/fast-export.c, we are passing\n> anonymous enums. We'll have to give them names so that we can declare\n> the appropriate pointer type within the callbacks.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I was tempted to push some of these globals into the cmd_foo()\n> functions, to make sure we don't make the same mistake again. But there\n> are lots of ripple effects from other functions which want to access the\n> globals (and passing them makes the code longer for little benefit).\n> Plus it would make things inconsistent, with some options globals and\n> some not. So that can be a cleanup for another day if somebody is\n> interested (and this takes us one tiny step closer).\n\nYup, as long as these globals are file-scope static that these\ncmd_foo() use to decide and prepare arguments to pass to the more\nlibrary-ish part of the code, I do not mind them not being on stack.\n\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index b28a4a1f82..718b5c3073 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)\n>  static int option_parse_exact_match(const struct option *opt, const char *arg,\n>  \t\t\t\t    int unset)\n>  {\n> +\tint *val = opt->value;\n> +\n>  \tBUG_ON_OPT_ARG(arg);\n>  \n> -\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n> +\t*val = unset ? DEFAULT_CANDIDATES : 0;\n>  \treturn 0;\n>  }\n>  \n> @@ -578,7 +580,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_BOOL(0, \"long\",       &longformat, N_(\"always use long format\")),\n>  \t\tOPT_BOOL(0, \"first-parent\", &first_parent, N_(\"only follow first parent\")),\n>  \t\tOPT__ABBREV(&abbrev),\n> -\t\tOPT_CALLBACK_F(0, \"exact-match\", NULL, NULL,\n> +\t\tOPT_CALLBACK_F(0, \"exact-match\", &max_candidates, NULL,\n>  \t\t\t       N_(\"only output exact matches\"),\n>  \t\t\t       PARSE_OPT_NOARG, option_parse_exact_match),\n>  \t\tOPT_INTEGER(0, \"candidates\", &max_candidates,\n\nIf we ever choose to libify builtin/describe.c::describe_commit(),\nmax_candidates may want to become a parameter to it, which would\nallow cmd_describe() to have it on its stack, but until that\nhappens, I agree that it is not a very useful churn.\n\n> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> index c5e8345265..6aadce6a1e 100644\n> --- a/builtin/interpret-trailers.c\n> +++ b/builtin/interpret-trailers.c\n> @@ -26,19 +26,19 @@ static enum trailer_if_missing if_missing;\n>  static int option_parse_where(const struct option *opt,\n>  \t\t\t      const char *arg, int unset)\n>  {\n> -\treturn trailer_set_where(&where, arg);\n> +\treturn trailer_set_where(opt->value, arg);\n>  }\n>  \n>  static int option_parse_if_exists(const struct option *opt,\n>  \t\t\t\t  const char *arg, int unset)\n>  {\n> -\treturn trailer_set_if_exists(&if_exists, arg);\n> +\treturn trailer_set_if_exists(opt->value, arg);\n>  }\n>  \n>  static int option_parse_if_missing(const struct option *opt,\n>  \t\t\t\t   const char *arg, int unset)\n>  {\n> -\treturn trailer_set_if_missing(&if_missing, arg);\n> +\treturn trailer_set_if_missing(opt->value, arg);\n>  }\n>  \n>  static void new_trailers_clear(struct list_head *trailers)\n> @@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_BOOL(0, \"in-place\", &opts.in_place, N_(\"edit files in place\")),\n>  \t\tOPT_BOOL(0, \"trim-empty\", &opts.trim_empty, N_(\"trim empty trailers\")),\n>  \n> -\t\tOPT_CALLBACK(0, \"where\", NULL, N_(\"action\"),\n> +\t\tOPT_CALLBACK(0, \"where\", &where, N_(\"action\"),\n>  \t\t\t     N_(\"where to place the new trailer\"), option_parse_where),\n> -\t\tOPT_CALLBACK(0, \"if-exists\", NULL, N_(\"action\"),\n> +\t\tOPT_CALLBACK(0, \"if-exists\", &if_exists, N_(\"action\"),\n>  \t\t\t     N_(\"action if trailer already exists\"), option_parse_if_exists),\n> -\t\tOPT_CALLBACK(0, \"if-missing\", NULL, N_(\"action\"),\n> +\t\tOPT_CALLBACK(0, \"if-missing\", &if_missing, N_(\"action\"),\n>  \t\t\t     N_(\"action if trailer is missing\"), option_parse_if_missing),\n>  \n>  \t\tOPT_BOOL(0, \"only-trailers\", &opts.only_trailers, N_(\"output only the trailers\")),\n\nOK.  These three variables should be fun (but not too difficult) to\nconvert to on-stack variables.  option_parse_trailer() needs to\nlearn to take a struct that holds \"trailers\", in addition to these\nthree variables as its members to lose its dependency on the\nglobals, and then these three callbacks can learn to take the same\nstruct and fill their own members in it.\n\nOf course that is outside the scope of this series.\n"},{"id":"481242","messageId":"xmqqil8vxjcd.fsf@gitster.g","threadId":"60176","inReplyTo":"20230831071820.GB3197751@coredump.intra.peff.net","subject":"Re: [PATCH 4/8] parse-options: mark unused \"opt\" parameter in callbacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T16:33:22Z","receivedAt":"2023-08-31T16:33:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\n> index f62f13f2b5..95b3717dd1 100644\n> --- a/builtin/checkout-index.c\n> +++ b/builtin/checkout-index.c\n> @@ -190,7 +190,7 @@ static const char * const builtin_checkout_index_usage[] = {\n>  \tNULL\n>  };\n>  \n> -static int option_parse_stage(const struct option *opt,\n> +static int option_parse_stage(const struct option *opt UNUSED,\n>  \t\t\t      const char *arg, int unset)\n>  {\n>  \tBUG_ON_OPT_NEG(unset);\n\nI suspect that the original is buggy; when given\n\n  $ git checkout-index --stage=all --stage=1 path\n\nthe first option turns --temp on, but the second one does not turn\nit off.  For now I think being bug-to-bug compatible and annotating\nthe opt as UNUSED is good, but as a follow-up, we could make the\ncaller:\n\n (1) point &checkout_stage with opt->value;\n\n (2) make to_tempfile to tristate <unspecified, false, true> by\n     initializing it to -1;\n\n (3) adjust to_tempfile that is still <unspecified> after\n     parse_options() returns, according to the value in\n     checkout_stage.\n\nand then this can follow the \"opt->value points at the variable that\nis affected\" pattern.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 8f93529505..cd2afe33e5 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -168,7 +168,8 @@ static int git_fetch_config(const char *k, const char *v,\n>  \treturn git_default_config(k, v, ctx, cb);\n>  }\n>  \n> -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n> +static int parse_refmap_arg(const struct option *opt UNUSED,\n> +\t\t\t    const char *arg, int unset)\n>  {\n>  \tBUG_ON_OPT_NEG(unset);\n\nCan't this just point opt->value at the global &refmap?  Obviously\nnot a huge deal, as we could have taken the \"annotate as UNUSED\"\napproach for all the functions in [3/8].\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 369bd43fb2..b842349d86 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)\n>  \tstrbuf_release(&config_name);\n>  }\n>  \n> -static int task_option_parse(const struct option *opt,\n> +static int task_option_parse(const struct option *opt UNUSED,\n>  \t\t\t     const char *arg, int unset)\n>  {\n>  \tint i, num_selected = 0;\n\nThe TASK__COUNT constant is very closely tied to the task[] array\nand it is not worth passing the address of the array in opt->value.\nThe loss of clarity inside the callback funtion is a more serious\ndownside than the value on the caller side to document what array\nis being modified.  So I agree this is a good change.\n\n> diff --git a/builtin/update-index.c b/builtin/update-index.c\n> index aee3cb8cbd..59acae3336 100644\n> --- a/builtin/update-index.c\n> +++ b/builtin/update-index.c\n> @@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,\n>  \treturn 0;\n>  }\n>  \n> -static int resolve_undo_clear_callback(const struct option *opt,\n> +static int resolve_undo_clear_callback(const struct option *opt UNUSED,\n>  \t\t\t\tconst char *arg, int unset)\n>  {\n>  \tBUG_ON_OPT_NEG(unset);\n> @@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,\n>  }\n>  \n>  static enum parse_opt_result cacheinfo_callback(\n> -\tstruct parse_opt_ctx_t *ctx, const struct option *opt,\n> +\tstruct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,\n>  \tconst char *arg, int unset)\n>  {\n>  \tstruct object_id oid;\n\nThese two refuses to take \"arg\" and the callback mechanism is used\npurely to trigger the side effect of the function, not as a part of\nparsing something for its \"value\", so I agree that these UNUSED\nannotations reflect the reality very well.\n\nThanks.\n\n"},{"id":"481243","messageId":"xmqqedjjxiej.fsf@gitster.g","threadId":"60176","inReplyTo":"20230831071845.GC3197751@coredump.intra.peff.net","subject":"Re: [PATCH 5/8] merge: do not pass unused opt->value parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T16:53:40Z","receivedAt":"2023-08-31T16:53:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The option_parse_strategy() callback does not look at opt->value;\n> instead it calls append_strategy(), which manipulates the global\n> use_strategies array directly. But the OPT_CALLBACK declaration assigns\n> \"&use_strategies\" to opt->value.\n>\n> One could argue this is good, as it tells the reader what we generally\n> expect the callback to do. But it is also bad, because it can mislead\n> you into thinking that swapping out \"&use_strategies\" there might have\n> any effect. Let's switch it to pass NULL (which is what every other\n> \"does not bother to look at opt->value\" callback does). If you want to\n> know what the callback does, it's easy to read the function itself.\n\nGood.  It is not like we have two options and each of them affect\nits own list of strategies.  There is a single use_strategies list\nthat is populated not just by the parse options callback but by\nother callers.  So I agree that the new way is less confusing.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/merge.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 0436986dab..545da0c8a1 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -264,7 +264,7 @@ static struct option builtin_merge_options[] = {\n>  \tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n>  \tOPT_BOOL(0, \"verify-signatures\", &verify_signatures,\n>  \t\tN_(\"verify that the named commit has a valid GPG signature\")),\n> -\tOPT_CALLBACK('s', \"strategy\", &use_strategies, N_(\"strategy\"),\n> +\tOPT_CALLBACK('s', \"strategy\", NULL, N_(\"strategy\"),\n>  \t\tN_(\"merge strategy to use\"), option_parse_strategy),\n>  \tOPT_STRVEC('X', \"strategy-option\", &xopts, N_(\"option=value\"),\n>  \t\tN_(\"option for selected merge strategy\")),\n"},{"id":"481244","messageId":"xmqq8r9rxi69.fsf@gitster.g","threadId":"60176","inReplyTo":"20230831071900.GD3197751@coredump.intra.peff.net","subject":"Re: [PATCH 6/8] parse-options: add more BUG_ON() annotations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T16:58:38Z","receivedAt":"2023-08-31T16:58:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> These callbacks are similar to the ones touched by 517fe807d6 (assert\n> NOARG/NONEG behavior of parse-options callbacks, 2018-11-05), but were\n> either missed in that commit (the one in add.c) or were added later (the\n> one in log.c).\n\nThey obviously look good.  Thanks.\n\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/add.c | 2 ++\n>  builtin/log.c | 2 ++\n>  2 files changed, 4 insertions(+)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 4b0dd798df..cf59108523 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -232,6 +232,8 @@ static char *chmod_arg;\n>  \n>  static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)\n>  {\n> +\tBUG_ON_OPT_ARG(arg);\n> +\n>  \t/* if we are told to ignore, we are not adding removals */\n>  \t*(int *)opt->value = !unset ? 0 : 1;\n>  \treturn 0;\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 87e29c4171..80fa642858 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -121,6 +121,8 @@ static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;\n>  static int clear_decorations_callback(const struct option *opt UNUSED,\n>  \t\t\t\t      const char *arg, int unset)\n>  {\n> +\tBUG_ON_OPT_NEG(unset);\n> +\tBUG_ON_OPT_ARG(arg);\n>  \tstring_list_clear(&decorate_refs_include, 0);\n>  \tstring_list_clear(&decorate_refs_exclude, 0);\n>  \tuse_default_decoration_filter = 0;\n"},{"id":"481245","messageId":"xmqq4jkfxhx2.fsf@gitster.g","threadId":"60176","inReplyTo":"20230831071945.GE3197751@coredump.intra.peff.net","subject":"Re: [PATCH 7/8] interpret-trailers: mark unused \"unset\" parameters in option callbacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T17:04:09Z","receivedAt":"2023-08-31T17:04:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> There are a few parse-option callbacks that do not look at their \"unset\"\n> parameters, but also do not set PARSE_OPT_NONEG. At first glance this\n> seems like a bug, as we'd ignore \"--no-if-exists\", etc.\n>\n> But they do work fine, because when \"unset\" is true, then \"arg\" is NULL.\n> And all three functions pass \"arg\" on to helper functions which do the\n> right thing with the NULL.\n\nYuck.  That is ugly.\n\n> Note that this shortcut would not be correct if any callback used\n> PARSE_OPT_NOARG (in which case \"arg\" would be NULL but \"unset\" would be\n> false). But none of these do.\n\nThat is even uglier.  Unlike the BUG_ON_OPT_NEG() and BUG_ON_OPT_ARG()\nthat catch discrepancies between options[] flags and the expectation\nby the callback function, there is no way for us to protect against\nsuch mistakes?\n\n> So the code is fine as-is. But we'll want to mark the unused \"unset\"\n> parameters to quiet -Wunused-parameter.\n\nOK.  Thanks.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/interpret-trailers.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> index 6aadce6a1e..a5f9adf6b9 100644\n> --- a/builtin/interpret-trailers.c\n> +++ b/builtin/interpret-trailers.c\n> @@ -24,19 +24,19 @@ static enum trailer_if_exists if_exists;\n>  static enum trailer_if_missing if_missing;\n>  \n>  static int option_parse_where(const struct option *opt,\n> -\t\t\t      const char *arg, int unset)\n> +\t\t\t      const char *arg, int unset UNUSED)\n>  {\n>  \treturn trailer_set_where(opt->value, arg);\n>  }\n>  \n>  static int option_parse_if_exists(const struct option *opt,\n> -\t\t\t\t  const char *arg, int unset)\n> +\t\t\t\t  const char *arg, int unset UNUSED)\n>  {\n>  \treturn trailer_set_if_exists(opt->value, arg);\n>  }\n>  \n>  static int option_parse_if_missing(const struct option *opt,\n> -\t\t\t\t   const char *arg, int unset)\n> +\t\t\t\t   const char *arg, int unset UNUSED)\n>  {\n>  \treturn trailer_set_if_missing(opt->value, arg);\n>  }\n"},{"id":"481246","messageId":"xmqqzg27w3b0.fsf@gitster.g","threadId":"60176","inReplyTo":"20230831072027.GF3197751@coredump.intra.peff.net","subject":"Re: [PATCH 8/8] parse-options: mark unused parameters in noop callback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T17:05:07Z","receivedAt":"2023-08-31T17:06:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Unsurprisingly, the noop options callback doesn't bother to look at any\n> of its parameters. Let's mark them so that -Wunused-parameter does not\n> complain.\n\n;-)\n\nThanks.\n\n> Another option would be to drop the callback and have parse-options\n> itself recognize OPT_NOOP_NOARG. But that seems like extra work for no\n> real benefit.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  parse-options-cb.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/parse-options-cb.c b/parse-options-cb.c\n> index a24521dee0..bdc7fae497 100644\n> --- a/parse-options-cb.c\n> +++ b/parse-options-cb.c\n> @@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)\n>  \treturn 0;\n>  }\n>  \n> -int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)\n> +int parse_opt_noop_cb(const struct option *opt UNUSED,\n> +\t\t      const char *arg UNUSED,\n> +\t\t      int unset UNUSED)\n>  {\n>  \treturn 0;\n>  }\n"},{"id":"481251","messageId":"20230831175018.GB3208283@coredump.intra.peff.net","threadId":"60176","inReplyTo":"xmqqil8vxjcd.fsf@gitster.g","subject":"Re: [PATCH 4/8] parse-options: mark unused \"opt\" parameter in callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T17:50:18Z","receivedAt":"2023-08-31T17:50:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 31, 2023 at 09:33:22AM -0700, Junio C Hamano wrote:\n\n> > -static int option_parse_stage(const struct option *opt,\n> > +static int option_parse_stage(const struct option *opt UNUSED,\n> >  \t\t\t      const char *arg, int unset)\n> >  {\n> >  \tBUG_ON_OPT_NEG(unset);\n> \n> I suspect that the original is buggy; when given\n> \n>   $ git checkout-index --stage=all --stage=1 path\n> \n> the first option turns --temp on, but the second one does not turn\n> it off.  For now I think being bug-to-bug compatible and annotating\n> the opt as UNUSED is good, but as a follow-up, we could make the\n> caller:\n> \n>  (1) point &checkout_stage with opt->value;\n> \n>  (2) make to_tempfile to tristate <unspecified, false, true> by\n>      initializing it to -1;\n> \n>  (3) adjust to_tempfile that is still <unspecified> after\n>      parse_options() returns, according to the value in\n>      checkout_stage.\n> \n> and then this can follow the \"opt->value points at the variable that\n> is affected\" pattern.\n\nYeah, I think that would work, with one extra bit:\n\n  (4) complain when (!to_tempfile && checkout_stage == CHECKOUT_ALL)\n\nI do think it would be better to fix separately, but maybe if I'm\nre-rolling I can do it as an early patch in the series (it is not much\ndifferent than the \"xopts\" fix in scope).\n\n> > -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n> > +static int parse_refmap_arg(const struct option *opt UNUSED,\n> > +\t\t\t    const char *arg, int unset)\n> >  {\n> >  \tBUG_ON_OPT_NEG(unset);\n> \n> Can't this just point opt->value at the global &refmap?  Obviously\n> not a huge deal, as we could have taken the \"annotate as UNUSED\"\n> approach for all the functions in [3/8].\n\nHmm, yeah. I think I looked at the abstract refspec_append() here and\nassumed that it might be touching other variables. But it's not. It's\noperating purely on the &refspec we pass it (and even though it uses\nALLOC_GROW, the \"nr\" and \"alloc\" are both contained in the struct). So\nyeah, it really should have been converted in patch 3.\n\nI think that is probably worth a re-roll.\n\n-Peff\n"},{"id":"481252","messageId":"20230831175638.GC3208283@coredump.intra.peff.net","threadId":"60176","inReplyTo":"xmqq4jkfxhx2.fsf@gitster.g","subject":"Re: [PATCH 7/8] interpret-trailers: mark unused \"unset\" parameters in option callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T17:56:38Z","receivedAt":"2023-08-31T17:56:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 31, 2023 at 10:04:09AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > There are a few parse-option callbacks that do not look at their \"unset\"\n> > parameters, but also do not set PARSE_OPT_NONEG. At first glance this\n> > seems like a bug, as we'd ignore \"--no-if-exists\", etc.\n> >\n> > But they do work fine, because when \"unset\" is true, then \"arg\" is NULL.\n> > And all three functions pass \"arg\" on to helper functions which do the\n> > right thing with the NULL.\n> \n> Yuck.  That is ugly.\n\nYep. I wondered about adding a comment here warning about the situation,\nbut it felt kind of content-less. Something like:\n\n  /* if unset, arg is NULL and handled below */\n  trailer_set_where(opt->value, arg);\n\n> > Note that this shortcut would not be correct if any callback used\n> > PARSE_OPT_NOARG (in which case \"arg\" would be NULL but \"unset\" would be\n> > false). But none of these do.\n> \n> That is even uglier.  Unlike the BUG_ON_OPT_NEG() and BUG_ON_OPT_ARG()\n> that catch discrepancies between options[] flags and the expectation\n> by the callback function, there is no way for us to protect against\n> such mistakes?\n\nI guess it would be something like:\n\n  if (!unset && !arg)\n\tBUG(\"unexpected use of PARSE_OPT_OPTARG\");\n\nI think it is less important than those other ones, though, because the\nmistake here is the OPT_CALLBACK declaration adding a flag that the\ncallback is not prepared to handle. Whereas in the other ones, the bug\nis that the declaration _forgot_ to use a flag, which is a much more\nlikely bug.\n\nSo I dunno. If it were more than this one case (well, three, but they\nare all of the same form) I'd be more worried.\n\n-Peff\n"},{"id":"481253","messageId":"20230831204829.GA3233491@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831175018.GB3208283@coredump.intra.peff.net","subject":"Re: [PATCH 4/8] parse-options: mark unused \"opt\" parameter in callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T20:48:29Z","receivedAt":"2023-08-31T20:48:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 31, 2023 at 01:50:18PM -0400, Jeff King wrote:\n\n> > > -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n> > > +static int parse_refmap_arg(const struct option *opt UNUSED,\n> > > +\t\t\t    const char *arg, int unset)\n> > >  {\n> > >  \tBUG_ON_OPT_NEG(unset);\n> > \n> > Can't this just point opt->value at the global &refmap?  Obviously\n> > not a huge deal, as we could have taken the \"annotate as UNUSED\"\n> > approach for all the functions in [3/8].\n> \n> Hmm, yeah. I think I looked at the abstract refspec_append() here and\n> assumed that it might be touching other variables. But it's not. It's\n> operating purely on the &refspec we pass it (and even though it uses\n> ALLOC_GROW, the \"nr\" and \"alloc\" are both contained in the struct). So\n> yeah, it really should have been converted in patch 3.\n\nOh, btw, there are two other cases in this patch that _could_ be\nconverted but I left alone. In log.c, we have a to_callback() and a\ncc_callback(), both of which take a single string list. But there is\nalso header_callback(), which resets both of those when it sees\n--no-add-header. So I left them alone as a set.\n\nHowever, it occurs to me that to_callback() and cc_callback() can just\nbe OPT_STRING_LIST these days. And that is probably a worthwhile\ncleanup to do.\n\n-Peff\n"},{"id":"481254","messageId":"ZPD+X0JihpYzPI6+@nand.local","threadId":"60176","inReplyTo":"xmqqcyz3z032.fsf@gitster.g","subject":"Re: [PATCH 1/8] merge: make xopts a strvec","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-31T20:55:59Z","receivedAt":"2023-08-31T20:56:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 31, 2023 at 08:46:25AM -0700, Junio C Hamano wrote:\n> > I guess you could argue this is a backwards-incompatible change, but the\n> > existing behavior of --no-strategy-option is so dumb that I can't\n> > believe somebody would prefer it (plus revert/cherry-pick already use\n> > OPT_STRVEC for their matching \"-X\").\n> >\n> > I didn't bother adding a test since we're just re-using OPT_STRVEC code\n> > that is used elsewhere.\n>\n> I do not think of any useful way to have \"--no-strategy-option\" on\n> the command line (either as an early part of an alias or in a\n> script) that does nothing (it's not like the command requires at\n> least one -X option on the command line), either.  Just like\n> fb60b9f3 (sequencer: use struct strvec to store merge strategy\n> options, 2023-04-10), which met no complaints about a possible\n> fallout by the behaviour change, I do not think that this change\n> even deserves an entry in the backward compatibility notes.\n\nI concur with both of you. In a project like this one, we should be\nrather generous with the set of things we expect users to do. But even\nin a quite generous interpretation, I cannot imagine anybody relying on\nthis behavior, so I think skipping a mention of it in the backwards\ncompatibility section makes sense.\n\nThanks,\nTaylor\n"},{"id":"481257","messageId":"20230831211637.GA949188@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831070935.GA3197495@coredump.intra.peff.net","subject":"[PATCH v2 0/10] more unused parameters in parseopt callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T21:16:37Z","receivedAt":"2023-08-31T21:16:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 31, 2023 at 03:09:35AM -0400, Jeff King wrote:\n\n> Here are some more patches silencing -Wunused-parameter warnings. I've\n> prepared them on top of the patches queued in jk/unused-post-2.42, but\n> they should apply equally well directly on 'master'.\n\nAnd here's a re-roll based on feedback from Junio:\n\n  [01/10]: merge: make xopts a strvec\n  [02/10]: merge: simplify parsing of \"-n\" option\n  [03/10]: format-patch: use OPT_STRING_LIST for to/cc options\n  [04/10]: checkout-index: delay automatic setting of to_tempfile\n  [05/10]: parse-options: prefer opt->value to globals in callbacks\n  [06/10]: parse-options: mark unused \"opt\" parameter in callbacks\n  [07/10]: merge: do not pass unused opt->value parameter\n  [08/10]: parse-options: add more BUG_ON() annotations\n  [09/10]: interpret-trailers: mark unused \"unset\" parameters in option callbacks\n  [10/10]: parse-options: mark unused parameters in noop callback\n\nRange-diff is below, but the most interesting changes are in the two new\npatches (3 and 4):\n\n  - patch 3 simplifies away a few string-list callbacks (which can then\n    be omitted from patch 6, the \"mark unused opt\" one).\n\n  - patch 4 fixes some minor bugs with \"checkout-index --stage\". In\n    doing so, it's now eligible for cleanup in patch 5 (\"prefer\n    opt->value\") and its annotation dropped from patch 6\n\n  - patch 5 (\"prefer opt->value\") converts the missed refmap callback,\n    and so its annotation is gone from patch 6 now\n\n  - a few extra clarifying comments\n\n 1:  8750d92016 !  1:  82940b6936 merge: make xopts a strvec\n    @@ Commit message\n     \n         As a bonus, this means that \"--no-strategy-option\", which was previously\n         a silent noop, now does something useful: like other list-like options,\n    -    it will clear the list of -X options seen so far.\n    +    it will clear the list of -X options seen so far. This matches the\n    +    behavior of revert/cherry-pick, which made the same change in fb60b9f37f\n    +    (sequencer: use struct strvec to store merge strategy options,\n    +    2023-04-10).\n     \n         Signed-off-by: Jeff King <peff@peff.net>\n     \n 2:  ffdae69bd6 =  2:  c98b87feb9 merge: simplify parsing of \"-n\" option\n -:  ---------- >  3:  615fef8f90 format-patch: use OPT_STRING_LIST for to/cc options\n -:  ---------- >  4:  66cf7bcf08 checkout-index: delay automatic setting of to_tempfile\n 3:  f00c78cc49 !  5:  a7c2bfecba parse-options: prefer opt->value to globals in callbacks\n    @@ Commit message\n     \n         Signed-off-by: Jeff King <peff@peff.net>\n     \n    + ## builtin/checkout-index.c ##\n    +@@ builtin/checkout-index.c: static const char * const builtin_checkout_index_usage[] = {\n    + static int option_parse_stage(const struct option *opt,\n    + \t\t\t      const char *arg, int unset)\n    + {\n    ++\tint *stage = opt->value;\n    ++\n    + \tBUG_ON_OPT_NEG(unset);\n    + \n    + \tif (!strcmp(arg, \"all\")) {\n    +-\t\tcheckout_stage = CHECKOUT_ALL;\n    ++\t\t*stage = CHECKOUT_ALL;\n    + \t} else {\n    + \t\tint ch = arg[0];\n    + \t\tif ('1' <= ch && ch <= '3')\n    +-\t\t\tcheckout_stage = arg[0] - '0';\n    ++\t\t\t*stage = arg[0] - '0';\n    + \t\telse\n    + \t\t\tdie(_(\"stage should be between 1 and 3 or all\"));\n    + \t}\n    +@@ builtin/checkout-index.c: int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n    + \t\t\tN_(\"write the content to temporary files\")),\n    + \t\tOPT_STRING(0, \"prefix\", &state.base_dir, N_(\"string\"),\n    + \t\t\tN_(\"when creating files, prepend <string>\")),\n    +-\t\tOPT_CALLBACK_F(0, \"stage\", NULL, \"(1|2|3|all)\",\n    ++\t\tOPT_CALLBACK_F(0, \"stage\", &checkout_stage, \"(1|2|3|all)\",\n    + \t\t\tN_(\"copy out the files from named stage\"),\n    + \t\t\tPARSE_OPT_NONEG, option_parse_stage),\n    + \t\tOPT_END()\n    +\n      ## builtin/describe.c ##\n     @@ builtin/describe.c: static void describe(const char *arg, int last_one)\n      static int option_parse_exact_match(const struct option *opt, const char *arg,\n    @@ builtin/fast-export.c: static int parse_opt_tag_of_filtered_mode(const struct op\n      \t\t\treturn error(\"Unknown reencoding mode: %s\", arg);\n      \t}\n     \n    + ## builtin/fetch.c ##\n    +@@ builtin/fetch.c: static int parse_refmap_arg(const struct option *opt, const char *arg, int unset\n    + \t * \"git fetch --refmap='' origin foo\"\n    + \t * can be used to tell the command not to store anywhere\n    + \t */\n    +-\trefspec_append(&refmap, arg);\n    ++\trefspec_append(opt->value, arg);\n    + \n    + \treturn 0;\n    + }\n    +@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n    + \t\t\t   PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules),\n    + \t\tOPT_BOOL(0, \"update-shallow\", &update_shallow,\n    + \t\t\t N_(\"accept refs that update .git/shallow\")),\n    +-\t\tOPT_CALLBACK_F(0, \"refmap\", NULL, N_(\"refmap\"),\n    ++\t\tOPT_CALLBACK_F(0, \"refmap\", &refmap, N_(\"refmap\"),\n    + \t\t\t       N_(\"specify fetch refmap\"), PARSE_OPT_NONEG, parse_refmap_arg),\n    + \t\tOPT_STRING_LIST('o', \"server-option\", &server_options, N_(\"server-specific\"), N_(\"option to transmit\")),\n    + \t\tOPT_IPVERSION(&family),\n    +\n      ## builtin/interpret-trailers.c ##\n     @@ builtin/interpret-trailers.c: static enum trailer_if_missing if_missing;\n      static int option_parse_where(const struct option *opt,\n 4:  19621fc01c !  6:  bd915fe4de parse-options: mark unused \"opt\" parameter in callbacks\n    @@ Commit message\n     \n         Signed-off-by: Jeff King <peff@peff.net>\n     \n    - ## builtin/checkout-index.c ##\n    -@@ builtin/checkout-index.c: static const char * const builtin_checkout_index_usage[] = {\n    - \tNULL\n    - };\n    - \n    --static int option_parse_stage(const struct option *opt,\n    -+static int option_parse_stage(const struct option *opt UNUSED,\n    - \t\t\t      const char *arg, int unset)\n    - {\n    - \tBUG_ON_OPT_NEG(unset);\n    -\n    - ## builtin/fetch.c ##\n    -@@ builtin/fetch.c: static int git_fetch_config(const char *k, const char *v,\n    - \treturn git_default_config(k, v, ctx, cb);\n    - }\n    - \n    --static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n    -+static int parse_refmap_arg(const struct option *opt UNUSED,\n    -+\t\t\t    const char *arg, int unset)\n    - {\n    - \tBUG_ON_OPT_NEG(unset);\n    - \n    -\n      ## builtin/gc.c ##\n     @@ builtin/gc.c: static void initialize_task_config(int schedule)\n      \tstrbuf_release(&config_name);\n    @@ builtin/log.c: static int inline_callback(const struct option *opt, const char *\n      {\n      \tif (unset) {\n      \t\tstring_list_clear(&extra_hdr, 0);\n    -@@ builtin/log.c: static int header_callback(const struct option *opt, const char *arg, int unset)\n    - \treturn 0;\n    - }\n    - \n    --static int to_callback(const struct option *opt, const char *arg, int unset)\n    -+static int to_callback(const struct option *opt UNUSED, const char *arg,\n    -+\t\t       int unset)\n    - {\n    - \tif (unset)\n    - \t\tstring_list_clear(&extra_to, 0);\n    -@@ builtin/log.c: static int to_callback(const struct option *opt, const char *arg, int unset)\n    - \treturn 0;\n    - }\n    - \n    --static int cc_callback(const struct option *opt, const char *arg, int unset)\n    -+static int cc_callback(const struct option *opt UNUSED, const char *arg,\n    -+\t\t       int unset)\n    - {\n    - \tif (unset)\n    - \t\tstring_list_clear(&extra_cc, 0);\n     \n      ## builtin/merge.c ##\n     @@ builtin/merge.c: static void append_strategy(struct strategy *s)\n 5:  2d6a3b9e1b =  7:  1720ddebc1 merge: do not pass unused opt->value parameter\n 6:  7669758cc7 =  8:  a93f155f21 parse-options: add more BUG_ON() annotations\n 7:  b80020daec !  9:  529573e281 interpret-trailers: mark unused \"unset\" parameters in option callbacks\n    @@ Commit message\n         false). But none of these do.\n     \n         So the code is fine as-is. But we'll want to mark the unused \"unset\"\n    -    parameters to quiet -Wunused-parameter.\n    +    parameters to quiet -Wunused-parameter. I've also added a comment to\n    +    make this rather subtle situation more explicit.\n     \n         Signed-off-by: Jeff King <peff@peff.net>\n     \n    @@ builtin/interpret-trailers.c: static enum trailer_if_exists if_exists;\n     -\t\t\t      const char *arg, int unset)\n     +\t\t\t      const char *arg, int unset UNUSED)\n      {\n    ++\t/* unset implies NULL arg, which is handled in our helper */\n      \treturn trailer_set_where(opt->value, arg);\n      }\n      \n      static int option_parse_if_exists(const struct option *opt,\n     -\t\t\t\t  const char *arg, int unset)\n     +\t\t\t\t  const char *arg, int unset UNUSED)\n      {\n    ++\t/* unset implies NULL arg, which is handled in our helper */\n      \treturn trailer_set_if_exists(opt->value, arg);\n      }\n      \n      static int option_parse_if_missing(const struct option *opt,\n     -\t\t\t\t   const char *arg, int unset)\n     +\t\t\t\t   const char *arg, int unset UNUSED)\n      {\n    ++\t/* unset implies NULL arg, which is handled in our helper */\n      \treturn trailer_set_if_missing(opt->value, arg);\n      }\n    + \n 8:  f21961ed23 = 10:  943161eaf2 parse-options: mark unused parameters in noop callback\n"},{"id":"481258","messageId":"20230831211711.GA949469@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831211637.GA949188@coredump.intra.peff.net","subject":"[PATCH v2 01/10] merge: make xopts a strvec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T21:17:11Z","receivedAt":"2023-08-31T21:17:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"xopts\" variable uses a custom array with ALLOC_GROW(). Using a\nstrvec simplifies things a bit. We need fewer variables, and we can also\nditch our custom parseopt callback in favor of OPT_STRVEC().\n\nAs a bonus, this means that \"--no-strategy-option\", which was previously\na silent noop, now does something useful: like other list-like options,\nit will clear the list of -X options seen so far. This matches the\nbehavior of revert/cherry-pick, which made the same change in fb60b9f37f\n(sequencer: use struct strvec to store merge strategy options,\n2023-04-10).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/merge.c | 26 +++++++-------------------\n 1 file changed, 7 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex de68910177..53e9fe70d9 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -79,8 +79,7 @@ static int overwrite_ignore = 1;\n static struct strbuf merge_msg = STRBUF_INIT;\n static struct strategy **use_strategies;\n static size_t use_strategies_nr, use_strategies_alloc;\n-static const char **xopts;\n-static size_t xopts_nr, xopts_alloc;\n+static struct strvec xopts = STRVEC_INIT;\n static const char *branch;\n static char *branch_mergeoptions;\n static int verbosity;\n@@ -242,17 +241,6 @@ static int option_parse_strategy(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_x(const struct option *opt,\n-\t\t\t  const char *arg, int unset)\n-{\n-\tif (unset)\n-\t\treturn 0;\n-\n-\tALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);\n-\txopts[xopts_nr++] = xstrdup(arg);\n-\treturn 0;\n-}\n-\n static int option_parse_n(const struct option *opt,\n \t\t\t  const char *arg, int unset)\n {\n@@ -287,8 +275,8 @@ static struct option builtin_merge_options[] = {\n \t\tN_(\"verify that the named commit has a valid GPG signature\")),\n \tOPT_CALLBACK('s', \"strategy\", &use_strategies, N_(\"strategy\"),\n \t\tN_(\"merge strategy to use\"), option_parse_strategy),\n-\tOPT_CALLBACK('X', \"strategy-option\", &xopts, N_(\"option=value\"),\n-\t\tN_(\"option for selected merge strategy\"), option_parse_x),\n+\tOPT_STRVEC('X', \"strategy-option\", &xopts, N_(\"option=value\"),\n+\t\tN_(\"option for selected merge strategy\")),\n \tOPT_CALLBACK('m', \"message\", &merge_msg, N_(\"message\"),\n \t\tN_(\"merge commit message (for a non-fast-forward merge)\"),\n \t\toption_parse_message),\n@@ -749,9 +737,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\to.show_rename_progress =\n \t\t\tshow_progress == -1 ? isatty(2) : show_progress;\n \n-\t\tfor (x = 0; x < xopts_nr; x++)\n-\t\t\tif (parse_merge_opt(&o, xopts[x]))\n-\t\t\t\tdie(_(\"unknown strategy option: -X%s\"), xopts[x]);\n+\t\tfor (x = 0; x < xopts.nr; x++)\n+\t\t\tif (parse_merge_opt(&o, xopts.v[x]))\n+\t\t\t\tdie(_(\"unknown strategy option: -X%s\"), xopts.v[x]);\n \n \t\to.branch1 = head_arg;\n \t\to.branch2 = merge_remote_util(remoteheads->item)->name;\n@@ -777,7 +765,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\treturn clean ? 0 : 1;\n \t} else {\n \t\treturn try_merge_command(the_repository,\n-\t\t\t\t\t strategy, xopts_nr, xopts,\n+\t\t\t\t\t strategy, xopts.nr, xopts.v,\n \t\t\t\t\t common, head_arg, remoteheads);\n \t}\n }\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481259","messageId":"20230831211716.GB949469@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831211637.GA949188@coredump.intra.peff.net","subject":"[PATCH v2 02/10] merge: simplify parsing of \"-n\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T21:17:16Z","receivedAt":"2023-08-31T21:17:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"-n\" option is implemented by an option callback, as it is really a\n\"reverse bool\". If given, it sets show_diffstat to 0. In theory, when\nnegated, it would set the same flag to 1. But it's not possible to\ntrigger that, since short options cannot be negated.\n\nSo in practice this is really just a SET_INT to 0. Let's use that\ninstead, which shortens the code.\n\nNote that negation here would do the wrong thing (as with any SET_INT\nwith a value of \"0\"). We could specify PARSE_OPT_NONEG to future-proof\nourselves against somebody adding a long option name (which would make\nit possible to negate). But there's not much point:\n\n  1. Nobody is going to do that, because the negated form already\n     exists, and is called \"--stat\" (which is defined separately so that\n     \"--no-stat\" works).\n\n  2. If they did, the BUG() check added by 3284b93862 (parse-options:\n     disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and\n     that check is smart enough to realize that our short-only option is\n     OK).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/merge.c | 13 ++-----------\n 1 file changed, 2 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 53e9fe70d9..21363b7985 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -241,18 +241,9 @@ static int option_parse_strategy(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_n(const struct option *opt,\n-\t\t\t  const char *arg, int unset)\n-{\n-\tBUG_ON_OPT_ARG(arg);\n-\tshow_diffstat = unset;\n-\treturn 0;\n-}\n-\n static struct option builtin_merge_options[] = {\n-\tOPT_CALLBACK_F('n', NULL, NULL, NULL,\n-\t\tN_(\"do not show a diffstat at the end of the merge\"),\n-\t\tPARSE_OPT_NOARG, option_parse_n),\n+\tOPT_SET_INT('n', NULL, &show_diffstat,\n+\t\tN_(\"do not show a diffstat at the end of the merge\"), 0),\n \tOPT_BOOL(0, \"stat\", &show_diffstat,\n \t\tN_(\"show a diffstat at the end of the merge\")),\n \tOPT_BOOL(0, \"summary\", &show_diffstat, N_(\"(synonym to --stat)\")),\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481260","messageId":"20230831211733.GC949469@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831211637.GA949188@coredump.intra.peff.net","subject":"[PATCH v2 03/10] format-patch: use OPT_STRING_LIST for to/cc options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T21:17:33Z","receivedAt":"2023-08-31T21:17:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The to_callback() and cc_callback() functions are identical to the\ngeneric parse_opt_string_list() function (except that they don't handle\noptional arguments, but that's OK because their callers do not use the\nOPTARG flag).\n\nLet's simplify the code by using OPT_STRING_LIST.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/log.c | 22 ++--------------------\n 1 file changed, 2 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex db3a88bfe9..fb90d43717 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1567,24 +1567,6 @@ static int header_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n-static int to_callback(const struct option *opt, const char *arg, int unset)\n-{\n-\tif (unset)\n-\t\tstring_list_clear(&extra_to, 0);\n-\telse\n-\t\tstring_list_append(&extra_to, arg);\n-\treturn 0;\n-}\n-\n-static int cc_callback(const struct option *opt, const char *arg, int unset)\n-{\n-\tif (unset)\n-\t\tstring_list_clear(&extra_cc, 0);\n-\telse\n-\t\tstring_list_append(&extra_cc, arg);\n-\treturn 0;\n-}\n-\n static int from_callback(const struct option *opt, const char *arg, int unset)\n {\n \tchar **from = opt->value;\n@@ -1957,8 +1939,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tOPT_GROUP(N_(\"Messaging\")),\n \t\tOPT_CALLBACK(0, \"add-header\", NULL, N_(\"header\"),\n \t\t\t    N_(\"add email header\"), header_callback),\n-\t\tOPT_CALLBACK(0, \"to\", NULL, N_(\"email\"), N_(\"add To: header\"), to_callback),\n-\t\tOPT_CALLBACK(0, \"cc\", NULL, N_(\"email\"), N_(\"add Cc: header\"), cc_callback),\n+\t\tOPT_STRING_LIST(0, \"to\", &extra_to, N_(\"email\"), N_(\"add To: header\")),\n+\t\tOPT_STRING_LIST(0, \"cc\", &extra_cc, N_(\"email\"), N_(\"add Cc: header\")),\n \t\tOPT_CALLBACK_F(0, \"from\", &from, N_(\"ident\"),\n \t\t\t    N_(\"set From address to <ident> (or committer ident if absent)\"),\n \t\t\t    PARSE_OPT_OPTARG, from_callback),\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481261","messageId":"20230831212051.GD949469@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831211637.GA949188@coredump.intra.peff.net","subject":"[PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T21:20:51Z","receivedAt":"2023-08-31T21:20:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Using --stage=all requires writing to tempfiles, since we cannot put\nmultiple stages into a single file. So --stage=all implies --temp.\n\nBut we do this by setting to_tempfile in the options callback for\n--stage, rather than after all options have been parsed. This leads to\ntwo bugs:\n\n  1. If you run \"checkout-index --stage=all --stage=2\", this should not\n     imply --temp, but it currently does. The callback cannot just unset\n     to_tempfile when it sees the \"2\" value, because it no longer knows\n     if its value was from the earlier --stage call, or if the user\n     specified --temp explicitly.\n\n  2. If you run \"checkout-index --stage=all --no-temp\", the --no-temp\n     will overwrite the earlier implied --temp. But this mode of\n     operation cannot work, and the command will fail with \"<path>\n     already exists\" when trying to write the higher stages.\n\nWe can fix both by lazily setting to_tempfile. We'll make it a tristate,\nwith -1 as \"not yet given\", and have --stage=all enable it only after\nall options are parsed. Likewise, after all options are parsed we can\ndetect and reject the bogus \"--no-temp\" case.\n\nNote that this does technically change the behavior for \"--stage=all\n--no-temp\" for paths which have only one stage present (which\naccidentally worked before, but is now forbidden). But this behavior was\nnever intended, and you'd have to go out of your way to try to trigger\nit.\n\nThe new tests cover both cases, as well the general \"--stage=all implies\n--temp\", as most of the other tests explicitly say \"--temp\". Ironically,\nthe test \"checkout --temp within subdir\" is the only one that _doesn't_\nuse \"--temp\", and so was implicitly covering this case. But it seems\nreasonable to have a more explicit test alongside the other related\nones.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOne other bug-let I noticed in this function: it only looks at the first\nbyte of the stage argument, so \"--stage=13\" is the same as \"--stage=1\".\nWe could tighten this, but I wonder if anybody does something weird like\n\"--stage=2ours\" or something. That seems pretty unlikely, but I'd still\nprefer to do it as a separate patch (if at all).\n\n builtin/checkout-index.c       |  8 ++++++--\n t/t2004-checkout-cache-temp.sh | 20 ++++++++++++++++++++\n 2 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex f62f13f2b5..6687a495ff 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -24,7 +24,7 @@\n static int nul_term_line;\n static int checkout_stage; /* default to checkout stage0 */\n static int ignore_skip_worktree; /* default to 0 */\n-static int to_tempfile;\n+static int to_tempfile = -1;\n static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];\n \n static struct checkout state = CHECKOUT_INIT;\n@@ -196,7 +196,6 @@ static int option_parse_stage(const struct option *opt,\n \tBUG_ON_OPT_NEG(unset);\n \n \tif (!strcmp(arg, \"all\")) {\n-\t\tto_tempfile = 1;\n \t\tcheckout_stage = CHECKOUT_ALL;\n \t} else {\n \t\tint ch = arg[0];\n@@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\tstate.base_dir = \"\";\n \tstate.base_dir_len = strlen(state.base_dir);\n \n+\tif (to_tempfile < 0)\n+\t\tto_tempfile = (checkout_stage == CHECKOUT_ALL);\n+\tif (!to_tempfile && checkout_stage == CHECKOUT_ALL)\n+\t\tdie(\"--stage=all and --no-temp are incompatible\");\n+\n \t/*\n \t * when --prefix is specified we do not want to update cache.\n \t */\ndiff --git a/t/t2004-checkout-cache-temp.sh b/t/t2004-checkout-cache-temp.sh\nindex b16d69ca4a..8eb2ef44eb 100755\n--- a/t/t2004-checkout-cache-temp.sh\n+++ b/t/t2004-checkout-cache-temp.sh\n@@ -117,6 +117,26 @@ test_expect_success 'checkout all stages/one file to temporary files' '\n \ttest $(cat $s3) = tree3path1)\n '\n \n+test_expect_success '--stage=all implies --temp' '\n+\trm -f path* .merge_* actual &&\n+\tgit checkout-index --stage=all -- path1 &&\n+\ttest_path_is_missing path1\n+'\n+\n+test_expect_success 'overriding --stage=all resets implied --temp' '\n+\trm -f path* .merge_* actual &&\n+\tgit checkout-index --stage=all --stage=2 -- path1 &&\n+\techo tree2path1 >expect &&\n+\ttest_cmp expect path1\n+'\n+\n+test_expect_success '--stage=all --no-temp is rejected' '\n+\trm -f path* .merge_* actual &&\n+\ttest_must_fail git checkout-index --stage=all --no-temp -- path1 2>err &&\n+\tgrep -v \"already exists\" err &&\n+\tgrep \"stage=all and --no-temp are incompatible\" err\n+'\n+\n test_expect_success 'checkout some stages/one file to temporary files' '\n \trm -f path* .merge_* actual &&\n \tgit checkout-index --stage=all --temp -- path2 >actual &&\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481262","messageId":"20230831212107.GE949469@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831211637.GA949188@coredump.intra.peff.net","subject":"[PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T21:21:07Z","receivedAt":"2023-08-31T21:21:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have several parse-options callbacks that ignore their \"opt\"\nparameters entirely. This is a little unusual, as we'd normally put the\nresult of the parsing into opt->value. In the case of these callbacks,\nthough, they directly manipulate global variables instead (and in\nmost cases the caller sets opt->value to NULL in the OPT_CALLBACK\ndeclaration).\n\nThe immediate symptom we'd like to deal with is that the unused \"opt\"\nvariables trigger -Wunused-parameter. But how to fix that is debatable.\nOne option is to annotate them with UNUSED. But another is to have the\ncaller pass in the appropriate variable via opt->value, and use it. That\nhas the benefit of making the callbacks reusable (in theory at least),\nand makes it clear from the OPT_CALLBACK declaration which variables\nwill be affected (doubly so for the cases in builtin/fast-export.c,\nwhere we do set opt->value, but it is completely ignored!).\n\nThe slight downside is that we lose type safety, since they're now\npassing through void pointers.\n\nI went with the \"just use them\" approach here. The loss of type safety\nis unfortunate, but that is already an issue with most of the other\ncallbacks. If we want to try to address that, we should do so more\nconsistently (and this patch would prepare these callbacks for whatever\nwe choose to do there).\n\nNote that in the cases in builtin/fast-export.c, we are passing\nanonymous enums. We'll have to give them names so that we can declare\nthe appropriate pointer type within the callbacks.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout-index.c     |  8 +++++---\n builtin/describe.c           |  6 ++++--\n builtin/fast-export.c        | 36 +++++++++++++++++++++---------------\n builtin/fetch.c              |  4 ++--\n builtin/interpret-trailers.c | 12 ++++++------\n builtin/pack-objects.c       | 21 ++++++++++++---------\n 6 files changed, 50 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 6687a495ff..6ef6ac4c2e 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -193,14 +193,16 @@ static const char * const builtin_checkout_index_usage[] = {\n static int option_parse_stage(const struct option *opt,\n \t\t\t      const char *arg, int unset)\n {\n+\tint *stage = opt->value;\n+\n \tBUG_ON_OPT_NEG(unset);\n \n \tif (!strcmp(arg, \"all\")) {\n-\t\tcheckout_stage = CHECKOUT_ALL;\n+\t\t*stage = CHECKOUT_ALL;\n \t} else {\n \t\tint ch = arg[0];\n \t\tif ('1' <= ch && ch <= '3')\n-\t\t\tcheckout_stage = arg[0] - '0';\n+\t\t\t*stage = arg[0] - '0';\n \t\telse\n \t\t\tdie(_(\"stage should be between 1 and 3 or all\"));\n \t}\n@@ -238,7 +240,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"write the content to temporary files\")),\n \t\tOPT_STRING(0, \"prefix\", &state.base_dir, N_(\"string\"),\n \t\t\tN_(\"when creating files, prepend <string>\")),\n-\t\tOPT_CALLBACK_F(0, \"stage\", NULL, \"(1|2|3|all)\",\n+\t\tOPT_CALLBACK_F(0, \"stage\", &checkout_stage, \"(1|2|3|all)\",\n \t\t\tN_(\"copy out the files from named stage\"),\n \t\t\tPARSE_OPT_NONEG, option_parse_stage),\n \t\tOPT_END()\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex b28a4a1f82..718b5c3073 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)\n static int option_parse_exact_match(const struct option *opt, const char *arg,\n \t\t\t\t    int unset)\n {\n+\tint *val = opt->value;\n+\n \tBUG_ON_OPT_ARG(arg);\n \n-\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n+\t*val = unset ? DEFAULT_CANDIDATES : 0;\n \treturn 0;\n }\n \n@@ -578,7 +580,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"long\",       &longformat, N_(\"always use long format\")),\n \t\tOPT_BOOL(0, \"first-parent\", &first_parent, N_(\"only follow first parent\")),\n \t\tOPT__ABBREV(&abbrev),\n-\t\tOPT_CALLBACK_F(0, \"exact-match\", NULL, NULL,\n+\t\tOPT_CALLBACK_F(0, \"exact-match\", &max_candidates, NULL,\n \t\t\t       N_(\"only output exact matches\"),\n \t\t\t       PARSE_OPT_NOARG, option_parse_exact_match),\n \t\tOPT_INTEGER(0, \"candidates\", &max_candidates,\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 56dc69fac1..70aff515ac 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -33,9 +33,9 @@ static const char *fast_export_usage[] = {\n };\n \n static int progress;\n-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;\n-static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;\n-static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;\n+static enum signed_tag_mode { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;\n+static enum tag_of_filtered_mode { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;\n+static enum reencode_mode { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;\n static int fake_missing_tagger;\n static int use_done_feature;\n static int no_data;\n@@ -53,16 +53,18 @@ static struct revision_sources revision_sources;\n static int parse_opt_signed_tag_mode(const struct option *opt,\n \t\t\t\t     const char *arg, int unset)\n {\n+\tenum signed_tag_mode *val = opt->value;\n+\n \tif (unset || !strcmp(arg, \"abort\"))\n-\t\tsigned_tag_mode = SIGNED_TAG_ABORT;\n+\t\t*val = SIGNED_TAG_ABORT;\n \telse if (!strcmp(arg, \"verbatim\") || !strcmp(arg, \"ignore\"))\n-\t\tsigned_tag_mode = VERBATIM;\n+\t\t*val = VERBATIM;\n \telse if (!strcmp(arg, \"warn\"))\n-\t\tsigned_tag_mode = WARN;\n+\t\t*val = WARN;\n \telse if (!strcmp(arg, \"warn-strip\"))\n-\t\tsigned_tag_mode = WARN_STRIP;\n+\t\t*val = WARN_STRIP;\n \telse if (!strcmp(arg, \"strip\"))\n-\t\tsigned_tag_mode = STRIP;\n+\t\t*val = STRIP;\n \telse\n \t\treturn error(\"Unknown signed-tags mode: %s\", arg);\n \treturn 0;\n@@ -71,12 +73,14 @@ static int parse_opt_signed_tag_mode(const struct option *opt,\n static int parse_opt_tag_of_filtered_mode(const struct option *opt,\n \t\t\t\t\t  const char *arg, int unset)\n {\n+\tenum tag_of_filtered_mode *val = opt->value;\n+\n \tif (unset || !strcmp(arg, \"abort\"))\n-\t\ttag_of_filtered_mode = TAG_FILTERING_ABORT;\n+\t\t*val = TAG_FILTERING_ABORT;\n \telse if (!strcmp(arg, \"drop\"))\n-\t\ttag_of_filtered_mode = DROP;\n+\t\t*val = DROP;\n \telse if (!strcmp(arg, \"rewrite\"))\n-\t\ttag_of_filtered_mode = REWRITE;\n+\t\t*val = REWRITE;\n \telse\n \t\treturn error(\"Unknown tag-of-filtered mode: %s\", arg);\n \treturn 0;\n@@ -85,21 +89,23 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,\n static int parse_opt_reencode_mode(const struct option *opt,\n \t\t\t\t   const char *arg, int unset)\n {\n+\tenum reencode_mode *val = opt->value;\n+\n \tif (unset) {\n-\t\treencode_mode = REENCODE_ABORT;\n+\t\t*val = REENCODE_ABORT;\n \t\treturn 0;\n \t}\n \n \tswitch (git_parse_maybe_bool(arg)) {\n \tcase 0:\n-\t\treencode_mode = REENCODE_NO;\n+\t\t*val = REENCODE_NO;\n \t\tbreak;\n \tcase 1:\n-\t\treencode_mode = REENCODE_YES;\n+\t\t*val = REENCODE_YES;\n \t\tbreak;\n \tdefault:\n \t\tif (!strcasecmp(arg, \"abort\"))\n-\t\t\treencode_mode = REENCODE_ABORT;\n+\t\t\t*val = REENCODE_ABORT;\n \t\telse\n \t\t\treturn error(\"Unknown reencoding mode: %s\", arg);\n \t}\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 8f93529505..fd134ba74d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -176,7 +176,7 @@ static int parse_refmap_arg(const struct option *opt, const char *arg, int unset\n \t * \"git fetch --refmap='' origin foo\"\n \t * can be used to tell the command not to store anywhere\n \t */\n-\trefspec_append(&refmap, arg);\n+\trefspec_append(opt->value, arg);\n \n \treturn 0;\n }\n@@ -2204,7 +2204,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t   PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules),\n \t\tOPT_BOOL(0, \"update-shallow\", &update_shallow,\n \t\t\t N_(\"accept refs that update .git/shallow\")),\n-\t\tOPT_CALLBACK_F(0, \"refmap\", NULL, N_(\"refmap\"),\n+\t\tOPT_CALLBACK_F(0, \"refmap\", &refmap, N_(\"refmap\"),\n \t\t\t       N_(\"specify fetch refmap\"), PARSE_OPT_NONEG, parse_refmap_arg),\n \t\tOPT_STRING_LIST('o', \"server-option\", &server_options, N_(\"server-specific\"), N_(\"option to transmit\")),\n \t\tOPT_IPVERSION(&family),\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex c5e8345265..6aadce6a1e 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -26,19 +26,19 @@ static enum trailer_if_missing if_missing;\n static int option_parse_where(const struct option *opt,\n \t\t\t      const char *arg, int unset)\n {\n-\treturn trailer_set_where(&where, arg);\n+\treturn trailer_set_where(opt->value, arg);\n }\n \n static int option_parse_if_exists(const struct option *opt,\n \t\t\t\t  const char *arg, int unset)\n {\n-\treturn trailer_set_if_exists(&if_exists, arg);\n+\treturn trailer_set_if_exists(opt->value, arg);\n }\n \n static int option_parse_if_missing(const struct option *opt,\n \t\t\t\t   const char *arg, int unset)\n {\n-\treturn trailer_set_if_missing(&if_missing, arg);\n+\treturn trailer_set_if_missing(opt->value, arg);\n }\n \n static void new_trailers_clear(struct list_head *trailers)\n@@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"in-place\", &opts.in_place, N_(\"edit files in place\")),\n \t\tOPT_BOOL(0, \"trim-empty\", &opts.trim_empty, N_(\"trim empty trailers\")),\n \n-\t\tOPT_CALLBACK(0, \"where\", NULL, N_(\"action\"),\n+\t\tOPT_CALLBACK(0, \"where\", &where, N_(\"action\"),\n \t\t\t     N_(\"where to place the new trailer\"), option_parse_where),\n-\t\tOPT_CALLBACK(0, \"if-exists\", NULL, N_(\"action\"),\n+\t\tOPT_CALLBACK(0, \"if-exists\", &if_exists, N_(\"action\"),\n \t\t\t     N_(\"action if trailer already exists\"), option_parse_if_exists),\n-\t\tOPT_CALLBACK(0, \"if-missing\", NULL, N_(\"action\"),\n+\t\tOPT_CALLBACK(0, \"if-missing\", &if_missing, N_(\"action\"),\n \t\t\t     N_(\"action if trailer is missing\"), option_parse_if_missing),\n \n \t\tOPT_BOOL(0, \"only-trailers\", &opts.only_trailers, N_(\"output only the trailers\")),\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d2a162d528..492372ee5d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4120,29 +4120,32 @@ static void add_extra_kept_packs(const struct string_list *names)\n static int option_parse_quiet(const struct option *opt, const char *arg,\n \t\t\t      int unset)\n {\n+\tint *val = opt->value;\n+\n \tBUG_ON_OPT_ARG(arg);\n \n \tif (!unset)\n-\t\tprogress = 0;\n-\telse if (!progress)\n-\t\tprogress = 1;\n+\t\t*val = 0;\n+\telse if (!*val)\n+\t\t*val = 1;\n \treturn 0;\n }\n \n static int option_parse_index_version(const struct option *opt,\n \t\t\t\t      const char *arg, int unset)\n {\n+\tstruct pack_idx_option *popts = opt->value;\n \tchar *c;\n \tconst char *val = arg;\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tpack_idx_opts.version = strtoul(val, &c, 10);\n-\tif (pack_idx_opts.version > 2)\n+\tpopts->version = strtoul(val, &c, 10);\n+\tif (popts->version > 2)\n \t\tdie(_(\"unsupported index version %s\"), val);\n \tif (*c == ',' && c[1])\n-\t\tpack_idx_opts.off32_limit = strtoul(c+1, &c, 0);\n-\tif (*c || pack_idx_opts.off32_limit & 0x80000000)\n+\t\tpopts->off32_limit = strtoul(c+1, &c, 0);\n+\tif (*c || popts->off32_limit & 0x80000000)\n \t\tdie(_(\"bad index version '%s'\"), val);\n \treturn 0;\n }\n@@ -4190,7 +4193,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tLIST_OBJECTS_FILTER_INIT;\n \n \tstruct option pack_objects_options[] = {\n-\t\tOPT_CALLBACK_F('q', \"quiet\", NULL, NULL,\n+\t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n \t\t\t       N_(\"do not show progress meter\"),\n \t\t\t       PARSE_OPT_NOARG, option_parse_quiet),\n \t\tOPT_SET_INT(0, \"progress\", &progress,\n@@ -4200,7 +4203,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"all-progress-implied\",\n \t\t\t &all_progress_implied,\n \t\t\t N_(\"similar to --all-progress when progress meter is shown\")),\n-\t\tOPT_CALLBACK_F(0, \"index-version\", NULL, N_(\"<version>[,<offset>]\"),\n+\t\tOPT_CALLBACK_F(0, \"index-version\", &pack_idx_opts, N_(\"<version>[,<offset>]\"),\n \t\t  N_(\"write the pack index file in the specified idx format version\"),\n \t\t  PARSE_OPT_NONEG, option_parse_index_version),\n \t\tOPT_MAGNITUDE(0, \"max-pack-size\", &pack_size_limit,\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481263","messageId":"20230831212128.GF949469@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831211637.GA949188@coredump.intra.peff.net","subject":"[PATCH v2 06/10] parse-options: mark unused \"opt\" parameter in callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T21:21:28Z","receivedAt":"2023-08-31T21:21:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit argued that parse-options callbacks should try to\nuse opt->value rather than touching globals directly. In some cases,\nhowever, that's awkward to do. Some callbacks touch multiple variables,\nor may even just call into an abstracted function that does so.\n\nIn some of these cases we _could_ convert them by stuffing the multiple\nvariables into a single struct and passing the struct pointer through\nopt->value. But that may make other parts of the code less readable,\nas the struct relationship has to be mentioned everywhere.\n\nLet's just accept that these cases are special and leave them as-is. But\nwe do need to mark their \"opt\" parameters to satisfy -Wunused-parameter.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/gc.c           |  2 +-\n builtin/log.c          | 10 ++++++----\n builtin/merge.c        |  2 +-\n builtin/pack-objects.c |  6 +++---\n builtin/read-tree.c    |  2 +-\n builtin/update-index.c |  4 ++--\n 6 files changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 369bd43fb2..b842349d86 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)\n \tstrbuf_release(&config_name);\n }\n \n-static int task_option_parse(const struct option *opt,\n+static int task_option_parse(const struct option *opt UNUSED,\n \t\t\t     const char *arg, int unset)\n {\n \tint i, num_selected = 0;\ndiff --git a/builtin/log.c b/builtin/log.c\nindex fb90d43717..3599063554 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -118,16 +118,17 @@ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;\n static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;\n static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;\n \n-static int clear_decorations_callback(const struct option *opt,\n-\t\t\t\t\t    const char *arg, int unset)\n+static int clear_decorations_callback(const struct option *opt UNUSED,\n+\t\t\t\t      const char *arg, int unset)\n {\n \tstring_list_clear(&decorate_refs_include, 0);\n \tstring_list_clear(&decorate_refs_exclude, 0);\n \tuse_default_decoration_filter = 0;\n \treturn 0;\n }\n \n-static int decorate_callback(const struct option *opt, const char *arg, int unset)\n+static int decorate_callback(const struct option *opt UNUSED, const char *arg,\n+\t\t\t     int unset)\n {\n \tif (unset)\n \t\tdecoration_style = 0;\n@@ -1555,7 +1556,8 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n-static int header_callback(const struct option *opt, const char *arg, int unset)\n+static int header_callback(const struct option *opt UNUSED, const char *arg,\n+\t\t\t   int unset)\n {\n \tif (unset) {\n \t\tstring_list_clear(&extra_hdr, 0);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 21363b7985..0436986dab 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -231,7 +231,7 @@ static void append_strategy(struct strategy *s)\n \tuse_strategies[use_strategies_nr++] = s;\n }\n \n-static int option_parse_strategy(const struct option *opt,\n+static int option_parse_strategy(const struct option *opt UNUSED,\n \t\t\t\t const char *name, int unset)\n {\n \tif (unset)\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 492372ee5d..91b4b7c177 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3739,7 +3739,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,\n \tshow_object(obj, name, data);\n }\n \n-static int option_parse_missing_action(const struct option *opt,\n+static int option_parse_missing_action(const struct option *opt UNUSED,\n \t\t\t\t       const char *arg, int unset)\n {\n \tassert(arg);\n@@ -4150,7 +4150,7 @@ static int option_parse_index_version(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_unpack_unreachable(const struct option *opt,\n+static int option_parse_unpack_unreachable(const struct option *opt UNUSED,\n \t\t\t\t\t   const char *arg, int unset)\n {\n \tif (unset) {\n@@ -4165,7 +4165,7 @@ static int option_parse_unpack_unreachable(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_cruft_expiration(const struct option *opt,\n+static int option_parse_cruft_expiration(const struct option *opt UNUSED,\n \t\t\t\t\t const char *arg, int unset)\n {\n \tif (unset) {\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 1fec702a04..8196ca9dd8 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -49,7 +49,7 @@ static const char * const read_tree_usage[] = {\n \tNULL\n };\n \n-static int index_output_cb(const struct option *opt, const char *arg,\n+static int index_output_cb(const struct option *opt UNUSED, const char *arg,\n \t\t\t\t int unset)\n {\n \tBUG_ON_OPT_NEG(unset);\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex aee3cb8cbd..59acae3336 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,\n \treturn 0;\n }\n \n-static int resolve_undo_clear_callback(const struct option *opt,\n+static int resolve_undo_clear_callback(const struct option *opt UNUSED,\n \t\t\t\tconst char *arg, int unset)\n {\n \tBUG_ON_OPT_NEG(unset);\n@@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,\n }\n \n static enum parse_opt_result cacheinfo_callback(\n-\tstruct parse_opt_ctx_t *ctx, const struct option *opt,\n+\tstruct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,\n \tconst char *arg, int unset)\n {\n \tstruct object_id oid;\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481264","messageId":"20230831212146.GG949469@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831211637.GA949188@coredump.intra.peff.net","subject":"[PATCH v2 07/10] merge: do not pass unused opt->value parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T21:21:46Z","receivedAt":"2023-08-31T21:21:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The option_parse_strategy() callback does not look at opt->value;\ninstead it calls append_strategy(), which manipulates the global\nuse_strategies array directly. But the OPT_CALLBACK declaration assigns\n\"&use_strategies\" to opt->value.\n\nOne could argue this is good, as it tells the reader what we generally\nexpect the callback to do. But it is also bad, because it can mislead\nyou into thinking that swapping out \"&use_strategies\" there might have\nany effect. Let's switch it to pass NULL (which is what every other\n\"does not bother to look at opt->value\" callback does). If you want to\nknow what the callback does, it's easy to read the function itself.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/merge.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 0436986dab..545da0c8a1 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -264,7 +264,7 @@ static struct option builtin_merge_options[] = {\n \tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n \tOPT_BOOL(0, \"verify-signatures\", &verify_signatures,\n \t\tN_(\"verify that the named commit has a valid GPG signature\")),\n-\tOPT_CALLBACK('s', \"strategy\", &use_strategies, N_(\"strategy\"),\n+\tOPT_CALLBACK('s', \"strategy\", NULL, N_(\"strategy\"),\n \t\tN_(\"merge strategy to use\"), option_parse_strategy),\n \tOPT_STRVEC('X', \"strategy-option\", &xopts, N_(\"option=value\"),\n \t\tN_(\"option for selected merge strategy\")),\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481265","messageId":"20230831212149.GH949469@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831211637.GA949188@coredump.intra.peff.net","subject":"[PATCH v2 08/10] parse-options: add more BUG_ON() annotations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T21:21:49Z","receivedAt":"2023-08-31T21:21:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These callbacks are similar to the ones touched by 517fe807d6 (assert\nNOARG/NONEG behavior of parse-options callbacks, 2018-11-05), but were\neither missed in that commit (the one in add.c) or were added later (the\none in log.c).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/add.c | 2 ++\n builtin/log.c | 2 ++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 4b0dd798df..cf59108523 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -232,6 +232,8 @@ static char *chmod_arg;\n \n static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)\n {\n+\tBUG_ON_OPT_ARG(arg);\n+\n \t/* if we are told to ignore, we are not adding removals */\n \t*(int *)opt->value = !unset ? 0 : 1;\n \treturn 0;\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 3599063554..190e1952e9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -121,6 +121,8 @@ static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;\n static int clear_decorations_callback(const struct option *opt UNUSED,\n \t\t\t\t      const char *arg, int unset)\n {\n+\tBUG_ON_OPT_NEG(unset);\n+\tBUG_ON_OPT_ARG(arg);\n \tstring_list_clear(&decorate_refs_include, 0);\n \tstring_list_clear(&decorate_refs_exclude, 0);\n \tuse_default_decoration_filter = 0;\n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481266","messageId":"20230831212215.GI949469@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831211637.GA949188@coredump.intra.peff.net","subject":"[PATCH v2 09/10] interpret-trailers: mark unused \"unset\" parameters in option callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T21:22:15Z","receivedAt":"2023-08-31T21:23:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are a few parse-option callbacks that do not look at their \"unset\"\nparameters, but also do not set PARSE_OPT_NONEG. At first glance this\nseems like a bug, as we'd ignore \"--no-if-exists\", etc.\n\nBut they do work fine, because when \"unset\" is true, then \"arg\" is NULL.\nAnd all three functions pass \"arg\" on to helper functions which do the\nright thing with the NULL.\n\nNote that this shortcut would not be correct if any callback used\nPARSE_OPT_NOARG (in which case \"arg\" would be NULL but \"unset\" would be\nfalse). But none of these do.\n\nSo the code is fine as-is. But we'll want to mark the unused \"unset\"\nparameters to quiet -Wunused-parameter. I've also added a comment to\nmake this rather subtle situation more explicit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI looked at another BUG_ON here, but it was just so ugly and one-off\nthat I preferred just adding the comments.\n\n builtin/interpret-trailers.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 6aadce6a1e..a110e69f83 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -24,20 +24,23 @@ static enum trailer_if_exists if_exists;\n static enum trailer_if_missing if_missing;\n \n static int option_parse_where(const struct option *opt,\n-\t\t\t      const char *arg, int unset)\n+\t\t\t      const char *arg, int unset UNUSED)\n {\n+\t/* unset implies NULL arg, which is handled in our helper */\n \treturn trailer_set_where(opt->value, arg);\n }\n \n static int option_parse_if_exists(const struct option *opt,\n-\t\t\t\t  const char *arg, int unset)\n+\t\t\t\t  const char *arg, int unset UNUSED)\n {\n+\t/* unset implies NULL arg, which is handled in our helper */\n \treturn trailer_set_if_exists(opt->value, arg);\n }\n \n static int option_parse_if_missing(const struct option *opt,\n-\t\t\t\t   const char *arg, int unset)\n+\t\t\t\t   const char *arg, int unset UNUSED)\n {\n+\t/* unset implies NULL arg, which is handled in our helper */\n \treturn trailer_set_if_missing(opt->value, arg);\n }\n \n-- \n2.42.0.561.gaa987ecc69\n\n"},{"id":"481267","messageId":"20230831212220.GJ949469@coredump.intra.peff.net","threadId":"60176","inReplyTo":"20230831211637.GA949188@coredump.intra.peff.net","subject":"[PATCH v2 10/10] parse-options: mark unused parameters in noop callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T21:22:20Z","receivedAt":"2023-08-31T21:23:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Unsurprisingly, the noop options callback doesn't bother to look at any\nof its parameters. Let's mark them so that -Wunused-parameter does not\ncomplain.\n\nAnother option would be to drop the callback and have parse-options\nitself recognize OPT_NOOP_NOARG. But that seems like extra work for no\nreal benefit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n parse-options-cb.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex a24521dee0..bdc7fae497 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n-int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)\n+int parse_opt_noop_cb(const struct option *opt UNUSED,\n+\t\t      const char *arg UNUSED,\n+\t\t      int unset UNUSED)\n {\n \treturn 0;\n }\n-- \n2.42.0.561.gaa987ecc69\n"},{"id":"481272","messageId":"xmqqr0nisvxt.fsf@gitster.g","threadId":"60176","inReplyTo":"20230831212051.GD949469@coredump.intra.peff.net","subject":"Re: [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T22:12:30Z","receivedAt":"2023-08-31T22:12:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Using --stage=all requires writing to tempfiles, since we cannot put\n> multiple stages into a single file. So --stage=all implies --temp.\n\nThis one is new, and it does read really well.  Thanks.\n"},{"id":"481320","messageId":"75100a68-78d1-b22c-0497-36548c518b7b@web.de","threadId":"60176","inReplyTo":"20230831211716.GB949469@coredump.intra.peff.net","subject":"Re: [PATCH v2 02/10] merge: simplify parsing of \"-n\" option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-02T06:20:28Z","receivedAt":"2023-09-02T06:20:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.08.23 um 23:17 schrieb Jeff King:\n> The \"-n\" option is implemented by an option callback, as it is really a\n> \"reverse bool\". If given, it sets show_diffstat to 0. In theory, when\n> negated, it would set the same flag to 1. But it's not possible to\n> trigger that, since short options cannot be negated.\n>\n> So in practice this is really just a SET_INT to 0. Let's use that\n> instead, which shortens the code.\n>\n> Note that negation here would do the wrong thing (as with any SET_INT\n> with a value of \"0\"). We could specify PARSE_OPT_NONEG to future-proof\n> ourselves against somebody adding a long option name (which would make\n> it possible to negate). But there's not much point:\n>\n>   1. Nobody is going to do that, because the negated form already\n>      exists, and is called \"--stat\" (which is defined separately so that\n>      \"--no-stat\" works).\n>\n>   2. If they did, the BUG() check added by 3284b93862 (parse-options:\n>      disallow negating OPTION_SET_INT 0, 2023-08-08) will catch it (and\n>      that check is smart enough to realize that our short-only option is\n>      OK).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/merge.c | 13 ++-----------\n>  1 file changed, 2 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 53e9fe70d9..21363b7985 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -241,18 +241,9 @@ static int option_parse_strategy(const struct option *opt,\n>  \treturn 0;\n>  }\n>\n> -static int option_parse_n(const struct option *opt,\n> -\t\t\t  const char *arg, int unset)\n> -{\n> -\tBUG_ON_OPT_ARG(arg);\n> -\tshow_diffstat = unset;\n> -\treturn 0;\n> -}\n\nGreat -- the less custom callbacks, the better.\n\n> -\n>  static struct option builtin_merge_options[] = {\n> -\tOPT_CALLBACK_F('n', NULL, NULL, NULL,\n> -\t\tN_(\"do not show a diffstat at the end of the merge\"),\n> -\t\tPARSE_OPT_NOARG, option_parse_n),\n> +\tOPT_SET_INT('n', NULL, &show_diffstat,\n> +\t\tN_(\"do not show a diffstat at the end of the merge\"), 0),\n>  \tOPT_BOOL(0, \"stat\", &show_diffstat,\n>  \t\tN_(\"show a diffstat at the end of the merge\")),\n\nMakes it easier to see that we can replace the two complementary\ndefinitions with a single one:\n\n\tOPT_NEGBIT('n', \"no-stat\",\n\t\tN_(\"do not show a diffstat at the end of the merge\"), 1),\n\nWhich is a separate topic, of course.  And if we did that, however, ...\n\n>  \tOPT_BOOL(0, \"summary\", &show_diffstat, N_(\"(synonym to --stat)\")),\n\n... we wouldn't be able to use OPT_ALIAS here anymore (which we arguably\nshould) because it doesn't allow referencing the negated (or\n\"positivated\") form -- but that is yet another separate topic.\n\nRené\n"},{"id":"481321","messageId":"c7855b08-46ee-5df0-4b0f-67ea57d84b18@web.de","threadId":"60176","inReplyTo":"20230831212051.GD949469@coredump.intra.peff.net","subject":"Re: [PATCH v2 04/10] checkout-index: delay automatic setting of to_tempfile","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-02T06:20:43Z","receivedAt":"2023-09-02T06:20:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.08.23 um 23:20 schrieb Jeff King:\n> @@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n>  \t\tstate.base_dir = \"\";\n>  \tstate.base_dir_len = strlen(state.base_dir);\n>\n> +\tif (to_tempfile < 0)\n> +\t\tto_tempfile = (checkout_stage == CHECKOUT_ALL);\n> +\tif (!to_tempfile && checkout_stage == CHECKOUT_ALL)\n> +\t\tdie(\"--stage=all and --no-temp are incompatible\");\n\nHow about making this message translatable from the start and following\nthe convention from 12909b6b8a (i18n: turn \"options are incompatible\"\ninto \"cannot be used together\", 2022-01-05) to reuse the existing\ntranslations?\n\nRené\n"},{"id":"481323","messageId":"8f03fa13-11b3-1f9f-1e1f-8f7d8ce74a23@web.de","threadId":"60176","inReplyTo":"20230831212107.GE949469@coredump.intra.peff.net","subject":"Re: [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-02T07:34:32Z","receivedAt":"2023-09-02T07:34:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.08.23 um 23:21 schrieb Jeff King:\n> We have several parse-options callbacks that ignore their \"opt\"\n> parameters entirely. This is a little unusual, as we'd normally put the\n> result of the parsing into opt->value. In the case of these callbacks,\n> though, they directly manipulate global variables instead (and in\n> most cases the caller sets opt->value to NULL in the OPT_CALLBACK\n> declaration).\n>\n> The immediate symptom we'd like to deal with is that the unused \"opt\"\n> variables trigger -Wunused-parameter. But how to fix that is debatable.\n> One option is to annotate them with UNUSED. But another is to have the\n> caller pass in the appropriate variable via opt->value, and use it. That\n> has the benefit of making the callbacks reusable (in theory at least),\n> and makes it clear from the OPT_CALLBACK declaration which variables\n> will be affected (doubly so for the cases in builtin/fast-export.c,\n> where we do set opt->value, but it is completely ignored!).\n\nWhich allows turning global variables into local ones later, so unlocks\nmore cleanup potential.\n\n> The slight downside is that we lose type safety, since they're now\n> passing through void pointers.\n>\n> I went with the \"just use them\" approach here. The loss of type safety\n> is unfortunate, but that is already an issue with most of the other\n> callbacks. If we want to try to address that, we should do so more\n> consistently (and this patch would prepare these callbacks for whatever\n> we choose to do there).\n\nMakes sense.  The types used below look OK to me.\n\n> Note that in the cases in builtin/fast-export.c, we are passing\n> anonymous enums. We'll have to give them names so that we can declare\n> the appropriate pointer type within the callbacks.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/checkout-index.c     |  8 +++++---\n>  builtin/describe.c           |  6 ++++--\n>  builtin/fast-export.c        | 36 +++++++++++++++++++++---------------\n>  builtin/fetch.c              |  4 ++--\n>  builtin/interpret-trailers.c | 12 ++++++------\n>  builtin/pack-objects.c       | 21 ++++++++++++---------\n>  6 files changed, 50 insertions(+), 37 deletions(-)\n>\n> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\n> index 6687a495ff..6ef6ac4c2e 100644\n> --- a/builtin/checkout-index.c\n> +++ b/builtin/checkout-index.c\n> @@ -193,14 +193,16 @@ static const char * const builtin_checkout_index_usage[] = {\n>  static int option_parse_stage(const struct option *opt,\n>  \t\t\t      const char *arg, int unset)\n>  {\n> +\tint *stage = opt->value;\n> +\n>  \tBUG_ON_OPT_NEG(unset);\n>\n>  \tif (!strcmp(arg, \"all\")) {\n> -\t\tcheckout_stage = CHECKOUT_ALL;\n> +\t\t*stage = CHECKOUT_ALL;\n>  \t} else {\n>  \t\tint ch = arg[0];\n>  \t\tif ('1' <= ch && ch <= '3')\n> -\t\t\tcheckout_stage = arg[0] - '0';\n> +\t\t\t*stage = arg[0] - '0';\n>  \t\telse\n>  \t\t\tdie(_(\"stage should be between 1 and 3 or all\"));\n>  \t}\n> @@ -238,7 +240,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n>  \t\t\tN_(\"write the content to temporary files\")),\n>  \t\tOPT_STRING(0, \"prefix\", &state.base_dir, N_(\"string\"),\n>  \t\t\tN_(\"when creating files, prepend <string>\")),\n> -\t\tOPT_CALLBACK_F(0, \"stage\", NULL, \"(1|2|3|all)\",\n> +\t\tOPT_CALLBACK_F(0, \"stage\", &checkout_stage, \"(1|2|3|all)\",\n>  \t\t\tN_(\"copy out the files from named stage\"),\n>  \t\t\tPARSE_OPT_NONEG, option_parse_stage),\n>  \t\tOPT_END()\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index b28a4a1f82..718b5c3073 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -561,9 +561,11 @@ static void describe(const char *arg, int last_one)\n>  static int option_parse_exact_match(const struct option *opt, const char *arg,\n>  \t\t\t\t    int unset)\n>  {\n> +\tint *val = opt->value;\n> +\n>  \tBUG_ON_OPT_ARG(arg);\n>\n> -\tmax_candidates = unset ? DEFAULT_CANDIDATES : 0;\n> +\t*val = unset ? DEFAULT_CANDIDATES : 0;\n>  \treturn 0;\n>  }\n>\n> @@ -578,7 +580,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_BOOL(0, \"long\",       &longformat, N_(\"always use long format\")),\n>  \t\tOPT_BOOL(0, \"first-parent\", &first_parent, N_(\"only follow first parent\")),\n>  \t\tOPT__ABBREV(&abbrev),\n> -\t\tOPT_CALLBACK_F(0, \"exact-match\", NULL, NULL,\n> +\t\tOPT_CALLBACK_F(0, \"exact-match\", &max_candidates, NULL,\n>  \t\t\t       N_(\"only output exact matches\"),\n>  \t\t\t       PARSE_OPT_NOARG, option_parse_exact_match),\n>  \t\tOPT_INTEGER(0, \"candidates\", &max_candidates,\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 56dc69fac1..70aff515ac 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -33,9 +33,9 @@ static const char *fast_export_usage[] = {\n>  };\n>\n>  static int progress;\n> -static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;\n> -static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;\n> -static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;\n> +static enum signed_tag_mode { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;\n> +static enum tag_of_filtered_mode { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;\n> +static enum reencode_mode { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;\n>  static int fake_missing_tagger;\n>  static int use_done_feature;\n>  static int no_data;\n> @@ -53,16 +53,18 @@ static struct revision_sources revision_sources;\n>  static int parse_opt_signed_tag_mode(const struct option *opt,\n>  \t\t\t\t     const char *arg, int unset)\n>  {\n> +\tenum signed_tag_mode *val = opt->value;\n> +\n>  \tif (unset || !strcmp(arg, \"abort\"))\n> -\t\tsigned_tag_mode = SIGNED_TAG_ABORT;\n> +\t\t*val = SIGNED_TAG_ABORT;\n>  \telse if (!strcmp(arg, \"verbatim\") || !strcmp(arg, \"ignore\"))\n> -\t\tsigned_tag_mode = VERBATIM;\n> +\t\t*val = VERBATIM;\n>  \telse if (!strcmp(arg, \"warn\"))\n> -\t\tsigned_tag_mode = WARN;\n> +\t\t*val = WARN;\n>  \telse if (!strcmp(arg, \"warn-strip\"))\n> -\t\tsigned_tag_mode = WARN_STRIP;\n> +\t\t*val = WARN_STRIP;\n>  \telse if (!strcmp(arg, \"strip\"))\n> -\t\tsigned_tag_mode = STRIP;\n> +\t\t*val = STRIP;\n>  \telse\n>  \t\treturn error(\"Unknown signed-tags mode: %s\", arg);\n>  \treturn 0;\n> @@ -71,12 +73,14 @@ static int parse_opt_signed_tag_mode(const struct option *opt,\n>  static int parse_opt_tag_of_filtered_mode(const struct option *opt,\n>  \t\t\t\t\t  const char *arg, int unset)\n>  {\n> +\tenum tag_of_filtered_mode *val = opt->value;\n> +\n>  \tif (unset || !strcmp(arg, \"abort\"))\n> -\t\ttag_of_filtered_mode = TAG_FILTERING_ABORT;\n> +\t\t*val = TAG_FILTERING_ABORT;\n>  \telse if (!strcmp(arg, \"drop\"))\n> -\t\ttag_of_filtered_mode = DROP;\n> +\t\t*val = DROP;\n>  \telse if (!strcmp(arg, \"rewrite\"))\n> -\t\ttag_of_filtered_mode = REWRITE;\n> +\t\t*val = REWRITE;\n>  \telse\n>  \t\treturn error(\"Unknown tag-of-filtered mode: %s\", arg);\n>  \treturn 0;\n> @@ -85,21 +89,23 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,\n>  static int parse_opt_reencode_mode(const struct option *opt,\n>  \t\t\t\t   const char *arg, int unset)\n>  {\n> +\tenum reencode_mode *val = opt->value;\n> +\n>  \tif (unset) {\n> -\t\treencode_mode = REENCODE_ABORT;\n> +\t\t*val = REENCODE_ABORT;\n>  \t\treturn 0;\n>  \t}\n>\n>  \tswitch (git_parse_maybe_bool(arg)) {\n>  \tcase 0:\n> -\t\treencode_mode = REENCODE_NO;\n> +\t\t*val = REENCODE_NO;\n>  \t\tbreak;\n>  \tcase 1:\n> -\t\treencode_mode = REENCODE_YES;\n> +\t\t*val = REENCODE_YES;\n>  \t\tbreak;\n>  \tdefault:\n>  \t\tif (!strcasecmp(arg, \"abort\"))\n> -\t\t\treencode_mode = REENCODE_ABORT;\n> +\t\t\t*val = REENCODE_ABORT;\n>  \t\telse\n>  \t\t\treturn error(\"Unknown reencoding mode: %s\", arg);\n>  \t}\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 8f93529505..fd134ba74d 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -176,7 +176,7 @@ static int parse_refmap_arg(const struct option *opt, const char *arg, int unset\n>  \t * \"git fetch --refmap='' origin foo\"\n>  \t * can be used to tell the command not to store anywhere\n>  \t */\n> -\trefspec_append(&refmap, arg);\n> +\trefspec_append(opt->value, arg);\n>\n>  \treturn 0;\n>  }\n> @@ -2204,7 +2204,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>  \t\t\t   PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules),\n>  \t\tOPT_BOOL(0, \"update-shallow\", &update_shallow,\n>  \t\t\t N_(\"accept refs that update .git/shallow\")),\n> -\t\tOPT_CALLBACK_F(0, \"refmap\", NULL, N_(\"refmap\"),\n> +\t\tOPT_CALLBACK_F(0, \"refmap\", &refmap, N_(\"refmap\"),\n>  \t\t\t       N_(\"specify fetch refmap\"), PARSE_OPT_NONEG, parse_refmap_arg),\n>  \t\tOPT_STRING_LIST('o', \"server-option\", &server_options, N_(\"server-specific\"), N_(\"option to transmit\")),\n>  \t\tOPT_IPVERSION(&family),\n> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> index c5e8345265..6aadce6a1e 100644\n> --- a/builtin/interpret-trailers.c\n> +++ b/builtin/interpret-trailers.c\n> @@ -26,19 +26,19 @@ static enum trailer_if_missing if_missing;\n>  static int option_parse_where(const struct option *opt,\n>  \t\t\t      const char *arg, int unset)\n>  {\n> -\treturn trailer_set_where(&where, arg);\n> +\treturn trailer_set_where(opt->value, arg);\n>  }\n>\n>  static int option_parse_if_exists(const struct option *opt,\n>  \t\t\t\t  const char *arg, int unset)\n>  {\n> -\treturn trailer_set_if_exists(&if_exists, arg);\n> +\treturn trailer_set_if_exists(opt->value, arg);\n>  }\n>\n>  static int option_parse_if_missing(const struct option *opt,\n>  \t\t\t\t   const char *arg, int unset)\n>  {\n> -\treturn trailer_set_if_missing(&if_missing, arg);\n> +\treturn trailer_set_if_missing(opt->value, arg);\n>  }\n\nNot your fault, but these all silently exit if \"arg\" contains an\nunrecognized value.  Reporting the error would be better.\n\n>  static void new_trailers_clear(struct list_head *trailers)\n> @@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_BOOL(0, \"in-place\", &opts.in_place, N_(\"edit files in place\")),\n>  \t\tOPT_BOOL(0, \"trim-empty\", &opts.trim_empty, N_(\"trim empty trailers\")),\n>\n> -\t\tOPT_CALLBACK(0, \"where\", NULL, N_(\"action\"),\n> +\t\tOPT_CALLBACK(0, \"where\", &where, N_(\"action\"),\n>  \t\t\t     N_(\"where to place the new trailer\"), option_parse_where),\n> -\t\tOPT_CALLBACK(0, \"if-exists\", NULL, N_(\"action\"),\n> +\t\tOPT_CALLBACK(0, \"if-exists\", &if_exists, N_(\"action\"),\n>  \t\t\t     N_(\"action if trailer already exists\"), option_parse_if_exists),\n> -\t\tOPT_CALLBACK(0, \"if-missing\", NULL, N_(\"action\"),\n> +\t\tOPT_CALLBACK(0, \"if-missing\", &if_missing, N_(\"action\"),\n>  \t\t\t     N_(\"action if trailer is missing\"), option_parse_if_missing),\n\nAnd I wonder if \"action\" should be replaced by \"(after|before|end|start)\",\n\"(addIfDifferent|addIfDifferentNeighbor|add|replace|doNothing)\" and\n\"(doNothing|add)\", respectively.  Gets a bit long in the middle, but would\nbe more helpful.  #leftoverbits\n\n>  \t\tOPT_BOOL(0, \"only-trailers\", &opts.only_trailers, N_(\"output only the trailers\")),\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index d2a162d528..492372ee5d 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -4120,29 +4120,32 @@ static void add_extra_kept_packs(const struct string_list *names)\n>  static int option_parse_quiet(const struct option *opt, const char *arg,\n>  \t\t\t      int unset)\n>  {\n> +\tint *val = opt->value;\n> +\n>  \tBUG_ON_OPT_ARG(arg);\n>\n>  \tif (!unset)\n> -\t\tprogress = 0;\n> -\telse if (!progress)\n> -\t\tprogress = 1;\n> +\t\t*val = 0;\n> +\telse if (!*val)\n> +\t\t*val = 1;\n>  \treturn 0;\n>  }\n>\n>  static int option_parse_index_version(const struct option *opt,\n>  \t\t\t\t      const char *arg, int unset)\n>  {\n> +\tstruct pack_idx_option *popts = opt->value;\n>  \tchar *c;\n>  \tconst char *val = arg;\n>\n>  \tBUG_ON_OPT_NEG(unset);\n>\n> -\tpack_idx_opts.version = strtoul(val, &c, 10);\n> -\tif (pack_idx_opts.version > 2)\n> +\tpopts->version = strtoul(val, &c, 10);\n> +\tif (popts->version > 2)\n>  \t\tdie(_(\"unsupported index version %s\"), val);\n>  \tif (*c == ',' && c[1])\n> -\t\tpack_idx_opts.off32_limit = strtoul(c+1, &c, 0);\n> -\tif (*c || pack_idx_opts.off32_limit & 0x80000000)\n> +\t\tpopts->off32_limit = strtoul(c+1, &c, 0);\n> +\tif (*c || popts->off32_limit & 0x80000000)\n>  \t\tdie(_(\"bad index version '%s'\"), val);\n>  \treturn 0;\n>  }\n> @@ -4190,7 +4193,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>  \t\tLIST_OBJECTS_FILTER_INIT;\n>\n>  \tstruct option pack_objects_options[] = {\n> -\t\tOPT_CALLBACK_F('q', \"quiet\", NULL, NULL,\n> +\t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n>  \t\t\t       N_(\"do not show progress meter\"),\n>  \t\t\t       PARSE_OPT_NOARG, option_parse_quiet),\n>  \t\tOPT_SET_INT(0, \"progress\", &progress,\n> @@ -4200,7 +4203,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_BOOL(0, \"all-progress-implied\",\n>  \t\t\t &all_progress_implied,\n>  \t\t\t N_(\"similar to --all-progress when progress meter is shown\")),\n> -\t\tOPT_CALLBACK_F(0, \"index-version\", NULL, N_(\"<version>[,<offset>]\"),\n> +\t\tOPT_CALLBACK_F(0, \"index-version\", &pack_idx_opts, N_(\"<version>[,<offset>]\"),\n>  \t\t  N_(\"write the pack index file in the specified idx format version\"),\n>  \t\t  PARSE_OPT_NONEG, option_parse_index_version),\n>  \t\tOPT_MAGNITUDE(0, \"max-pack-size\", &pack_size_limit,\n"},{"id":"481326","messageId":"98d1cd21-fb2a-269a-8d0b-f3e050682739@web.de","threadId":"60176","inReplyTo":"20230831212128.GF949469@coredump.intra.peff.net","subject":"Re: [PATCH v2 06/10] parse-options: mark unused \"opt\" parameter in callbacks","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-02T10:12:56Z","receivedAt":"2023-09-02T10:13:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.08.23 um 23:21 schrieb Jeff King:\n> The previous commit argued that parse-options callbacks should try to\n> use opt->value rather than touching globals directly. In some cases,\n> however, that's awkward to do. Some callbacks touch multiple variables,\n> or may even just call into an abstracted function that does so.\n>\n> In some of these cases we _could_ convert them by stuffing the multiple\n> variables into a single struct and passing the struct pointer through\n> opt->value. But that may make other parts of the code less readable,\n> as the struct relationship has to be mentioned everywhere.\n\nDoes that imply you'd be willing to use other methods?  Let's find out\nbelow. :)\n\n>\n> Let's just accept that these cases are special and leave them as-is. But\n> we do need to mark their \"opt\" parameters to satisfy -Wunused-parameter.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/gc.c           |  2 +-\n>  builtin/log.c          | 10 ++++++----\n>  builtin/merge.c        |  2 +-\n>  builtin/pack-objects.c |  6 +++---\n>  builtin/read-tree.c    |  2 +-\n>  builtin/update-index.c |  4 ++--\n>  6 files changed, 14 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 369bd43fb2..b842349d86 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)\n>  \tstrbuf_release(&config_name);\n>  }\n>\n> -static int task_option_parse(const struct option *opt,\n> +static int task_option_parse(const struct option *opt UNUSED,\n\nOnly the global variable \"tasks\" seems to be used in here if you don't\ncount the constant \"TASK__COUNT\", so you could pass it in.  This could\nalso be converted to OPT_STRING_LIST with parsing and duplicate checking\ndone later.\n\nAnd I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION)\non error, but that's a different matter.\n\n>  \t\t\t     const char *arg, int unset)\n>  {\n>  \tint i, num_selected = 0;\n> diff --git a/builtin/log.c b/builtin/log.c\n> index fb90d43717..3599063554 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -118,16 +118,17 @@ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;\n>  static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;\n>  static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;\n>\n> -static int clear_decorations_callback(const struct option *opt,\n> -\t\t\t\t\t    const char *arg, int unset)\n> +static int clear_decorations_callback(const struct option *opt UNUSED,\n> +\t\t\t\t      const char *arg, int unset)\n>  {\n>  \tstring_list_clear(&decorate_refs_include, 0);\n>  \tstring_list_clear(&decorate_refs_exclude, 0);\n>  \tuse_default_decoration_filter = 0;\n>  \treturn 0;\n>  }\n>\n\nMeta: Why do we get seven lines of context in an -U3 patch here?  Did\nyou use --inter-hunk-context?\n\nThis patch would be better viewed with --function-context to see that\nthe callbacks change multiple variables or do other funky things.  Only\ndoubles the line count.\n\n> -static int decorate_callback(const struct option *opt, const char *arg, int unset)\n> +static int decorate_callback(const struct option *opt UNUSED, const char *arg,\n> +\t\t\t     int unset)\n>  {\n>  \tif (unset)\n>  \t\tdecoration_style = 0;\n\nSets decoration_style and decoration_given.  Never sets decoration_given\nto a negative value.  This could be used to initialize decoration_style\nto -1 and set decoration_given after parsing, then you could pass in\njust decoration_style.\n\n> @@ -1555,7 +1556,8 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)\n>  \treturn 0;\n>  }\n>\n> -static int header_callback(const struct option *opt, const char *arg, int unset)\n> +static int header_callback(const struct option *opt UNUSED, const char *arg,\n\nSorts strings into three string_lists based on prefix.  Could become a\nvanilla OPT_STRING_LIST with parsing and sorting done later, if it\nwouldn't interact with to_callback() and cc_callback().\n\n(--function-context is not enough to see that, it requires a look at the\ndefinition of those other functions as well.)\n\n> +\t\t\t   int unset)\n>  {\n>  \tif (unset) {\n>  \t\tstring_list_clear(&extra_hdr, 0);\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 21363b7985..0436986dab 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -231,7 +231,7 @@ static void append_strategy(struct strategy *s)\n>  \tuse_strategies[use_strategies_nr++] = s;\n>  }\n>\n> -static int option_parse_strategy(const struct option *opt,\n> +static int option_parse_strategy(const struct option *opt UNUSED,\n\nCould be an OPT_STRING_LIST and parsing done later.  Except that\n--no-strategy does nothing, which is weird.\n\n>  \t\t\t\t const char *name, int unset)\n>  {\n>  \tif (unset)\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 492372ee5d..91b4b7c177 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3739,7 +3739,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,\n>  \tshow_object(obj, name, data);\n>  }\n>\n> -static int option_parse_missing_action(const struct option *opt,\n> +static int option_parse_missing_action(const struct option *opt UNUSED,\n\nSets the enum arg_missing_action, fetch_if_missing and fn_show_object.\nCould be changed to set only the enum and then you could pass it in.\n\n>  \t\t\t\t       const char *arg, int unset)\n>  {\n>  \tassert(arg);\n> @@ -4150,7 +4150,7 @@ static int option_parse_index_version(const struct option *opt,\n>  \treturn 0;\n>  }\n>\n> -static int option_parse_unpack_unreachable(const struct option *opt,\n> +static int option_parse_unpack_unreachable(const struct option *opt UNUSED,\n\nSets both unpack_unreachable and unpack_unreachable_expiration.  Perhaps\ncould set only unpack_unreachable_expiration and use TIME_MAX if no\nargument is given, then set unpack_unreachable after parsing?  Not sure.\n\n>  \t\t\t\t\t   const char *arg, int unset)\n>  {\n>  \tif (unset) {\n> @@ -4165,7 +4165,7 @@ static int option_parse_unpack_unreachable(const struct option *opt,\n>  \treturn 0;\n>  }\n>\n> -static int option_parse_cruft_expiration(const struct option *opt,\n> +static int option_parse_cruft_expiration(const struct option *opt UNUSED,\n\nDoes the same as option_parse_unpack_unreachable(), just with two\ndifferent variables.  And interacts with --cruft.\n\n>  \t\t\t\t\t const char *arg, int unset)\n>  {\n>  \tif (unset) {\n> diff --git a/builtin/read-tree.c b/builtin/read-tree.c\n> index 1fec702a04..8196ca9dd8 100644\n> --- a/builtin/read-tree.c\n> +++ b/builtin/read-tree.c\n> @@ -49,7 +49,7 @@ static const char * const read_tree_usage[] = {\n>  \tNULL\n>  };\n>\n> -static int index_output_cb(const struct option *opt, const char *arg,\n> +static int index_output_cb(const struct option *opt UNUSED, const char *arg,\n\nCalls set_alternate_index_output().  Could become an OPT_STRING_F if we\nassume that the last value wins and earlier calls have no side effects.\n\n>  \t\t\t\t int unset)\n>  {\n>  \tBUG_ON_OPT_NEG(unset);\n> diff --git a/builtin/update-index.c b/builtin/update-index.c\n> index aee3cb8cbd..59acae3336 100644\n> --- a/builtin/update-index.c\n> +++ b/builtin/update-index.c\n> @@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,\n>  \treturn 0;\n>  }\n>\n> -static int resolve_undo_clear_callback(const struct option *opt,\n> +static int resolve_undo_clear_callback(const struct option *opt UNUSED,\n\nAffects the_index.  This option (--clear-resolve-undo) is not mentioned\nin Documentation/, by the way.  Not sure if it interacts with other\ncallbacks, like the one below.  Otherwise could be an OPT_BOOL and its\naction done after parsing.  Perhaps pass in &the_index?\n\n>  \t\t\t\tconst char *arg, int unset)\n>  {\n>  \tBUG_ON_OPT_NEG(unset);\n> @@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,\n>  }\n>\n>  static enum parse_opt_result cacheinfo_callback(\n> -\tstruct parse_opt_ctx_t *ctx, const struct option *opt,\n> +\tstruct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,\n\nA low-level callback for a change.  Affects the_index.  Pass it in?\nadd_cacheinfo() would need to grow a parameter to receive it.  It would\ndocument that this option directly changes the_index.\n\n>  \tconst char *arg, int unset)\n>  {\n>  \tstruct object_id oid;\n\nPhew, looks like that's stuff for a whole new series or two!\n\nRené\n"},{"id":"481327","messageId":"3bf1bd4a-9d28-32c1-7838-09acd2c85d03@web.de","threadId":"60176","inReplyTo":"20230831212220.GJ949469@coredump.intra.peff.net","subject":"Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-02T11:37:08Z","receivedAt":"2023-09-02T11:39:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.08.23 um 23:22 schrieb Jeff King:\n> Unsurprisingly, the noop options callback doesn't bother to look at any\n> of its parameters. Let's mark them so that -Wunused-parameter does not\n> complain.\n>\n> Another option would be to drop the callback and have parse-options\n> itself recognize OPT_NOOP_NOARG. But that seems like extra work for no\n> real benefit.\n\nI'm not sure about that -- we don't need to add special flags or option\ntypes to drop parse_opt_noop_cb().\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  parse-options-cb.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/parse-options-cb.c b/parse-options-cb.c\n> index a24521dee0..bdc7fae497 100644\n> --- a/parse-options-cb.c\n> +++ b/parse-options-cb.c\n> @@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)\n>  \treturn 0;\n>  }\n>\n> -int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)\n> +int parse_opt_noop_cb(const struct option *opt UNUSED,\n> +\t\t      const char *arg UNUSED,\n> +\t\t      int unset UNUSED)\n>  {\n>  \treturn 0;\n>  }\n\nYour patch is simple and makes sense.  The one below is more\ncomplicated, but leaves the code in a slightly better state.  I'd go\nwith that variant if I'd had to add OPT_NOOP_NOARG today, and I\nslightly prefer it, but similar to you think that the benefit is low\n(though non-zero).  So I'm unsure if that's enough to be worth it or\njust bikeshedding with some hindsight..\n\nRené\n\n\n\n--- >8 ---\nSubject: [PATCH] parse-options: let NULL callback be a noop\n\nAllow OPTION_CALLBACK options to have a NULL callback function pointer\nand do nothing in that case, yet still handle arguments as usual.  Use\nthis to replace parse_opt_noop_cb().\n\nWe lose the ability to distinguish between a forgotten function pointer\nand intentional noop, but that will be caught by a compiler warning\nabout an unused function in many cases and having an option do nothing\nis a benign failure mode.\n\nWe also lose the ability to add a warning to the callback, but we\nhaven't done that since it was added by 6acec0380b (parseopt: add\nOPT_NOOP_NOARG, 2011-09-28), so we probably won't do it soon.  We can\nadd a callback back when we want to show a warning.\n\nBy letting go of features we don't need this patch simplifies the\nparse-options API and gets rid of an exported empty function.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n parse-options-cb.c | 5 -----\n parse-options.c    | 7 +++----\n parse-options.h    | 2 --\n 3 files changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex a24521dee0..e79cd54ec2 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -227,11 +227,6 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n\n-int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)\n-{\n-\treturn 0;\n-}\n-\n /**\n  * Recreates the command-line option in the strbuf.\n  */\ndiff --git a/parse-options.c b/parse-options.c\nindex 76d2e76b49..5be8de93f5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -201,8 +201,9 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t}\n \t\tif (opt->callback)\n \t\t\treturn (*opt->callback)(opt, p_arg, p_unset) ? (-1) : 0;\n-\t\telse\n+\t\tif (opt->ll_callback)\n \t\t\treturn (*opt->ll_callback)(p, opt, p_arg, p_unset);\n+\t\treturn 0;\n \t}\n \tcase OPTION_INTEGER:\n \t\tif (unset) {\n@@ -494,9 +495,7 @@ static void parse_options_check(const struct option *opts)\n \t\t\t\toptbug(opts, \"should not accept an argument\");\n \t\t\tbreak;\n \t\tcase OPTION_CALLBACK:\n-\t\t\tif (!opts->callback && !opts->ll_callback)\n-\t\t\t\toptbug(opts, \"OPTION_CALLBACK needs one callback\");\n-\t\t\telse if (opts->callback && opts->ll_callback)\n+\t\t\tif (opts->callback && opts->ll_callback)\n \t\t\t\toptbug(opts, \"OPTION_CALLBACK can't have two callbacks\");\n \t\t\tbreak;\n \t\tcase OPTION_LOWLEVEL_CALLBACK:\ndiff --git a/parse-options.h b/parse-options.h\nindex 57a7fe9d91..41bb47120d 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -348,7 +348,6 @@ struct option {\n \t.long_name = (l), \\\n \t.help = N_(\"no-op (backward compatibility)\"), \\\n \t.flags = PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, \\\n-\t.callback = parse_opt_noop_cb, \\\n }\n\n #define OPT_ALIAS(s, l, source_long_name) { \\\n@@ -490,7 +489,6 @@ int parse_opt_commit(const struct option *, const char *, int);\n int parse_opt_tertiary(const struct option *, const char *, int);\n int parse_opt_string_list(const struct option *, const char *, int);\n int parse_opt_strvec(const struct option *, const char *, int);\n-int parse_opt_noop_cb(const struct option *, const char *, int);\n int parse_opt_passthru(const struct option *, const char *, int);\n int parse_opt_passthru_argv(const struct option *, const char *, int);\n /* value is enum branch_track* */\n--\n2.42.0\n"},{"id":"481388","messageId":"20230905070948.GD199565@coredump.intra.peff.net","threadId":"60176","inReplyTo":"3bf1bd4a-9d28-32c1-7838-09acd2c85d03@web.de","subject":"Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-05T07:09:48Z","receivedAt":"2023-09-05T16:00:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 02, 2023 at 01:37:08PM +0200, René Scharfe wrote:\n\n> --- >8 ---\n> Subject: [PATCH] parse-options: let NULL callback be a noop\n> \n> Allow OPTION_CALLBACK options to have a NULL callback function pointer\n> and do nothing in that case, yet still handle arguments as usual.  Use\n> this to replace parse_opt_noop_cb().\n> \n> We lose the ability to distinguish between a forgotten function pointer\n> and intentional noop, but that will be caught by a compiler warning\n> about an unused function in many cases and having an option do nothing\n> is a benign failure mode.\n\nYeah, my concern would be missing an accidental NULL here. I guess that\nis pretty unlikely in practice, though. I'd guess the worst case would\nbe somebody accidentally putting the function into the \"opt->value\" slot\nwhere the compiler might then think it is used (though I don't know\noff-hand if it would complain about an implicit conversion of a function\npointer into a \"void *\").\n\n> We also lose the ability to add a warning to the callback, but we\n> haven't done that since it was added by 6acec0380b (parseopt: add\n> OPT_NOOP_NOARG, 2011-09-28), so we probably won't do it soon.  We can\n> add a callback back when we want to show a warning.\n> \n> By letting go of features we don't need this patch simplifies the\n> parse-options API and gets rid of an exported empty function.\n\nYour patch looks correct to me. I probably wouldn't have bothered to\nwrite it, but now you did. :) My inclination would be to go with my\ndumb-simple one for this series, and it looks like you may have\ncollected a few slight-more-risky-but-maybe-worthwhile parseopt cleanups\non top that could make their own series.\n\n-Peff\n"},{"id":"481389","messageId":"20230905071259.GE199565@coredump.intra.peff.net","threadId":"60176","inReplyTo":"c7855b08-46ee-5df0-4b0f-67ea57d84b18@web.de","subject":"[PATCH v3 04/10] checkout-index: delay automatic setting of to_tempfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-05T07:12:59Z","receivedAt":"2023-09-05T16:00:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 02, 2023 at 08:20:43AM +0200, René Scharfe wrote:\n\n> Am 31.08.23 um 23:20 schrieb Jeff King:\n> > @@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n> >  \t\tstate.base_dir = \"\";\n> >  \tstate.base_dir_len = strlen(state.base_dir);\n> >\n> > +\tif (to_tempfile < 0)\n> > +\t\tto_tempfile = (checkout_stage == CHECKOUT_ALL);\n> > +\tif (!to_tempfile && checkout_stage == CHECKOUT_ALL)\n> > +\t\tdie(\"--stage=all and --no-temp are incompatible\");\n> \n> How about making this message translatable from the start and following\n> the convention from 12909b6b8a (i18n: turn \"options are incompatible\"\n> into \"cannot be used together\", 2022-01-05) to reuse the existing\n> translations?\n\nGood catch. I forgot that we had standardized some of these. The other\nerror messages in the file aren't translated, but I don't think that's\nan intentional choice (even though it is plumbing). Some of them are\nobviously quite old and don't match our usual style (like starting with\n\"checkout-index: \").\n\nRather than re-send the whole series, I _think_ this is the only patch I\nwould change in a re-roll (if you buy me \"sure, go ahead and send it on\ntop\" evasions in my other responses).\n\nSo here's a replacement patch 4 that fixes up the message.\n\n-- >8 --\nSubject: checkout-index: delay automatic setting of to_tempfile\n\nUsing --stage=all requires writing to tempfiles, since we cannot put\nmultiple stages into a single file. So --stage=all implies --temp.\n\nBut we do so by setting to_tempfile in the options callback for --stage,\nrather than after all options have been parsed. This leads to two bugs:\n\n  1. If you run \"checkout-index --stage=all --stage=2\", this should not\n     imply --temp, but it currently does. The callback cannot just unset\n     to_tempfile when it sees the \"2\" value, because it no longer knows\n     if its value was from the earlier --stage call, or if the user\n     specified --temp explicitly.\n\n  2. If you run \"checkout-index --stage=all --no-temp\", the --no-temp\n     will overwrite the earlier implied --temp. But this mode of\n     operation cannot work, and the command will fail with \"<path>\n     already exists\" when trying to write the higher stages.\n\nWe can fix both by lazily setting to_tempfile. We'll make it a tristate,\nwith -1 as \"not yet given\", and have --stage=all enable it only after\nall options are parsed. Likewise, after all options are parsed we can\ndetect and reject the bogus \"--no-temp\" case.\n\nNote that this does technically change the behavior for \"--stage=all\n--no-temp\" for paths which have only one stage present (which\naccidentally worked before, but is now forbidden). But this behavior was\nnever intended, and you'd have to go out of your way to try to trigger\nit.\n\nThe new tests cover both cases, as well the general \"--stage=all implies\n--temp\", as most of the other tests explicitly say \"--temp\". Ironically,\nthe test \"checkout --temp within subdir\" is the only one that _doesn't_\nuse \"--temp\", and so was implicitly covering this case. But it seems\nreasonable to have a more explicit test alongside the other related\nones.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout-index.c       |  9 +++++++--\n t/t2004-checkout-cache-temp.sh | 20 ++++++++++++++++++++\n 2 files changed, 27 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex f62f13f2b5..526c210bcb 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -24,7 +24,7 @@\n static int nul_term_line;\n static int checkout_stage; /* default to checkout stage0 */\n static int ignore_skip_worktree; /* default to 0 */\n-static int to_tempfile;\n+static int to_tempfile = -1;\n static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];\n \n static struct checkout state = CHECKOUT_INIT;\n@@ -196,7 +196,6 @@ static int option_parse_stage(const struct option *opt,\n \tBUG_ON_OPT_NEG(unset);\n \n \tif (!strcmp(arg, \"all\")) {\n-\t\tto_tempfile = 1;\n \t\tcheckout_stage = CHECKOUT_ALL;\n \t} else {\n \t\tint ch = arg[0];\n@@ -269,6 +268,12 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\tstate.base_dir = \"\";\n \tstate.base_dir_len = strlen(state.base_dir);\n \n+\tif (to_tempfile < 0)\n+\t\tto_tempfile = (checkout_stage == CHECKOUT_ALL);\n+\tif (!to_tempfile && checkout_stage == CHECKOUT_ALL)\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t    \"--stage=all\", \"--no-temp\");\n+\n \t/*\n \t * when --prefix is specified we do not want to update cache.\n \t */\ndiff --git a/t/t2004-checkout-cache-temp.sh b/t/t2004-checkout-cache-temp.sh\nindex b16d69ca4a..45dd1bc858 100755\n--- a/t/t2004-checkout-cache-temp.sh\n+++ b/t/t2004-checkout-cache-temp.sh\n@@ -117,6 +117,26 @@ test_expect_success 'checkout all stages/one file to temporary files' '\n \ttest $(cat $s3) = tree3path1)\n '\n \n+test_expect_success '--stage=all implies --temp' '\n+\trm -f path* .merge_* actual &&\n+\tgit checkout-index --stage=all -- path1 &&\n+\ttest_path_is_missing path1\n+'\n+\n+test_expect_success 'overriding --stage=all resets implied --temp' '\n+\trm -f path* .merge_* actual &&\n+\tgit checkout-index --stage=all --stage=2 -- path1 &&\n+\techo tree2path1 >expect &&\n+\ttest_cmp expect path1\n+'\n+\n+test_expect_success '--stage=all --no-temp is rejected' '\n+\trm -f path* .merge_* actual &&\n+\ttest_must_fail git checkout-index --stage=all --no-temp -- path1 2>err &&\n+\tgrep -v \"already exists\" err &&\n+\tgrep \"options .--stage=all. and .--no-temp. cannot be used together\" err\n+'\n+\n test_expect_success 'checkout some stages/one file to temporary files' '\n \trm -f path* .merge_* actual &&\n \tgit checkout-index --stage=all --temp -- path2 >actual &&\n-- \n2.42.0.567.gc396a4a104\n\n"},{"id":"481395","messageId":"20230905065237.GB199565@coredump.intra.peff.net","threadId":"60176","inReplyTo":"8f03fa13-11b3-1f9f-1e1f-8f7d8ce74a23@web.de","subject":"Re: [PATCH v2 05/10] parse-options: prefer opt->value to globals in callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-05T06:52:37Z","receivedAt":"2023-09-05T16:00:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 02, 2023 at 09:34:32AM +0200, René Scharfe wrote:\n\n> >  static int option_parse_if_missing(const struct option *opt,\n> >  \t\t\t\t   const char *arg, int unset)\n> >  {\n> > -\treturn trailer_set_if_missing(&if_missing, arg);\n> > +\treturn trailer_set_if_missing(opt->value, arg);\n> >  }\n> \n> Not your fault, but these all silently exit if \"arg\" contains an\n> unrecognized value.  Reporting the error would be better.\n\nHmm, yeah. On the config side, git_trailer_default_config() issues a\nwarning after the functions return -1. I'd have guessed we at least\nprinted a usage message or something here, but we don't even do that.\nYou get a silent 129 exit code.\n\nGross, but it's something I think we should fix separately, as it's\northogonal to this series.\n\n> >  static void new_trailers_clear(struct list_head *trailers)\n> > @@ -97,11 +97,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n> >  \t\tOPT_BOOL(0, \"in-place\", &opts.in_place, N_(\"edit files in place\")),\n> >  \t\tOPT_BOOL(0, \"trim-empty\", &opts.trim_empty, N_(\"trim empty trailers\")),\n> >\n> > -\t\tOPT_CALLBACK(0, \"where\", NULL, N_(\"action\"),\n> > +\t\tOPT_CALLBACK(0, \"where\", &where, N_(\"action\"),\n> >  \t\t\t     N_(\"where to place the new trailer\"), option_parse_where),\n> > -\t\tOPT_CALLBACK(0, \"if-exists\", NULL, N_(\"action\"),\n> > +\t\tOPT_CALLBACK(0, \"if-exists\", &if_exists, N_(\"action\"),\n> >  \t\t\t     N_(\"action if trailer already exists\"), option_parse_if_exists),\n> > -\t\tOPT_CALLBACK(0, \"if-missing\", NULL, N_(\"action\"),\n> > +\t\tOPT_CALLBACK(0, \"if-missing\", &if_missing, N_(\"action\"),\n> >  \t\t\t     N_(\"action if trailer is missing\"), option_parse_if_missing),\n> \n> And I wonder if \"action\" should be replaced by \"(after|before|end|start)\",\n> \"(addIfDifferent|addIfDifferentNeighbor|add|replace|doNothing)\" and\n> \"(doNothing|add)\", respectively.  Gets a bit long in the middle, but would\n> be more helpful.  #leftoverbits\n\nI don't have a strong opinion. It is sometimes nice to provide more\ndetail to save the user having to look it up separately. But sometimes\nthose details can be overwhelming and make it hard to read, especially\nif they grow over time. I'm thinking less of \"-h\" output and more of\npeople like to put:\n\n  git foo [--optionA|--optionB|--optionC]\n\nand so on, until eventually the options block is like 4 lines long. Just\nsaying [options] and then listing them is more friendly. I'm not sure if\nwe're approaching that kind of problem here or not.\n\n-Peff\n"},{"id":"481397","messageId":"20230905070512.GC199565@coredump.intra.peff.net","threadId":"60176","inReplyTo":"98d1cd21-fb2a-269a-8d0b-f3e050682739@web.de","subject":"Re: [PATCH v2 06/10] parse-options: mark unused \"opt\" parameter in callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-05T07:05:12Z","receivedAt":"2023-09-05T16:01:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 02, 2023 at 12:12:56PM +0200, René Scharfe wrote:\n\n> Am 31.08.23 um 23:21 schrieb Jeff King:\n> > The previous commit argued that parse-options callbacks should try to\n> > use opt->value rather than touching globals directly. In some cases,\n> > however, that's awkward to do. Some callbacks touch multiple variables,\n> > or may even just call into an abstracted function that does so.\n> >\n> > In some of these cases we _could_ convert them by stuffing the multiple\n> > variables into a single struct and passing the struct pointer through\n> > opt->value. But that may make other parts of the code less readable,\n> > as the struct relationship has to be mentioned everywhere.\n> \n> Does that imply you'd be willing to use other methods?  Let's find out\n> below. :)\n\nWell, I'm not necessarily _opposed_. :) Mostly my cutoff was cases where\nthe end result was not obviously and immediately more readable. It is\nnot that big a deal to add an UNUSED annotation. My main goal was to use\nthe opportunity to check that we aren't papering over an obvious bug.\n\nSo for example...\n\n> > diff --git a/builtin/gc.c b/builtin/gc.c\n> > index 369bd43fb2..b842349d86 100644\n> > --- a/builtin/gc.c\n> > +++ b/builtin/gc.c\n> > @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)\n> >  \tstrbuf_release(&config_name);\n> >  }\n> >\n> > -static int task_option_parse(const struct option *opt,\n> > +static int task_option_parse(const struct option *opt UNUSED,\n> \n> Only the global variable \"tasks\" seems to be used in here if you don't\n> count the constant \"TASK__COUNT\", so you could pass it in.  This could\n> also be converted to OPT_STRING_LIST with parsing and duplicate checking\n> done later.\n\n...in many cases things can be simplified by parsing into a string, and\nthen validating or acting on the string later. And sometimes that's even\na better strategy (because it lets the arg parsing handle all the \"last\none wins\" logic and we just get the result).\n\nBut it can also make the code harder to follow, too, because it's now\nsplit it into segments (although one is mostly declarative, which is\nnice).\n\nSo I generally tried to err on the side of not touching working\ncode. Both to avoid breaking it, but also to keep the focus on the goal\nof the series. And I think that applies to a lot of the other cases you\nmentioned below (I won't respond to each; I think some of them could be\nfine, but it also feels like writing and review effort for not much\ngain. I'm not opposed if you want to dig into them, though).\n\n> And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION)\n> on error, but that's a different matter.\n\nYeah, that doesn't make sense at all.\n\n> > -static int clear_decorations_callback(const struct option *opt,\n> > -\t\t\t\t\t    const char *arg, int unset)\n> > +static int clear_decorations_callback(const struct option *opt UNUSED,\n> > +\t\t\t\t      const char *arg, int unset)\n> >  {\n> >  \tstring_list_clear(&decorate_refs_include, 0);\n> >  \tstring_list_clear(&decorate_refs_exclude, 0);\n> >  \tuse_default_decoration_filter = 0;\n> >  \treturn 0;\n> >  }\n> >\n> \n> Meta: Why do we get seven lines of context in an -U3 patch here?  Did\n> you use --inter-hunk-context?\n\nYes, I set diff.interhunkcontext=1 in my config (and have for many\nyears; I think this is the first time anybody noticed in a patch). My\nrationale is that with \"1\" the output is never longer, since you save\nthe hunk header. It is a little funny in this case, since the two\nchanges aren't really connected.\n\n> This patch would be better viewed with --function-context to see that\n> the callbacks change multiple variables or do other funky things.  Only\n> doubles the line count.\n\nI do sometimes use options like that to make a patch more readable, if I\nnotice the difference. I didn't happen to in this case (though I don't\ndisagree with you). As a reviewer, I typically apply and run \"git show\"\nmyself if I want to dig more (or just go directly look at the preimage\nin my local repo, of course).\n\n> > -static int option_parse_strategy(const struct option *opt,\n> > +static int option_parse_strategy(const struct option *opt UNUSED,\n> \n> Could be an OPT_STRING_LIST and parsing done later.  Except that\n> --no-strategy does nothing, which is weird.\n\nYeah, I think the handling of \"unset\" is a bug (similar to the xopts one\nfixed earlier).\n\n-Peff\n"},{"id":"481400","messageId":"20230905064307.GA199565@coredump.intra.peff.net","threadId":"60176","inReplyTo":"75100a68-78d1-b22c-0497-36548c518b7b@web.de","subject":"Re: [PATCH v2 02/10] merge: simplify parsing of \"-n\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-05T06:43:07Z","receivedAt":"2023-09-05T16:18:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 02, 2023 at 08:20:28AM +0200, René Scharfe wrote:\n\n> >  static struct option builtin_merge_options[] = {\n> > -\tOPT_CALLBACK_F('n', NULL, NULL, NULL,\n> > -\t\tN_(\"do not show a diffstat at the end of the merge\"),\n> > -\t\tPARSE_OPT_NOARG, option_parse_n),\n> > +\tOPT_SET_INT('n', NULL, &show_diffstat,\n> > +\t\tN_(\"do not show a diffstat at the end of the merge\"), 0),\n> >  \tOPT_BOOL(0, \"stat\", &show_diffstat,\n> >  \t\tN_(\"show a diffstat at the end of the merge\")),\n> \n> Makes it easier to see that we can replace the two complementary\n> definitions with a single one:\n> \n> \tOPT_NEGBIT('n', \"no-stat\",\n> \t\tN_(\"do not show a diffstat at the end of the merge\"), 1),\n> \n> Which is a separate topic, of course.  And if we did that, however, ...\n\nAh, I thought we had a \"reverse bool\" of some kind, but I couldn't find\nit. NEGBIT was what I was looking for. But yeah, I agree it gets more\ncomplicated with the various aliases. I think what I have here is a good\nstopping point for this series, but if you want to go further on it, be\nmy guest. :)\n\n-Peff\n"},{"id":"481516","messageId":"9c3c935f-6661-e763-9036-f86c03f6267f@web.de","threadId":"60176","inReplyTo":"20230905070948.GD199565@coredump.intra.peff.net","subject":"Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-07T20:20:50Z","receivedAt":"2023-09-07T20:21:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.09.23 um 09:09 schrieb Jeff King:\n>\n> Your patch looks correct to me. I probably wouldn't have bothered to\n> write it, but now you did. :) My inclination would be to go with my\n> dumb-simple one for this series,\n\nFair enough.\n\n> and it looks like you may have\n> collected a few slight-more-risky-but-maybe-worthwhile parseopt cleanups\n> on top that could make their own series.\n\nWell, my aim is more to simplify and improve type safety, but I get\ncarried away a bit at times.\n\nRené\n"},{"id":"482006","messageId":"3c3960cf-155c-4129-913e-31db45671eef@web.de","threadId":"60176","inReplyTo":"20230905070512.GC199565@coredump.intra.peff.net","subject":"Re: [PATCH v2 06/10] parse-options: mark unused \"opt\" parameter in callbacks","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-19T07:42:01Z","receivedAt":"2023-09-19T07:42:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.09.23 um 09:05 schrieb Jeff King:\n> On Sat, Sep 02, 2023 at 12:12:56PM +0200, René Scharfe wrote:\n>\n>> And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION)\n>> on error, but that's a different matter.\n>\n> Yeah, that doesn't make sense at all.\n\nActually it does: any non-zero return from a callback is interpreted as an\nerror.  \"return error(...);\" would be shorter, but \"error(...); return 1;\"\nis not wrong.  Sorry for the noise!\n\nRené\n"}]}