On Tue, Nov 24, 2020 at 11:41:34PM -0800, Junio C Hamano wrote: > Patrick Steinhardt 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