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

> On Fri, Apr 09, 2021 at 03:43:30PM +0200, Patrick Steinhardt wrote:
> 
> > In order to have git run in a fully controlled environment without any
> > misconfiguration, it may be desirable for users or scripts to override
> > global- and system-level configuration files. We already have a way of
> > doing this, which is to unset both HOME and XDG_CONFIG_HOME environment
> > variables and to set `GIT_CONFIG_NOGLOBAL=true`. This is quite kludgy,
> > and unsetting the first two variables likely has an impact on other
> > executables spawned by such a script.
> > 
> > The obvious way to fix this would be to introduce `GIT_CONFIG_NOSYSTEM`
> > as an equivalent to `GIT_CONFIG_NOGLOBAL`. But in the past, it has
> 
> I think you have NOSYSTEM and NOGLOBAL mixed up in both paragraphs here?
> 
> Otherwise the motivation and description here look very good (and I like
> the overall direction).
> 
> > diff --git a/Documentation/git.txt b/Documentation/git.txt
> > index 3a9c44987f..2129629296 100644
> > --- a/Documentation/git.txt
> > +++ b/Documentation/git.txt
> > @@ -670,6 +670,16 @@ 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_GLOBAL`::
> > +`GIT_CONFIG_SYSTEM`::
> > +	Take the configuration from the given files instead from global or
> > +	system-level configuration files. The files must exist and be readable
> > +	by the current user. If `GIT_CONFIG_SYSTEM` is set, `/etc/gitconfig`
> > +	will not be read. Likewise, if `GIT_CONFIG_GLOBAL` is set, neither
> > +	`$HOME/.gitconfig` nor `$XDG_CONFIG_HOME/git/config` will be read. Can
> > +	be set to `/dev/null` to skip reading configuration files of the
> > +	respective level.
> 
> Makes sense. The reference to `/etc/gitconfig` here may not be accurate,
> depending on the build parameters. I notice below in the context that we
> say:
> 
> >  `GIT_CONFIG_NOSYSTEM`::
> >  	Whether to skip reading settings from the system-wide
> >  	`$(prefix)/etc/gitconfig` file.  This environment variable can
> 
> which is _also_ not quite right (if $(prefix) is "/usr", then the file
> really is /etc/gitconfig).
> 
> I think it might be possible to pull the value of the ETC_GITCONFIG
> Makefile variable into the documentation, so we could probably give the
> "real" value. But I wonder if it would suffice to just say:
> 
>    ...the system config (usually `/etc/gitconfig`) will not be read.
> 
> Or is that too confusing (it invites the question "when is it not
> /etc/gitconfig")? I guess we could say "the system config file defined
> at build-time (usually `/etc/gitconfig`)", which is perhaps more clear.
> 
> > @@ -1847,8 +1847,22 @@ static int git_config_from_blob_ref(config_fn_t fn,
> >  const char *git_system_config(void)
> >  {
> >  	static const char *system_wide;
> > -	if (!system_wide)
> > -		system_wide = system_path(ETC_GITCONFIG);
> > +
> > +	if (!system_wide) {
> > +		system_wide = xstrdup_or_null(getenv("GIT_CONFIG_SYSTEM"));
> 
> I wondered, given the "const char *" return values in the last patch, if
> you might pass back the result of getenv() directly. But you duplicate
> it here, which is good, as it avoids portability problems hanging on to
> the result of getenv().
> 
> > +		if (system_wide) {
> > +			/*
> > +			 * If GIT_CONFIG_SYSTEM is set, it overrides the
> > +			 * /etc/gitconfig. Furthermore, the file must exist in
> > +			 * order to prevent any typos by the user.
> > +			 */
> > +			if (access(system_wide, R_OK))
> > +				die(_("cannot access '%s'"), system_wide);
> > +		} else {
> > +			system_wide = system_path(ETC_GITCONFIG);
> > +		}
> > +	}
> 
> I was on the fence about whether to enforce the "this file must exist"
> property, with respect to the overall design. But seeing how we must
> actually add extra code here to handle it makes me want to just treat it
> exactly like the other files.
> 
> Using a separate access() here is also a TOCTOU race, but I'm pretty
> sure the existing config code makes the same mistake (and it's not that
> big a deal in this context).
> 
> > @@ -1857,8 +1871,20 @@ void git_global_config(const char **user, const char **xdg)
> >  	static const char *user_config, *xdg_config;
> >  
> >  	if (!user_config) {
> > -		user_config = expand_user_path("~/.gitconfig", 0);
> > -		xdg_config = xdg_config_home("config");
> > +		user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
> > +		if (user_config) {
> > +			/*
> > +			 * If GIT_CONFIG_GLOBAL is set, then it overrides both
> > +			 * the ~/.gitconfig and the XDG configuration file.
> > +			 * Furthermore, the file must exist in order to prevent
> > +			 * any typos by the user.
> > +			 */
> > +			if (access(user_config, R_OK))
> > +				die(_("cannot access '%s'"), user_config);
> > +		} else {
> > +			user_config = expand_user_path("~/.gitconfig", 0);
> > +			xdg_config = xdg_config_home("config");
> > +		}
> >  	}
> 
> And this looks as I'd expect (but the same comments as above apply, of
> course).
> 
> > +test_expect_success 'override global and system config' '
> > +	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 >custom-global-config <<-EOF &&
> > +	[global]
> > +		config = true
> > +	EOF
> > +	cat >custom-system-config <<-EOF &&
> > +	[system]
> > +		config = true
> > +	EOF
> 
> Minor style nit, but we usually prefer non-interpolating "\EOF" if we
> don't intend to interpolate within the here-doc. It does look like
> t1300 has quite a mix of styles, though.
> 
> > +	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 &&
> > +
> > +	sane_unset GIT_CONFIG_NOSYSTEM &&
> > +
> > +	cat >expect <<-EOF &&
> > +	system	system.config=true
> > +	global	global.config=true
> > +	local	local.config=true
> > +	EOF
> > +	GIT_CONFIG_SYSTEM=custom-system-config GIT_CONFIG_GLOBAL=custom-global-config \
> > +		git config --show-scope --list >output &&
> > +	test_cmp expect output &&
> > +
> > +	cat >expect <<-EOF &&
> > +	local	local.config=true
> > +	EOF
> > +	GIT_CONFIG_SYSTEM=/dev/null GIT_CONFIG_GLOBAL=/dev/null git config --show-scope --list >output &&
> > +	test_cmp expect output
> > +'
> 
> And this test covers all of the new stuff we care about. Good.
> 
> > +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 version &&
> > +	test_must_fail env GIT_CONFIG_SYSTEM=does-not-exist git version &&
> > +	GIT_CONFIG_NOSYSTEM=true GIT_CONFIG_SYSTEM=does-not-exist git version
> > +'
> 
> Makes sense to test given the patch, though if we rip out the "missing"
> check, then obviously this goes away.
> 
> > +test_expect_success 'write to overridden global and system config' '
> 
> Hmm. I hadn't really considered _writing_ to these files (after all, you
> can just use "git config --file" to do so). I guess it is consistent,
> and would probably be more work (and more error-prone) to try to
> distinguish reading versus writing in the code.
> 
> > +	cat >expect <<EOF &&
> > +[config]
> > +	key = value
> > +EOF
> 
> No "<<-EOF" here to fix the indentation?
> 
> > +	test_must_fail env GIT_CONFIG_GLOBAL=write-to-global git config --global config.key value &&
> > +	touch write-to-global &&
> > +	GIT_CONFIG_GLOBAL=write-to-global git config --global config.key value &&
> > +	test_cmp expect write-to-global &&
> 
> In the writing case, the "must exist" thing makes it even weirder, since
> we might be intending to create the file! If we leave in the writing,
> that makes me even more convinced that we should drop the "must exist"
> check.
> 
> -Peff