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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2261 bytes --]

On Wed, Nov 18, 2020 at 02:49:39PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Nov 17 2020, Junio C Hamano wrote:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >>> 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?)
> >>
> >> You only have to unset `GIT_CONFIG_KEY_1` as the parser will stop
> >> iterating on the first missing key. More generally, if you set `n` keys,
> >> it's sufficient to unset key `n+1`.
> >
> > Yes, but those who want to set N keys would likely to be content
> > with setting 1..N and happily forget unsetting N+1, and that is
> > where "how would one cleanse the environment to give a clean slate?"
> > comes from.
> 
> Not as an argument from whataboutism, but just to note a bug/existing
> prior art:
> 
> Nobody in this thread has mentioned GIT_PUSH_OPTION_* which works pretty
> much like Patrick's suggestion, and it looks like --local-env-vars
> misses those:
> 
>     $ GIT_PUSH_OPTION_0=foo GIT_PUSH_OPTION_COUNT=20 git rev-parse --local-env-vars | grep GIT_PUSH
>     $
> 
> I haven't tested this, but I expect there's a bug where a push hook
> itself does a local push to another repo and that repo has a hook, that
> the push options are erroneously carried forward to the sub-process.
> 
> That might also be a feature, depending on your point of view.

I didn't actually know about it, thanks for pointing it out!

If we're going to use the same `_COUNT` approach, then I think the
issues which were discussed would mostly go away. No gaps needed, no
requirement to unset `$n+1`. Any properly behaving user would know to
set it as otherwise the written code/script cannot work. Also,
git-rev-parse(1) wouldn't have to dynamically adjust based on whether a
previously existing gap was filled with new keys or not.

I'd be happy to pursue that road, but I'll wait for some feedback first.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]