Factorize i18n

From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <[email protected]>

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 <[email protected]>

---
 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(-)

builtin/commit.c +2 -9

index b9ed0374e30..910f4c912bf 100644
@@ -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)

builtin/commit.c +2 -3

 	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;
-

builtin/commit.c +3 -0

+	f += interactive;
+	if (f > 1)
+		die(_("options '%s' and '%s' cannot be used together"), f_options[0], f_options[1]);
 
 	if (fixup_message) {
 		/*

builtin/difftool.c +6 -2

index c79fbbf67e5..92854bc7737 100644
@@ -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++;
+	}

builtin/log.c +15 -2

 	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();

builtin/merge-base.c +2 -2

index 6719ac198dc..1447f1c493a 100644
@@ -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);

t/t7500-commit-template-squash-signoff.sh +0 -0

index 91964653a0b..5fcaa0b4f2a 100755

Am 22.01.22 um 19:35 schrieb Jean-Noël Avila via GitGitGadget:

Quoted 4 lines

From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <[email protected]>

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...

Quoted 45 lines

Signed-off-by Jean-Noël Avila <[email protected]>

---
  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(-)

builtin/commit.c +3 -10

index b9ed0374e30..910f4c912bf 100644
@@ -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");

builtin/commit.c static int parse_and_validate_options(int argc, const char *argv[], · line 1280

Quoted 8 lines

builtin/commit.c +8 -0

+	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.

Quoted 14 lines

builtin/commit.c +2 -3

  	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;
-

builtin/commit.c static int parse_and_validate_options(int argc, const char *argv[], · line 1316

Quoted 10 lines

builtin/commit.c +8 -2

-	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...

Quoted 29 lines

builtin/commit.c +3 -0

+	f += interactive;
+	if (f > 1)
+		die(_("options '%s' and '%s' cannot be used together"), f_options[0], f_options[1]);
 
  	if (fixup_message) {
  		/*

builtin/difftool.c +6 -2

index c79fbbf67e5..92854bc7737 100644
@@ -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++;
+	}

builtin/difftool.c int cmd_difftool(int argc, const char **argv, const char *prefix) · line 750

Quoted 10 lines

builtin/difftool.c +10 -0

+	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...)

builtin/grep.c int cmd_grep(int argc, const char **argv, const char *prefix) · line 1171

Quoted 13 lines

builtin/difftool.c +0 -0

  	if (use_gui_tool)
  		setenv("GIT_MERGETOOL_GUI", "true", 1);

builtin/grep.c +1 -1

index 9e34a820ad4..b199781cb27 100644
@@ -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...

builtin/grep.c int cmd_grep(int argc, const char **argv, const char *prefix) · line 1177

Quoted 7 lines

builtin/grep.c +1 -1

  	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?

builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 1764

Quoted 11 lines

builtin/log.c +3 -0

index 4b493408cc5..59b4a2fd380 100644
@@ -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.

Quoted 51 lines

builtin/log.c +15 -2

  	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();

builtin/merge-base.c +2 -2

index 6719ac198dc..1447f1c493a 100644
@@ -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);

t/t7500-commit-template-squash-signoff.sh +0 -0

index 91964653a0b..5fcaa0b4f2a 100755

t/t7500-commit-template-squash-signoff.sh test_expect_success '--fixup=reword: give error with pathsec' ' · line 448

Quoted 8 lines

t/t7500-commit-template-squash-signoff.sh +1 -1

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

Quoted 6 lines · Johannes Sixt

Am 22.01.22 um 19:35 schrieb Jean-Noël Avila via GitGitGadget:

Quoted 4 lines

From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <[email protected]>

When more than two options are mutually exclusive, print the ones which are actually on the command line.

Quoted 7 lines

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.

Quoted 1 line

Review static int parse_and_validate_options(int argc,

Quoted 1 line

(quoted diff) +0 -0

@@ -1268,19 +1269,19 @@ static int parse_and_validate_options(int argc, 

Quoted 1 line

Review static int parse_and_validate_options(int argc, · line 1269

Quoted 1 line

(quoted diff) +0 -0

  			die(_("You are in the middle of a rebase -- 

Quoted 3 lines

builtin/commit.c static int parse_and_validate_options(int argc, const char *argv[],

Quoted 3 lines

builtin/commit.c +0 -1

  	}
  	if (fixup_message && squash_message)
-		die(_("Options --squash and --fixup cannot be used 

Quoted 9 lines

builtin/commit.c static int parse_and_validate_options(int argc, const char *argv[],

Quoted 9 lines

builtin/commit.c +1 -8

-	if (use_message)
-		f++;
-	if (edit_message)
-		f++;
-	if (fixup_message)
-		f++;
-	if (logfile)
-		f++;
+		die(_("options '%s' and '%s' cannot be used together"), 

Quoted 8 lines

builtin/commit.c static int parse_and_validate_options(int argc, const char *argv[], · line 1280

Quoted 8 lines

builtin/commit.c +8 -0

+	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

Quoted 16 lines

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...)

Quoted 1 line

Quoted 7 lines

builtin/grep.c int cmd_grep(int argc, const char **argv, const

Quoted 7 lines

  	if (use_gui_tool)
  		setenv("GIT_MERGETOOL_GUI", "true", 1);

builtin/grep.c +0 -0

index 9e34a820ad4..b199781cb27 100644
@@ -1168,10 +1168,10 @@ int cmd_grep(int argc, const char **argv, const 

Quoted 4 lines

builtin/grep.c int cmd_grep(int argc, const char **argv, const char *prefix)

Quoted 4 lines

builtin/grep.c +0 -1

  		setup_pager();
 
  	if (!use_index && (untracked || cached))
-		die(_("--cached or --untracked cannot be used with --

Quoted 1 line

builtin/grep.c int cmd_grep(int argc, const char **argv, const · line 1171

Quoted 1 line

builtin/grep.c +1 -0

+		die(_("options '%s' and '%s' cannot be used 

Quoted 2 lines

Huh? "--cached/--untracked"? Which one was used on the command line?

Quoted 5 lines

builtin/grep.c int cmd_grep(int argc, const char **argv, const char *prefix)

Quoted 5 lines

builtin/grep.c +1 -1

  	if (untracked && cached)
-		die(_("--untracked cannot be used with --cached"));
+		die(_("options '%s' and '%s' cannot be used together"), 

Quoted 3 lines

builtin/grep.c int cmd_grep(int argc, const char **argv, const · line 1175

Quoted 3 lines

builtin/grep.c +0 -0

  	if (!use_index || untracked) {
  		int use_exclude = (opt_exclude < 0) ? use_index : !!

Quoted 9 lines

... doesn't this logic imply that --cached, --untracked, and --no-index are three mutually exclusive options?

builtin/log.c int cmd_format_patch(int argc, const char **argv,

Quoted 5 lines

builtin/log.c +0 -0

index 4b493408cc5..59b4a2fd380 100644
@@ -1759,6 +1759,9 @@ int cmd_format_patch(int argc, const char **argv, 

Quoted 10 lines

builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix) · line 1764

Quoted 6 lines

builtin/log.c +3 -0

  	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.

Quoted 3 lines

builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix)

Quoted 3 lines

builtin/log.c +0 -0

  	const struct option builtin_format_patch_options[] = {
  		OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
  			    N_("use [PATCH n/m] even with a single 

Quoted 1 line

builtin/log.c int cmd_format_patch(int argc, const char **argv,

Quoted 1 line

builtin/log.c +0 -0

@@ -1978,8 +1981,21 @@ int cmd_format_patch(int argc, const char **argv, 

Quoted 5 lines

builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix)

Quoted 5 lines

builtin/log.c +0 -2

  	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 

Quoted 7 lines

Quoted 7 lines

builtin/log.c +7 -0

+	if (use_stdout) {
+		f_options[f] = "--stdout";
+		f++;
+	}
+	if (rev.diffopt.close_file) {
+		f_options[f] = "--output";
+		f++;

Quoted 8 lines

builtin/log.c int cmd_format_patch(int argc, const char **argv, const char *prefix)

Quoted 8 lines

builtin/log.c +8 -0

+	}
+	if (output_directory) {
+		f_options[f] = "--output-directory";
+		f++;
+	}
+
+	if (f > 1)
+		die(_("options '%s'and '%s' cannot be used together"), 

Quoted 8 lines

builtin/merge-base.c int cmd_merge_base(int argc, const char **argv,

Quoted 8 lines

builtin/log.c +0 -0

  	if (use_stdout) {
  		setup_pager();

builtin/merge-base.c +0 -0

index 6719ac198dc..1447f1c493a 100644
@@ -159,12 +159,12 @@ int cmd_merge_base(int argc, const char **argv, 

Quoted 2 lines

builtin/merge-base.c int cmd_merge_base(int argc, const char **argv, · line 160

Quoted 2 lines

builtin/merge-base.c +0 -0

  		if (argc < 2)
  			usage_with_options(merge_base_usage, 

Quoted 2 lines

builtin/merge-base.c int cmd_merge_base(int argc, const char **argv, · line 162

Quoted 2 lines

builtin/merge-base.c +0 -1

  		if (show_all)
-			die("--is-ancestor cannot be used with --

Quoted 1 line

builtin/merge-base.c int cmd_merge_base(int argc, const char **argv, · line 162

Quoted 1 line

builtin/merge-base.c +1 -0

+			die(_("options '%s' and '%s' cannot be used 

Quoted 6 lines

builtin/merge-base.c int cmd_merge_base(int argc, const char **argv, const char *prefix)

Quoted 6 lines

builtin/merge-base.c +1 -1

  		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 

Quoted 4 lines

t/t7500-commit-

Quoted 4 lines

builtin/merge-base.c +0 -0

  	if (cmdmode == 'o')
  		return handle_octopus(argc, argv, show_all);

t/t7500-commit- renamed from t/t7500-commit-template-squash-signoff.sh +0 -0

Quoted 4 lines

t/t7500-commit-template-squash-signoff.sh test_expect_success '--fixup=reword: give error with

Quoted 4 lines

index 91964653a0b..5fcaa0b4f2a 100755

t/t7500-commit-template-squash-signoff.sh +0 -0

@@ -442,7 +442,7 @@ test_expect_success '--fixup=reword: give error with 

Quoted 5 lines

t/t7500-commit-template-squash-signoff.sh test_expect_success '--fixup=reword: give error with pathsec' '

Quoted 5 lines

t/t7500-commit-template-squash-signoff.sh +1 -1

  '
 
  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 

Quoted 10 lines

t/t7500-commit-template-squash-signoff.sh test_expect_success '--fixup=reword: give error with pathsec' ' · line 448

Quoted 3 lines

t/t7500-commit-template-squash-signoff.sh +0 -0

  	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

Quoted 10 lines

	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

Quoted 7 lines · Johannes Sixt

Quoted 6 lines

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

Quoted 5 lines

Quoted 5 lines

	check_exclusive_opts3(use_stdout, "--stdout",
		rev.diffopt.close_file, "--output",
		output_directory, "--output-directory");

Quoted 3 lines

Quoted 3 lines

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?

From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <[email protected]>

Use the same message when an invalid value is passed to a command line option or a configuration variable.

Signed-off-by Jean-Noël Avila <[email protected]>

---
  builtin/am.c           | 7 ++++---
  builtin/blame.c        | 6 +++---
  builtin/fetch.c        | 4 ++--
  builtin/pack-objects.c | 2 +-
  builtin/pull.c         | 6 +++---
  builtin/push.c         | 2 +-
  builtin/send-pack.c    | 2 +-
  diff-merges.c          | 2 +-
  gpg-interface.c        | 4 ++--
  ls-refs.c              | 2 +-
  parallel-checkout.c    | 3 ++-
  sequencer.c            | 2 +-
  setup.c                | 2 +-
  submodule-config.c     | 2 +-
  t/t4150-am.sh          | 2 +-
  15 files changed, 25 insertions(+), 23 deletions(-)

builtin/am.c +4 -3

index b6be1f1cb11..ba1dacbc034 100644
@@ -199,7 +199,7 @@ static int am_option_parse_empty(const struct option *opt,
  	else if (!strcmp(arg, "keep"))
  		*opt_value = KEEP_EMPTY_COMMIT;
  	else
-		return error(_("Invalid value for --empty: %s"), arg);
+		return error(_("invalid value for '%s': '%s'"), "--empty", arg);
 
  	return 0;
  }

@@ -2239,7 +2239,7 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int
  	 * when you add new options
  	 */
  	else
-		return error(_("Invalid value for --patch-format: %s"), arg);
+		return error(_("invalid value for '%s': '%s'"), "--patch-format", arg);
  	return 0;
  }
 

