diff mbox series

config: Introduce GIT_CONFIG_NOGLOBAL

Message ID a23382059bb57022dd1e40d1c2c9a11307b0ff3b.1617891426.git.ps@pks.im (mailing list archive)
State New
Headers show
Series config: Introduce GIT_CONFIG_NOGLOBAL | expand

Commit Message

Patrick Steinhardt April 8, 2021, 2:17 p.m. UTC
While it's already possible to stop git from reading the system config
via GIT_CONFIG_NOSYSTEM, doing the same for global config files requires
the user to unset both HOME and XDG_CONFIG_HOME. This is an awkward
interface and may even pose a problem e.g. when git hooks rely on these
variables to be present.

Introduce a new GIT_CONFIG_NOGLOBAL envvar, which is the simple
equivalent to GIT_CONFIG_NOSYSTEM. If set to true, git will skip reading
both `~/.gitconfig` and `$XDG_CONFIG_HOME/git/config`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-config.txt |  4 ++++
 Documentation/git.txt        | 16 ++++++++++++----
 config.c                     |  9 +++++++--
 t/t1300-config.sh            | 31 +++++++++++++++++++++++++++++++
 4 files changed, 54 insertions(+), 6 deletions(-)

Comments

Eric Sunshine April 8, 2021, 4:44 p.m. UTC | #1
On Thu, Apr 8, 2021 at 10:18 AM Patrick Steinhardt <ps@pks.im> wrote:
> While it's already possible to stop git from reading the system config
> via GIT_CONFIG_NOSYSTEM, doing the same for global config files requires
> the user to unset both HOME and XDG_CONFIG_HOME. This is an awkward
> interface and may even pose a problem e.g. when git hooks rely on these
> variables to be present.
>
> Introduce a new GIT_CONFIG_NOGLOBAL envvar, which is the simple
> equivalent to GIT_CONFIG_NOSYSTEM. If set to true, git will skip reading
> both `~/.gitconfig` and `$XDG_CONFIG_HOME/git/config`.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> @@ -670,13 +670,21 @@ for further details.
> +`GIT_CONFIG_NOGLOBAL`::
> +       Whether to skip reading settings from the system-wide `~/.gitconfig`
> +       and `$XDG_CONFIG_HOME/git/config` files.  This environment variable can
> +       be used along with `$GIT_CONFIG_NOSYSTEM` to create a predictable
> +       environment for a picky script, or you can set it temporarily to avoid
> +       using a buggy global config file while waiting for someone with
> +       sufficient permissions to fix it.

Not necessarily a new problem since you mostly copied the new text
from GIT_CONFIG_NOSYSTEM, but this doesn't tell the reader what value
to assign to this variable. As currently written, I would end up
having to consult the source code to figure out how to use this
variable, which makes the documentation less useful than it should be.
Perhaps it could be rewritten something like:

    If set to any value, suppress reading global configuration
    from `~/.gitconfig` and `$XDG_CONFIG_HOME/git/config`
    files. This environment variable...

The bit about waiting for someone to fix a buggy ~/.gitconfig is
somewhat questionable; it might make sense to drop everything after
"picky script".

>  `GIT_CONFIG_NOSYSTEM`::
>         Whether to skip reading settings from the system-wide
>         `$(prefix)/etc/gitconfig` file.  This environment variable can
> -       be used along with `$HOME` and `$XDG_CONFIG_HOME` to create a
> -       predictable environment for a picky script, or you can set it
> -       temporarily to avoid using a buggy `/etc/gitconfig` file while
> -       waiting for someone with sufficient permissions to fix it.
> +       be used along with `$GIT_CONFIG_NOGLOBAL` to create a predictable
> +       environment for a picky script, or you can set it temporarily to avoid
> +       using a buggy `/etc/gitconfig` file while waiting for someone with
> +       sufficient permissions to fix it.

This suffers the same problem of not telling the reader what value to
assign. A similar rewrite could improve it, as well.
Junio C Hamano April 8, 2021, 5:25 p.m. UTC | #2
Patrick Steinhardt <ps@pks.im> writes:

> While it's already possible to stop git from reading the system config
> via GIT_CONFIG_NOSYSTEM, doing the same for global config files requires
> the user to unset both HOME and XDG_CONFIG_HOME. This is an awkward
> interface and may even pose a problem e.g. when git hooks rely on these
> variables to be present.

