Re: [PATCH v2 5/6] i18n: factorize read-cache error messages

[Junio C Hamano <gitster@pobox.com>]

"Jean-Noël Avila via GitGitGadget"  <gitgitgadget@gmail.com> writes:

> From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@free.fr>
>
> Signed-off-by: Jean-Noël Avila <jn.avila@free.fr>
> ---
>  read-cache.c     | 8 ++++----
>  t/t1600-index.sh | 6 +++---
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 1ad56d02e1d..639765e8ff1 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1735,8 +1735,8 @@ static unsigned int get_index_format_default(struct repository *r)
>  		if (r->settings.index_version >= 0)
>  			version = r->settings.index_version;
>  		if (version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {
> -			warning(_("index.version set, but the value is invalid.\n"
> -				  "Using version %i"), INDEX_FORMAT_DEFAULT);
> +			warning(_("'%s' set, but the value is invalid.\n"
> +				  "Using version %i"), "index.version", INDEX_FORMAT_DEFAULT);
>  			return INDEX_FORMAT_DEFAULT;

Not the focus of this patch, which (I am guessing without the log
message) is to prevent translators from touching the constant
'index.version' by mistake, but I wonder if the message is helpful
to users.  When I see such a warning, how would I figure out what
to fix so that I do not have to see the same warning again?

We should somehow at least hint that we are talking about an entry
in .git/config file (or it may be coming ~/.gitconfig by mistake, in
which case telling where we see a funny version string does help the
user even more).

At least, telling that index.version is a configuration variable may
nudge the user in the right direction, or is it too obvious and
everybody who touches Git instinctively know when told that their
qfwfq.zfzfz is set to an incorrect value, we are referring to their
configuration variable?  I dunno.

> @@ -1745,8 +1745,8 @@ static unsigned int get_index_format_default(struct repository *r)
>  	version = strtoul(envversion, &endp, 10);
>  	if (*endp ||
>  	    version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {
> -		warning(_("GIT_INDEX_VERSION set, but the value is invalid.\n"
> -			  "Using version %i"), INDEX_FORMAT_DEFAULT);
> +		warning(_("'%s' set, but the value is invalid.\n"
> +			  "Using version %i"), "GIT_INDEX_VERSION", INDEX_FORMAT_DEFAULT);

Likewise, but this is for an envirionment variable.

	The environment variable '%s' set, but the value is invalid.

may be overkill and more importantly makes it impossible to reuse
the previous message, but the latter may be an improvement.
configuration and environment variables may have different
gramatical gender or something, perhaps.