@@ -2282,7 +2282,8 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
  				break;
  		}
  		if (new_value >= ARRAY_SIZE(valid_modes))
-			return error(_("Invalid value for --show-current-patch: %s"), arg);
+			return error(_("invalid value for '%s': '%s'"),
+						 "--show-current-patch", arg);
  	}
 
  	if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)

builtin/blame.c +3 -3

index 7fafeac4081..ca821420d68 100644
@@ -721,8 +721,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)
  	}
  	if (!strcmp(var, "color.blame.repeatedlines")) {
  		if (color_parse_mem(value, strlen(value), repeated_meta_color))
-			warning(_("invalid color '%s' in color.blame.repeatedLines"),
-				value);
+			warning(_("invalid value for '%s': '%s'"),
+					"color.blame.repeatedLines", value);
  		return 0;
  	}
  	if (!strcmp(var, "color.blame.highlightrecent")) {

@@ -739,7 +739,7 @@ static int git_blame_config(const char *var, const char *value, void *cb)
  			coloring_mode &= ~(OUTPUT_COLOR_LINE |
  					    OUTPUT_SHOW_AGE_WITH_COLOR);
  		} else {
-			warning(_("invalid value for blame.coloring"));
+			warning(_("invalid value for '%s': '%s'"), "blame.coloring", value);
  			return 0;
  		}
  	}

