On Tue, Nov 24, 2020 at 07:39:48PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
