Patrick Steinhardt <ps@xxxxxx> 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.