builtin/fetch.c +2 -2

index eaab8056bf9..19ec48f3330 100644
@@ -758,8 +758,8 @@ static void prepare_format_display(struct ref *ref_map)
  	else if (!strcasecmp(format, "compact"))
  		compact_format = 1;
  	else
-		die(_("configuration fetch.output contains invalid value %s"),
-		    format);
+		die(_("invalid value for '%s': '%s'"),
+			  "fetch.output", format);
 
  	for (rm = ref_map; rm; rm = rm->next) {
  		if (rm->status == REF_STATUS_REJECT_SHALLOW ||

builtin/pack-objects.c +1 -1

index ba2006f2212..192c3ca305e 100644
@@ -3504,7 +3504,7 @@ static int option_parse_missing_action(const struct option *opt,
  		return 0;
  	}
 
-	die(_("invalid value for --missing"));
+	die(_("invalid value for '%s': '%s'"), "--missing", arg);
  	return 0;
  }
 

builtin/pull.c +3 -3

index 100cbf9fb85..e54a0ccadc6 100644
@@ -42,9 +42,9 @@ static enum rebase_type parse_config_rebase(const char *key, const char *value,
  		return v;
 
  	if (fatal)
-		die(_("Invalid value for %s: %s"), key, value);
+		die(_("invalid value for '%s': '%s'"), key, value);
  	else
-		error(_("Invalid value for %s: %s"), key, value);
+		error(_("invalid value for '%s': '%s'"), key, value);
 
  	return REBASE_INVALID;
  }

@@ -318,7 +318,7 @@ static const char *config_get_ff(void)
  	if (!strcmp(value, "only"))
  		return "--ff-only";
 
-	die(_("Invalid value for pull.ff: %s"), value);
+	die(_("invalid value for '%s': '%s'"), "pull.ff", value);
  }
 
  /**

builtin/push.c +1 -1

index 359db90321c..cad997965a7 100644
@@ -486,7 +486,7 @@ static int git_push_config(const char *k, const char *v, void *cb)
  				if (value && !strcasecmp(value, "if-asked"))
  					set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_IF_ASKED);
  				else
-					return error("Invalid value for '%s'", k);
+					return error(_("invalid value for '%s'"), k);
  			}
  		}
  	} else if (!strcmp(k, "push.recursesubmodules")) {

builtin/send-pack.c +1 -1

index 69c432ef1a6..64962be0168 100644
@@ -145,7 +145,7 @@ static int send_pack_config(const char *k, const char *v, void *cb)
  				if (value && !strcasecmp(value, "if-asked"))
  					args.push_cert = SEND_PACK_PUSH_CERT_IF_ASKED;
  				else
-					return error("Invalid value for '%s'", k);
+					return error(_("invalid value for '%s'"), k);
  			}
  		}
  	}

diff-merges.c +1 -1

index 5060ccd890b..cd6c102a0d5 100644
@@ -67,7 +67,7 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)
  	diff_merges_setup_func_t func = func_by_opt(optarg);
 
  	if (!func)
-		die(_("unknown value for --diff-merges: %s"), optarg);
+		die(_("invalid value for '%s': '%s'"), "--diff-merges", optarg);
 
  	func(revs);
 

gpg-interface.c +2 -2

index b52eb0e2e04..04d751a16a8 100644
@@ -702,7 +702,7 @@ int git_gpg_config(const char *var, const char *value, void *cb)
  			return config_error_nonbool(var);
  		fmt = get_format_by_name(value);
  		if (!fmt)
-			return error("unsupported value for %s: %s",
+			return error("invalid value for '%s': '%s'",
  				     var, value);
  		use_format = fmt;
  		return 0;

@@ -717,7 +717,7 @@ int git_gpg_config(const char *var, const char *value, void *cb)
  		free(trust);
 
  		if (ret)
-			return error("unsupported value for %s: %s", var,
+			return error("invalid value for '%s': '%s'", var,
  				     value);
  		return 0;
  	}

ls-refs.c +1 -1

index 54078323dcb..ae09dd59478 100644
@@ -34,7 +34,7 @@ static void ensure_config_read(void)
  		} else if (!strcmp(str, "ignore")) {
  			/* do nothing */
  		} else {
-			die(_("invalid value '%s' for lsrefs.unborn"), str);
+			die(_("invalid value for '%s': '%s'"), "lsrefs.unborn", str);
  		}
  	}
  	config_read = 1;

