git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>
Subject: Re: [PATCH v2 3/3] config: allow overriding of global and system configuration
Date: Fri, 9 Apr 2021 11:38:31 -0400	[thread overview]
Message-ID: <YHB092HN2oVLmqC1@coredump.intra.peff.net> (raw)
In-Reply-To: <272a3b31aa73da8d65b04e647b1b9ca860f4e783.1617975637.git.ps@pks.im>

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

  reply	other threads:[~2021-04-09 15:38 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-08 14:17 [PATCH] config: Introduce GIT_CONFIG_NOGLOBAL Patrick Steinhardt
2021-04-08 16:44 ` Eric Sunshine
2021-04-08 17:25 ` Junio C Hamano
2021-04-08 23:18   ` Jeff King
2021-04-08 23:43     ` Junio C Hamano
2021-04-09  0:25       ` Jeff King
2021-04-08 23:34   ` Ævar Arnfjörð Bjarmason
2021-04-08 23:39   ` Ævar Arnfjörð Bjarmason
2021-04-08 23:30 ` Ævar Arnfjörð Bjarmason
2021-04-08 23:56   ` Junio C Hamano
2021-04-09 13:43 ` [PATCH v2 0/3] config: allow overriding global/system config Patrick Steinhardt
2021-04-09 13:43   ` [PATCH v2 1/3] config: rename `git_etc_config()` Patrick Steinhardt
2021-04-09 15:13     ` Jeff King
2021-04-09 13:43   ` [PATCH v2 2/3] config: unify code paths to get global config paths Patrick Steinhardt
2021-04-09 15:21     ` Jeff King
2021-04-09 13:43   ` [PATCH v2 3/3] config: allow overriding of global and system configuration Patrick Steinhardt
2021-04-09 15:38     ` Jeff King [this message]
2021-04-12 14:04       ` Patrick Steinhardt
2021-04-09 22:18     ` Junio C Hamano
2021-04-09 15:41   ` [PATCH v2 0/3] config: allow overriding global/system config Jeff King
2021-04-12 14:46   ` [PATCH v3 " Patrick Steinhardt
2021-04-12 14:46     ` [PATCH v3 1/3] config: rename `git_etc_config()` Patrick Steinhardt
2021-04-12 14:46     ` [PATCH v3 2/3] config: unify code paths to get global config paths Patrick Steinhardt
2021-04-12 14:46     ` [PATCH v3 3/3] config: allow overriding of global and system configuration Patrick Steinhardt
2021-04-12 17:04       ` Junio C Hamano
2021-04-13  7:11     ` [PATCH v4 0/3] config: allow overriding global/system config Patrick Steinhardt
2021-04-13  7:11       ` [PATCH v4 1/3] config: rename `git_etc_config()` Patrick Steinhardt
2021-04-13  7:25         ` Jeff King
2021-04-16 21:14         ` SZEDER Gábor
2021-04-17  8:44           ` Jeff King
2021-04-17 21:37             ` Junio C Hamano
2021-04-18  5:39               ` Jeff King
2021-04-19 11:03                 ` Patrick Steinhardt
2021-04-23  9:27                   ` Jeff King
2021-04-13  7:11       ` [PATCH v4 2/3] config: unify code paths to get global config paths Patrick Steinhardt
2021-04-13  7:11       ` [PATCH v4 3/3] config: allow overriding of global and system configuration Patrick Steinhardt
2021-04-13  7:33         ` Jeff King
2021-04-13  7:54           ` Patrick Steinhardt
2021-04-13  7:33       ` [PATCH v4 0/3] config: allow overriding global/system config Jeff King
2021-04-13 17:49       ` Junio C Hamano
2021-04-14  5:37         ` Patrick Steinhardt
2021-04-19 12:31       ` [PATCH v5 " Patrick Steinhardt
2021-04-19 12:31         ` [PATCH v5 1/3] config: rename `git_etc_config()` Patrick Steinhardt
2021-04-19 12:31         ` [PATCH v5 2/3] config: unify code paths to get global config paths Patrick Steinhardt
2021-04-19 12:31         ` [PATCH v5 3/3] config: allow overriding of global and system configuration Patrick Steinhardt
2021-04-21 20:46           ` SZEDER Gábor
2021-04-21 21:06             ` SZEDER Gábor
2021-04-22  5:36               ` Patrick Steinhardt
2021-04-23  5:47             ` [PATCH] t1300: fix unset of GIT_CONFIG_NOSYSTEM leaking into subsequent tests Patrick Steinhardt
2021-04-19 21:55         ` [PATCH v5 0/3] config: allow overriding global/system config Junio C Hamano
2021-04-23  9:32         ` Jeff King
2021-04-12 14:46 ` [PATCH v3] config: allow overriding of global and system configuration Patrick Steinhardt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YHB092HN2oVLmqC1@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=ps@pks.im \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).