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:

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

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

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

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