parallel-checkout.c +2 -1

index 8dd7e7bad40..237c02f1d81 100644
@@ -39,7 +39,8 @@ void get_parallel_checkout_configs(int *num_workers, int *threshold)
 
  	if (env_workers && *env_workers) {
  		if (strtol_i(env_workers, 10, num_workers)) {
-			die("invalid value for GIT_TEST_CHECKOUT_WORKERS: '%s'",
+			die(_("invalid value for '%s': '%s'"),
+				"GIT_TEST_CHECKOUT_WORKERS",
  			    env_workers);
  		}
  		if (*num_workers < 1)

sequencer.c +1 -1

index 6abd72160cc..9b985e8648d 100644
@@ -2806,7 +2806,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
  		return error(_("invalid key: %s"), key);
 
  	if (!error_flag)
-		return error(_("invalid value for %s: %s"), key, value);
+		return error(_("invalid value for '%s': '%s'"), key, value);
 
  	return 0;
  }

setup.c +1 -1

index af3b8c09abe..fe74092a022 100644
@@ -559,7 +559,7 @@ static enum extension_result handle_extension(const char *var,
  			return config_error_nonbool(var);
  		format = hash_algo_by_name(value);
  		if (format == GIT_HASH_UNKNOWN)
-			return error("invalid value for 'extensions.objectformat'");
+			return error(_("invalid value for '%s': '%s'"), "extensions.objectformat", value);
  		data->hash_algo = format;
  		return EXTENSION_OK;
  	}

submodule-config.c +1 -1

index f95344028b5..fb95a026f48 100644
@@ -496,7 +496,7 @@ static int parse_config(const char *var, const char *value, void *data)
  		else if (parse_submodule_update_strategy(value,
  			 &submodule->update_strategy) < 0 ||
  			 submodule->update_strategy.type == SM_UPDATE_COMMAND)
-			die(_("invalid value for %s"), var);
+			die(_("invalid value for '%s'"), var);
  	} else if (!strcmp(item.buf, "shallow")) {
  		if (!me->overwrite && submodule->recommend_shallow != -1)
  			warn_multiple_config(me->treeish_name, submodule->name,

t/t4150-am.sh +1 -1

index 6caff0ca397..159fae8d016 100755
@@ -1169,7 +1169,7 @@ test_expect_success 'invalid when passing the --empty option alone' '
  	test_when_finished "git am --abort || :" &&
  	git checkout empty-commit^ &&
  	test_must_fail git am --empty empty-commit.patch 2>err &&
-	echo "error: Invalid value for --empty: empty-commit.patch" >expected &&
+	echo "error: invalid value for '\''--empty'\'': '\''empty-commit.patch'\''" >expected &&
  	test_cmp expected err
  '
 

From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <[email protected]>

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 <[email protected]>

---
 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(-)

builtin/commit.c +1 -12

index b9ed0374e30..1966f965008 100644
@@ -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++;

builtin/commit.c +4 -3

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

builtin/difftool.c +3 -2

index c79fbbf67e5..ae487785735 100644
@@ -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);

builtin/grep.c +3 -5

index 9e34a820ad4..cdf52667710 100644
@@ -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;

builtin/log.c +3 -2

index 4b493408cc5..048b2c37470 100644
@@ -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();

builtin/merge-base.c +0 -1

index 6719ac198dc..1447f1c493a 100644
@@ -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");

parse-options.c +40 -0

index a8283037be9..fb9e1976ab3 100644
@@ -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;

parse-options.h +0 -0

index e22846d3b7b..cf393839ac4 100644
@@ -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"))
 

Quoted 20 lines

parse-options.c +20 -0

+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;

parse-options.c void NORETURN usage_msg_opt(const char *msg, · line 1131

Quoted 10 lines

parse-options.c +10 -0

+	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().

Quoted 2 lines · Johannes Sixt

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:

Quoted 17 lines

Quoted 2 lines · Johannes Sixt

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.

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

Am 29.01.22 um 00:58 schrieb Junio C Hamano:

Quoted 6 lines

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.

Quoted 1 line

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.

From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <[email protected]> [...]

parse-options.c +49 -0

+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;
+	}
+}

parse-options.h +9 -0

index e22846d3b7b..cf393839ac4 100644
@@ -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

t/t7500-commit-template-squash-signoff.sh +0 -0

index 91964653a0b..5fcaa0b4f2a 100755