Re: [PATCH v2 2/2] config: allow specifying config entries via envvar pairs
On Tue, Nov 24, 2020 at 11:41:34PM -0800, Junio C Hamano wrote:
Show 35 quoted lines
> 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"?Oh, right. No idea what I was thinking there.
Show 17 quoted lines
> > 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.
Patrick Steinhardt <ps@pks.im>