Re: [PATCH 3/3] config: store "git -c" variables using more robust format

From: Jeff King <peff@peff.net>
Date: Dec 11, 2020, 13:21 UTC
Message-ID: <X9NyPfvPjgg1mfZH@coredump.intra.peff.net>
In-Reply-To: <87pn3hwfd5.fsf@evledraar.gmail.com>

On Thu, Dec 10, 2020 at 09:55:18PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 22 quoted lines

> > diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> > index bd602e7720..e06961767f 100755
> > --- a/t/t1300-config.sh
> > +++ b/t/t1300-config.sh
> > @@ -1413,6 +1413,14 @@ test_expect_success 'git -c and --config-env override each other' '
> >  	test_cmp expect actual
> >  '
> >  
> > +test_expect_success '--config-env handles keys with equals' '
> > +	echo value=with=equals >expect &&
> > +	ENVVAR=value=with=equals git \
> > +		--config-env=section.subsection=with=equals.key=ENVVAR \
> > +		config section.subsection=with=equals.key >actual &&
> > +	test_cmp expect actual
> > +'
> > +
> 
> Maybe worth adding a test for the strrchr() semantics here with:
> 
>     perl -we '$ENV{"Y=Z"}="why and zed"; system "Z=zed git --config-env=X=Y=Z ..."'
> 
> Which would show that we can't look up "Y=Z", but will always get "Z".

We're already testing those here, though. If we used strchr(), then we'd end up setting section.subsection in the above test.

I.e., your X=Y=Z can be tested in either of two ways:

  - check that X=Y is set to $Z

  - check that X is not set to Y=Z

It's sufficient to test only one, because success in one implies the other. And of the two, I think testing the first is much more interesting (because testing "this expected thing happened" is much more robust than "this unexpected thing didn't happen").

> I.e. I can't imagine someone wants an env var with "=" in it, even
> though POSIX makes such a thing possible (you just can't do it in a
> shellscript).

Yeah, I'm definitely OK with that limitation, but we should document it.

-Peff