| Message ID | 844e01391e1198960072844536d736f51573cac6.1643408644.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Factorize i18n | expand |
Am 28.01.22 um 23:24 schrieb Jean-Noël Avila via GitGitGadget: > From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@free.fr> > > Find more incompatible options to factorize. > > When more than two options are mutually exclusive, print the ones > which are actually on the command line. > > Signed-off-by: Jean-Noël Avila <jn.avila@free.fr> > --- > builtin/commit.c | 39 +++++++++--------- > builtin/difftool.c | 5 ++- > builtin/grep.c | 8 ++-- > builtin/log.c | 5 ++- > builtin/merge-base.c | 4 +- > parse-options.c | 50 +++++++++++++++++++++++ > parse-options.h | 9 ++++ > t/t7500-commit-template-squash-signoff.sh | 2 +- > 8 files changed, 90 insertions(+), 32 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index b9ed0374e30..1966f965008 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1242,8 +1242,6 @@ static int parse_and_validate_options(int argc, const char *argv[], > struct commit *current_head, > struct wt_status *s) > { > - int f = 0; > - > argc = parse_options(argc, argv, prefix, options, usage, 0); > finalize_deferred_config(s); > > @@ -1251,7 +1249,7 @@ static int parse_and_validate_options(int argc, const char *argv[], > force_author = find_author_by_nickname(force_author); > > if (force_author && renew_authorship) > - die(_("Using both --reset-author and --author does not make sense")); > + die(_("options '%s' and '%s' cannot be used together"), "--reset-author", "--author"); > > if (logfile || have_option_m || use_message) > use_editor = 0; > @@ -1268,20 +1266,20 @@ static int parse_and_validate_options(int argc, const char *argv[], > die(_("You are in the middle of a rebase -- cannot amend.")); > } > if (fixup_message && squash_message) > - die(_("Options --squash and --fixup cannot be used together")); > - if (use_message) > - f++; > - if (edit_message) > - f++; > - if (fixup_message) > - f++; > - if (logfile) > - f++; > - if (f > 1) > - die(_("Only one of -c/-C/-F/--fixup can be used.")); > - if (have_option_m && (edit_message || use_message || logfile)) > - die((_("Option -m cannot be combined with -c/-C/-F."))); > - if (f || have_option_m) > + die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup"); > + die_if_incompatible_opt4(!!use_message, "-C", > + !!edit_message, "-c", > + !!logfile, "-F", > + !!fixup_message, "--fixup"); Please check the tab width setting of your editor; indentations here and in other calls and function declarations below are too wide. Looks like you have it at 4 instead of 8. > + if (have_option_m) { > + if (edit_message) > + die(_("options '%s' and '%s' cannot be used together"), "-m", "-c"); > + else if (use_message) > + die(_("options '%s' and '%s' cannot be used together"), "-m", "-C"); > + else if (logfile) > + die(_("options '%s' and '%s' cannot be used together"), "-m", "-F"); > + } This conditional could have been another die_if_incompatible_opt4() call, although it would only check a conflict of -m with at most one of -c, -C, or -F. > + if (use_message || edit_message || logfile ||fixup_message || have_option_m) Missing space before fixup_message. > template_file = NULL; > if (edit_message) > use_message = edit_message; The rewrite of the logic in this hunk looks correct. > @@ -1306,9 +1304,10 @@ static int parse_and_validate_options(int argc, const char *argv[], > if (patch_interactive) > interactive = 1; > > - if (also + only + all + interactive > 1) > - die(_("Only one of --include/--only/--all/--interactive/--patch can be used.")); > - > + die_if_incompatible_opt4(also, "-i/--include", > + only, "-o/--only", > + all, "-a/--all", > + interactive, "--interactive/-p/--patch"); > if (fixup_message) { > /* > * We limit --fixup's suboptions to only alpha characters. > diff --git a/builtin/difftool.c b/builtin/difftool.c > index c79fbbf67e5..ae487785735 100644 > --- a/builtin/difftool.c > +++ b/builtin/difftool.c > @@ -732,8 +732,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) > } else if (dir_diff) > die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index"); > > - if (use_gui_tool + !!difftool_cmd + !!extcmd > 1) > - die(_("options '%s', '%s', and '%s' cannot be used together"), "--gui", "--tool", "--extcmd"); > + die_if_incompatible_opt3(use_gui_tool, "--gui", > + !!difftool_cmd, "--tool", > + !!extcmd, "--extcmd"); > > if (use_gui_tool) > setenv("GIT_MERGETOOL_GUI", "true", 1); > diff --git a/builtin/grep.c b/builtin/grep.c > index 9e34a820ad4..cdf52667710 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -1167,11 +1167,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix) > if (!show_in_pager && !opt.status_only) > setup_pager(); > > - if (!use_index && (untracked || cached)) > - die(_("--cached or --untracked cannot be used with --no-index")); > - > - if (untracked && cached) > - die(_("--untracked cannot be used with --cached")); > + die_if_incompatible_opt3(!use_index, "--no-index", > + untracked, "--untracked", > + cached, "--cached"); > > if (!use_index || untracked) { > int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude; > diff --git a/builtin/log.c b/builtin/log.c > index 4b493408cc5..048b2c37470 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1978,8 +1978,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > if (rev.show_notes) > load_display_notes(&rev.notes_opt); > > - if (use_stdout + rev.diffopt.close_file + !!output_directory > 1) > - die(_("options '%s', '%s', and '%s' cannot be used together"), "--stdout", "--output", "--output-directory"); > + die_if_incompatible_opt3(use_stdout, "--stdout", > + rev.diffopt.close_file, "--output", > + !!output_directory, "--output-directory"); > > if (use_stdout) { > setup_pager(); > diff --git a/builtin/merge-base.c b/builtin/merge-base.c > index 6719ac198dc..1447f1c493a 100644 > --- a/builtin/merge-base.c > +++ b/builtin/merge-base.c > @@ -159,12 +159,12 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix) > if (argc < 2) > usage_with_options(merge_base_usage, options); > if (show_all) > - die("--is-ancestor cannot be used with --all"); > + die(_("options '%s' and '%s' cannot be used together"),"--is-ancestor", "--all"); > return handle_is_ancestor(argc, argv); > } > > if (cmdmode == 'r' && show_all) > - die("--independent cannot be used with --all"); > + die(_("options '%s' and '%s' cannot be used together"),"--independent", "--all"); > > if (cmdmode == 'o') > return handle_octopus(argc, argv, show_all); These rewrites are good. Note the unusually wide indentation in the new function calls, though. > diff --git a/parse-options.c b/parse-options.c > index a8283037be9..fb9e1976ab3 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -1079,3 +1079,53 @@ void NORETURN usage_msg_opt(const char *msg, > die_message("%s\n", msg); /* The extra \n is intentional */ > usage_with_options(usagestr, options); > } > + > +void die_if_incompatible_opt3(int opt1, const char *opt1_name, > + int opt2, const char *opt2_name, > + int opt3, const char *opt3_name) > +{ > + int count = 0; > + const char *options[3]; > + > + if (opt1) > + options[count++] = opt1_name; > + if (opt2) > + options[count++] = opt2_name; > + if (opt3) > + options[count++] = opt3_name; > + if (count > 2) > + die(_("options '%s', '%s', and '%s' cannot be used together"), opt1_name, opt2_name, opt3_name); > + else if (count > 1) > + die(_("options '%s' and '%s' cannot be used together"), options[0], options[1]); > +} > + > +void die_if_incompatible_opt4(int opt1, const char *opt1_name, > + int opt2, const char *opt2_name, > + int opt3, const char *opt3_name, > + int opt4, const char *opt4_name) > +{ > + int count = 0; > + const char *options[4]; > + > + if (opt1) > + options[count++] = opt1_name; > + if (opt2) > + options[count++] = opt2_name; > + if (opt3) > + options[count++] = opt3_name; > + if (opt4) > + options[count++] = opt4_name; > + switch (count) { > + case 4: > + die(_("options '%s', '%s', '%s', and '%s' cannot be used together"), opt1_name, opt2_name, opt3_name, opt4_name); > + break; > + case 3: > + die(_("options '%s', '%s', and '%s' cannot be used together"), options[0], options[1], options[2]); > + break; > + case 2: > + die(_("options '%s' and '%s' cannot be used together"), options[0], options[1]); > + break; > + default: > + break; > + } > +} Generally, this is good. I wonder whether we have to expect compiler warnings about unreachable break statements after the die() calls. A bit of code duplication could be avoided if die_if_incompatible_opt3() forwarded with an additional pair 0, "" to die_if_incompatible_opt4(). > diff --git a/parse-options.h b/parse-options.h > index e22846d3b7b..cf393839ac4 100644 > --- a/parse-options.h > +++ b/parse-options.h > @@ -339,4 +339,13 @@ int parse_opt_tracking_mode(const struct option *, const char *, int); > #define OPT_PATHSPEC_FILE_NUL(v) OPT_BOOL(0, "pathspec-file-nul", v, N_("with --pathspec-from-file, pathspec elements are separated with NUL character")) > #define OPT_AUTOSTASH(v) OPT_BOOL(0, "autostash", v, N_("automatically stash/stash pop before and after")) > > +void die_if_incompatible_opt3(int opt1, const char *opt1_name, > + int opt2, const char *opt2_name, > + int opt3, const char *opt3_name); > + > +void die_if_incompatible_opt4(int opt1, const char *opt1_name, > + int opt2, const char *opt2_name, > + int opt3, const char *opt3_name, > + int opt4, const char *opt4_name); > + > #endif I would have placed these declarations near usage_msg_opt further above in the header file. > diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh > index 91964653a0b..5fcaa0b4f2a 100755 > --- a/t/t7500-commit-template-squash-signoff.sh > +++ b/t/t7500-commit-template-squash-signoff.sh > @@ -442,7 +442,7 @@ test_expect_success '--fixup=reword: give error with pathsec' ' > ' > > test_expect_success '--fixup=reword: -F give error message' ' > - echo "fatal: Only one of -c/-C/-F/--fixup can be used." >expect && > + echo "fatal: options '\''-F'\'' and '\''--fixup'\'' cannot be used together" >expect && > test_must_fail git commit --fixup=reword:HEAD~ -F msg 2>actual && > test_cmp expect actual > ' -- Hannes
Johannes Sixt <j6t@kdbg.org> writes: >> +void die_if_incompatible_opt4(int opt1, const char *opt1_name, >> + int opt2, const char *opt2_name, >> + int opt3, const char *opt3_name, >> + int opt4, const char *opt4_name) >> +{ >> + int count = 0; >> + const char *options[4]; >> + >> + if (opt1) >> + options[count++] = opt1_name; >> + if (opt2) >> + options[count++] = opt2_name; >> + if (opt3) >> + options[count++] = opt3_name; >> + if (opt4) >> + options[count++] = opt4_name; >> + switch (count) { >> + case 4: >> + die(_("options '%s', '%s', '%s', and '%s' cannot be used together"), opt1_name, opt2_name, opt3_name, opt4_name); >> + break; >> + case 3: >> + die(_("options '%s', '%s', and '%s' cannot be used together"), options[0], options[1], options[2]); >> + break; >> + case 2: >> + die(_("options '%s' and '%s' cannot be used together"), options[0], options[1]); >> + break; >> + default: >> + break; >> + } >> +} > > Generally, this is good. I wonder whether we have to expect compiler > warnings about unreachable break statements after the die() calls. > > A bit of code duplication could be avoided if die_if_incompatible_opt3() > forwarded with an additional pair 0, "" to die_if_incompatible_opt4(). I wondered if a single varargs function void die_if_incompatible_optN(const char *name1, int opt1, ...); that takes a name=NULL terminated sequence of <name, opt> would work, but (1) it would require the caller to always spell out the terminating NULL, which may be ugly. (2) it would tempt people to programatically generate message for N options, which leads to sentence lego, which is the exact opposite of what this series wants to achieve. In any case, I do agree with you that the callers of _opt3() variants can just pass (0, "unused") in the 4-th slot. If this were made varargs, then it only needs to pass NULL at the end, so that might be an improvement, though. Also, isn't "if" in the name of the function misleading? as far as I can tell, this function is not "check if these options are compatible and die if some are incompatible with each other", but the caller is convinced that these options are incompatible before it decides to call this function and there is no "if" in what this function can do. void die_for_incompatible_opts(const char *name1, int opt1, ...) { const char *name[4]; int count = 0; va_list params; va_start(params, name1); if (opt1) name[count++] = name1; while (count < ARRAY_SIZE(name)) { const char *n = va_arg(params, const char *); if (!n) break; if (va_arg(params, int)) name[count++] = n; } va_end(params); switch (count) { default: BUG("die-for-incompatible-opts can only take up to %d args", ARRAY_SIZE(name)); case 4: die(_("options '%s', '%s', '%s', and '%s'" " cannot be used together"), name[0], name[1], name[2], name[3]); break; case 3: die(_("options '%s', '%s', and '%s'" " cannot be used together"), name[0], name[1], name[2]); break; ... } } might be easier to extend when somebody discovers there needs the "opt5" variant.
Am 29.01.22 um 00:58 schrieb Junio C Hamano: > Johannes Sixt <j6t@kdbg.org> writes: >> A bit of code duplication could be avoided if die_if_incompatible_opt3() >> forwarded with an additional pair 0, "" to die_if_incompatible_opt4(). > > I wondered if a single varargs function > > void die_if_incompatible_optN(const char *name1, int opt1, ...); > > that takes a name=NULL terminated sequence of <name, opt> would > work, but > > (1) it would require the caller to always spell out the terminating > NULL, which may be ugly. > > (2) it would tempt people to programatically generate message for N > options, which leads to sentence lego, which is the exact > opposite of what this series wants to achieve. The reason I did not suggest a varargs version is because (3) it is not typesafe. A varargs implementation could be used as an implementation helper, but should not be a public interface. > In any case, I do agree with you that the callers of _opt3() > variants can just pass (0, "unused") in the 4-th slot. If this were > made varargs, then it only needs to pass NULL at the end, so that > might be an improvement, though. > > Also, isn't "if" in the name of the function misleading? as far as > I can tell, this function is not "check if these options are > compatible and die if some are incompatible with each other", but > the caller is convinced that these options are incompatible before > it decides to call this function and there is no "if" in what this > function can do. Good point. > void die_for_incompatible_opts(const char *name1, int opt1, ...) > { > const char *name[4]; > int count = 0; > va_list params; > > va_start(params, name1); > > if (opt1) > name[count++] = name1; > while (count < ARRAY_SIZE(name)) { > const char *n = va_arg(params, const char *); > > if (!n) > break; > if (va_arg(params, int)) > name[count++] = n; > } > va_end(params); > > switch (count) { > default: > BUG("die-for-incompatible-opts can only take up to %d args", > ARRAY_SIZE(name)); The problems here are: (1) this case triggers only if there is an actual invocation with all 5+ incompatible options, and (2) at that time, the out-of-bounds write to the name array has already happened. I know this implementation is just a rough show-case. But since it's been written by a very proficient mind, I'm now convinced: it's not sufficiently easy to write the varargs version, so let's not go there. > case 4: > die(_("options '%s', '%s', '%s', and '%s'" > " cannot be used together"), > name[0], name[1], name[2], name[3]); > break; > case 3: > die(_("options '%s', '%s', and '%s'" > " cannot be used together"), > name[0], name[1], name[2]); > break; > ... > } > } > > might be easier to extend when somebody discovers there needs the > "opt5" variant. -- Hannes
Le samedi 29 janvier 2022, 09:08:34 CET Johannes Sixt a écrit : > Am 29.01.22 um 00:58 schrieb Junio C Hamano: > > Johannes Sixt <j6t@kdbg.org> writes: > >> A bit of code duplication could be avoided if die_if_incompatible_opt3() > >> forwarded with an additional pair 0, "" to die_if_incompatible_opt4(). > > > > I wondered if a single varargs function > > > > void die_if_incompatible_optN(const char *name1, int opt1, ...); > > > > that takes a name=NULL terminated sequence of <name, opt> would > > work, but > > > > (1) it would require the caller to always spell out the terminating > > NULL, which may be ugly. > > > > (2) it would tempt people to programatically generate message for N > > options, which leads to sentence lego, which is the exact > > opposite of what this series wants to achieve. This is the second part of the functions, and they need a finite number of args to check in order to use the correct i18n string. This function cannot be made generic for any number of options to check. > > The reason I did not suggest a varargs version is because > > (3) it is not typesafe. > > A varargs implementation could be used as an implementation helper, but > should not be a public interface. > > > In any case, I do agree with you that the callers of _opt3() > > variants can just pass (0, "unused") in the 4-th slot. If this were > > made varargs, then it only needs to pass NULL at the end, so that > > might be an improvement, though. I still prefer to keep a public function signature which is explicit. Using varargs for alternating types is really not type safe. In such case, declaring the functions with less options as inline calls to the one with the greatest number is a good balance. > > > > Also, isn't "if" in the name of the function misleading? as far as > > I can tell, this function is not "check if these options are > > compatible and die if some are incompatible with each other", but > > the caller is convinced that these options are incompatible before > > it decides to call this function and there is no "if" in what this > > function can do. > > Good point. > I don't get the point: the function checks if incompatible options are present on the command line and dies in such case. The caller does not know whether the function will make the program die, it really depends on the result of the check performed in the function. In case there's no or only one of the options, the function does nothing. JN
Am 29.01.22 um 11:41 schrieb Jean-Noël AVILA: > Le samedi 29 janvier 2022, 09:08:34 CET Johannes Sixt a écrit : >> Am 29.01.22 um 00:58 schrieb Junio C Hamano: >>> Also, isn't "if" in the name of the function misleading? as far as >>> I can tell, this function is not "check if these options are >>> compatible and die if some are incompatible with each other", but >>> the caller is convinced that these options are incompatible before >>> it decides to call this function and there is no "if" in what this >>> function can do. >> >> Good point. > > I don't get the point: the function checks if incompatible options are present > on the command line and dies in such case. The caller does not know whether > the function will make the program die, it really depends on the result of the > check performed in the function. In case there's no or only one of the > options, the function does nothing. The function call reads like die if options are incompatible That sounds as if the caller were asking "are these options incompatible?" But that is not what it wants to ask because it already knows that the options are incompatible. It actually wants to ask "is there any problem with these incompatible options?" The name Junio suggested is die_for_incompatible_opts(). -- Hannes
On Fri, Jan 28 2022, Jean-Noël Avila via GitGitGadget wrote: > From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@free.fr> > [...] > +void die_if_incompatible_opt3(int opt1, const char *opt1_name, > + int opt2, const char *opt2_name, > + int opt3, const char *opt3_name) > +{ > + int count = 0; > + const char *options[3]; > + > + if (opt1) > + options[count++] = opt1_name; > + if (opt2) > + options[count++] = opt2_name; > + if (opt3) > + options[count++] = opt3_name; > + if (count > 2) > + die(_("options '%s', '%s', and '%s' cannot be used together"), opt1_name, opt2_name, opt3_name); > + else if (count > 1) > + die(_("options '%s' and '%s' cannot be used together"), options[0], options[1]); > +} > + > +void die_if_incompatible_opt4(int opt1, const char *opt1_name, > + int opt2, const char *opt2_name, > + int opt3, const char *opt3_name, > + int opt4, const char *opt4_name) > +{ > + int count = 0; > + const char *options[4]; > + > + if (opt1) > + options[count++] = opt1_name; > + if (opt2) > + options[count++] = opt2_name; > + if (opt3) > + options[count++] = opt3_name; > + if (opt4) > + options[count++] = opt4_name; > + switch (count) { > + case 4: > + die(_("options '%s', '%s', '%s', and '%s' cannot be used together"), opt1_name, opt2_name, opt3_name, opt4_name); > + break; > + case 3: > + die(_("options '%s', '%s', and '%s' cannot be used together"), options[0], options[1], options[2]); > + break; > + case 2: > + die(_("options '%s' and '%s' cannot be used together"), options[0], options[1]); > + break; > + default: > + break; > + } > +} > diff --git a/parse-options.h b/parse-options.h > index e22846d3b7b..cf393839ac4 100644 > --- a/parse-options.h > +++ b/parse-options.h > @@ -339,4 +339,13 @@ int parse_opt_tracking_mode(const struct option *, const char *, int); > #define OPT_PATHSPEC_FILE_NUL(v) OPT_BOOL(0, "pathspec-file-nul", v, N_("with --pathspec-from-file, pathspec elements are separated with NUL character")) > #define OPT_AUTOSTASH(v) OPT_BOOL(0, "autostash", v, N_("automatically stash/stash pop before and after")) > > +void die_if_incompatible_opt3(int opt1, const char *opt1_name, > + int opt2, const char *opt2_name, > + int opt3, const char *opt3_name); > + > +void die_if_incompatible_opt4(int opt1, const char *opt1_name, > + int opt2, const char *opt2_name, > + int opt3, const char *opt3_name, > + int opt4, const char *opt4_name); > + > #endif > diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh > index 91964653a0b..5fcaa0b4f2a 100755 > --- a/t/t7500-commit-template-squash-signoff.sh > +++ b/t/t7500-commit-template-squash-signoff.sh > @@ -442,7 +442,7 @@ test_expect_success '--fixup=reword: give error with pathsec' ' > ' > > test_expect_success '--fixup=reword: -F give error message' ' > - echo "fatal: Only one of -c/-C/-F/--fixup can be used." >expect && > + echo "fatal: options '\''-F'\'' and '\''--fixup'\'' cannot be used together" >expect && > test_must_fail git commit --fixup=reword:HEAD~ -F msg 2>actual && > test_cmp expect actual > ' I've really wanted (and have been meaning to find time to work on) some expansion of what we can get from the "these are incompatible" that OPT_CMDMODE gives us for a while. But I think doing it like this is really going in the wrong direction, i.e. we have no need to run all of parse_options(), callbacks and all, and populate all the variables, only to after the fact die when we notice both "a" and "b" were set, and those are incompatible. I.e. to have the API work like something resembling this: diff --git a/builtin/grep.c b/builtin/grep.c index 75e07b5623a..80ea323f957 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -849,8 +849,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix) struct option options[] = { OPT_BOOL(0, "cached", &cached, N_("search in index instead of in the work tree")), + OPT_INCOMPATIBLE("cached", "untracked"), OPT_NEGBIT(0, "no-index", &use_index, N_("find in contents not managed by git"), 1), + OPT_INCOMPATIBLE("no-index", "untracked"), + OPT_INCOMPATIBLE("no-index", "cached"), OPT_BOOL(0, "untracked", &untracked, N_("search in both tracked and untracked files")), OPT_SET_INT(0, "exclude-standard", &opt_exclude, @@ -1167,12 +1170,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix) if (!show_in_pager && !opt.status_only) setup_pager(); - if (!use_index && (untracked || cached)) - die(_("--cached or --untracked cannot be used with --no-index")); - - if (untracked && cached) - die(_("--untracked cannot be used with --cached")); - if (!use_index || untracked) { int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude; hit = grep_directory(&opt, &pathspec, use_exclude, use_index); I.e. to have this be a new parse_opt_type. Then we'd just make note of what's incompatible with what other stuff, and in parse_options_step() check if the option we're currently looking at (e.g. --no-index) is still the only one set out of a list-of-lists incompatible options. And for i18n I really don't think we need to spend effort on detecting a case where --foo --bar and --baz are all incompatible, and saying one of: --bar is incompatible with --foo Or: --bar is incompatible with --foo and --baz Depending on whether the command-line is "--foo --bar" or "--foo --bar --baz", let's just in both cases say: --bar is incompatible with --foo Then if the user adjusts "--foo --bar --baz" to "--foo --baz" we'll just show them a new error, using the same template: --baz is incompatible with --foo I.e. doing this as we iterate options is Good Enough, it's not worth the complexity or translator time to try to exhaustively list all conflicting options at once.
diff --git a/builtin/commit.c b/builtin/commit.c index b9ed0374e30..1966f965008 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1242,8 +1242,6 @@ static int parse_and_validate_options(int argc, const char *argv[], struct commit *current_head, struct wt_status *s) { - int f = 0; - argc = parse_options(argc, argv, prefix, options, usage, 0); finalize_deferred_config(s); @@ -1251,7 +1249,7 @@ static int parse_and_validate_options(int argc, const char *argv[], force_author = find_author_by_nickname(force_author); if (force_author && renew_authorship) - die(_("Using both --reset-author and --author does not make sense")); + die(_("options '%s' and '%s' cannot be used together"), "--reset-author", "--author"); if (logfile || have_option_m || use_message) use_editor = 0; @@ -1268,20 +1266,20 @@ static int parse_and_validate_options(int argc, const char *argv[], die(_("You are in the middle of a rebase -- cannot amend.")); } if (fixup_message && squash_message) - die(_("Options --squash and --fixup cannot be used together")); - if (use_message) - f++; - if (edit_message) - f++; - if (fixup_message) - f++; - if (logfile) - f++; - if (f > 1) - die(_("Only one of -c/-C/-F/--fixup can be used.")); - if (have_option_m && (edit_message || use_message || logfile)) - die((_("Option -m cannot be combined with -c/-C/-F."))); - if (f || have_option_m) + die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup"); + die_if_incompatible_opt4(!!use_message, "-C", + !!edit_message, "-c", + !!logfile, "-F", + !!fixup_message, "--fixup"); + if (have_option_m) { + if (edit_message) + die(_("options '%s' and '%s' cannot be used together"), "-m", "-c"); + else if (use_message) + die(_("options '%s' and '%s' cannot be used together"), "-m", "-C"); + else if (logfile) + die(_("options '%s' and '%s' cannot be used together"), "-m", "-F"); + } + if (use_message || edit_message || logfile ||fixup_message || have_option_m) template_file = NULL; if (edit_message) use_message = edit_message; @@ -1306,9 +1304,10 @@ static int parse_and_validate_options(int argc, const char *argv[], if (patch_interactive) interactive = 1; - if (also + only + all + interactive > 1) - die(_("Only one of --include/--only/--all/--interactive/--patch can be used.")); - + die_if_incompatible_opt4(also, "-i/--include", + only, "-o/--only", + all, "-a/--all", + interactive, "--interactive/-p/--patch"); if (fixup_message) { /* * We limit --fixup's suboptions to only alpha characters. diff --git a/builtin/difftool.c b/builtin/difftool.c index c79fbbf67e5..ae487785735 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -732,8 +732,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) } else if (dir_diff) die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index"); - if (use_gui_tool + !!difftool_cmd + !!extcmd > 1) - die(_("options '%s', '%s', and '%s' cannot be used together"), "--gui", "--tool", "--extcmd"); + die_if_incompatible_opt3(use_gui_tool, "--gui", + !!difftool_cmd, "--tool", + !!extcmd, "--extcmd"); if (use_gui_tool) setenv("GIT_MERGETOOL_GUI", "true", 1); diff --git a/builtin/grep.c b/builtin/grep.c index 9e34a820ad4..cdf52667710 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -1167,11 +1167,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix) if (!show_in_pager && !opt.status_only) setup_pager(); - if (!use_index && (untracked || cached)) - die(_("--cached or --untracked cannot be used with --no-index")); - - if (untracked && cached) - die(_("--untracked cannot be used with --cached")); + die_if_incompatible_opt3(!use_index, "--no-index", + untracked, "--untracked", + cached, "--cached"); if (!use_index || untracked) { int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude; diff --git a/builtin/log.c b/builtin/log.c index 4b493408cc5..048b2c37470 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1978,8 +1978,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (rev.show_notes) load_display_notes(&rev.notes_opt); - if (use_stdout + rev.diffopt.close_file + !!output_directory > 1) - die(_("options '%s', '%s', and '%s' cannot be used together"), "--stdout", "--output", "--output-directory"); + die_if_incompatible_opt3(use_stdout, "--stdout", + rev.diffopt.close_file, "--output", + !!output_directory, "--output-directory"); if (use_stdout) { setup_pager(); diff --git a/builtin/merge-base.c b/builtin/merge-base.c index 6719ac198dc..1447f1c493a 100644 --- a/builtin/merge-base.c +++ b/builtin/merge-base.c @@ -159,12 +159,12 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix) if (argc < 2) usage_with_options(merge_base_usage, options); if (show_all) - die("--is-ancestor cannot be used with --all"); + die(_("options '%s' and '%s' cannot be used together"),"--is-ancestor", "--all"); return handle_is_ancestor(argc, argv); } if (cmdmode == 'r' && show_all) - die("--independent cannot be used with --all"); + die(_("options '%s' and '%s' cannot be used together"),"--independent", "--all"); if (cmdmode == 'o') return handle_octopus(argc, argv, show_all); diff --git a/parse-options.c b/parse-options.c index a8283037be9..fb9e1976ab3 100644 --- a/parse-options.c +++ b/parse-options.c @@ -1079,3 +1079,53 @@ void NORETURN usage_msg_opt(const char *msg, die_message("%s\n", msg); /* The extra \n is intentional */ usage_with_options(usagestr, options); } + +void die_if_incompatible_opt3(int opt1, const char *opt1_name, + int opt2, const char *opt2_name, + int opt3, const char *opt3_name) +{ + int count = 0; + const char *options[3]; + + if (opt1) + options[count++] = opt1_name; + if (opt2) + options[count++] = opt2_name; + if (opt3) + options[count++] = opt3_name; + if (count > 2) + die(_("options '%s', '%s', and '%s' cannot be used together"), opt1_name, opt2_name, opt3_name); + else if (count > 1) + die(_("options '%s' and '%s' cannot be used together"), options[0], options[1]); +} + +void die_if_incompatible_opt4(int opt1, const char *opt1_name, + int opt2, const char *opt2_name, + int opt3, const char *opt3_name, + int opt4, const char *opt4_name) +{ + int count = 0; + const char *options[4]; + + if (opt1) + options[count++] = opt1_name; + if (opt2) + options[count++] = opt2_name; + if (opt3) + options[count++] = opt3_name; + if (opt4) + options[count++] = opt4_name; + switch (count) { + case 4: + die(_("options '%s', '%s', '%s', and '%s' cannot be used together"), opt1_name, opt2_name, opt3_name, opt4_name); + break; + case 3: + die(_("options '%s', '%s', and '%s' cannot be used together"), options[0], options[1], options[2]); + break; + case 2: + die(_("options '%s' and '%s' cannot be used together"), options[0], options[1]); + break; + default: + break; + } +} diff --git a/parse-options.h b/parse-options.h index e22846d3b7b..cf393839ac4 100644 --- a/parse-options.h +++ b/parse-options.h @@ -339,4 +339,13 @@ int parse_opt_tracking_mode(const struct option *, const char *, int); #define OPT_PATHSPEC_FILE_NUL(v) OPT_BOOL(0, "pathspec-file-nul", v, N_("with --pathspec-from-file, pathspec elements are separated with NUL character")) #define OPT_AUTOSTASH(v) OPT_BOOL(0, "autostash", v, N_("automatically stash/stash pop before and after")) +void die_if_incompatible_opt3(int opt1, const char *opt1_name, + int opt2, const char *opt2_name, + int opt3, const char *opt3_name); + +void die_if_incompatible_opt4(int opt1, const char *opt1_name, + int opt2, const char *opt2_name, + int opt3, const char *opt3_name, + int opt4, const char *opt4_name); + #endif diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh index 91964653a0b..5fcaa0b4f2a 100755 --- a/t/t7500-commit-template-squash-signoff.sh +++ b/t/t7500-commit-template-squash-signoff.sh @@ -442,7 +442,7 @@ test_expect_success '--fixup=reword: give error with pathsec' ' ' test_expect_success '--fixup=reword: -F give error message' ' - echo "fatal: Only one of -c/-C/-F/--fixup can be used." >expect && + echo "fatal: options '\''-F'\'' and '\''--fixup'\'' cannot be used together" >expect && test_must_fail git commit --fixup=reword:HEAD~ -F msg 2>actual && test_cmp expect actual '