On Sun, Jan 10, 2021 at 04:29:01PM -0800, Junio C Hamano wrote:
> Simon Ruderich <simon@xxxxxxxxxxxx> 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.
> 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.
> > 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
Attachment:
signature.asc
Description: PGP signature
