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

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> 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"?

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