Re: [PATCH] config: Introduce GIT_CONFIG_NOGLOBAL

[Junio C Hamano <gitster@pobox.com>]

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.