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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.