Re: [PATCH v2 2/2] config: allow specifying config entries via envvar pairs

From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Patrick Steinhardt <ps@pks.im>,
	git@vger.kernel.org, Jeff King <peff@peff.net>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Philip Oakley <philipoakley@iee.email>
Subject: Re: [PATCH v2 2/2] config: allow specifying config entries via envvar pairs
Date: Wed, 25 Nov 2020 11:50:24 -0800	[thread overview]
Message-ID: <xmqqzh35dxmn.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <87360xq08k.fsf@evledraar.gmail.com> (=?utf-8?B?IsOGdmFyIEFy?= =?utf-8?B?bmZqw7Zyw7A=?= Bjarmason"'s message of "Wed, 25 Nov 2020 10:00:59 +0100")

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, Nov 24 2020, Patrick Steinhardt wrote:
>
> ...some more feedback.
>
>> +GIT_CONFIG_COUNT,GIT_CONFIG_KEY_<n>,GIT_CONFIG_VALUE_<n>::
>> +	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`.
>
> Perhaps work in some/all of some version of these:
>
>  - There's also a GIT_CONFIG_PARAMETERS variable, which is considered
>    internal to Git itself. Users are expected to set these.

"are", or "are not"?  I think it is the latter, and if so I agree
that it is a good thing to say here or somewhere nearby.

>    --> I.e. even if we're not going to support some format for
>    --> GIT_CONFIG_PARAMETERS document what it is.

My preference is to keep it an implementation detail, especially if
we were to be adding this new thing as a documented feature, so
documenting it beyond its existence and nature is counterproductive.

>  - This is analogous to the pre-receive `GIT_PUSH_OPTION_*` variables
>    (see linkgit:githooks[5]), but unlike those the `-c` option to
>    linkgit:git(1) does not set `GIT_CONFIG_*`.

I am slightly negative about this.  It would be an irrelevant noise
to readers who are interested in environment variables that affect
how "git config" works (which is what this section is about).  Also
for those who want to learn about GIT_PUSH_OPTION variable, I do not
think they would look for it in "git config" documentation and check
its ENVIRONMENT section.  It would be much more likely for them to
look for them in the documentation for receive-pack or push (and then
redirected to githooks doc).

>  - Saying "command scope" here I think is wrong/misleading. If I didn't
>    know how this worked I'd expect the first git process to see it to
>    delete it from the env, so e.g. the "fetch" command would see it, but
>    not the "gc" it spawned (different commands). Maybe just say "the
>    scope of these is as with other GIT_* environment variables, they'll
>    be inherited by subprocesses".

OK.

> Re the indent question to make the diff more readable question Junio
> had: could set some "do we have this or that" variables here to not
> reindent the existing code, but maybe not worth the effort...

I was leaving a clue for those who want to futz with "diff"
algorithm that this change can be a good test case for their
improvement.

I didn't mean that as a suggestion to help "diff" produce a better
result by twisting code.  We should not tweak our code to please
"git show" output.  Tweaking code to please "cat/less $file" output
is very much welcome, though.

Thanks.