On Tue, Nov 24, 2020 at 07:39:48PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +GIT_CONFIG_COUNT,GIT_CONFIG_KEY_<n>,GIT_CONFIG_VALUE_<n>::
> 
> I think we write a header with multiple/related items like this
> instead:
> 
>     GIT_CONFIG_COUNT::
>     GIT_CONFIG_KEY_<n>::
>     GIT_CONFIG_VALUE_<n>::
> 
> See how -f/--file is marked up in an earlier part of the same file.

Ah, thanks. I wondered how to format these but didn't spot other
examples.

> > +	If GIT_CONFIG_COUNT is set to a positive number, all environment pairs
> > +	GIT_CONFIG_KEY_<n> and GIT_CONFIG_VALUE_<n> up to that number will be
> > +	added to the process's runtime configuration. The config pairs are
> > +	zero-indexed. Any missing key or value is treated as an error. An empty
> > +	GIT_CONFIG_COUNT is treated the same as GIT_CONFIG_COUNT=0, namely no
> > +	pairs are processed. Config entries set this way have command scope,
> > +	but will be overridden by any explicit options passed via `git -c`.
> > +
> >  See also <<FILES>>.
> 
> Doesn't this <<FILES>> refer to GIT_CONFIG and GIT_CONFIG_NOSYSTEM
> that are described earlier?  It certainly looks out of place to see
> it after the KEY/VALUE thing.

Right, my fault.

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

Anyway, I'm not opposed to changing this if you think it to be
necessary.

> > +			if (config_parse_pair(key, value, fn, data) < 0) {
> > +				ret = -1;
> > +				goto out;
> > +			}
> > +		}
> >  	}
> >  
> > -	for (i = 0; i < nr; i++) {
> > -		if (git_config_parse_parameter(argv[i], fn, data) < 0) {
> > -			ret = -1;
> > +	env = getenv(CONFIG_DATA_ENVIRONMENT);
> 
> > +	if (env) {
> > +		int nr = 0, alloc = 0;
> > +
> > +		/* sq_dequote will write over it */
> > +		envw = xstrdup(env);
> > +
> > +		if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) {
> > +			ret = error(_("bogus format in %s"), CONFIG_DATA_ENVIRONMENT);
> >  			goto out;
> >  		}
> > +
> > +		for (i = 0; i < nr; i++) {
> > +			if (git_config_parse_parameter(argv[i], fn, data) < 0) {
> > +				ret = -1;
> > +				goto out;
> > +			}
> > +		}
> >  	}
> >  
> >  out:
> > +	strbuf_release(&envvar);
> >  	free(argv);
> >  	free(envw);
> >  	cf = source.prev;
> 
> With re-indentation this patch does, it is a bit hard to see the
> correspondence between common lines in preimage and postimage, but I
> think the patch adds the support of the new style environments
> before the existing support of the GIT_CONFIG_DATA, but when there
> is no compelling reason not to, new code should be added near the
> bottom, not before the existing code, in the function.
> 
> Otherwise, this part of the patch looks OK to me.
> 
> Thanks.

It is required as this is what sets precedence of GIT_CONFIG_PARAMETERS
and thus `git -c` over GIT_CONFIG_COUNT. It's easy enough to split this
into two patches though, with a first refactoring which does the
indentation and a second one which adds the new code.

Patrick