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

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 11, 2021, 08:24 UTC
Message-ID: <X/wLURSbxN3jS4eH@ncase>
In-Reply-To: <xmqqr1ms8gfm.fsf@gitster.c.googlers.com>

On Sun, Jan 10, 2021 at 04:29:01PM -0800, Junio C Hamano wrote:

Show 29 quoted lines

> Simon Ruderich <simon@ruderich.org> 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."

Indeed.

Show 8 quoted lines

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

No, I think that this error message is quite useful as it points out both what's missing and why we expect it to exist in the first place.

Show 6 quoted lines

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

Right, will fix.

Patrick