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

From: Junio C Hamano <[email-masked]>
Date: Sun, 10 Jan 2021 16:29:01 -0800

Simon Ruderich <[email-masked]> writes:

> 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.