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@xxxxxxx>
>
> 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@xxxxxxx>
> ---
> 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
