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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 17, 2020, 14:03 UTC
Message-ID: <875z64f5cb.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqy2j1851k.fsf@gitster.c.googlers.com>

On Mon, Nov 16 2020, Junio C Hamano wrote:

Show 24 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Fri, Nov 13 2020, Patrick Steinhardt wrote:
>>
>>> While not document, it is currently possible to specify config entries
>>
>> "While not documented..."
>>
>>> +		strbuf_addf(&envvar, "GIT_CONFIG_KEY_%d", i);
>>> +		if ((key = getenv(envvar.buf)) == NULL)
>>> +			break;
>>
>> The convention in git.git is to avoid explicit NULL checks. So maybe
>> something like this, which also avoids the assignment inside an "if"
>>
>>     key = getenv(envvar.buf);
>>     if (!key)
>>         break;
>
> All good suggestions, but...
>
> "While not documented" yes, for sure, but we do not document it for
> a good reason---it is a pure implementation detail between Git
> process that runs another one as its internal implementation detail.

*nod* I didn't mean it should be treated as some API, just "it happens
 to work". I do agree with Jeff downthread that it would be nice to have
 it explicitly supported.

Show 6 quoted lines

> I especially do not think we want to read from unbounded number of
> GIT_CONFIG_KEY_<N> variables like this patch does.  How would a
> script cleanse its environment to protect itself from stray such
> environment variable pair?  Count up from 1 to forever?  Run "env"
> and grep for "GIT_CONFIG_KEY_[0-9]*=" (the answer is No.  What if
> some environment variables have newline in its values?)

Purely on an implementation note, if we went that route we could provide something based on compat/unsetenv.c (or environ iteration in general) that would loop over the env, but I agree it would be better to make GIT_CONFIG_PARAMETERS nice.