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

Simon Ruderich [off-list ref] writes:

On Thu, Jan 07, 2021 at 07:36:52AM +0100, Patrick Steinhardt wrote:

quoted

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

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

Simon Ruderich [off-list ref] writes:

quoted

On Thu, Jan 07, 2021 at 07:36:52AM +0100, Patrick Steinhardt wrote:

quoted

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

quoted

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