Yeah, having to unset HOME would affect not just Git.  Git may call
out something else (e.g. an editor) that may want to know where HOME
is, Git may be used in something else (e.g. an IDE) that may want to
know where HOME is.  Same for XDG_CONFIG_HOME.  If it is a valid need
to not reading from $HOME/.gitconfig, unsetting HOME is an unacceptable
approach to satisfying that need.

> Introduce a new GIT_CONFIG_NOGLOBAL envvar, which is the simple
> equivalent to GIT_CONFIG_NOSYSTEM. If set to true, git will skip reading
> both `~/.gitconfig` and `$XDG_CONFIG_HOME/git/config`.

I do not think we'd add an unbounded number of new configuration
sources to the existing three-level hierarchy of system-wide
(/etc/gitconfig), per-user ($HOME/.gitconfig), and local
(.git/config), so it is not too bad, from scalability point of view,
to keep adding new GIT_CONFIG_NO$FROTZ variables.

Let me go in a couple of different tangents a bit, thinking aloud.

Tangent One.  I wonder if the presense of includeIf.<cond>.path
changes the "we won't have unbounded, so adding another here is OK"
reasoning.  If somebody wanted to say "Do not use the paths that
match this and that pattern", it is likely that we'd end up having
to support a single variable that allows more than one "value".  In
a straw-man syntax "GIT_CONFIG_EXCLUDE='/home/gitster/*
/home/peff/*'" might be a way to say "do not use files that are
under these two directories.

And when that happens, we'd probably notice that it is easier for
users to configure, if they can treat 'system' and 'global' as just
another special pattern to place on that list. //system and //global
might be the syntax we'd choose when time comes, i.e.

	GIT_CONFIG_EXCLUDE='//system //global'

might become a more scalable replacement for

	GIT_CONFIG_NOSYSTEM=yes GIT_CONFIG_NOHOME=yes

Tangent Two.  One thing we never managed to properly test in our
test suite is the unctioning of GIT_CONFIG_NOSYSTEM.  As we do not
want to get broken by whatever is in /etc/gitconfig, all our tests
run with the environment variable set.  For the same reason, in
order to avoid getting influenced by whatever the tester has in
$HOME/.gitconfig, we export HOME set to the throw-away directory
created during the test and control what is in the config file in
that directory.  In hindsight, it might have been a better design to
use GIT_CONFIG_SYSTEM variable that points at a path to be used as a
replacement for /etc/gitconfig when it is set; pointing /dev/null
with the variable would have been the natural way to say "do not use
anything from system configuration".  And GIT_CONFIG_GLOBAL can
easily follow the same pattern.

So, from these two perspective, for the longer term end-user
experience, I am not 100% onboard with GIT_CONFIG_NOGLOBAL.  An
alternative that allows us to move away from GIT_CONFIG_NOSYSTEM in
the direction to the tangent #2 would be not to repeat the same
mistake by doing GIT_CONFIG_NOGLOBAL, and instead adding
GIT_CONFIG_GLOBAL, which is

 (1) when not set, it does not do anything,

 (2) when set to "/dev/null" (a literal string; you do not have to
    spell it "NUL" when on Windows), it acts just like the case
    where your GIT_CONFIG_NOSYSTEM is set,

 (3) when set to any other string, it is taken as a filename that
     has the global configuration.  Unlike $HOME/.gitconfig or
     $XDG_HOME/git/config, it is an error if the named file does not
     exist (this is to catch misconfiguration).

And once this is accepted by users and established as a pattern, we
could migrate GIT_CONFIG_NOSYSTEM to GIT_CONFIG_SYSTEM=/dev/null


Having said all that (meaning: I am not 100% onboard with _NOGLOBAL
and think _GLOBAL=/dev/null might be a better design), let's give it
a review under the assumption that _NOGLOBAL is the design we would
want to choose.

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 4b4cc5c5e8..88cd064abb 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -340,6 +340,10 @@ GIT_CONFIG::
>  	Using the "--global" option forces this to ~/.gitconfig. Using the
>  	"--system" option forces this to $(prefix)/etc/gitconfig.
>  
> +GIT_CONFIG_NOGLOBAL::
> +	Whether to skip reading settings from the global ~/.gitconfig and
> +	$XDG_CONFIG_HOME/git/config files. See linkgit:git[1] for details.
> +
>  GIT_CONFIG_NOSYSTEM::
>  	Whether to skip reading settings from the system-wide
>  	$(prefix)/etc/gitconfig file. See linkgit:git[1] for details.

OK.  The new one mimics existing _NOSYSTEM and there is nothing
surprising in the new description.

> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 3a9c44987f..4462bd2da9 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -670,13 +670,21 @@ for further details.
>  	If this environment variable is set to `0`, git will not prompt
>  	on the terminal (e.g., when asking for HTTP authentication).
>  
> +`GIT_CONFIG_NOGLOBAL`::
> +	Whether to skip reading settings from the system-wide `~/.gitconfig`
> +	and `$XDG_CONFIG_HOME/git/config` files.  This environment variable can
> +	be used along with `$GIT_CONFIG_NOSYSTEM` to create a predictable
> +	environment for a picky script, or you can set it temporarily to avoid
> +	using a buggy global config file while waiting for someone with
> +	sufficient permissions to fix it.

"while waiting for someone with permissions" is a good backstory for
inventing _NOSYSTEM because you might not be able to futz with
/etc/gitconfig, but does not sound like an appropriate reasoning for
_NOGLOBAL that is about $HOME/.gitconfig or $HOME/.config/git/config.

>  `GIT_CONFIG_NOSYSTEM`::
>  	Whether to skip reading settings from the system-wide
>  	`$(prefix)/etc/gitconfig` file.  This environment variable can
> -	be used along with `$HOME` and `$XDG_CONFIG_HOME` to create a
> -	predictable environment for a picky script, or you can set it
> -	temporarily to avoid using a buggy `/etc/gitconfig` file while
> -	waiting for someone with sufficient permissions to fix it.
> +	be used along with `$GIT_CONFIG_NOGLOBAL` to create a predictable
> +	environment for a picky script, or you can set it temporarily to avoid
> +	using a buggy `/etc/gitconfig` file while waiting for someone with
> +	sufficient permissions to fix it.

> diff --git a/config.c b/config.c
> index 6428393a41..19c1b31c75 100644
> --- a/config.c
> +++ b/config.c
> @@ -1879,6 +1879,11 @@ int git_config_system(void)
>  	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
>  }
>  
> +static int git_config_global(void)
> +{
> +	return !git_env_bool("GIT_CONFIG_NOGLOBAL", 0);
> +}
> +
>  static int do_git_config_sequence(const struct config_options *opts,
>  				  config_fn_t fn, void *data)
>  {
> @@ -1903,10 +1908,10 @@ static int do_git_config_sequence(const struct config_options *opts,
>  					    data);
>  
>  	current_parsing_scope = CONFIG_SCOPE_GLOBAL;
> -	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
> +	if (git_config_global() && xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
>  		ret += git_config_from_file(fn, xdg_config, data);
>  
> -	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
> +	if (git_config_global() && user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
>  		ret += git_config_from_file(fn, user_config, data);
>  
>  	current_parsing_scope = CONFIG_SCOPE_LOCAL;

The implementation itself is quite straightforward for _NOSYSTEM; I
see nothing wrong in the code change for the given design.

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index e0dd5d65ce..0754189974 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -2059,6 +2059,37 @@ test_expect_success '--show-scope with --show-origin' '
>  	test_cmp expect output
>  '
>  
> +test_expect_success 'GIT_CONFIG_NOGLOBAL' '
> +	test_when_finished rm -f "$HOME"/.config/git &&
> +	cat >"$HOME"/.gitconfig <<-EOF &&
> +	[home]
> +		config = true
> +	EOF
> +	mkdir -p "$HOME"/.config/git &&
> +	cat >"$HOME"/.config/git/config <<-EOF &&
> +	[xdg]
> +		config = true
> +	EOF
> +	cat >.git/config <<-EOF &&
> +	[local]
> +		config = true
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	global	xdg.config=true
> +	global	home.config=true
> +	local	local.config=true
> +	EOF
> +	git config --show-scope --list >output &&
> +	test_cmp expect output &&
> +
> +	cat >expect <<-EOF &&
> +	local	local.config=true
> +	EOF
> +	GIT_CONFIG_NOGLOBAL=true git config --show-scope --list >output &&
> +	test_cmp expect output
> +'

This test was what initially piqued my curiosity, as the fact that
the result filtered with the new mechanism has only 'local' misled
me to incorrectly think that we are suppressing both 'system' and
'global' with the single variable.  Until I realized that we cannot
test the 'system' configuration in our test suite, that is.

Thanks.
Jeff King April 8, 2021, 11:18 p.m. UTC | #3
On Thu, Apr 08, 2021 at 10:25:15AM -0700, Junio C Hamano wrote:

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > While it's already possible to stop git from reading the system config
> > via GIT_CONFIG_NOSYSTEM, doing the same for global config files requires
> > the user to unset both HOME and XDG_CONFIG_HOME. This is an awkward
> > interface and may even pose a problem e.g. when git hooks rely on these
> > variables to be present.
> 
> Yeah, having to unset HOME would affect not just Git.  Git may call
> out something else (e.g. an editor) that may want to know where HOME
> is, Git may be used in something else (e.g. an IDE) that may want to
> know where HOME is.  Same for XDG_CONFIG_HOME.  If it is a valid need
> to not reading from $HOME/.gitconfig, unsetting HOME is an unacceptable
> approach to satisfying that need.

We actually used to have GIT_CONFIG_NOGLOBAL, which was used in the test
suite. But after switching to setting $HOME, it went away in 8f323c00dd
(config: drop support for GIT_CONFIG_NOGLOBAL, 2011-03-15). I agree that
it's a little more awkward these days because of $XDG_CONFIG_HOME (and
also because it influences other programs besides Git).

I'm not particularly opposed to re-adding it, but I do think having an
environment variable for "read this file instead", as you suggested
below, would be much better. It's a superset of the "no" functionality,
and would also let us improve our test coverage (especially if we add a
matching one for "system" config).

Looking at your suggestion:

> So, from these two perspective, for the longer term end-user
> experience, I am not 100% onboard with GIT_CONFIG_NOGLOBAL.  An
> alternative that allows us to move away from GIT_CONFIG_NOSYSTEM in
> the direction to the tangent #2 would be not to repeat the same
> mistake by doing GIT_CONFIG_NOGLOBAL, and instead adding
> GIT_CONFIG_GLOBAL, which is
> 
>  (1) when not set, it does not do anything,
> 
>  (2) when set to "/dev/null" (a literal string; you do not have to
>     spell it "NUL" when on Windows), it acts just like the case
>     where your GIT_CONFIG_NOSYSTEM is set,
> 
>  (3) when set to any other string, it is taken as a filename that
>      has the global configuration.  Unlike $HOME/.gitconfig or
>      $XDG_HOME/git/config, it is an error if the named file does not
>      exist (this is to catch misconfiguration).
> 
> And once this is accepted by users and established as a pattern, we
> could migrate GIT_CONFIG_NOSYSTEM to GIT_CONFIG_SYSTEM=/dev/null

That seems pretty reasonable. I'm on the fence on your (3). Conceivably
somebody could want to override the baked-in defaults without being sure
the file is present. But I'm not sure how useful that would be in
practice.

Some other things to consider:

  - does setting GIT_CONFIG_GLOBAL override both the $HOME and
    $XDG_CONFIG_HOME? If the plan is to override them, that makes sense.
    But we do usually read from both of them, so conceivably you might
    want to override just one? That's probably over-engineering, though.

  - if we have config for "read from this file instead of the system
    config" and "read from this instead of the user-level config", then
    I wonder if people will want "read this instead of the repo config".
    We have resisted having GIT_CONFIG mean that for many years, because
    I think it gets awkward in some cases (e.g., we'd still want to read
    it for core.repositoryformatversion, etc). I'm OK with drawing the
    line there and saying it's not a support feature, but we should be
    prepared to explain it to users (in the docs or at least in the
    commit message adding the system/global override variables).

-Peff
Ævar Arnfjörð Bjarmason April 8, 2021, 11:30 p.m. UTC | #4
On Thu, Apr 08 2021, Patrick Steinhardt wrote:

> +`GIT_CONFIG_NOGLOBAL`::
> +	Whether to skip reading settings from the system-wide `~/.gitconfig`
> +	and `$XDG_CONFIG_HOME/git/config` files.  This environment variable can

Let's not call ~/.gitconfig system-wide with /etc/gitconfig being,
saying "global" is consistent with git-config's own
--global/--system/--local etc. Still a bit odd, but at least the same
nomenclature.
Ævar Arnfjörð Bjarmason April 8, 2021, 11:34 p.m. UTC | #5
On Thu, Apr 08 2021, Junio C Hamano wrote:

> Patrick Steinhardt <ps@pks.im> writes:
>> +test_expect_success 'GIT_CONFIG_NOGLOBAL' '
>> +	test_when_finished rm -f "$HOME"/.config/git &&
>> +	cat >"$HOME"/.gitconfig <<-EOF &&
>> +	[home]
>> +		config = true
>> +	EOF
>> +	mkdir -p "$HOME"/.config/git &&
>> +	cat >"$HOME"/.config/git/config <<-EOF &&
>> +	[xdg]
>> +		config = true
>> +	EOF
>> +	cat >.git/config <<-EOF &&
>> +	[local]
>> +		config = true
>> +	EOF
>> +
>> +	cat >expect <<-EOF &&
>> +	global	xdg.config=true
>> +	global	home.config=true
>> +	local	local.config=true
>> +	EOF
>> +	git config --show-scope --list >output &&
>> +	test_cmp expect output &&
>> +
>> +	cat >expect <<-EOF &&
>> +	local	local.config=true
>> +	EOF
>> +	GIT_CONFIG_NOGLOBAL=true git config --show-scope --list >output &&
>> +	test_cmp expect output
>> +'
>
> This test was what initially piqued my curiosity, as the fact that
> the result filtered with the new mechanism has only 'local' misled
> me to incorrectly think that we are suppressing both 'system' and
> 'global' with the single variable.  Until I realized that we cannot
> test the 'system' configuration in our test suite, that is.

I haven't tested this, but that seems like a thing we want to mock in
the test suite by just having git_etc_gitconfig() check a GIT_TEST_*
variable.

I had a git_env_str() in [1] that we could use for that, or maybe it
should be git_env_path() in this case with the same --path handling, or
just do what repo_default_branch_name() does.

1. https://lore.kernel.org/git/20201113161320.16458-1-avarab@gmail.com/
Ævar Arnfjörð Bjarmason April 8, 2021, 11:39 p.m. UTC | #6
On Thu, Apr 08 2021, Junio C Hamano wrote:

> Patrick Steinhardt <ps@pks.im> writes:
> [...]
>> Introduce a new GIT_CONFIG_NOGLOBAL envvar, which is the simple
>> equivalent to GIT_CONFIG_NOSYSTEM. If set to true, git will skip reading
>> both `~/.gitconfig` and `$XDG_CONFIG_HOME/git/config`.
>
> I do not think we'd add an unbounded number of new configuration
> sources to the existing three-level hierarchy of system-wide
> (/etc/gitconfig), per-user ($HOME/.gitconfig), and local
> (.git/config), so it is not too bad, from scalability point of view,
> to keep adding new GIT_CONFIG_NO$FROTZ variables.
>
> Let me go in a couple of different tangents a bit, thinking aloud.
>
> Tangent One.  I wonder if the presense of includeIf.<cond>.path
> changes the "we won't have unbounded, so adding another here is OK"
> reasoning.  If somebody wanted to say "Do not use the paths that
> match this and that pattern", it is likely that we'd end up having
> to support a single variable that allows more than one "value".  In
> a straw-man syntax "GIT_CONFIG_EXCLUDE='/home/gitster/*
> /home/peff/*'" might be a way to say "do not use files that are
> under these two directories.
>
> And when that happens, we'd probably notice that it is easier for
> users to configure, if they can treat 'system' and 'global' as just
> another special pattern to place on that list. //system and //global
> might be the syntax we'd choose when time comes, i.e.
>
> 	GIT_CONFIG_EXCLUDE='//system //global'
>
> might become a more scalable replacement for
>
> 	GIT_CONFIG_NOSYSTEM=yes GIT_CONFIG_NOHOME=yes
>
> Tangent Two.  One thing we never managed to properly test in our
> test suite is the unctioning of GIT_CONFIG_NOSYSTEM.  As we do not
> want to get broken by whatever is in /etc/gitconfig, all our tests
> run with the environment variable set.  For the same reason, in
> order to avoid getting influenced by whatever the tester has in
> $HOME/.gitconfig, we export HOME set to the throw-away directory
> created during the test and control what is in the config file in
> that directory.  In hindsight, it might have been a better design to
> use GIT_CONFIG_SYSTEM variable that points at a path to be used as a
> replacement for /etc/gitconfig when it is set; pointing /dev/null
> with the variable would have been the natural way to say "do not use
> anything from system configuration".  And GIT_CONFIG_GLOBAL can
> easily follow the same pattern.
>
> So, from these two perspective, for the longer term end-user
> experience, I am not 100% onboard with GIT_CONFIG_NOGLOBAL.  An
> alternative that allows us to move away from GIT_CONFIG_NOSYSTEM in
> the direction to the tangent #2 would be not to repeat the same
> mistake by doing GIT_CONFIG_NOGLOBAL, and instead adding
> GIT_CONFIG_GLOBAL, which is
>
>  (1) when not set, it does not do anything,
>
>  (2) when set to "/dev/null" (a literal string; you do not have to
>     spell it "NUL" when on Windows), it acts just like the case
>     where your GIT_CONFIG_NOSYSTEM is set,
>
>  (3) when set to any other string, it is taken as a filename that
>      has the global configuration.  Unlike $HOME/.gitconfig or
>      $XDG_HOME/git/config, it is an error if the named file does not
>      exist (this is to catch misconfiguration).
>
> And once this is accepted by users and established as a pattern, we
> could migrate GIT_CONFIG_NOSYSTEM to GIT_CONFIG_SYSTEM=/dev/null
>
>
> Having said all that (meaning: I am not 100% onboard with _NOGLOBAL
> and think _GLOBAL=/dev/null might be a better design), let's give it
> a review under the assumption that _NOGLOBAL is the design we would
> want to choose.

I think doing this via env vars is inherently nasty, but can't think of
a good way to implement it properly without major refactoring.

IMO the "properly" would be that we'd just support this sort of thing as
first-class config syntax, as I've suggested before e.g. in the repo
hooks/config topic.

But to do that we couldn't "stream" the config reading as we do now,
we'd need to read system/global/local, and when we saw some new
meta-syntax apply those ignores to files we already read, and only then
start streaming things to config callbacks.

See "config.ignore.*" in
https://lore.kernel.org/git/87y2ebo16v.fsf@evledraar.gmail.com/
Junio C Hamano April 8, 2021, 11:43 p.m. UTC | #7
Jeff King <peff@peff.net> writes:

>>  (3) when set to any other string, it is taken as a filename that
>>      has the global configuration.  Unlike $HOME/.gitconfig or
>>      $XDG_HOME/git/config, it is an error if the named file does not
>>      exist (this is to catch misconfiguration).
>> 
>> And once this is accepted by users and established as a pattern, we
>> could migrate GIT_CONFIG_NOSYSTEM to GIT_CONFIG_SYSTEM=/dev/null
>
> That seems pretty reasonable. I'm on the fence on your (3). Conceivably
> somebody could want to override the baked-in defaults without being sure
> the file is present. But I'm not sure how useful that would be in
> practice.

I was also on the fence.  

If your plan is to create $HOME/.alternate-config and point at it by
setting GIT_CONFIG_GLOBAL=$HOME/.alternate-config, there are two
places you can make typo.  You may write a file with a wrong name.
You may export a variable with a wrong name.

> Some other things to consider:
>
>   - does setting GIT_CONFIG_GLOBAL override both the $HOME and
>     $XDG_CONFIG_HOME? If the plan is to override them, that makes sense.
>     But we do usually read from both of them, so conceivably you might
>     want to override just one? That's probably over-engineering, though.

I viewed this to be working at the more conceptual "here is the file
to read 'system' (or 'per-user') stuff from" level, and not at the
level of the individual file, as I consider that it is a mere
implementation detail that 'per-user' may read from multiple files.

>   - if we have config for "read from this file instead of the system
>     config" and "read from this instead of the user-level config", then
>     I wonder if people will want "read this instead of the repo config".
>     We have resisted having GIT_CONFIG mean that for many years, because
>     I think it gets awkward in some cases (e.g., we'd still want to read
>     it for core.repositoryformatversion, etc). I'm OK with drawing the
>     line there and saying it's not a support feature, but we should be
>     prepared to explain it to users (in the docs or at least in the
>     commit message adding the system/global override variables).

We may have to bite the bullet and make an official catalog of
really "structurely fundamental" configuration variables that must
appear in the per-repository file, and a mechanism to enforce that
by always reading these variables from "$GIT_DIR/config" and always
ignoring appearances of them in any other places.

That alone would probably be a good thing to do regardless of the
GIT_CONFIG_NOGLOBAL issue, as I suspect you may be able to wreak
havoc by adding random configuration like [extension] in
$HOME/.gitconfig ;-)

With that, it would make sense to allow overriding the per-repo
configuration in a similar way, only as a mechanism to override
configuration variables that are about "preferences" (as opposed to
the structurally fundamental ones).
Junio C Hamano April 8, 2021, 11:56 p.m. UTC | #8
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Apr 08 2021, Patrick Steinhardt wrote:
>
>> +`GIT_CONFIG_NOGLOBAL`::
>> +	Whether to skip reading settings from the system-wide `~/.gitconfig`
>> +	and `$XDG_CONFIG_HOME/git/config` files.  This environment variable can
>
> Let's not call ~/.gitconfig system-wide with /etc/gitconfig being,
> saying "global" is consistent with git-config's own
> --global/--system/--local etc. Still a bit odd, but at least the same
> nomenclature.

Yeah, GLOBAL should be renamed to PER-USER everywhere.
Jeff King April 9, 2021, 12:25 a.m. UTC | #9
On Thu, Apr 08, 2021 at 04:43:08PM -0700, Junio C Hamano wrote:

> > Some other things to consider:
> >
> >   - does setting GIT_CONFIG_GLOBAL override both the $HOME and
> >     $XDG_CONFIG_HOME? If the plan is to override them, that makes sense.
> >     But we do usually read from both of them, so conceivably you might
> >     want to override just one? That's probably over-engineering, though.
> 
> I viewed this to be working at the more conceptual "here is the file
> to read 'system' (or 'per-user') stuff from" level, and not at the
> level of the individual file, as I consider that it is a mere
> implementation detail that 'per-user' may read from multiple files.

Yeah. I'm OK with that. I think it's conceptually simpler. I was going
to also say "less flexible", but once you have pointed Git at the file
of your choice, it is easy to [include] any other one if you want.

> We may have to bite the bullet and make an official catalog of
> really "structurely fundamental" configuration variables that must
> appear in the per-repository file, and a mechanism to enforce that
> by always reading these variables from "$GIT_DIR/config" and always
> ignoring appearances of them in any other places.
> 
> That alone would probably be a good thing to do regardless of the
> GIT_CONFIG_NOGLOBAL issue, as I suspect you may be able to wreak
> havoc by adding random configuration like [extension] in
> $HOME/.gitconfig ;-)

I think we've historically had some problems there, but I think these
days it's not too bad. We look directly at $GIT_DIR/config with
read_repository_format(), loading only stuff that check_repo_format()
cares about:

  - core.repositoryformatversion

  - extensions.*

  - core.bare

  - core.worktree

And those _should_ be ignored by the regular config-reading paths. So I
think it would be OK to have a $GIT_CONFIG_LOCAL variable that overrides
the location in do_git_config_sequence() only, and that would cover only
the preferences parts. I do agree it wouldn't hurt to document which
options are read in which context.

I do wonder how useful such an option would be, though. I can see why
you would want to redirect or disable system-level or user-level config
once, and then use it for many invocations. But in a single repo, you
might as well just set the repo config, or override it for a single
invocation with "git -c", etc.

I guess one possible use could be "I got a repository I do not trust as
a tarball and want to investigate it without paying attention to its
local config". But that's a pretty narrow case, and you'd probably be
better off just vetting the config yourself anyway (since depending on
what you want to do in the repo, you may need some of that config, like
remote.*, etc).

I dunno.

-Peff
diff mbox series

Patch

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4b4cc5c5e8..88cd064abb 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -340,6 +340,10 @@  GIT_CONFIG::
 	Using the "--global" option forces this to ~/.gitconfig. Using the
 	"--system" option forces this to $(prefix)/etc/gitconfig.
 
+GIT_CONFIG_NOGLOBAL::
+	Whether to skip reading settings from the global ~/.gitconfig and
+	$XDG_CONFIG_HOME/git/config files. See linkgit:git[1] for details.
+
 GIT_CONFIG_NOSYSTEM::
 	Whether to skip reading settings from the system-wide
 	$(prefix)/etc/gitconfig file. See linkgit:git[1] for details.
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 3a9c44987f..4462bd2da9 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -670,13 +670,21 @@  for further details.
 	If this environment variable is set to `0`, git will not prompt
 	on the terminal (e.g., when asking for HTTP authentication).
 
+`GIT_CONFIG_NOGLOBAL`::
+	Whether to skip reading settings from the system-wide `~/.gitconfig`
+	and `$XDG_CONFIG_HOME/git/config` files.  This environment variable can
+	be used along with `$GIT_CONFIG_NOSYSTEM` to create a predictable
+	environment for a picky script, or you can set it temporarily to avoid
+	using a buggy global config file while waiting for someone with
+	sufficient permissions to fix it.
+
 `GIT_CONFIG_NOSYSTEM`::
 	Whether to skip reading settings from the system-wide
 	`$(prefix)/etc/gitconfig` file.  This environment variable can
-	be used along with `$HOME` and `$XDG_CONFIG_HOME` to create a
-	predictable environment for a picky script, or you can set it
-	temporarily to avoid using a buggy `/etc/gitconfig` file while
-	waiting for someone with sufficient permissions to fix it.
+	be used along with `$GIT_CONFIG_NOGLOBAL` to create a predictable
+	environment for a picky script, or you can set it temporarily to avoid
+	using a buggy `/etc/gitconfig` file while waiting for someone with
+	sufficient permissions to fix it.
 
 `GIT_FLUSH`::
 	If this environment variable is set to "1", then commands such
diff --git a/config.c b/config.c
index 6428393a41..19c1b31c75 100644
--- a/config.c
+++ b/config.c
@@ -1879,6 +1879,11 @@  int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
+static int git_config_global(void)
+{
+	return !git_env_bool("GIT_CONFIG_NOGLOBAL", 0);
+}
+
 static int do_git_config_sequence(const struct config_options *opts,
 				  config_fn_t fn, void *data)
 {
@@ -1903,10 +1908,10 @@  static int do_git_config_sequence(const struct config_options *opts,
 					    data);
 
 	current_parsing_scope = CONFIG_SCOPE_GLOBAL;
-	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
+	if (git_config_global() && xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
 		ret += git_config_from_file(fn, xdg_config, data);
 
-	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
+	if (git_config_global() && user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
 		ret += git_config_from_file(fn, user_config, data);
 
 	current_parsing_scope = CONFIG_SCOPE_LOCAL;
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e0dd5d65ce..0754189974 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2059,6 +2059,37 @@  test_expect_success '--show-scope with --show-origin' '
 	test_cmp expect output
 '
 
+test_expect_success 'GIT_CONFIG_NOGLOBAL' '
+	test_when_finished rm -f "$HOME"/.config/git &&
+	cat >"$HOME"/.gitconfig <<-EOF &&
+	[home]
+		config = true
+	EOF
+	mkdir -p "$HOME"/.config/git &&
+	cat >"$HOME"/.config/git/config <<-EOF &&
+	[xdg]
+		config = true
+	EOF
+	cat >.git/config <<-EOF &&
+	[local]
+		config = true
+	EOF
+
+	cat >expect <<-EOF &&
+	global	xdg.config=true
+	global	home.config=true
+	local	local.config=true
+	EOF
+	git config --show-scope --list >output &&
+	test_cmp expect output &&
+
+	cat >expect <<-EOF &&
+	local	local.config=true
+	EOF
+	GIT_CONFIG_NOGLOBAL=true git config --show-scope --list >output &&
+	test_cmp expect output
+'
+
 for opt in --local --worktree
 do
 	test_expect_success "$opt requires a repo" '