From patchwork Sat Oct 28 11:53:01 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Ren=C3=A9_Scharfe?= X-Patchwork-Id: 13439455 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id ABFFAB65D for ; Sat, 28 Oct 2023 11:53:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=web.de header.i=l.s.r@web.de header.b="R7Kex3J0" Received: from mout.web.de (mout.web.de [212.227.17.11]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8698FE5 for ; Sat, 28 Oct 2023 04:53:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=web.de; s=s29768273; t=1698493982; x=1699098782; i=l.s.r@web.de; bh=ZB/NM6UY+RNDou+YEieD0pazBnbGgF4b6U90Xof2Txk=; h=X-UI-Sender-Class:Date:To:Cc:From:Subject; b=R7Kex3J0P44O41i5JlLYV9BH3qoNU9bKY5PVjuzFIjK6TOz34WfLKOEQbh800U1x yL5eRhtrDKls7mR4kAWl6SfOFh5ioGnwcy5biFIXq1viJTmfwXiT6iUEw5uwXoLwC ORWVaNqyjqZzKZ1c+lcBVkPasgf5ng5eHBwAJSDcZSQnuqrWN8oK3CcM+CVaophB2 0xxuUNXZCyDLOAXj5v1dZllihUnca37BSY4I0ZfDTZgvyBD2NKuNM9I+1R66Au81o zQ9WZrvsQzxmyIANRo32NG/IwRu+uhc/cekgZmNn06o1Ls6tjrBK+KYCHKWPLxDhC IMQrI/jU99h5JvRBCA== X-UI-Sender-Class: 814a7b36-bfc1-4dae-8640-3722d8ec6cd6 Received: from [192.168.178.29] ([91.47.159.209]) by smtp.web.de (mrweb106 [213.165.67.124]) with ESMTPSA (Nemesis) id 1MsrhK-1rl6fN3rOy-00t6pc; Sat, 28 Oct 2023 13:53:01 +0200 Message-ID: Date: Sat, 28 Oct 2023 13:53:01 +0200 Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Git List Cc: Junio C Hamano From: =?utf-8?q?Ren=C3=A9_Scharfe?= Subject: [PATCH 1/2] parse-options: make CMDMODE errors more precise X-Provags-ID: V03:K1:0AZT+5YN2Kho70mIVd4OAYZ2V5JNq8Sm/VM1876mCIu3nHOCgSN pV87upWfpXI+HcrekWCkzqxVtUj8gbcSmCoW/oM55HWutfpCDhgf1LljNM5YPWnAfBzUcgd JHNx2TfFs01hqJnuLbyziqtY7hdJPtYG37I1UaCraCxOt1Yilp9EL+6Xqofwi/E8kznyMxP r3jzV9kt8dA1WbKrRm+9g== UI-OutboundReport: notjunk:1;M01:P0:MX5OZpz8RrI=;oKVxAVX/Qrpc2egdUrkz4JF/I7S +iFG+c9c1k8zYUsZ52WtMuOP2tK+89B/Q8fvNtwnXSZef2NRTS1PTkXh59wZSq0bO3lP99wsr bkZ6w5Ys3jIKi8mFcPo/5zaRCzM8+MWyii2gnDc2Hl1fZq1Kj9v1XuTRyxS5LB3onekV+RAjL MEFA0PtOh0LtfvhmIfxlvPV42pTBmoJVLFG0fdiOWwzTmJ2DqAOPi9Lc2b+EuB8+aMeeRsQW7 yVcfFNJn0KDgzA0gMjbquPEvVJNt0vUY2QvLgQPx0sGrQl9Dihi2k7ZEfwYye9k8MDkH8HjAh Fwr7/Xj2/8el+oaS4/FTbHOu6rK/h+BxMuwWO0hbgY4ZJOei/lrgkCnG5VLAmHHmvvmRwUcE0 4Xf36JV3LBjxf2nY3b/d7b5bUAAI0WBbvrG8pWQ2AryH4MiVUQI5nVjx6cxA2of5W5/Yr74to lzjtO/3+tHKlgBzlN+jsh+vzN42onW253MVpSCgKg81nX1wzlXqvk91XtxA618zKq4a0b059v FWKFEYeE00Oz166cpaC9RubFd4sQCOpqhtQnkaF52Isxq06rKuVCeB8V0/t7v/ptCv7WUIBQM 1GlEtLuimjh2u14NL1uZZ4pbnTTroZb3rxobQ5gsuWXg4ssf82yy/ko2H/g5ljy/4oWiaiVHL SWjUmo4KvX/MMheZnAbtHxyHaz3kA2YI8fSbBgDlj+wn27PcYWJGUQ8Z9RGJAdx0Uc6ekfR3D mw4ttllmjF8PlMBIjCLURPQkCR18aeq2xQk9ctATIun0HEbD9IevlCaEf3HJNVtYeebiCZ2wj vuZO2lRVO1IhAnILFIRggyuJUXEKlrZc7sRUif3L2c977b9qka+xoAtJfpbQqFFE50vS62r+O aAMEp2tIRDlP7dZvwEcHrYY5Gk0/pfVGqiRjuLSgOTbWNoqiOXgTvFk725m/m8LfmHi/0sqSA iCh1lMsxhFrmgMfoiE7b7zbxXoU= Only a single PARSE_OPT_CMDMODE option can be specified for the same variable at the same time. This is enforced by get_value(), but the error messages are imprecise in three ways: 1. If a non-PARSE_OPT_CMDMODE option changes the value variable of a PARSE_OPT_CMDMODE option then an ominously vague message is shown: $ t/helper/test-tool parse-options --set23 --mode1 error: option `mode1' : incompatible with something else Worse: If the order of options is reversed then no error is reported at all: $ t/helper/test-tool parse-options --mode1 --set23 boolean: 0 integer: 23 magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 verbose: -1 quiet: 0 dry run: no file: (not set) Fortunately this can currently only happen in the test helper; actual Git commands don't share the same variable for the value of options with and without the flag PARSE_OPT_CMDMODE. 2. If there are multiple options with the same value (synonyms), then the one that is defined first is shown rather than the one actually given on the command line, which is confusing: $ git am --resolved --quit error: option `quit' is incompatible with --continue 3. Arguments of PARSE_OPT_CMDMODE options are not handled by the parse-option machinery. This is left to the callback function. We currently only have a single affected option, --show-current-patch of git am. Errors for it can show an argument that was not actually given on the command line: $ git am --show-current-patch --show-current-patch=diff error: options '--show-current-patch=diff' and '--show-current-patch=raw' cannot be used together The options --show-current-patch and --show-current-patch=raw are synonyms, but the error accuses the user of input they did not actually made. Or it can awkwardly print a NULL pointer: $ git am --show-current-patch=diff --show-current-patch error: options '--show-current-patch=(null)' and '--show-current-patch=diff' cannot be used together The reasons for these shortcomings is that the current code checks incompatibility only when encountering a PARSE_OPT_CMDMODE option at the command line, and that it searches the previous incompatible option by value. Fix the first two points by checking all PARSE_OPT_CMDMODE variables after parsing each option and by storing all relevant details if their value changed. Do that whether or not the changing options has the flag PARSE_OPT_CMDMODE set. Report an incompatibility only if two options change the variable to different values and at least one of them is a PARSE_OPT_CMDMODE option. This changes the output of the first three examples above to: $ t/helper/test-tool parse-options --set23 --mode1 error: --mode1 is incompatible with --set23 $ t/helper/test-tool parse-options --mode1 --set23 error: --set23 is incompatible with --mode1 $ git am --resolved --quit error: --quit is incompatible with --resolved Store the argument of PARSE_OPT_CMDMODE options of type OPTION_CALLBACK as well to allow taking over the responsibility for compatibility checking from the callback function. The next patch will use this capability to fix the messages for git am --show-current-patch. Use a linked list for storing the PARSE_OPT_CMDMODE variables. This somewhat outdated data structure is simple and suffices, as the number of elements per command is currently only zero or one. We do support multiple different command modes variables per command, but I don't expect that we'd ever use a significant number of them. Once we do we can switch to a hashmap. Since we no longer need to search the conflicting option, the all_opts parameter of get_value() is no longer used. Remove it. Extend the tests to check for both conflicting option names, but don't insist on a particular order. Signed-off-by: René Scharfe --- parse-options.c | 144 ++++++++++++++++++++++------------ parse-options.h | 3 + t/helper/test-parse-options.c | 16 ++++ t/t0040-parse-options.sh | 33 ++++++-- 4 files changed, 139 insertions(+), 57 deletions(-) -- 2.42.0 diff --git a/parse-options.c b/parse-options.c index 093eaf2db8..e0c94b0546 100644 --- a/parse-options.c +++ b/parse-options.c @@ -70,42 +70,10 @@ static void fix_filename(const char *prefix, char **file) *file = prefix_filename_except_for_dash(prefix, *file); } -static enum parse_opt_result opt_command_mode_error( - const struct option *opt, - const struct option *all_opts, - enum opt_parsed flags) -{ - const struct option *that; - struct strbuf that_name = STRBUF_INIT; - - /* - * Find the other option that was used to set the variable - * already, and report that this is not compatible with it. - */ - for (that = all_opts; that->type != OPTION_END; that++) { - if (that == opt || - !(that->flags & PARSE_OPT_CMDMODE) || - that->value != opt->value || - that->defval != *(int *)opt->value) - continue; - - if (that->long_name) - strbuf_addf(&that_name, "--%s", that->long_name); - else - strbuf_addf(&that_name, "-%c", that->short_name); - error(_("%s is incompatible with %s"), - optname(opt, flags), that_name.buf); - strbuf_release(&that_name); - return PARSE_OPT_ERROR; - } - return error(_("%s : incompatible with something else"), - optname(opt, flags)); -} - -static enum parse_opt_result get_value(struct parse_opt_ctx_t *p, - const struct option *opt, - const struct option *all_opts, - enum opt_parsed flags) +static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p, + const struct option *opt, + enum opt_parsed flags, + const char **argp) { const char *s, *arg; const int unset = flags & OPT_UNSET; @@ -118,14 +86,6 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p, if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG)) return error(_("%s takes no value"), optname(opt, flags)); - /* - * Giving the same mode option twice, although unnecessary, - * is not a grave error, so let it pass. - */ - if ((opt->flags & PARSE_OPT_CMDMODE) && - *(int *)opt->value && *(int *)opt->value != opt->defval) - return opt_command_mode_error(opt, all_opts, flags); - switch (opt->type) { case OPTION_LOWLEVEL_CALLBACK: return opt->ll_callback(p, opt, NULL, unset); @@ -200,6 +160,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p, p_unset = 0; p_arg = arg; } + if (opt->flags & PARSE_OPT_CMDMODE) + *argp = p_arg; if (opt->callback) return (*opt->callback)(opt, p_arg, p_unset) ? (-1) : 0; else @@ -247,16 +209,91 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p, } } +struct parse_opt_cmdmode_list { + int value, *value_ptr; + const struct option *opt; + const char *arg; + enum opt_parsed flags; + struct parse_opt_cmdmode_list *next; +}; + +static void build_cmdmode_list(struct parse_opt_ctx_t *ctx, + const struct option *opts) +{ + ctx->cmdmode_list = NULL; + + for (; opts->type != OPTION_END; opts++) { + struct parse_opt_cmdmode_list *elem = ctx->cmdmode_list; + int *value_ptr = opts->value; + + if (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr) + continue; + + while (elem && elem->value_ptr != value_ptr) + elem = elem->next; + if (elem) + continue; + + CALLOC_ARRAY(elem, 1); + elem->value_ptr = value_ptr; + elem->value = *value_ptr; + elem->next = ctx->cmdmode_list; + ctx->cmdmode_list = elem; + } +} + +static char *optnamearg(const struct option *opt, const char *arg, + enum opt_parsed flags) +{ + if (flags & OPT_SHORT) + return xstrfmt("-%c%s", opt->short_name, arg ? arg : ""); + return xstrfmt("--%s%s%s%s", flags & OPT_UNSET ? "no-" : "", + opt->long_name, arg ? "=" : "", arg ? arg : ""); +} + +static enum parse_opt_result get_value(struct parse_opt_ctx_t *p, + const struct option *opt, + enum opt_parsed flags) +{ + const char *arg = NULL; + enum parse_opt_result result = do_get_value(p, opt, flags, &arg); + struct parse_opt_cmdmode_list *elem = p->cmdmode_list; + char *opt_name, *other_opt_name; + + for (; elem; elem = elem->next) { + if (*elem->value_ptr == elem->value) + continue; + + if (elem->opt && + (elem->opt->flags | opt->flags) & PARSE_OPT_CMDMODE) + break; + + elem->opt = opt; + elem->arg = arg; + elem->flags = flags; + elem->value = *elem->value_ptr; + } + + if (result || !elem) + return result; + + opt_name = optnamearg(opt, arg, flags); + other_opt_name = optnamearg(elem->opt, elem->arg, elem->flags); + error(_("%s is incompatible with %s"), opt_name, other_opt_name); + free(opt_name); + free(other_opt_name); + return -1; +} + static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p, const struct option *options) { - const struct option *all_opts = options; const struct option *numopt = NULL; for (; options->type != OPTION_END; options++) { if (options->short_name == *p->opt) { p->opt = p->opt[1] ? p->opt + 1 : NULL; - return get_value(p, options, all_opts, OPT_SHORT); + return get_value(p, options, OPT_SHORT); } /* @@ -318,7 +355,6 @@ static enum parse_opt_result parse_long_opt( struct parse_opt_ctx_t *p, const char *arg, const struct option *options) { - const struct option *all_opts = options; const char *arg_end = strchrnul(arg, '='); const struct option *abbrev_option = NULL, *ambiguous_option = NULL; enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG; @@ -387,7 +423,7 @@ static enum parse_opt_result parse_long_opt( continue; p->opt = rest + 1; } - return get_value(p, options, all_opts, flags ^ opt_flags); + return get_value(p, options, flags ^ opt_flags); } if (disallow_abbreviated_options && (ambiguous_option || abbrev_option)) @@ -405,7 +441,7 @@ static enum parse_opt_result parse_long_opt( return PARSE_OPT_HELP; } if (abbrev_option) - return get_value(p, abbrev_option, all_opts, abbrev_flags); + return get_value(p, abbrev_option, abbrev_flags); return PARSE_OPT_UNKNOWN; } @@ -413,13 +449,11 @@ static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg, const struct option *options) { - const struct option *all_opts = options; - for (; options->type != OPTION_END; options++) { if (!(options->flags & PARSE_OPT_NODASH)) continue; if (options->short_name == arg[0] && arg[1] == '\0') - return get_value(p, options, all_opts, OPT_SHORT); + return get_value(p, options, OPT_SHORT); } return PARSE_OPT_ERROR; } @@ -574,6 +608,7 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx, (flags & PARSE_OPT_KEEP_ARGV0)) BUG("Can't keep argv0 if you don't have it"); parse_options_check(options); + build_cmdmode_list(ctx, options); } void parse_options_start(struct parse_opt_ctx_t *ctx, @@ -1006,6 +1041,11 @@ int parse_options(int argc, const char **argv, precompose_argv_prefix(argc, argv, NULL); free_preprocessed_options(real_options); free(ctx.alias_groups); + for (struct parse_opt_cmdmode_list *elem = ctx.cmdmode_list; elem;) { + struct parse_opt_cmdmode_list *next = elem->next; + free(elem); + elem = next; + } return parse_options_end(&ctx); } diff --git a/parse-options.h b/parse-options.h index 4a66ec3bf5..bd62e20268 100644 --- a/parse-options.h +++ b/parse-options.h @@ -445,6 +445,8 @@ static inline void die_for_incompatible_opt3(int opt1, const char *opt1_name, /*----- incremental advanced APIs -----*/ +struct parse_opt_cmdmode_list; + /* * It's okay for the caller to consume argv/argc in the usual way. * Other fields of that structure are private to parse-options and should not @@ -459,6 +461,7 @@ struct parse_opt_ctx_t { unsigned has_subcommands; const char *prefix; const char **alias_groups; /* must be in groups of 3 elements! */ + struct parse_opt_cmdmode_list *cmdmode_list; }; void parse_options_start(struct parse_opt_ctx_t *ctx, diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index a4f6e24b0c..ded8116cc5 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -21,6 +21,19 @@ static struct { int unset; } length_cb; +static int mode34_callback(const struct option *opt, const char *arg, int unset) +{ + if (unset) + *(int *)opt->value = 0; + else if (!strcmp(arg, "3")) + *(int *)opt->value = 3; + else if (!strcmp(arg, "4")) + *(int *)opt->value = 4; + else + return error("invalid value for '%s': '%s'", "--mode34", arg); + return 0; +} + static int length_callback(const struct option *opt, const char *arg, int unset) { length_cb.called = 1; @@ -124,6 +137,9 @@ int cmd__parse_options(int argc, const char **argv) OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23), OPT_CMDMODE(0, "mode1", &integer, "set integer to 1 (cmdmode option)", 1), OPT_CMDMODE(0, "mode2", &integer, "set integer to 2 (cmdmode option)", 2), + OPT_CALLBACK_F(0, "mode34", &integer, "(3|4)", + "set integer to 3 or 4 (cmdmode option)", + PARSE_OPT_CMDMODE, mode34_callback), OPT_CALLBACK('L', "length", &integer, "str", "get length of ", length_callback), OPT_FILENAME('F', "file", &file, "set file to "), diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index a0ad6192d6..06fb9e6457 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -28,6 +28,7 @@ usage: test-tool parse-options --[no-]set23 set integer to 23 --mode1 set integer to 1 (cmdmode option) --mode2 set integer to 2 (cmdmode option) + --[no-]mode34 (3|4) set integer to 3 or 4 (cmdmode option) -L, --[no-]length get length of -F, --[no-]file @@ -366,19 +367,41 @@ test_expect_success 'OPT_NEGBIT() works' ' ' test_expect_success 'OPT_CMDMODE() works' ' - test-tool parse-options --expect="integer: 1" --mode1 + test-tool parse-options --expect="integer: 1" --mode1 && + test-tool parse-options --expect="integer: 3" --mode34=3 ' -test_expect_success 'OPT_CMDMODE() detects incompatibility' ' +test_expect_success 'OPT_CMDMODE() detects incompatibility (1)' ' test_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err && test_must_be_empty output && - test_i18ngrep "incompatible with --mode" output.err + test_i18ngrep "mode1" output.err && + test_i18ngrep "mode2" output.err && + test_i18ngrep "is incompatible with" output.err ' -test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' ' +test_expect_success 'OPT_CMDMODE() detects incompatibility (2)' ' test_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err && test_must_be_empty output && - test_i18ngrep "incompatible with something else" output.err + test_i18ngrep "mode2" output.err && + test_i18ngrep "set23" output.err && + test_i18ngrep "is incompatible with" output.err +' + +test_expect_success 'OPT_CMDMODE() detects incompatibility (3)' ' + test_must_fail test-tool parse-options --mode2 --set23 >output 2>output.err && + test_must_be_empty output && + test_i18ngrep "mode2" output.err && + test_i18ngrep "set23" output.err && + test_i18ngrep "is incompatible with" output.err +' + +test_expect_success 'OPT_CMDMODE() detects incompatibility (4)' ' + test_must_fail test-tool parse-options --mode2 --mode34=3 \ + >output 2>output.err && + test_must_be_empty output && + test_i18ngrep "mode2" output.err && + test_i18ngrep "mode34.3" output.err && + test_i18ngrep "is incompatible with" output.err ' test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' ' From patchwork Sat Oct 28 11:57:44 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Ren=C3=A9_Scharfe?= X-Patchwork-Id: 13439458 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 756FC525F for ; Sat, 28 Oct 2023 11:57:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=web.de header.i=l.s.r@web.de header.b="rHtVctQP" Received: from mout.web.de (mout.web.de [217.72.192.78]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6DE5EE1 for ; Sat, 28 Oct 2023 04:57:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=web.de; s=s29768273; t=1698494264; x=1699099064; i=l.s.r@web.de; bh=H8XrgXj2Ns8sHH+fJjQnWPXQ3+sYiCY35nPPIASi08g=; h=X-UI-Sender-Class:Date:Subject:From:To:Cc:References: In-Reply-To; b=rHtVctQPGeVoTRAFfp+hp/N6F3dEctESJwXcm2QG3Rpd7rstIVaAdOU+fYLQZVqO 5HjQHlv5vkzdB2bAkZKBI0znIYm/u8ftLnSB+St4JtLtZ/VzOPtHky376Gnev+p99 2k3JaTu7cH7GpoFVtRiSJue2aepWUgR1ZZeY5TiqI278sm4x78VzwJbB/FACXuKku +lX0b3DR7S/COgi3WFZprxQe2uZ3/P1Ya+A3RN+TaaOsmET4aPyOMS5gKbaRtZtQQ XYGscYrIwhtyMlSyc9EYS6vd9J2hRCvOspPnJUgTjNn3ptt2dbEfGwHaIopCRgaz9 LfgboRIcycKKhjuQpA== X-UI-Sender-Class: 814a7b36-bfc1-4dae-8640-3722d8ec6cd6 Received: from [192.168.178.29] ([91.47.159.209]) by smtp.web.de (mrweb106 [213.165.67.124]) with ESMTPSA (Nemesis) id 1M5QmF-1qvsT73Hmv-001YBn; Sat, 28 Oct 2023 13:57:44 +0200 Message-ID: <4520156b-9418-493c-a50c-e61b42e805b3@web.de> Date: Sat, 28 Oct 2023 13:57:44 +0200 Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: [PATCH 2/2] am: simplify --show-current-patch handling Content-Language: en-US From: =?utf-8?q?Ren=C3=A9_Scharfe?= To: Git List Cc: Junio C Hamano References: In-Reply-To: X-Provags-ID: V03:K1:Qc6Jzn5+CjF+9jTUBZ6unah17NPXbio5Oq2KBQO5jUTEWVhv7O7 15AdzhHxaoRlDeRzwalCejfAXahkQJlpMRDxoKk+VNNbCufkAXZxmZNQusooGo7jrbdtQRF f21JENORw9jxAFGONu+iAro04keFHpvGbpH3qDtKtwrxLosHk1HEkwF85rBwvXniJ7rEBVf z3MEd9QwYoLEalJ5phmfQ== UI-OutboundReport: notjunk:1;M01:P0:cIWOPPgIM0s=;PwDWeKIJpmgsQF4UkDfC3vLqjOS TVRpsrDTkeX1fwJVOPczRIXabz25+5Hd0uaDI+sgJAZzyX2M+CSv8gFFaSEAIQqsyFeeTPlcT YyPA6jIIl1Ybm86AkGNFJAZbOWk4bLBWQxMdm2Syb3DwvGL+tozOEq1RfbTWDunaE8wWWFjFM vp6L00FyGUTZ9jdbBPaar+HEY6WT8d8PzAHsQZ0vtV05ehHFzi+ZfM3ZshvgJ+Yn0S+A4YTER kMpwAtnpJKSFwW7pUcVhqb5JXlNHXzyJG1y7xS5vgBIcwPOxTRx2QL4Sg/wtPSJGfmM4FoQpn FAhUe1xnFCJJeB5Zd4Gp/zUprKwNazhO7XMNhG272ZwdxHqAXa5MzB1zbi809y+6hVVw3ewYn FikN/0AahwwBdQSFpFISAdtcYYcidEkC7pzFrJVvQIO7fuXlaGWmjeRyCPeBLaYshVGSyHno/ g2IBspnMxMeXcKKQ7v/QLKzkOltvRJoltkKiCUu51wUR7S2nlQcKu0ox87YP98LmA9FENIEK+ iJrM0MnROkHqDRBKYBp8I74mnVEo/6mSlQ36Ls6WI4w/7N93srMD0AoXjNEZSLxUY/RBbePXs xjx1wy3xGMPQr0kJLa98ZlwRxtm0eFn0kKwgqCWB+AVVhf3voONG+bnxBATK2aOXPgL3/5gpM kGnjTmELkWoF2Lot8EimsGy0Fk1h02V5d2PsFbdEka+HP+UYe9lW9cfh2UbC6zOsMw6OeU/4A riMhsfdi2M8X5t4fmwIKiYzqlooeG64qVJ7e5xFyyaTNhL4dme06FxyOBo77tjxQsLdZjxHfN fLGkhuGx2IJHn6bK+j6+Jz0p8xt9/ygTQEm6HjLzr6DNXvP6/PTdIDVTV8ovkXXnaKa2kWeFz 5NSDCEw0tIiv2hgqM6mirVGfy3zXLrjA+qmS/P07T/MsZMLDz9jMvWqOCIxQXjYgSpQTs1OBd UVCL9K1mlqCoh+RJNEdr9awv/TM= Let the parse-options code detect and handle the use of options that are incompatible with --show-current-patch. This requires exposing the distinction between the "raw" and "diff" sub-modes. Do that by splitting the mode RESUME_SHOW_PATCH into RESUME_SHOW_PATCH_RAW and RESUME_SHOW_PATCH_DIFF and stop tracking sub-modes in a separate struct. The result is a simpler callback function and more precise error messages. The original reports a spurious argument or a NULL pointer: $ git am --show-current-patch --show-current-patch=diff error: options '--show-current-patch=diff' and '--show-current-patch=raw' cannot be used together $ git am --show-current-patch=diff --show-current-patch error: options '--show-current-patch=(null)' and '--show-current-patch=diff' cannot be used together With this patch we get the more precise: $ git am --show-current-patch --show-current-patch=diff error: --show-current-patch=diff is incompatible with --show-current-patch $ git am --show-current-patch=diff --show-current-patch error: --show-current-patch is incompatible with --show-current-patch=diff Signed-off-by: René Scharfe --- builtin/am.c | 112 ++++++++++++++++++++------------------------------- 1 file changed, 44 insertions(+), 68 deletions(-) -- 2.42.0 diff --git a/builtin/am.c b/builtin/am.c index 6655059a57..ef5b9459af 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -92,9 +92,16 @@ enum signoff_type { SIGNOFF_EXPLICIT /* --signoff was set on the command-line */ }; -enum show_patch_type { - SHOW_PATCH_RAW = 0, - SHOW_PATCH_DIFF = 1, +enum resume_type { + RESUME_FALSE = 0, + RESUME_APPLY, + RESUME_RESOLVED, + RESUME_SKIP, + RESUME_ABORT, + RESUME_QUIT, + RESUME_SHOW_PATCH_RAW, + RESUME_SHOW_PATCH_DIFF, + RESUME_ALLOW_EMPTY, }; enum empty_action { @@ -2191,7 +2198,7 @@ static void am_abort(struct am_state *state) am_destroy(state); } -static int show_patch(struct am_state *state, enum show_patch_type sub_mode) +static int show_patch(struct am_state *state, enum resume_type resume_mode) { struct strbuf sb = STRBUF_INIT; const char *patch_path; @@ -2206,11 +2213,11 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode) return run_command(&cmd); } - switch (sub_mode) { - case SHOW_PATCH_RAW: + switch (resume_mode) { + case RESUME_SHOW_PATCH_RAW: patch_path = am_path(state, msgnum(state)); break; - case SHOW_PATCH_DIFF: + case RESUME_SHOW_PATCH_DIFF: patch_path = am_path(state, "patch"); break; default: @@ -2257,57 +2264,25 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int return 0; } -enum resume_type { - RESUME_FALSE = 0, - RESUME_APPLY, - RESUME_RESOLVED, - RESUME_SKIP, - RESUME_ABORT, - RESUME_QUIT, - RESUME_SHOW_PATCH, - RESUME_ALLOW_EMPTY, -}; - -struct resume_mode { - enum resume_type mode; - enum show_patch_type sub_mode; -}; - static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset) { int *opt_value = opt->value; - struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode); + BUG_ON_OPT_NEG(unset); + + if (!arg) + *opt_value = opt->defval; + else if (!strcmp(arg, "raw")) + *opt_value = RESUME_SHOW_PATCH_RAW; + else if (!strcmp(arg, "diff")) + *opt_value = RESUME_SHOW_PATCH_DIFF; /* * Please update $__git_showcurrentpatch in git-completion.bash * when you add new options */ - const char *valid_modes[] = { - [SHOW_PATCH_DIFF] = "diff", - [SHOW_PATCH_RAW] = "raw" - }; - int new_value = SHOW_PATCH_RAW; - - BUG_ON_OPT_NEG(unset); - - if (arg) { - for (new_value = 0; new_value < ARRAY_SIZE(valid_modes); new_value++) { - if (!strcmp(arg, valid_modes[new_value])) - break; - } - if (new_value >= ARRAY_SIZE(valid_modes)) - return error(_("invalid value for '%s': '%s'"), - "--show-current-patch", arg); - } - - if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode) - return error(_("options '%s=%s' and '%s=%s' " - "cannot be used together"), - "--show-current-patch", arg, - "--show-current-patch", valid_modes[resume->sub_mode]); - - resume->mode = RESUME_SHOW_PATCH; - resume->sub_mode = new_value; + else + return error(_("invalid value for '%s': '%s'"), + "--show-current-patch", arg); return 0; } @@ -2317,7 +2292,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) int binary = -1; int keep_cr = -1; int patch_format = PATCH_FORMAT_UNKNOWN; - struct resume_mode resume = { .mode = RESUME_FALSE }; + enum resume_type resume_mode = RESUME_FALSE; int in_progress; int ret = 0; @@ -2388,27 +2363,27 @@ int cmd_am(int argc, const char **argv, const char *prefix) PARSE_OPT_NOARG), OPT_STRING(0, "resolvemsg", &state.resolvemsg, NULL, N_("override error message when patch failure occurs")), - OPT_CMDMODE(0, "continue", &resume.mode, + OPT_CMDMODE(0, "continue", &resume_mode, N_("continue applying patches after resolving a conflict"), RESUME_RESOLVED), - OPT_CMDMODE('r', "resolved", &resume.mode, + OPT_CMDMODE('r', "resolved", &resume_mode, N_("synonyms for --continue"), RESUME_RESOLVED), - OPT_CMDMODE(0, "skip", &resume.mode, + OPT_CMDMODE(0, "skip", &resume_mode, N_("skip the current patch"), RESUME_SKIP), - OPT_CMDMODE(0, "abort", &resume.mode, + OPT_CMDMODE(0, "abort", &resume_mode, N_("restore the original branch and abort the patching operation"), RESUME_ABORT), - OPT_CMDMODE(0, "quit", &resume.mode, + OPT_CMDMODE(0, "quit", &resume_mode, N_("abort the patching operation but keep HEAD where it is"), RESUME_QUIT), - { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode, + { OPTION_CALLBACK, 0, "show-current-patch", &resume_mode, "(diff|raw)", N_("show the patch being applied"), PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, - parse_opt_show_current_patch, RESUME_SHOW_PATCH }, - OPT_CMDMODE(0, "allow-empty", &resume.mode, + parse_opt_show_current_patch, RESUME_SHOW_PATCH_RAW }, + OPT_CMDMODE(0, "allow-empty", &resume_mode, N_("record the empty patch as an empty commit"), RESUME_ALLOW_EMPTY), OPT_BOOL(0, "committer-date-is-author-date", @@ -2463,12 +2438,12 @@ int cmd_am(int argc, const char **argv, const char *prefix) * intend to feed us a patch but wanted to continue * unattended. */ - if (argc || (resume.mode == RESUME_FALSE && !isatty(0))) + if (argc || (resume_mode == RESUME_FALSE && !isatty(0))) die(_("previous rebase directory %s still exists but mbox given."), state.dir); - if (resume.mode == RESUME_FALSE) - resume.mode = RESUME_APPLY; + if (resume_mode == RESUME_FALSE) + resume_mode = RESUME_APPLY; if (state.signoff == SIGNOFF_EXPLICIT) am_append_signoff(&state); @@ -2482,7 +2457,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) * stray directories. */ if (file_exists(state.dir) && !state.rebasing) { - if (resume.mode == RESUME_ABORT || resume.mode == RESUME_QUIT) { + if (resume_mode == RESUME_ABORT || resume_mode == RESUME_QUIT) { am_destroy(&state); am_state_release(&state); return 0; @@ -2493,7 +2468,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) state.dir); } - if (resume.mode) + if (resume_mode) die(_("Resolve operation not in progress, we are not resuming.")); for (i = 0; i < argc; i++) { @@ -2511,7 +2486,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) strvec_clear(&paths); } - switch (resume.mode) { + switch (resume_mode) { case RESUME_FALSE: am_run(&state, 0); break; @@ -2520,7 +2495,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) break; case RESUME_RESOLVED: case RESUME_ALLOW_EMPTY: - am_resolve(&state, resume.mode == RESUME_ALLOW_EMPTY ? 1 : 0); + am_resolve(&state, resume_mode == RESUME_ALLOW_EMPTY ? 1 : 0); break; case RESUME_SKIP: am_skip(&state); @@ -2532,8 +2507,9 @@ int cmd_am(int argc, const char **argv, const char *prefix) am_rerere_clear(); am_destroy(&state); break; - case RESUME_SHOW_PATCH: - ret = show_patch(&state, resume.sub_mode); + case RESUME_SHOW_PATCH_RAW: + case RESUME_SHOW_PATCH_DIFF: + ret = show_patch(&state, resume_mode); break; default: BUG("invalid resume value");