Re: [PATCH v6 2/8] config: add new way to pass config via `--config-env`

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 11, 2021, 00:29 UTC
Message-ID: <xmqqr1ms8gfm.fsf@gitster.c.googlers.com>
In-Reply-To: <X/tjtVGRKRhX0ZvU@ruderich.org>

Simon Ruderich <simon@ruderich.org> writes:

Show 20 quoted lines

> On Thu, Jan 07, 2021 at 07:36:52AM +0100, Patrick Steinhardt wrote:
>> [snip]
>>
>> +void git_config_push_env(const char *spec)
>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +	const char *env_name;
>> +	const char *env_value;
>> +
>> +	env_name = strrchr(spec, '=');
>> +	if (!env_name)
>> +		die("invalid config format: %s", spec);
>> +	env_name++;
>> +
>> +	env_value = getenv(env_name);
>> +	if (!env_value)
>> +		die("config variable missing for '%s'", env_name);
>
> I think "environment variable" should be mentioned in the error
> message to make it clear what kind of "variable" is missing.

Good observation. This parses foo=bar and complains about bar missing in the environment; It is not a "config variable" that is missing.

It is "'bar', which is supposed to be there whose value is going to be used as the value of configuration variable 'foo', is missing."

I wonder if we should also talk about 'foo' at the same time as a hint for what went wrong? E.g.

	die(_("missing environment variable '%s' for configuration '%.*s'"),
            env_name, (int)((env_name-1) - spec), spec);

I don't offhand know if that is too much info that may not be all that useful, though.

> Btw. shouldn't these strings get translated (or does die() do
> that automatically)?

The format string given to die/error/warn should be marked with _().

Thanks.