Re: [PATCH 1/4] i18n: factorize more 'incompatible options' messages

["Jean-Noël AVILA" <jn.avila@free.fr>]

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?