Re: [PATCH v2 6/6] i18n: factorize "bad argument" messages
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Apr 3, 2022, 14:41 UTC
- Message-ID
- <220403.86o81ikywa.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <febe64a8a069dd81523c33a93cfea9553e40c283.1648915853.git.gitgitgadget@gmail.com>
On Sat, Apr 02 2022, Jean-Noël Avila via GitGitGadget wrote:
> From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@free.fr> > > We also quote the placeholders as they replace constant strings.
I don't think this change is good in that it takes two different cases we cared about distinguishing before, and squashes them into one. I.e.:
Show 9 quoted lines
> +++ b/builtin/bisect--helper.c
> @@ -268,7 +268,7 @@ static int bisect_write(const char *state, const char *rev,
> } else if (one_of(state, terms->term_good, "skip", NULL)) {
> strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
> } else {
> - res = error(_("Bad bisect_write argument: %s"), state);
> + res = error(_("bad '%s' argument: '%s'"), "bisect_write", state);
> goto finish;
> }I think these should all say "bad value provided for '%s' argument: '%s'", or similar, or maybe the terse version is better. Just a suggestion.
Show 11 quoted lines
> diff --git a/builtin/config.c b/builtin/config.c
> index e7b88a9c08d..2ac36e4f641 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -106,7 +106,7 @@ static int option_parse_type(const struct option *opt, const char *arg,
> else if (!strcmp(arg, "color"))
> new_type = TYPE_COLOR;
> else
> - die(_("unrecognized --type argument, %s"), arg);
> + die(_("bad '%s' argument: '%s'"), "--type", arg);
> }I thought some of these were introducing logic errors, because we were conflating unrecognized arguments with bad values, but these all seem to actually mean "bad value", not "unknown flag".
Show 11 quoted lines
> diff --git a/grep.c b/grep.c
> index 82eb7da1022..6aabfc58bb3 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -43,7 +43,7 @@ static int parse_pattern_type_arg(const char *opt, const char *arg)
> return GREP_PATTERN_TYPE_FIXED;
> else if (!strcmp(arg, "perl"))
> return GREP_PATTERN_TYPE_PCRE;
> - die("bad %s argument: %s", opt, arg);
> + die("bad '%s' argument: '%s'", opt, arg);
> }This and many other changes that follow have nothing to do with i18n.
I think it's a worthwhile cleanup to mark some of these for _(), but shouldn't that come first, or at least after?