git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jean-Noël AVILA <jn.avila@free.fr>
Date
Jan 25, 2022, 20:52 UTC
Message-ID
<2894278.sRLLl5rxgE@cayenne>
In-Reply-To
<80593381-2ecc-5de5-76b7-0e6c6452559f@gmail.com>
Le lundi 24 janvier 2022, 12:06:19 CET Phillip Wood a écrit :
Show 17 quoted lines
> 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"));
Show 9 quoted lines
> >> -	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");
Show 11 quoted lines
> >> +	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.

Show 15 quoted lines
> 
> 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...)
Show 9 quoted lines
> > 
> >>   
> >>   	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...
Show 5 quoted lines
> > 
> >>   
> >>   	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;
Show 9 quoted lines
> > 
> > ... 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)
Show 13 quoted lines
> >>   	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)
Show 5 quoted lines
> >>   	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");
Show 15 quoted lines
> >> +	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]);
Show 8 quoted lines
> >>   
> >>   	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");
Show 6 quoted lines
> >>   		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' '
Show 5 quoted lines
> >>   '
> >>   
> >>   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 &&
Show 10 quoted lines
> >>   	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
Show 11 quoted 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
>

Will factorize away such exclusive options, but I'm not sure where. Should it be in git-compat-util.h?

Previous: Phillip WoodNext: Johannes Sixt
Message 5 of 41 in “Factorize i18n”
  1. 0/4 Factorize i18nJean-Noël Avila via GitGitGadget, Jan 22, 2022
  2. 1/4 i18n: factorize more 'incompatible options' messagesJean-Noël Avila via GitGitGadget, Jan 22, 2022
  3. Johannes SixtJan 24, 2022
  4. Phillip WoodJan 24, 2022
  5. Jean-Noël AVILAJan 25, 2022
  6. Johannes SixtJan 25, 2022
  7. 2/4 i18n: factorize "invalid value" messagesJean-Noël Avila via GitGitGadget, Jan 22, 2022
  8. Phillip WoodJan 24, 2022
  9. 4/4 i18n: transfer variables into placeholders in command synopsisJean-Noël Avila via GitGitGadget, Jan 22, 2022
  10. 3/4 i18n: remove from i18n strings that do not hold translatable partsJean-Noël Avila via GitGitGadget, Jan 22, 2022
  11. 0/4 Factorize i18nJean-Noël Avila via GitGitGadget, Jan 28, 2022
  12. 1/4 i18n: factorize more 'incompatible options' messagesJean-Noël Avila via GitGitGadget, Jan 28, 2022
  13. Johannes SixtJan 28, 2022
  14. Junio C HamanoJan 28, 2022
  15. Johannes SixtJan 29, 2022
  16. Jean-Noël AVILAJan 29, 2022
  17. Johannes SixtJan 29, 2022
  18. Ævar Arnfjörð BjarmasonFeb 1, 2022
  19. 2/4 i18n: factorize "invalid value" messagesJean-Noël Avila via GitGitGadget, Jan 28, 2022
  20. 3/4 i18n: remove from i18n strings that do not hold translatable partsJean-Noël Avila via GitGitGadget, Jan 28, 2022
  21. 4/4 i18n: transfer variables into placeholders in command synopsisJean-Noël Avila via GitGitGadget, Jan 28, 2022
  22. 0/4 Factorize i18nJean-Noël Avila via GitGitGadget, Jan 30, 2022
  23. 1/4 i18n: factorize more 'incompatible options' messagesJean-Noël Avila via GitGitGadget, Jan 30, 2022
  24. Johannes SixtJan 31, 2022
  25. Phillip WoodJan 31, 2022
  26. 2/4 i18n: factorize "invalid value" messagesJean-Noël Avila via GitGitGadget, Jan 30, 2022
  27. 4/4 i18n: transfer variables into placeholders in command synopsisJean-Noël Avila via GitGitGadget, Jan 30, 2022
  28. Phillip WoodJan 31, 2022
  29. Jean-Noël AvilaJan 31, 2022
  30. 3/4 i18n: remove from i18n strings that do not hold translatable partsJean-Noël Avila via GitGitGadget, Jan 30, 2022
  31. Johannes SixtJan 31, 2022
  32. 0/4 Factorize i18nJean-Noël Avila via GitGitGadget, Jan 31, 2022
  33. 1/4 i18n: factorize more 'incompatible options' messagesJean-Noël Avila via GitGitGadget, Jan 31, 2022
  34. Junio C HamanoJan 31, 2022
  35. Johannes SixtFeb 1, 2022
  36. Junio C HamanoFeb 1, 2022
  37. Jean-Noël AvilaFeb 2, 2022
  38. Johannes SixtFeb 2, 2022
  39. 2/4 i18n: factorize "invalid value" messagesJean-Noël Avila via GitGitGadget, Jan 31, 2022
  40. 3/4 i18n: remove from i18n strings that do not hold translatable partsJean-Noël Avila via GitGitGadget, Jan 31, 2022
  41. 4/4 i18n: fix some misformated placeholders in command synopsisJean-Noël Avila via GitGitGadget, Jan 31, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.