Subject: Re: [PATCH v2 2/2] config: allow specifying config entries via envvar pairs

From: Patrick Steinhardt <[email-masked]>
Date: Wed, 25 Nov 2020 08:57:34 +0100

On Tue, Nov 24, 2020 at 11:41:34PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <[email-masked]> writes:
> >> > +		for (i = 0; i < count; i++) {
> >> > +			const char *key, *value;
> >> > +
> >> > +			strbuf_addf(&envvar, "GIT_CONFIG_KEY_%d", i);
> >> > +			key = getenv(envvar.buf);
> >> > +			if (!key) {
> >> > +				ret = error(_("missing config key %s"), envvar.buf);
> >> > +				goto out;
> >> > +			}
> >> > +			strbuf_reset(&envvar);
> >> > +
> >> > +			strbuf_addf(&envvar, "GIT_CONFIG_VALUE_%d", i);
> >> > +			value = getenv(envvar.buf);
> >> > +			if (!value) {
> >> > +				ret = error(_("missing config value %s"), envvar.buf);
> >> > +				goto out;
> >> > +			}
> >> > +			strbuf_reset(&envvar);
> >> 
> >> Didn't we got bitten by number of times that the string returned by
> >> getenv() are not necessarily nonvolatile depending on platforms?  I
> >> think the result of getenv() would need to be xstrdup'ed.
> >> 
> >> cf. 6776a84d (diff: ensure correct lifetime of external_diff_cmd,
> >> 2019-01-11)
> >
> > We did, but do we have to in this case? There is no interleaving calls
> > to getenv(3P), so we don't depend on at least $n getenv(3P) calls
> > succeeding without clobbering old values. It's true that it could be
> > that any other caller in the callchain clobbers the value, but as far as
> > I can see none does.
> 
> Doesn't the code expect "key" will stay valid even after another
> call to getenv() grabs "value"?

Oh, right. No idea what I was thinking there.

> > It is required as this is what sets precedence of GIT_CONFIG_PARAMETERS
> > and thus `git -c` over GIT_CONFIG_COUNT.
> 
> OK, that is what the "will be overridden by any explicit options"
> was about.  Perhaps that deserves an in-code comment, something like
> 
> 	/*
> 	 * process GIT_CONFIG_KEY_N/GIT_CONFIG_VALUE_N pairs
> 	 * first, to be overridden by GIT_CONFIG_PARAMETERS
> 	 * inherited from parent Git processes' "git -c var=val"
> 	 * later
> 	 */
> 
> before we check GIT_CONFIG_COUNT and loop over the new style
> environment variables.
> 
> Thanks.

Will do, thanks!

Patrick