Re: [PATCH v4 3/3] config: allow overriding of global and system configuration

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 13, 2021, 07:54 UTC
Message-ID: <YHVOUhThkCJiSqmj@ncase>
In-Reply-To: <YHVJNOjJhysn7LBy@coredump.intra.peff.net>

On Tue, Apr 13, 2021 at 03:33:08AM -0400, Jeff King wrote:

Show 13 quoted lines

> On Tue, Apr 13, 2021 at 09:11:52AM +0200, Patrick Steinhardt wrote:
> 
> > +test_expect_success 'override global and system config with missing file' '
> > +	sane_unset GIT_CONFIG_NOSYSTEM &&
> > +	test_must_fail env GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=/dev/null git config --global --list >actual &&
> > +	test_must_fail env GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=does-not-exist git config --system --list >actual &&
> > +	GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=does-not-exist git version
> > +'
> 
> I was slightly surprised to see these still marked as test_must_fail.
> But it's because git-config, when given a _specific_ file to read, will
> complain if the file doesn't exist. And that is true independent of your
> patch.

Yeah, surprising at first but then again it does feel sensible when thinking about it: we ask git-config(1) to explitly list the global/system-level configuration, and when it does not exist it does feel sane to complain.

Show 11 quoted lines

> There is one interesting implication there. Running:
> 
>   GIT_CONFIG_SYSTEM=/dev/null git config --system --list
> 
> does _not_ complain, even though:
> 
>   GIT_CONFIG_NOSYSTEM=1 git config --system --list
> 
> does. IMHO that is quite sensible, but I wanted to point it out, as
> using /dev/null is not an exact replacement for GIT_CONFIG_NOSYSTEM (in
> my opinion it is even better ;) ).

Whereas this is the other edge case: we do ask it to explicitly list the system-level configuration, and it _does_ exist even though it's empty. I wouldn't expect to get an error here, and I do prefer this behaviour to NOSYSTEM, too.

Patrick