| Message ID | 81ff928567fdf274000c660fb791641fc2ceccc9.1642876553.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Factorize i18n | expand |
Am 22.01.22 um 19:35 schrieb Jean-Noël Avila via GitGitGadget: > From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@free.fr> > > When more than two options are mutually exclusive, print the ones > which are actually on the command line. Reading this, I expect that all mutually exclusive options are listed in the error messages. But the updated code lists only the first and second option even if more are on the command line. What is the justification for that? Just "make the work for translators easier"? I am not 100% sure that this is the right balance. Wouldn't it be helpful for users to get to know which set of options is incompatible, or is an error message not the right place for this kind of education? Don't know... > > Signed-off-by: Jean-Noël Avila <jn.avila@free.fr> > --- > builtin/commit.c | 39 ++++++++++++++--------- > builtin/difftool.c | 18 +++++++++-- > builtin/grep.c | 4 +-- > builtin/log.c | 20 ++++++++++-- > builtin/merge-base.c | 4 +-- > t/t7500-commit-template-squash-signoff.sh | 2 +- > 6 files changed, 63 insertions(+), 24 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index b9ed0374e30..910f4c912bf 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1243,6 +1243,7 @@ static int parse_and_validate_options(int argc, const char *argv[], > struct wt_status *s) > { > int f = 0; > + char * f_options[4]; > > argc = parse_options(argc, argv, prefix, options, usage, 0); > finalize_deferred_config(s); > @@ -1251,7 +1252,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,19 +1269,19 @@ 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++; > + die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup"); > + f_options[f] = "-C"; > + f+= !!use_message; > + f_options[f] = "-c"; > + f+=!!edit_message; > + f_options[f] = "-F"; > + f+=!!logfile; > + f_options[f] = "--fixup"; > + f+=!!fixup_message; The format style violations are quite obvious here. But see below for a suggestion to write this differently. > if (f > 1) > - die(_("Only one of -c/-C/-F/--fixup can be used.")); > + die(_("options '%s' and '%s' cannot be used together"), f_options[0], f_options[1]); > if (have_option_m && (edit_message || use_message || logfile)) > - die((_("Option -m cannot be combined with -c/-C/-F."))); > + die(_("options '%s' and '%s' cannot be used together"), "-m", f_options[0]); > if (f || have_option_m) > template_file = NULL; > if (edit_message) > @@ -1305,9 +1306,17 @@ 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.")); > + f = 0; > + f_options[f] = "-i/--include"; > + f += also; > + f_options[f] = "-o/--only"; > + f += only; > + f_options[f] = "-a/--all"; > + f += all; > + f_options[f] = "--interactive/-p/--patch"; Interesting. -p and --interactive do different things, but can be used together (-p overrides --interactive). The original error message suggests that this is not the case. Just a marginal note... > + f += interactive; > + if (f > 1) > + die(_("options '%s' and '%s' cannot be used together"), f_options[0], f_options[1]); > > if (fixup_message) { > /* > diff --git a/builtin/difftool.c b/builtin/difftool.c > index c79fbbf67e5..92854bc7737 100644 > --- a/builtin/difftool.c > +++ b/builtin/difftool.c > @@ -685,6 +685,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) > int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0, > tool_help = 0, no_index = 0; > static char *difftool_cmd = NULL, *extcmd = NULL; > + int f = 0; > + char *f_options[3]; > struct option builtin_difftool_options[] = { > OPT_BOOL('g', "gui", &use_gui_tool, > N_("use `diff.guitool` instead of `diff.tool`")), > @@ -732,8 +734,20 @@ 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"); > + if (use_gui_tool) { > + f_options[f] = "--gui"; > + f++; > + } > + if (difftool_cmd) { > + f_options[f] = "--tool"; > + f++; > + } > + if (extcmd) { > + f_options[f] = "--extcmd"; > + f++; > + } > + if (f > 1) > + die(_("options '%s' and '%s' cannot be used together"), f_options[0], f_options[1]); I prefer this if-cascade over the "f += truth_value" style, because I find it is easier to understand. If you write it as if (extcmd) f_options[f++] = "--extcmd"; you get each branch down to two lines. (But then others may disagree with the easy-to-understand argument due to the f++ buried in the expression. Unsure...) > > if (use_gui_tool) > setenv("GIT_MERGETOOL_GUI", "true", 1); > diff --git a/builtin/grep.c b/builtin/grep.c > index 9e34a820ad4..b199781cb27 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -1168,10 +1168,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix) > setup_pager(); > > if (!use_index && (untracked || cached)) > - die(_("--cached or --untracked cannot be used with --no-index")); > + die(_("options '%s' and '%s' cannot be used together"),"--cached/--untracked", "--no-index"); Huh? "--cached/--untracked"? Which one was used on the command line? But... > > if (untracked && cached) > - die(_("--untracked cannot be used with --cached")); > + die(_("options '%s' and '%s' cannot be used together"), "--untracked", "--cached"); > > if (!use_index || untracked) { > int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude; ... doesn't this logic imply that --cached, --untracked, and --no-index are three mutually exclusive options? > diff --git a/builtin/log.c b/builtin/log.c > index 4b493408cc5..59b4a2fd380 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1759,6 +1759,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > struct strbuf rdiff_title = STRBUF_INIT; > int creation_factor = -1; > > + int f = 0; > + char * f_options[4]; > + Style: char *f_options[4]; don't needlessly separate these new variables from the others by an empty line. > const struct option builtin_format_patch_options[] = { > OPT_CALLBACK_F('n', "numbered", &numbered, NULL, > N_("use [PATCH n/m] even with a single patch"), > @@ -1978,8 +1981,21 @@ 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"); > + if (use_stdout) { > + f_options[f] = "--stdout"; > + f++; > + } > + if (rev.diffopt.close_file) { > + f_options[f] = "--output"; > + f++; > + } > + if (output_directory) { > + f_options[f] = "--output-directory"; > + f++; > + } > + > + if (f > 1) > + die(_("options '%s'and '%s' cannot be used together"), f_options[0], f_options[1]); > > 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/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 > ' A general comment: To me, it looks like you didn't want to change the variable name 'f' in the first hunk above, and then just named the array 'f_options' to go with 'f'. Perhaps `exclusive_opt` (not plural!) for the array. Now, renaming 'f' to something longer makes the code a bit unwieldy. Therefore, let me suggest yet another approach: factor out functions check_exclusive_opts3(), check_exclusive_opts4(), to be used like check_exclusive_opts3(use_stdout, "--stdout", rev.diffopt.close_file, "--output", output_directory, "--output-directory"); I am not yet proposing check_exclusive_opts2(), but others may think it is an improvement, too, (if they think that such functions are an improvement in the first place). -- Hannes
On 24/01/2022 07:14, Johannes Sixt wrote: > Am 22.01.22 um 19:35 schrieb Jean-Noël Avila via GitGitGadget: >> From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@free.fr> >> >> When more than two options are mutually exclusive, print the ones >> which are actually on the command line. > > Reading this, I expect that all mutually exclusive options are listed in > the error messages. But the updated code lists only the first and second > option even if more are on the command line. What is the justification > for that? Just "make the work for translators easier"? I am not 100% > sure that this is the right balance. Wouldn't it be helpful for users to > get to know which set of options is incompatible, or is an error message > not the right place for this kind of education? Don't know... That was my feeling as well when reading this patch, I think the loss of information in the error messages is a shame. >> Signed-off-by: Jean-Noël Avila <jn.avila@free.fr> >> --- >> builtin/commit.c | 39 ++++++++++++++--------- >> builtin/difftool.c | 18 +++++++++-- >> builtin/grep.c | 4 +-- >> builtin/log.c | 20 ++++++++++-- >> builtin/merge-base.c | 4 +-- >> t/t7500-commit-template-squash-signoff.sh | 2 +- >> 6 files changed, 63 insertions(+), 24 deletions(-) >> >> diff --git a/builtin/commit.c b/builtin/commit.c >> index b9ed0374e30..910f4c912bf 100644 >> --- a/builtin/commit.c >> +++ b/builtin/commit.c >> @@ -1243,6 +1243,7 @@ static int parse_and_validate_options(int argc, const char *argv[], >> struct wt_status *s) >> { >> int f = 0; >> + char * f_options[4]; >> >> argc = parse_options(argc, argv, prefix, options, usage, 0); >> finalize_deferred_config(s); >> @@ -1251,7 +1252,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,19 +1269,19 @@ 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++; >> + die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup"); >> + f_options[f] = "-C"; >> + f+= !!use_message; >> + f_options[f] = "-c"; >> + f+=!!edit_message; >> + f_options[f] = "-F"; >> + f+=!!logfile; >> + f_options[f] = "--fixup"; >> + f+=!!fixup_message; This feels like an out of bounds access waiting to happen when someone adds a new option but forgets to increase the size of f_options above Best Wishes Phillip > The format style violations are quite obvious here. But see below for a > suggestion to write this differently. > >> if (f > 1) >> - die(_("Only one of -c/-C/-F/--fixup can be used.")); >> + die(_("options '%s' and '%s' cannot be used together"), f_options[0], f_options[1]); >> if (have_option_m && (edit_message || use_message || logfile)) >> - die((_("Option -m cannot be combined with -c/-C/-F."))); >> + die(_("options '%s' and '%s' cannot be used together"), "-m", f_options[0]); >> if (f || have_option_m) >> template_file = NULL; >> if (edit_message) >> @@ -1305,9 +1306,17 @@ 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.")); >> + f = 0; >> + f_options[f] = "-i/--include"; >> + f += also; >> + f_options[f] = "-o/--only"; >> + f += only; >> + f_options[f] = "-a/--all"; >> + f += all; >> + f_options[f] = "--interactive/-p/--patch"; > > Interesting. -p and --interactive do different things, but can be used > together (-p overrides --interactive). The original error message > suggests that this is not the case. Just a marginal note... > >> + f += interactive; >> + if (f > 1) >> + die(_("options '%s' and '%s' cannot be used together"), f_options[0], f_options[1]); >> >> if (fixup_message) { >> /* >> diff --git a/builtin/difftool.c b/builtin/difftool.c >> index c79fbbf67e5..92854bc7737 100644 >> --- a/builtin/difftool.c >> +++ b/builtin/difftool.c >> @@ -685,6 +685,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) >> int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0, >> tool_help = 0, no_index = 0; >> static char *difftool_cmd = NULL, *extcmd = NULL; >> + int f = 0; >> + char *f_options[3]; >> struct option builtin_difftool_options[] = { >> OPT_BOOL('g', "gui", &use_gui_tool, >> N_("use `diff.guitool` instead of `diff.tool`")), >> @@ -732,8 +734,20 @@ 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"); >> + if (use_gui_tool) { >> + f_options[f] = "--gui"; >> + f++; >> + } >> + if (difftool_cmd) { >> + f_options[f] = "--tool"; >> + f++; >> + } >> + if (extcmd) { >> + f_options[f] = "--extcmd"; >> + f++; >> + } >> + if (f > 1) >> + die(_("options '%s' and '%s' cannot be used together"), f_options[0], f_options[1]); > > I prefer this if-cascade over the "f += truth_value" style, because I > find it is easier to understand. If you write it as > > if (extcmd) > f_options[f++] = "--extcmd"; > > you get each branch down to two lines. (But then others may disagree > with the easy-to-understand argument due to the f++ buried in the > expression. Unsure...) > >> >> if (use_gui_tool) >> setenv("GIT_MERGETOOL_GUI", "true", 1); >> diff --git a/builtin/grep.c b/builtin/grep.c >> index 9e34a820ad4..b199781cb27 100644 >> --- a/builtin/grep.c >> +++ b/builtin/grep.c >> @@ -1168,10 +1168,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix) >> setup_pager(); >> >> if (!use_index && (untracked || cached)) >> - die(_("--cached or --untracked cannot be used with --no-index")); >> + die(_("options '%s' and '%s' cannot be used together"),"--cached/--untracked", "--no-index"); > > Huh? "--cached/--untracked"? Which one was used on the command line? But... > >> >> if (untracked && cached) >> - die(_("--untracked cannot be used with --cached")); >> + die(_("options '%s' and '%s' cannot be used together"), "--untracked", "--cached"); >> >> if (!use_index || untracked) { >> int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude; > > ... doesn't this logic imply that --cached, --untracked, and --no-index > are three mutually exclusive options? > >> diff --git a/builtin/log.c b/builtin/log.c >> index 4b493408cc5..59b4a2fd380 100644 >> --- a/builtin/log.c >> +++ b/builtin/log.c >> @@ -1759,6 +1759,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) >> struct strbuf rdiff_title = STRBUF_INIT; >> int creation_factor = -1; >> >> + int f = 0; >> + char * f_options[4]; >> + > > Style: char *f_options[4]; don't needlessly separate these new variables > from the others by an empty line. > >> const struct option builtin_format_patch_options[] = { >> OPT_CALLBACK_F('n', "numbered", &numbered, NULL, >> N_("use [PATCH n/m] even with a single patch"), >> @@ -1978,8 +1981,21 @@ 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"); >> + if (use_stdout) { >> + f_options[f] = "--stdout"; >> + f++; >> + } >> + if (rev.diffopt.close_file) { >> + f_options[f] = "--output"; >> + f++; >> + } >> + if (output_directory) { >> + f_options[f] = "--output-directory"; >> + f++; >> + } >> + >> + if (f > 1) >> + die(_("options '%s'and '%s' cannot be used together"), f_options[0], f_options[1]); >> >> 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/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 >> ' > > A general comment: To me, it looks like you didn't want to change the > variable name 'f' in the first hunk above, and then just named the array > 'f_options' to go with 'f'. Perhaps `exclusive_opt` (not plural!) for > the array. Now, renaming 'f' to something longer makes the code a bit > unwieldy. Therefore, let me suggest yet another approach: factor out > functions check_exclusive_opts3(), check_exclusive_opts4(), to be used like > > check_exclusive_opts3(use_stdout, "--stdout", > rev.diffopt.close_file, "--output", > output_directory, "--output-directory"); > > I am not yet proposing check_exclusive_opts2(), but others may think it > is an improvement, too, (if they think that such functions are an > improvement in the first place). > > -- Hannes
Le lundi 24 janvier 2022, 12:06:19 CET Phillip Wood a écrit : > On 24/01/2022 07:14, Johannes Sixt wrote: > > Am 22.01.22 um 19:35 schrieb Jean-Noël Avila via GitGitGadget: > >> From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@free.fr> > >> > >> When more than two options are mutually exclusive, print the ones > >> which are actually on the command line. > > > > Reading this, I expect that all mutually exclusive options are listed in > > the error messages. But the updated code lists only the first and second > > option even if more are on the command line. What is the justification > > for that? Just "make the work for translators easier"? I am not 100% > > sure that this is the right balance. Wouldn't it be helpful for users to > > get to know which set of options is incompatible, or is an error message > > not the right place for this kind of education? Don't know... > > That was my feeling as well when reading this patch, I think the loss of > information in the error messages is a shame. The enhancement aims at being more precise as to which options actually present on the command line are mutually exclusive, instead of letting the user sort out where the clash comes from. Personal taste, but the "--foo/--bar" is poor user interaction: better a partial but precise message than a (hopefully) complete but generic one. > >> @@ -1268,19 +1269,19 @@ 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++; > >> + die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup"); > >> + f_options[f] = "-C"; > >> + f+= !!use_message; > >> + f_options[f] = "-c"; > >> + f+=!!edit_message; > >> + f_options[f] = "-F"; > >> + f+=!!logfile; > >> + f_options[f] = "--fixup"; > >> + f+=!!fixup_message; > > This feels like an out of bounds access waiting to happen when someone > adds a new option but forgets to increase the size of f_options above That's one of the advantages of C99: declaring the variables near their use site so that you can keep them in you brain while reading the code. > > Best Wishes > > Phillip > > > > > I prefer this if-cascade over the "f += truth_value" style, because I > > find it is easier to understand. If you write it as > > > > if (extcmd) > > f_options[f++] = "--extcmd"; > > > > you get each branch down to two lines. (But then others may disagree > > with the easy-to-understand argument due to the f++ buried in the > > expression. Unsure...) > > > >> > >> if (use_gui_tool) > >> setenv("GIT_MERGETOOL_GUI", "true", 1); > >> diff --git a/builtin/grep.c b/builtin/grep.c > >> index 9e34a820ad4..b199781cb27 100644 > >> --- a/builtin/grep.c > >> +++ b/builtin/grep.c > >> @@ -1168,10 +1168,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix) > >> setup_pager(); > >> > >> if (!use_index && (untracked || cached)) > >> - die(_("--cached or --untracked cannot be used with -- no-index")); > >> + die(_("options '%s' and '%s' cannot be used together"),"--cached/--untracked", "--no-index"); > > > > Huh? "--cached/--untracked"? Which one was used on the command line? But... > > > >> > >> if (untracked && cached) > >> - die(_("--untracked cannot be used with --cached")); > >> + die(_("options '%s' and '%s' cannot be used together"), "--untracked", "--cached"); > >> > >> if (!use_index || untracked) { > >> int use_exclude = (opt_exclude < 0) ? use_index : !! opt_exclude; > > > > ... doesn't this logic imply that --cached, --untracked, and --no-index > > are three mutually exclusive options? > > > >> diff --git a/builtin/log.c b/builtin/log.c > >> index 4b493408cc5..59b4a2fd380 100644 > >> --- a/builtin/log.c > >> +++ b/builtin/log.c > >> @@ -1759,6 +1759,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > >> struct strbuf rdiff_title = STRBUF_INIT; > >> int creation_factor = -1; > >> > >> + int f = 0; > >> + char * f_options[4]; > >> + > > > > Style: char *f_options[4]; don't needlessly separate these new variables > > from the others by an empty line. > > > >> const struct option builtin_format_patch_options[] = { > >> OPT_CALLBACK_F('n', "numbered", &numbered, NULL, > >> N_("use [PATCH n/m] even with a single patch"), > >> @@ -1978,8 +1981,21 @@ 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"); > >> + if (use_stdout) { > >> + f_options[f] = "--stdout"; > >> + f++; > >> + } > >> + if (rev.diffopt.close_file) { > >> + f_options[f] = "--output"; > >> + f++; > >> + } > >> + if (output_directory) { > >> + f_options[f] = "--output-directory"; > >> + f++; > >> + } > >> + > >> + if (f > 1) > >> + die(_("options '%s'and '%s' cannot be used together"), f_options[0], f_options[1]); > >> > >> 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/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 > >> ' > > > > A general comment: To me, it looks like you didn't want to change the > > variable name 'f' in the first hunk above, and then just named the array > > 'f_options' to go with 'f'. Perhaps `exclusive_opt` (not plural!) for > > the array. Now, renaming 'f' to something longer makes the code a bit > > unwieldy. Therefore, let me suggest yet another approach: factor out > > functions check_exclusive_opts3(), check_exclusive_opts4(), to be used like > > > > check_exclusive_opts3(use_stdout, "--stdout", > > rev.diffopt.close_file, "--output", > > output_directory, "--output-directory"); > > > > I am not yet proposing check_exclusive_opts2(), but others may think it > > is an improvement, too, (if they think that such functions are an > > improvement in the first place). > > > > -- Hannes > Will factorize away such exclusive options, but I'm not sure where. Should it be in git-compat-util.h?
Am 25.01.22 um 21:52 schrieb Jean-Noël AVILA: > Le lundi 24 janvier 2022, 12:06:19 CET Phillip Wood a écrit : >> On 24/01/2022 07:14, Johannes Sixt wrote: >>> A general comment: To me, it looks like you didn't want to change the >>> variable name 'f' in the first hunk above, and then just named the array >>> 'f_options' to go with 'f'. Perhaps `exclusive_opt` (not plural!) for >>> the array. Now, renaming 'f' to something longer makes the code a bit >>> unwieldy. Therefore, let me suggest yet another approach: factor out >>> functions check_exclusive_opts3(), check_exclusive_opts4(), to be used > like >>> >>> check_exclusive_opts3(use_stdout, "--stdout", >>> rev.diffopt.close_file, "--output", >>> output_directory, "--output-directory"); >>> >>> I am not yet proposing check_exclusive_opts2(), but others may think it >>> is an improvement, too, (if they think that such functions are an >>> improvement in the first place). > > Will factorize away such exclusive options, but I'm not sure where. Should it > be in git-compat-util.h? How about parse-options.*? There are already some helper functions near the end of parse-options.c. Perhaps the names I suggested must be revised, though. -- Hannes
diff --git a/builtin/commit.c b/builtin/commit.c index b9ed0374e30..910f4c912bf 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1243,6 +1243,7 @@ static int parse_and_validate_options(int argc, const char *argv[], struct wt_status *s) { int f = 0; + char * f_options[4]; argc = parse_options(argc, argv, prefix, options, usage, 0); finalize_deferred_config(s); @@ -1251,7 +1252,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,19 +1269,19 @@ 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++; + die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup"); + f_options[f] = "-C"; + f+= !!use_message; + f_options[f] = "-c"; + f+=!!edit_message; + f_options[f] = "-F"; + f+=!!logfile; + f_options[f] = "--fixup"; + f+=!!fixup_message; if (f > 1) - die(_("Only one of -c/-C/-F/--fixup can be used.")); + die(_("options '%s' and '%s' cannot be used together"), f_options[0], f_options[1]); if (have_option_m && (edit_message || use_message || logfile)) - die((_("Option -m cannot be combined with -c/-C/-F."))); + die(_("options '%s' and '%s' cannot be used together"), "-m", f_options[0]); if (f || have_option_m) template_file = NULL; if (edit_message) @@ -1305,9 +1306,17 @@ 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.")); + f = 0; + f_options[f] = "-i/--include"; + f += also; + f_options[f] = "-o/--only"; + f += only; + f_options[f] = "-a/--all"; + f += all; + f_options[f] = "--interactive/-p/--patch"; + f += interactive; + if (f > 1) + die(_("options '%s' and '%s' cannot be used together"), f_options[0], f_options[1]); if (fixup_message) { /* diff --git a/builtin/difftool.c b/builtin/difftool.c index c79fbbf67e5..92854bc7737 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -685,6 +685,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0, no_index = 0; static char *difftool_cmd = NULL, *extcmd = NULL; + int f = 0; + char *f_options[3]; struct option builtin_difftool_options[] = { OPT_BOOL('g', "gui", &use_gui_tool, N_("use `diff.guitool` instead of `diff.tool`")), @@ -732,8 +734,20 @@ 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"); + if (use_gui_tool) { + f_options[f] = "--gui"; + f++; + } + if (difftool_cmd) { + f_options[f] = "--tool"; + f++; + } + if (extcmd) { + f_options[f] = "--extcmd"; + f++; + } + if (f > 1) + die(_("options '%s' and '%s' cannot be used together"), f_options[0], f_options[1]); if (use_gui_tool) setenv("GIT_MERGETOOL_GUI", "true", 1); diff --git a/builtin/grep.c b/builtin/grep.c index 9e34a820ad4..b199781cb27 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -1168,10 +1168,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix) setup_pager(); if (!use_index && (untracked || cached)) - die(_("--cached or --untracked cannot be used with --no-index")); + die(_("options '%s' and '%s' cannot be used together"),"--cached/--untracked", "--no-index"); if (untracked && cached) - die(_("--untracked cannot be used with --cached")); + die(_("options '%s' and '%s' cannot be used together"), "--untracked", "--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..59b4a2fd380 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1759,6 +1759,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) struct strbuf rdiff_title = STRBUF_INIT; int creation_factor = -1; + int f = 0; + char * f_options[4]; + const struct option builtin_format_patch_options[] = { OPT_CALLBACK_F('n', "numbered", &numbered, NULL, N_("use [PATCH n/m] even with a single patch"), @@ -1978,8 +1981,21 @@ 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"); + if (use_stdout) { + f_options[f] = "--stdout"; + f++; + } + if (rev.diffopt.close_file) { + f_options[f] = "--output"; + f++; + } + if (output_directory) { + f_options[f] = "--output-directory"; + f++; + } + + if (f > 1) + die(_("options '%s'and '%s' cannot be used together"), f_options[0], f_options[1]); 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/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 '