{"thread":{"id":"60438","subject":"[PATCH 1/2] parse-options: make CMDMODE errors more precise","startedAt":"2023-10-28T11:53:11Z","lastAt":"2023-10-30T00:13:05Z","messageCount":3,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"484022","messageId":"cefdba32-db0b-4f68-954e-9d31fc12b1a0@web.de","threadId":"60438","inReplyTo":null,"subject":"[PATCH 1/2] parse-options: make CMDMODE errors more precise","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-10-28T11:53:01Z","receivedAt":"2023-10-28T11:53:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Only a single PARSE_OPT_CMDMODE option can be specified for the same\nvariable at the same time.  This is enforced by get_value(), but the\nerror messages are imprecise in three ways:\n\n1. If a non-PARSE_OPT_CMDMODE option changes the value variable of a\nPARSE_OPT_CMDMODE option then an ominously vague message is shown:\n\n   $ t/helper/test-tool parse-options --set23 --mode1\n   error: option `mode1' : incompatible with something else\n\nWorse: If the order of options is reversed then no error is reported at\nall:\n\n   $ t/helper/test-tool parse-options --mode1 --set23\n   boolean: 0\n   integer: 23\n   magnitude: 0\n   timestamp: 0\n   string: (not set)\n   abbrev: 7\n   verbose: -1\n   quiet: 0\n   dry run: no\n   file: (not set)\n\nFortunately this can currently only happen in the test helper; actual\nGit commands don't share the same variable for the value of options with\nand without the flag PARSE_OPT_CMDMODE.\n\n2. If there are multiple options with the same value (synonyms), then\nthe one that is defined first is shown rather than the one actually\ngiven on the command line, which is confusing:\n\n   $ git am --resolved --quit\n   error: option `quit' is incompatible with --continue\n\n3. Arguments of PARSE_OPT_CMDMODE options are not handled by the\nparse-option machinery.  This is left to the callback function.  We\ncurrently only have a single affected option, --show-current-patch of\ngit am.  Errors for it can show an argument that was not actually given\non the command line:\n\n   $ git am --show-current-patch --show-current-patch=diff\n   error: options '--show-current-patch=diff' and '--show-current-patch=raw' cannot be used together\n\nThe options --show-current-patch and --show-current-patch=raw are\nsynonyms, but the error accuses the user of input they did not actually\nmade.  Or it can awkwardly print a NULL pointer:\n\n   $ git am --show-current-patch=diff --show-current-patch\n   error: options '--show-current-patch=(null)' and '--show-current-patch=diff' cannot be used together\n\nThe reasons for these shortcomings is that the current code checks\nincompatibility only when encountering a PARSE_OPT_CMDMODE option at the\ncommand line, and that it searches the previous incompatible option by\nvalue.\n\nFix the first two points by checking all PARSE_OPT_CMDMODE variables\nafter parsing each option and by storing all relevant details if their\nvalue changed.  Do that whether or not the changing options has the flag\nPARSE_OPT_CMDMODE set.  Report an incompatibility only if two options\nchange the variable to different values and at least one of them is a\nPARSE_OPT_CMDMODE option.  This changes the output of the first three\nexamples above to:\n\n   $ t/helper/test-tool parse-options --set23 --mode1\n   error: --mode1 is incompatible with --set23\n   $ t/helper/test-tool parse-options --mode1 --set23\n   error: --set23 is incompatible with --mode1\n   $ git am --resolved --quit\n   error: --quit is incompatible with --resolved\n\nStore the argument of PARSE_OPT_CMDMODE options of type OPTION_CALLBACK\nas well to allow taking over the responsibility for compatibility\nchecking from the callback function.  The next patch will use this\ncapability to fix the messages for git am --show-current-patch.\n\nUse a linked list for storing the PARSE_OPT_CMDMODE variables.  This\nsomewhat outdated data structure is simple and suffices, as the number\nof elements per command is currently only zero or one.  We do support\nmultiple different command modes variables per command, but I don't\nexpect that we'd ever use a significant number of them.  Once we do we\ncan switch to a hashmap.\n\nSince we no longer need to search the conflicting option, the all_opts\nparameter of get_value() is no longer used.  Remove it.\n\nExtend the tests to check for both conflicting option names, but don't\ninsist on a particular order.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n parse-options.c               | 144 ++++++++++++++++++++++------------\n parse-options.h               |   3 +\n t/helper/test-parse-options.c |  16 ++++\n t/t0040-parse-options.sh      |  33 ++++++--\n 4 files changed, 139 insertions(+), 57 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 093eaf2db8..e0c94b0546 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -70,42 +70,10 @@ static void fix_filename(const char *prefix, char **file)\n \t\t*file = prefix_filename_except_for_dash(prefix, *file);\n }\n\n-static enum parse_opt_result opt_command_mode_error(\n-\tconst struct option *opt,\n-\tconst struct option *all_opts,\n-\tenum opt_parsed flags)\n-{\n-\tconst struct option *that;\n-\tstruct strbuf that_name = STRBUF_INIT;\n-\n-\t/*\n-\t * Find the other option that was used to set the variable\n-\t * already, and report that this is not compatible with it.\n-\t */\n-\tfor (that = all_opts; that->type != OPTION_END; that++) {\n-\t\tif (that == opt ||\n-\t\t    !(that->flags & PARSE_OPT_CMDMODE) ||\n-\t\t    that->value != opt->value ||\n-\t\t    that->defval != *(int *)opt->value)\n-\t\t\tcontinue;\n-\n-\t\tif (that->long_name)\n-\t\t\tstrbuf_addf(&that_name, \"--%s\", that->long_name);\n-\t\telse\n-\t\t\tstrbuf_addf(&that_name, \"-%c\", that->short_name);\n-\t\terror(_(\"%s is incompatible with %s\"),\n-\t\t      optname(opt, flags), that_name.buf);\n-\t\tstrbuf_release(&that_name);\n-\t\treturn PARSE_OPT_ERROR;\n-\t}\n-\treturn error(_(\"%s : incompatible with something else\"),\n-\t\t     optname(opt, flags));\n-}\n-\n-static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n-\t\t\t\t       const struct option *opt,\n-\t\t\t\t       const struct option *all_opts,\n-\t\t\t\t       enum opt_parsed flags)\n+static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n+\t\t\t\t\t  const struct option *opt,\n+\t\t\t\t\t  enum opt_parsed flags,\n+\t\t\t\t\t  const char **argp)\n {\n \tconst char *s, *arg;\n \tconst int unset = flags & OPT_UNSET;\n@@ -118,14 +86,6 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \tif (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))\n \t\treturn error(_(\"%s takes no value\"), optname(opt, flags));\n\n-\t/*\n-\t * Giving the same mode option twice, although unnecessary,\n-\t * is not a grave error, so let it pass.\n-\t */\n-\tif ((opt->flags & PARSE_OPT_CMDMODE) &&\n-\t    *(int *)opt->value && *(int *)opt->value != opt->defval)\n-\t\treturn opt_command_mode_error(opt, all_opts, flags);\n-\n \tswitch (opt->type) {\n \tcase OPTION_LOWLEVEL_CALLBACK:\n \t\treturn opt->ll_callback(p, opt, NULL, unset);\n@@ -200,6 +160,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t\tp_unset = 0;\n \t\t\tp_arg = arg;\n \t\t}\n+\t\tif (opt->flags & PARSE_OPT_CMDMODE)\n+\t\t\t*argp = p_arg;\n \t\tif (opt->callback)\n \t\t\treturn (*opt->callback)(opt, p_arg, p_unset) ? (-1) : 0;\n \t\telse\n@@ -247,16 +209,91 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t}\n }\n\n+struct parse_opt_cmdmode_list {\n+\tint value, *value_ptr;\n+\tconst struct option *opt;\n+\tconst char *arg;\n+\tenum opt_parsed flags;\n+\tstruct parse_opt_cmdmode_list *next;\n+};\n+\n+static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,\n+\t\t\t       const struct option *opts)\n+{\n+\tctx->cmdmode_list = NULL;\n+\n+\tfor (; opts->type != OPTION_END; opts++) {\n+\t\tstruct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;\n+\t\tint *value_ptr = opts->value;\n+\n+\t\tif (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)\n+\t\t\tcontinue;\n+\n+\t\twhile (elem && elem->value_ptr != value_ptr)\n+\t\t\telem = elem->next;\n+\t\tif (elem)\n+\t\t\tcontinue;\n+\n+\t\tCALLOC_ARRAY(elem, 1);\n+\t\telem->value_ptr = value_ptr;\n+\t\telem->value = *value_ptr;\n+\t\telem->next = ctx->cmdmode_list;\n+\t\tctx->cmdmode_list = elem;\n+\t}\n+}\n+\n+static char *optnamearg(const struct option *opt, const char *arg,\n+\t\t\tenum opt_parsed flags)\n+{\n+\tif (flags & OPT_SHORT)\n+\t\treturn xstrfmt(\"-%c%s\", opt->short_name, arg ? arg : \"\");\n+\treturn xstrfmt(\"--%s%s%s%s\", flags & OPT_UNSET ? \"no-\" : \"\",\n+\t\t       opt->long_name, arg ? \"=\" : \"\", arg ? arg : \"\");\n+}\n+\n+static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n+\t\t\t\t       const struct option *opt,\n+\t\t\t\t       enum opt_parsed flags)\n+{\n+\tconst char *arg = NULL;\n+\tenum parse_opt_result result = do_get_value(p, opt, flags, &arg);\n+\tstruct parse_opt_cmdmode_list *elem = p->cmdmode_list;\n+\tchar *opt_name, *other_opt_name;\n+\n+\tfor (; elem; elem = elem->next) {\n+\t\tif (*elem->value_ptr == elem->value)\n+\t\t\tcontinue;\n+\n+\t\tif (elem->opt &&\n+\t\t    (elem->opt->flags | opt->flags) & PARSE_OPT_CMDMODE)\n+\t\t\tbreak;\n+\n+\t\telem->opt = opt;\n+\t\telem->arg = arg;\n+\t\telem->flags = flags;\n+\t\telem->value = *elem->value_ptr;\n+\t}\n+\n+\tif (result || !elem)\n+\t\treturn result;\n+\n+\topt_name = optnamearg(opt, arg, flags);\n+\tother_opt_name = optnamearg(elem->opt, elem->arg, elem->flags);\n+\terror(_(\"%s is incompatible with %s\"), opt_name, other_opt_name);\n+\tfree(opt_name);\n+\tfree(other_opt_name);\n+\treturn -1;\n+}\n+\n static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,\n \t\t\t\t\t     const struct option *options)\n {\n-\tconst struct option *all_opts = options;\n \tconst struct option *numopt = NULL;\n\n \tfor (; options->type != OPTION_END; options++) {\n \t\tif (options->short_name == *p->opt) {\n \t\t\tp->opt = p->opt[1] ? p->opt + 1 : NULL;\n-\t\t\treturn get_value(p, options, all_opts, OPT_SHORT);\n+\t\t\treturn get_value(p, options, OPT_SHORT);\n \t\t}\n\n \t\t/*\n@@ -318,7 +355,6 @@ static enum parse_opt_result parse_long_opt(\n \tstruct parse_opt_ctx_t *p, const char *arg,\n \tconst struct option *options)\n {\n-\tconst struct option *all_opts = options;\n \tconst char *arg_end = strchrnul(arg, '=');\n \tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n \tenum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;\n@@ -387,7 +423,7 @@ static enum parse_opt_result parse_long_opt(\n \t\t\t\tcontinue;\n \t\t\tp->opt = rest + 1;\n \t\t}\n-\t\treturn get_value(p, options, all_opts, flags ^ opt_flags);\n+\t\treturn get_value(p, options, flags ^ opt_flags);\n \t}\n\n \tif (disallow_abbreviated_options && (ambiguous_option || abbrev_option))\n@@ -405,7 +441,7 @@ static enum parse_opt_result parse_long_opt(\n \t\treturn PARSE_OPT_HELP;\n \t}\n \tif (abbrev_option)\n-\t\treturn get_value(p, abbrev_option, all_opts, abbrev_flags);\n+\t\treturn get_value(p, abbrev_option, abbrev_flags);\n \treturn PARSE_OPT_UNKNOWN;\n }\n\n@@ -413,13 +449,11 @@ static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p,\n \t\t\t\t\t      const char *arg,\n \t\t\t\t\t      const struct option *options)\n {\n-\tconst struct option *all_opts = options;\n-\n \tfor (; options->type != OPTION_END; options++) {\n \t\tif (!(options->flags & PARSE_OPT_NODASH))\n \t\t\tcontinue;\n \t\tif (options->short_name == arg[0] && arg[1] == '\\0')\n-\t\t\treturn get_value(p, options, all_opts, OPT_SHORT);\n+\t\t\treturn get_value(p, options, OPT_SHORT);\n \t}\n \treturn PARSE_OPT_ERROR;\n }\n@@ -574,6 +608,7 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \t    (flags & PARSE_OPT_KEEP_ARGV0))\n \t\tBUG(\"Can't keep argv0 if you don't have it\");\n \tparse_options_check(options);\n+\tbuild_cmdmode_list(ctx, options);\n }\n\n void parse_options_start(struct parse_opt_ctx_t *ctx,\n@@ -1006,6 +1041,11 @@ int parse_options(int argc, const char **argv,\n \tprecompose_argv_prefix(argc, argv, NULL);\n \tfree_preprocessed_options(real_options);\n \tfree(ctx.alias_groups);\n+\tfor (struct parse_opt_cmdmode_list *elem = ctx.cmdmode_list; elem;) {\n+\t\tstruct parse_opt_cmdmode_list *next = elem->next;\n+\t\tfree(elem);\n+\t\telem = next;\n+\t}\n \treturn parse_options_end(&ctx);\n }\n\ndiff --git a/parse-options.h b/parse-options.h\nindex 4a66ec3bf5..bd62e20268 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -445,6 +445,8 @@ static inline void die_for_incompatible_opt3(int opt1, const char *opt1_name,\n\n /*----- incremental advanced APIs -----*/\n\n+struct parse_opt_cmdmode_list;\n+\n /*\n  * It's okay for the caller to consume argv/argc in the usual way.\n  * Other fields of that structure are private to parse-options and should not\n@@ -459,6 +461,7 @@ struct parse_opt_ctx_t {\n \tunsigned has_subcommands;\n \tconst char *prefix;\n \tconst char **alias_groups; /* must be in groups of 3 elements! */\n+\tstruct parse_opt_cmdmode_list *cmdmode_list;\n };\n\n void parse_options_start(struct parse_opt_ctx_t *ctx,\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex a4f6e24b0c..ded8116cc5 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -21,6 +21,19 @@ static struct {\n \tint unset;\n } length_cb;\n\n+static int mode34_callback(const struct option *opt, const char *arg, int unset)\n+{\n+\tif (unset)\n+\t\t*(int *)opt->value = 0;\n+\telse if (!strcmp(arg, \"3\"))\n+\t\t*(int *)opt->value = 3;\n+\telse if (!strcmp(arg, \"4\"))\n+\t\t*(int *)opt->value = 4;\n+\telse\n+\t\treturn error(\"invalid value for '%s': '%s'\", \"--mode34\", arg);\n+\treturn 0;\n+}\n+\n static int length_callback(const struct option *opt, const char *arg, int unset)\n {\n \tlength_cb.called = 1;\n@@ -124,6 +137,9 @@ int cmd__parse_options(int argc, const char **argv)\n \t\tOPT_SET_INT(0, \"set23\", &integer, \"set integer to 23\", 23),\n \t\tOPT_CMDMODE(0, \"mode1\", &integer, \"set integer to 1 (cmdmode option)\", 1),\n \t\tOPT_CMDMODE(0, \"mode2\", &integer, \"set integer to 2 (cmdmode option)\", 2),\n+\t\tOPT_CALLBACK_F(0, \"mode34\", &integer, \"(3|4)\",\n+\t\t\t\"set integer to 3 or 4 (cmdmode option)\",\n+\t\t\tPARSE_OPT_CMDMODE, mode34_callback),\n \t\tOPT_CALLBACK('L', \"length\", &integer, \"str\",\n \t\t\t\"get length of <str>\", length_callback),\n \t\tOPT_FILENAME('F', \"file\", &file, \"set file to <file>\"),\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex a0ad6192d6..06fb9e6457 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -28,6 +28,7 @@ usage: test-tool parse-options <options>\n     --[no-]set23          set integer to 23\n     --mode1               set integer to 1 (cmdmode option)\n     --mode2               set integer to 2 (cmdmode option)\n+    --[no-]mode34 (3|4)   set integer to 3 or 4 (cmdmode option)\n     -L, --[no-]length <str>\n                           get length of <str>\n     -F, --[no-]file <file>\n@@ -366,19 +367,41 @@ test_expect_success 'OPT_NEGBIT() works' '\n '\n\n test_expect_success 'OPT_CMDMODE() works' '\n-\ttest-tool parse-options --expect=\"integer: 1\" --mode1\n+\ttest-tool parse-options --expect=\"integer: 1\" --mode1 &&\n+\ttest-tool parse-options --expect=\"integer: 3\" --mode34=3\n '\n\n-test_expect_success 'OPT_CMDMODE() detects incompatibility' '\n+test_expect_success 'OPT_CMDMODE() detects incompatibility (1)' '\n \ttest_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&\n \ttest_must_be_empty output &&\n-\ttest_i18ngrep \"incompatible with --mode\" output.err\n+\ttest_i18ngrep \"mode1\" output.err &&\n+\ttest_i18ngrep \"mode2\" output.err &&\n+\ttest_i18ngrep \"is incompatible with\" output.err\n '\n\n-test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '\n+test_expect_success 'OPT_CMDMODE() detects incompatibility (2)' '\n \ttest_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&\n \ttest_must_be_empty output &&\n-\ttest_i18ngrep \"incompatible with something else\" output.err\n+\ttest_i18ngrep \"mode2\" output.err &&\n+\ttest_i18ngrep \"set23\" output.err &&\n+\ttest_i18ngrep \"is incompatible with\" output.err\n+'\n+\n+test_expect_success 'OPT_CMDMODE() detects incompatibility (3)' '\n+\ttest_must_fail test-tool parse-options --mode2 --set23 >output 2>output.err &&\n+\ttest_must_be_empty output &&\n+\ttest_i18ngrep \"mode2\" output.err &&\n+\ttest_i18ngrep \"set23\" output.err &&\n+\ttest_i18ngrep \"is incompatible with\" output.err\n+'\n+\n+test_expect_success 'OPT_CMDMODE() detects incompatibility (4)' '\n+\ttest_must_fail test-tool parse-options --mode2 --mode34=3 \\\n+\t\t>output 2>output.err &&\n+\ttest_must_be_empty output &&\n+\ttest_i18ngrep \"mode2\" output.err &&\n+\ttest_i18ngrep \"mode34.3\" output.err &&\n+\ttest_i18ngrep \"is incompatible with\" output.err\n '\n\n test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '\n--\n2.42.0\n"},{"id":"484023","messageId":"4520156b-9418-493c-a50c-e61b42e805b3@web.de","threadId":"60438","inReplyTo":"cefdba32-db0b-4f68-954e-9d31fc12b1a0@web.de","subject":"[PATCH 2/2] am: simplify --show-current-patch handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-10-28T11:57:44Z","receivedAt":"2023-10-28T11:57:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Let the parse-options code detect and handle the use of options that are\nincompatible with --show-current-patch.  This requires exposing the\ndistinction between the \"raw\" and \"diff\" sub-modes.  Do that by\nsplitting the mode RESUME_SHOW_PATCH into RESUME_SHOW_PATCH_RAW and\nRESUME_SHOW_PATCH_DIFF and stop tracking sub-modes in a separate struct.\n\nThe result is a simpler callback function and more precise error\nmessages.  The original reports a spurious argument or a NULL pointer:\n\n   $ git am --show-current-patch --show-current-patch=diff\n   error: options '--show-current-patch=diff' and '--show-current-patch=raw' cannot be used together\n   $ git am --show-current-patch=diff --show-current-patch\n   error: options '--show-current-patch=(null)' and '--show-current-patch=diff' cannot be used together\n\nWith this patch we get the more precise:\n\n   $ git am --show-current-patch --show-current-patch=diff\n   error: --show-current-patch=diff is incompatible with --show-current-patch\n   $ git am --show-current-patch=diff --show-current-patch\n   error: --show-current-patch is incompatible with --show-current-patch=diff\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/am.c | 112 ++++++++++++++++++++-------------------------------\n 1 file changed, 44 insertions(+), 68 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 6655059a57..ef5b9459af 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -92,9 +92,16 @@ enum signoff_type {\n \tSIGNOFF_EXPLICIT /* --signoff was set on the command-line */\n };\n\n-enum show_patch_type {\n-\tSHOW_PATCH_RAW = 0,\n-\tSHOW_PATCH_DIFF = 1,\n+enum resume_type {\n+\tRESUME_FALSE = 0,\n+\tRESUME_APPLY,\n+\tRESUME_RESOLVED,\n+\tRESUME_SKIP,\n+\tRESUME_ABORT,\n+\tRESUME_QUIT,\n+\tRESUME_SHOW_PATCH_RAW,\n+\tRESUME_SHOW_PATCH_DIFF,\n+\tRESUME_ALLOW_EMPTY,\n };\n\n enum empty_action {\n@@ -2191,7 +2198,7 @@ static void am_abort(struct am_state *state)\n \tam_destroy(state);\n }\n\n-static int show_patch(struct am_state *state, enum show_patch_type sub_mode)\n+static int show_patch(struct am_state *state, enum resume_type resume_mode)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *patch_path;\n@@ -2206,11 +2213,11 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)\n \t\treturn run_command(&cmd);\n \t}\n\n-\tswitch (sub_mode) {\n-\tcase SHOW_PATCH_RAW:\n+\tswitch (resume_mode) {\n+\tcase RESUME_SHOW_PATCH_RAW:\n \t\tpatch_path = am_path(state, msgnum(state));\n \t\tbreak;\n-\tcase SHOW_PATCH_DIFF:\n+\tcase RESUME_SHOW_PATCH_DIFF:\n \t\tpatch_path = am_path(state, \"patch\");\n \t\tbreak;\n \tdefault:\n@@ -2257,57 +2264,25 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int\n \treturn 0;\n }\n\n-enum resume_type {\n-\tRESUME_FALSE = 0,\n-\tRESUME_APPLY,\n-\tRESUME_RESOLVED,\n-\tRESUME_SKIP,\n-\tRESUME_ABORT,\n-\tRESUME_QUIT,\n-\tRESUME_SHOW_PATCH,\n-\tRESUME_ALLOW_EMPTY,\n-};\n-\n-struct resume_mode {\n-\tenum resume_type mode;\n-\tenum show_patch_type sub_mode;\n-};\n-\n static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)\n {\n \tint *opt_value = opt->value;\n-\tstruct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);\n\n+\tBUG_ON_OPT_NEG(unset);\n+\n+\tif (!arg)\n+\t\t*opt_value = opt->defval;\n+\telse if (!strcmp(arg, \"raw\"))\n+\t\t*opt_value = RESUME_SHOW_PATCH_RAW;\n+\telse if (!strcmp(arg, \"diff\"))\n+\t\t*opt_value = RESUME_SHOW_PATCH_DIFF;\n \t/*\n \t * Please update $__git_showcurrentpatch in git-completion.bash\n \t * when you add new options\n \t */\n-\tconst char *valid_modes[] = {\n-\t\t[SHOW_PATCH_DIFF] = \"diff\",\n-\t\t[SHOW_PATCH_RAW] = \"raw\"\n-\t};\n-\tint new_value = SHOW_PATCH_RAW;\n-\n-\tBUG_ON_OPT_NEG(unset);\n-\n-\tif (arg) {\n-\t\tfor (new_value = 0; new_value < ARRAY_SIZE(valid_modes); new_value++) {\n-\t\t\tif (!strcmp(arg, valid_modes[new_value]))\n-\t\t\t\tbreak;\n-\t\t}\n-\t\tif (new_value >= ARRAY_SIZE(valid_modes))\n-\t\t\treturn error(_(\"invalid value for '%s': '%s'\"),\n-\t\t\t\t     \"--show-current-patch\", arg);\n-\t}\n-\n-\tif (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n-\t\treturn error(_(\"options '%s=%s' and '%s=%s' \"\n-\t\t\t\t\t   \"cannot be used together\"),\n-\t\t\t     \"--show-current-patch\", arg,\n-\t\t\t     \"--show-current-patch\", valid_modes[resume->sub_mode]);\n-\n-\tresume->mode = RESUME_SHOW_PATCH;\n-\tresume->sub_mode = new_value;\n+\telse\n+\t\treturn error(_(\"invalid value for '%s': '%s'\"),\n+\t\t\t     \"--show-current-patch\", arg);\n \treturn 0;\n }\n\n@@ -2317,7 +2292,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tint binary = -1;\n \tint keep_cr = -1;\n \tint patch_format = PATCH_FORMAT_UNKNOWN;\n-\tstruct resume_mode resume = { .mode = RESUME_FALSE };\n+\tenum resume_type resume_mode = RESUME_FALSE;\n \tint in_progress;\n \tint ret = 0;\n\n@@ -2388,27 +2363,27 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_NOARG),\n \t\tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n \t\t\tN_(\"override error message when patch failure occurs\")),\n-\t\tOPT_CMDMODE(0, \"continue\", &resume.mode,\n+\t\tOPT_CMDMODE(0, \"continue\", &resume_mode,\n \t\t\tN_(\"continue applying patches after resolving a conflict\"),\n \t\t\tRESUME_RESOLVED),\n-\t\tOPT_CMDMODE('r', \"resolved\", &resume.mode,\n+\t\tOPT_CMDMODE('r', \"resolved\", &resume_mode,\n \t\t\tN_(\"synonyms for --continue\"),\n \t\t\tRESUME_RESOLVED),\n-\t\tOPT_CMDMODE(0, \"skip\", &resume.mode,\n+\t\tOPT_CMDMODE(0, \"skip\", &resume_mode,\n \t\t\tN_(\"skip the current patch\"),\n \t\t\tRESUME_SKIP),\n-\t\tOPT_CMDMODE(0, \"abort\", &resume.mode,\n+\t\tOPT_CMDMODE(0, \"abort\", &resume_mode,\n \t\t\tN_(\"restore the original branch and abort the patching operation\"),\n \t\t\tRESUME_ABORT),\n-\t\tOPT_CMDMODE(0, \"quit\", &resume.mode,\n+\t\tOPT_CMDMODE(0, \"quit\", &resume_mode,\n \t\t\tN_(\"abort the patching operation but keep HEAD where it is\"),\n \t\t\tRESUME_QUIT),\n-\t\t{ OPTION_CALLBACK, 0, \"show-current-patch\", &resume.mode,\n+\t\t{ OPTION_CALLBACK, 0, \"show-current-patch\", &resume_mode,\n \t\t  \"(diff|raw)\",\n \t\t  N_(\"show the patch being applied\"),\n \t\t  PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n-\t\t  parse_opt_show_current_patch, RESUME_SHOW_PATCH },\n-\t\tOPT_CMDMODE(0, \"allow-empty\", &resume.mode,\n+\t\t  parse_opt_show_current_patch, RESUME_SHOW_PATCH_RAW },\n+\t\tOPT_CMDMODE(0, \"allow-empty\", &resume_mode,\n \t\t\tN_(\"record the empty patch as an empty commit\"),\n \t\t\tRESUME_ALLOW_EMPTY),\n \t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n@@ -2463,12 +2438,12 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t *    intend to feed us a patch but wanted to continue\n \t\t *    unattended.\n \t\t */\n-\t\tif (argc || (resume.mode == RESUME_FALSE && !isatty(0)))\n+\t\tif (argc || (resume_mode == RESUME_FALSE && !isatty(0)))\n \t\t\tdie(_(\"previous rebase directory %s still exists but mbox given.\"),\n \t\t\t\tstate.dir);\n\n-\t\tif (resume.mode == RESUME_FALSE)\n-\t\t\tresume.mode = RESUME_APPLY;\n+\t\tif (resume_mode == RESUME_FALSE)\n+\t\t\tresume_mode = RESUME_APPLY;\n\n \t\tif (state.signoff == SIGNOFF_EXPLICIT)\n \t\t\tam_append_signoff(&state);\n@@ -2482,7 +2457,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t * stray directories.\n \t\t */\n \t\tif (file_exists(state.dir) && !state.rebasing) {\n-\t\t\tif (resume.mode == RESUME_ABORT || resume.mode == RESUME_QUIT) {\n+\t\t\tif (resume_mode == RESUME_ABORT || resume_mode == RESUME_QUIT) {\n \t\t\t\tam_destroy(&state);\n \t\t\t\tam_state_release(&state);\n \t\t\t\treturn 0;\n@@ -2493,7 +2468,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate.dir);\n \t\t}\n\n-\t\tif (resume.mode)\n+\t\tif (resume_mode)\n \t\t\tdie(_(\"Resolve operation not in progress, we are not resuming.\"));\n\n \t\tfor (i = 0; i < argc; i++) {\n@@ -2511,7 +2486,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tstrvec_clear(&paths);\n \t}\n\n-\tswitch (resume.mode) {\n+\tswitch (resume_mode) {\n \tcase RESUME_FALSE:\n \t\tam_run(&state, 0);\n \t\tbreak;\n@@ -2520,7 +2495,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \tcase RESUME_RESOLVED:\n \tcase RESUME_ALLOW_EMPTY:\n-\t\tam_resolve(&state, resume.mode == RESUME_ALLOW_EMPTY ? 1 : 0);\n+\t\tam_resolve(&state, resume_mode == RESUME_ALLOW_EMPTY ? 1 : 0);\n \t\tbreak;\n \tcase RESUME_SKIP:\n \t\tam_skip(&state);\n@@ -2532,8 +2507,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tam_rerere_clear();\n \t\tam_destroy(&state);\n \t\tbreak;\n-\tcase RESUME_SHOW_PATCH:\n-\t\tret = show_patch(&state, resume.sub_mode);\n+\tcase RESUME_SHOW_PATCH_RAW:\n+\tcase RESUME_SHOW_PATCH_DIFF:\n+\t\tret = show_patch(&state, resume_mode);\n \t\tbreak;\n \tdefault:\n \t\tBUG(\"invalid resume value\");\n--\n2.42.0\n"},{"id":"484061","messageId":"xmqqfs1tge7q.fsf@gitster.g","threadId":"60438","inReplyTo":"cefdba32-db0b-4f68-954e-9d31fc12b1a0@web.de","subject":"Re: [PATCH 1/2] parse-options: make CMDMODE errors more precise","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-30T00:12:57Z","receivedAt":"2023-10-30T00:13:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Store the argument of PARSE_OPT_CMDMODE options of type OPTION_CALLBACK\n> as well to allow taking over the responsibility for compatibility\n> checking from the callback function.  The next patch will use this\n> capability to fix the messages for git am --show-current-patch.\n>\n> Use a linked list for storing the PARSE_OPT_CMDMODE variables.  This\n> somewhat outdated data structure is simple and suffices, as the number\n> of elements per command is currently only zero or one.  We do support\n> multiple different command modes variables per command, but I don't\n> expect that we'd ever use a significant number of them.  Once we do we\n> can switch to a hashmap.\n\nMakes quite a lot of sense.  I would have expected to see this in\nthe parse_options_check() function, where other sanity checks are\ndone, but I think the reason you added the call to its caller\nbecause parse_options_check() does not take the context.\n\nIt is not like we want to perform a sanity check that is independent\nfrom a particular invocation of parse_options() on the options[]\narray only just once, and want to reuse the array that is known to\nbe sane multiple times.  The check is called once for every\ninvocation, so with or without this change, I do not see a good\nreason for parse_options_check() not to take context, though.\n\n> +static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n> +\t\t\t\t       const struct option *opt,\n> +\t\t\t\t       enum opt_parsed flags)\n> +{\n> +\tconst char *arg = NULL;\n> +\tenum parse_opt_result result = do_get_value(p, opt, flags, &arg);\n> +\tstruct parse_opt_cmdmode_list *elem = p->cmdmode_list;\n> +\tchar *opt_name, *other_opt_name;\n> +\n> +\tfor (; elem; elem = elem->next) {\n> +\t\tif (*elem->value_ptr == elem->value)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (elem->opt &&\n> +\t\t    (elem->opt->flags | opt->flags) & PARSE_OPT_CMDMODE)\n> +\t\t\tbreak;\n> +\n> +\t\telem->opt = opt;\n> +\t\telem->arg = arg;\n> +\t\telem->flags = flags;\n> +\t\telem->value = *elem->value_ptr;\n> +\t}\n> +\n> +\tif (result || !elem)\n> +\t\treturn result;\n> +\n> +\topt_name = optnamearg(opt, arg, flags);\n> +\tother_opt_name = optnamearg(elem->opt, elem->arg, elem->flags);\n> +\terror(_(\"%s is incompatible with %s\"), opt_name, other_opt_name);\n> +\tfree(opt_name);\n> +\tfree(other_opt_name);\n> +\treturn -1;\n> +}\n\nLooks quite involved but the overhead is number of supported cmdmode\noptions per each command line option, and the problems outlined in\nthe proposed log message are worth addressing.  OK.\n\n> @@ -1006,6 +1041,11 @@ int parse_options(int argc, const char **argv,\n>  \tprecompose_argv_prefix(argc, argv, NULL);\n>  \tfree_preprocessed_options(real_options);\n>  \tfree(ctx.alias_groups);\n> +\tfor (struct parse_opt_cmdmode_list *elem = ctx.cmdmode_list; elem;) {\n> +\t\tstruct parse_opt_cmdmode_list *next = elem->next;\n> +\t\tfree(elem);\n> +\t\telem = next;\n> +\t}\n>  \treturn parse_options_end(&ctx);\n>  }\n\nOK.\n\n"}]}