Re: [PATCH v3 0/4] Factorize i18n

[Johannes Sixt <j6t@kdbg.org>]

Am 30.01.22 um 23:01 schrieb Jean-Noël Avila via GitGitGadget:
> This is a small series of i18n factorizations. Again the idea is to:
> 
>  * reduce the number of strings to translate
>  * make life easier for translators by removing constant parts of the
>    strings to translate
>  * uniformize the UI by using a type of message for the a given type of
>    condition.
> 
> Changes since V1:
> 
>  * factorize checks of more than 2 incompatible options into dedicated
>    functions which list all the incompatible options present on the command
>    line
> 
> Changes since V2:
> 
>  * switch to tab size 8
>  * rename die_if* to die_for*
>  * make 3 arg check a call to 4 arg check
>  * switch another part to die_for_incompatible_opt4

This iteration looks good overall. There is an unnecessary empty-line
addition hunk in 1/2, though.

-- Hannes

> 
> Jean-Noël Avila (4):
>   i18n: factorize more 'incompatible options' messages
>   i18n: factorize "invalid value" messages
>   i18n: remove from i18n strings that do not hold translatable parts
>   i18n: transfer variables into placeholders in command synopsis
> 
>  archive.c                                 |  2 +-
>  builtin/am.c                              |  8 ++++--
>  builtin/bisect--helper.c                  |  6 ++--
>  builtin/blame.c                           |  7 +++--
>  builtin/commit.c                          | 35 ++++++++++-------------
>  builtin/count-objects.c                   |  2 +-
>  builtin/difftool.c                        |  5 ++--
>  builtin/fast-export.c                     |  2 +-
>  builtin/fetch.c                           |  4 +--
>  builtin/grep.c                            |  8 ++----
>  builtin/hash-object.c                     |  2 +-
>  builtin/help.c                            |  4 +--
>  builtin/log.c                             |  5 ++--
>  builtin/merge-base.c                      |  6 ++--
>  builtin/mktag.c                           |  2 +-
>  builtin/mktree.c                          |  2 +-
>  builtin/notes.c                           |  6 ++--
>  builtin/pack-objects.c                    |  2 +-
>  builtin/prune-packed.c                    |  2 +-
>  builtin/pull.c                            |  6 ++--
>  builtin/push.c                            |  2 +-
>  builtin/rebase.c                          |  2 +-
>  builtin/reflog.c                          |  6 ++--
>  builtin/remote.c                          |  2 +-
>  builtin/replace.c                         |  2 +-
>  builtin/rev-list.c                        |  2 +-
>  builtin/send-pack.c                       |  2 +-
>  builtin/sparse-checkout.c                 |  8 +++---
>  builtin/stripspace.c                      |  4 +--
>  builtin/submodule--helper.c               |  2 +-
>  builtin/update-server-info.c              |  2 +-
>  diff-merges.c                             |  2 +-
>  gpg-interface.c                           |  6 ++--
>  ls-refs.c                                 |  3 +-
>  parallel-checkout.c                       |  4 +--
>  parse-options.c                           | 34 ++++++++++++++++++++++
>  parse-options.h                           | 17 +++++++++++
>  sequencer.c                               |  2 +-
>  setup.c                                   |  3 +-
>  submodule-config.c                        |  2 +-
>  t/t4150-am.sh                             |  2 +-
>  t/t7500-commit-template-squash-signoff.sh |  2 +-
>  42 files changed, 140 insertions(+), 87 deletions(-)
> 
> 
> base-commit: 5d01301f2b865aa8dba1654d3f447ce9d21db0b5
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1123%2Fjnavila%2Ffactorize_i18n-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1123/jnavila/factorize_i18n-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/1123
> 
> Range-diff vs v2:
> 
>  1:  844e01391e1 ! 1:  aac3f9821c7 i18n: factorize more 'incompatible options' messages
>      @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar
>       -		die((_("Option -m cannot be combined with -c/-C/-F.")));
>       -	if (f || have_option_m)
>       +		die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup");
>      -+	die_if_incompatible_opt4(!!use_message, "-C",
>      -+							 !!edit_message, "-c",
>      -+							 !!logfile, "-F",
>      -+							 !!fixup_message, "--fixup");
>      -+	if (have_option_m) {
>      -+		if (edit_message)
>      -+			die(_("options '%s' and '%s' cannot be used together"), "-m", "-c");
>      -+		else if  (use_message)
>      -+			die(_("options '%s' and '%s' cannot be used together"), "-m", "-C");
>      -+		else if (logfile)
>      -+			die(_("options '%s' and '%s' cannot be used together"), "-m", "-F");
>      -+	}
>      ++	die_for_incompatible_opt4(!!use_message, "-C",
>      ++				  !!edit_message, "-c",
>      ++				  !!logfile, "-F",
>      ++				  !!fixup_message, "--fixup");
>      ++	die_for_incompatible_opt4(have_option_m, "-m",
>      ++				  !!edit_message, "-c",
>      ++				  !!use_message, "-C",
>      ++				  !!logfile, "-F");
>       +	if (use_message || edit_message || logfile ||fixup_message || have_option_m)
>        		template_file = NULL;
>        	if (edit_message)
>      @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar
>       -	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");
>      ++	die_for_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: int cmd_difftool(int argc, const char **argv, const char *pr
>        
>       -	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");
>      ++	die_for_incompatible_opt3(use_gui_tool, "--gui",
>      ++				  !!difftool_cmd, "--tool",
>      ++				  !!extcmd, "--extcmd");
>        
>        	if (use_gui_tool)
>        		setenv("GIT_MERGETOOL_GUI", "true", 1);
>      @@ builtin/grep.c: int cmd_grep(int argc, const char **argv, const char *prefix)
>       -
>       -	if (untracked && cached)
>       -		die(_("--untracked cannot be used with --cached"));
>      -+	die_if_incompatible_opt3(!use_index, "--no-index",
>      -+							 untracked, "--untracked",
>      -+							 cached, "--cached");
>      ++	die_for_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: int cmd_format_patch(int argc, const char **argv, const char *pre
>        
>       -	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");
>      ++	die_for_incompatible_opt3(use_stdout, "--stdout",
>      ++				  rev.diffopt.close_file, "--output",
>      ++				  !!output_directory, "--output-directory");
>        
>        	if (use_stdout) {
>        		setup_pager();
>      @@ builtin/merge-base.c: int cmd_merge_base(int argc, const char **argv, const char
>        			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");
>      ++			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");
>      ++		die(_("options '%s' and '%s' cannot be used together"),
>      ++		    "--independent", "--all");
>        
>        	if (cmdmode == 'o')
>        		return handle_octopus(argc, argv, show_all);
>      @@ parse-options.c: void NORETURN usage_msg_opt(const char *msg,
>        	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)
>      ++void die_for_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];
>      @@ parse-options.c: void NORETURN usage_msg_opt(const char *msg,
>       +		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);
>      ++		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]);
>      ++		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]);
>      ++		die(_("options '%s' and '%s' cannot be used together"),
>      ++		    options[0], options[1]);
>       +		break;
>       +	default:
>       +		break;
>      @@ parse-options.c: void NORETURN usage_msg_opt(const char *msg,
>       +}
>       
>        ## parse-options.h ##
>      +@@ parse-options.h: NORETURN void usage_msg_opt(const char *msg,
>      + 			    const char * const *usagestr,
>      + 			    const struct option *options);
>      + 
>      ++void die_for_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);
>      ++
>      ++
>      ++inline void die_for_incompatible_opt3(int opt1, const char *opt1_name,
>      ++				      int opt2, const char *opt2_name,
>      ++				      int opt3, const char *opt3_name)
>      ++{
>      ++	die_for_incompatible_opt4(opt1, opt1_name,
>      ++				  opt2, opt2_name,
>      ++				  opt3, opt3_name,
>      ++				  0, "");
>      ++}
>      ++
>      + /*
>      +  * Use these assertions for callbacks that expect to be called with NONEG and
>      +  * NOARG respectively, and do not otherwise handle the "unset" and "arg"
>       @@ parse-options.h: 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
>       
>  2:  4c860698d69 ! 2:  078336ae418 i18n: factorize "invalid value" messages
>      @@ builtin/am.c: static int parse_opt_patchformat(const struct option *opt, const c
>        	 */
>        	else
>       -		return error(_("Invalid value for --patch-format: %s"), arg);
>      -+		return error(_("invalid value for '%s': '%s'"), "--patch-format", arg);
>      ++		return error(_("invalid value for '%s': '%s'"),
>      ++			     "--patch-format", arg);
>        	return 0;
>        }
>        
>      @@ builtin/am.c: static int parse_opt_show_current_patch(const struct option *opt,
>        		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);
>      ++				     "--show-current-patch", arg);
>        	}
>        
>        	if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>      @@ builtin/blame.c: static int git_blame_config(const char *var, const char *value,
>       -			warning(_("invalid color '%s' in color.blame.repeatedLines"),
>       -				value);
>       +			warning(_("invalid value for '%s': '%s'"),
>      -+					"color.blame.repeatedLines", value);
>      ++				"color.blame.repeatedLines", value);
>        		return 0;
>        	}
>        	if (!strcmp(var, "color.blame.highlightrecent")) {
>      @@ builtin/blame.c: static int git_blame_config(const char *var, const char *value,
>        					    OUTPUT_SHOW_AGE_WITH_COLOR);
>        		} else {
>       -			warning(_("invalid value for blame.coloring"));
>      -+			warning(_("invalid value for '%s': '%s'"), "blame.coloring", value);
>      ++			warning(_("invalid value for '%s': '%s'"),
>      ++				"blame.coloring", value);
>        			return 0;
>        		}
>        	}
>      @@ builtin/fetch.c: static void prepare_format_display(struct ref *ref_map)
>       -		die(_("configuration fetch.output contains invalid value %s"),
>       -		    format);
>       +		die(_("invalid value for '%s': '%s'"),
>      -+			  "fetch.output", format);
>      ++		    "fetch.output", format);
>        
>        	for (rm = ref_map; rm; rm = rm->next) {
>        		if (rm->status == REF_STATUS_REJECT_SHALLOW ||
>      @@ gpg-interface.c: int git_gpg_config(const char *var, const char *value, void *cb
>        		fmt = get_format_by_name(value);
>        		if (!fmt)
>       -			return error("unsupported value for %s: %s",
>      -+			return error("invalid value for '%s': '%s'",
>      ++			return error(_("invalid value for '%s': '%s'"),
>        				     var, value);
>        		use_format = fmt;
>        		return 0;
>      @@ gpg-interface.c: int git_gpg_config(const char *var, const char *value, void *cb
>        
>        		if (ret)
>       -			return error("unsupported value for %s: %s", var,
>      -+			return error("invalid value for '%s': '%s'", var,
>      - 				     value);
>      +-				     value);
>      ++			return error(_("invalid value for '%s': '%s'"),
>      ++				     var, value);
>        		return 0;
>        	}
>      + 
>       
>        ## ls-refs.c ##
>       @@ ls-refs.c: static void ensure_config_read(void)
>      @@ ls-refs.c: static void ensure_config_read(void)
>        			/* do nothing */
>        		} else {
>       -			die(_("invalid value '%s' for lsrefs.unborn"), str);
>      -+			die(_("invalid value for '%s': '%s'"), "lsrefs.unborn", str);
>      ++			die(_("invalid value for '%s': '%s'"),
>      ++			    "lsrefs.unborn", str);
>        		}
>        	}
>        	config_read = 1;
>      @@ parallel-checkout.c: void get_parallel_checkout_configs(int *num_workers, int *t
>        	if (env_workers && *env_workers) {
>        		if (strtol_i(env_workers, 10, num_workers)) {
>       -			die("invalid value for GIT_TEST_CHECKOUT_WORKERS: '%s'",
>      +-			    env_workers);
>       +			die(_("invalid value for '%s': '%s'"),
>      -+				"GIT_TEST_CHECKOUT_WORKERS",
>      - 			    env_workers);
>      ++			    "GIT_TEST_CHECKOUT_WORKERS", env_workers);
>        		}
>        		if (*num_workers < 1)
>      + 			*num_workers = online_cpus();
>       
>        ## sequencer.c ##
>       @@ sequencer.c: static int populate_opts_cb(const char *key, const char *value, void *data)
>      @@ setup.c: static enum extension_result handle_extension(const char *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);
>      ++			return error(_("invalid value for '%s': '%s'"),
>      ++				     "extensions.objectformat", value);
>        		data->hash_algo = format;
>        		return EXTENSION_OK;
>        	}
>  3:  376f8b56908 = 3:  d5ddd903df7 i18n: remove from i18n strings that do not hold translatable parts
>  4:  8b0057f6180 = 4:  b8e80e178ee i18n: transfer variables into placeholders in command synopsis
>