* [PATCH v2 1/3] config: rename `git_etc_config()`
2021-04-09 13:43 ` [PATCH v2 0/3] config: allow overriding global/system config Patrick Steinhardt
@ 2021-04-09 13:43 ` 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
` (3 subsequent siblings)
4 siblings, 1 reply; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-09 13:43 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 2865 bytes --]
The `git_etc_gitconfig()` function retrieves the system-level path of
the configuration file. We're about to introduce a way to override it
via an environment variable, at which point the name of this function
would start to become misleading.
Rename the function to `git_system_config()` as a preparatory step.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/config.c | 2 +-
config.c | 6 +++---
config.h | 3 ++-
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index f71fa39b38..02ed0b3fe7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -695,7 +695,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
}
else if (use_system_config) {
- given_config_source.file = git_etc_gitconfig();
+ given_config_source.file = git_system_config();
given_config_source.scope = CONFIG_SCOPE_SYSTEM;
} else if (use_local_config) {
given_config_source.file = git_pathdup("config");
diff --git a/config.c b/config.c
index 6428393a41..c552ab4ad9 100644
--- a/config.c
+++ b/config.c
@@ -1844,7 +1844,7 @@ static int git_config_from_blob_ref(config_fn_t fn,
return git_config_from_blob_oid(fn, name, &oid, data);
}
-const char *git_etc_gitconfig(void)
+const char *git_system_config(void)
{
static const char *system_wide;
if (!system_wide)
@@ -1896,10 +1896,10 @@ static int do_git_config_sequence(const struct config_options *opts,
repo_config = NULL;
current_parsing_scope = CONFIG_SCOPE_SYSTEM;
- if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
+ if (git_config_system() && !access_or_die(git_system_config(), R_OK,
opts->system_gently ?
ACCESS_EACCES_OK : 0))
- ret += git_config_from_file(fn, git_etc_gitconfig(),
+ ret += git_config_from_file(fn, git_system_config(),
data);
current_parsing_scope = CONFIG_SCOPE_GLOBAL;
diff --git a/config.h b/config.h
index 19a9adbaa9..8e8376ae19 100644
--- a/config.h
+++ b/config.h
@@ -318,7 +318,6 @@ int git_config_rename_section(const char *, const char *);
int git_config_rename_section_in_file(const char *, const char *, const char *);
int git_config_copy_section(const char *, const char *);
int git_config_copy_section_in_file(const char *, const char *, const char *);
-const char *git_etc_gitconfig(void);
int git_env_bool(const char *, int);
unsigned long git_env_ulong(const char *, unsigned long);
int git_config_system(void);
@@ -327,6 +326,8 @@ int config_error_nonbool(const char *);
#define config_error_nonbool(s) (config_error_nonbool(s), const_error())
#endif
+const char *git_system_config(void);
+
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
enum config_scope current_config_scope(void);
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 1/3] config: rename `git_etc_config()`
2021-04-09 13:43 ` [PATCH v2 1/3] config: rename `git_etc_config()` Patrick Steinhardt
@ 2021-04-09 15:13 ` Jeff King
0 siblings, 0 replies; 52+ messages in thread
From: Jeff King @ 2021-04-09 15:13 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Eric Sunshine
On Fri, Apr 09, 2021 at 03:43:21PM +0200, Patrick Steinhardt wrote:
> The `git_etc_gitconfig()` function retrieves the system-level path of
> the configuration file. We're about to introduce a way to override it
> via an environment variable, at which point the name of this function
> would start to become misleading.
>
> Rename the function to `git_system_config()` as a preparatory step.
Looks good. This name has been a minor annoyance for years. :)
-Peff
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 2/3] config: unify code paths to get global config paths
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 13:43 ` 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
` (2 subsequent siblings)
4 siblings, 1 reply; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-09 13:43 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 3732 bytes --]
There's two callsites which assemble global config paths, once in the
config loading code and once in the git-config(1) builtin. We're about
to implement a way to override global config paths via an environment
variable which would require us to adjust both sites.
Unify both code paths into a single `git_global_config()` function which
returns both paths for `~/.gitconfig` and the XDG config file. This will
make the subsequent patch which introduces the new envvar easier to
implement.
No functional changes are expected from this patch.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/config.c | 6 ++----
config.c | 20 ++++++++++++++++----
config.h | 1 +
3 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 02ed0b3fe7..604a0973a5 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -671,9 +671,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
if (use_global_config) {
- char *user_config = expand_user_path("~/.gitconfig", 0);
- char *xdg_config = xdg_config_home("config");
+ const char *user_config, *xdg_config;
+ git_global_config(&user_config, &xdg_config);
if (!user_config)
/*
* It is unknown if HOME/.gitconfig exists, so
@@ -688,10 +688,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
if (access_or_warn(user_config, R_OK, 0) &&
xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
given_config_source.file = xdg_config;
- free(user_config);
} else {
given_config_source.file = user_config;
- free(xdg_config);
}
}
else if (use_system_config) {
diff --git a/config.c b/config.c
index c552ab4ad9..6af0244085 100644
--- a/config.c
+++ b/config.c
@@ -1852,6 +1852,19 @@ const char *git_system_config(void)
return system_wide;
}
+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 = user_config;
+ *xdg = xdg_config;
+}
+
/*
* Parse environment variable 'k' as a boolean (in various
* possible spellings); if missing, use the default value 'def'.
@@ -1883,9 +1896,8 @@ static int do_git_config_sequence(const struct config_options *opts,
config_fn_t fn, void *data)
{
int ret = 0;
- char *xdg_config = xdg_config_home("config");
- char *user_config = expand_user_path("~/.gitconfig", 0);
char *repo_config;
+ const char *user_config, *xdg_config;
enum config_scope prev_parsing_scope = current_parsing_scope;
if (opts->commondir)
@@ -1903,6 +1915,8 @@ static int do_git_config_sequence(const struct config_options *opts,
data);
current_parsing_scope = CONFIG_SCOPE_GLOBAL;
+ git_global_config(&user_config, &xdg_config);
+
if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
ret += git_config_from_file(fn, xdg_config, data);
@@ -1927,8 +1941,6 @@ static int do_git_config_sequence(const struct config_options *opts,
die(_("unable to parse command-line config"));
current_parsing_scope = prev_parsing_scope;
- free(xdg_config);
- free(user_config);
free(repo_config);
return ret;
}
diff --git a/config.h b/config.h
index 8e8376ae19..53a782e0f5 100644
--- a/config.h
+++ b/config.h
@@ -327,6 +327,7 @@ int config_error_nonbool(const char *);
#endif
const char *git_system_config(void);
+void git_global_config(const char **user, const char **xdg);
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] config: unify code paths to get global config paths
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
0 siblings, 0 replies; 52+ messages in thread
From: Jeff King @ 2021-04-09 15:21 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Eric Sunshine
On Fri, Apr 09, 2021 at 03:43:25PM +0200, Patrick Steinhardt wrote:
> There's two callsites which assemble global config paths, once in the
> config loading code and once in the git-config(1) builtin. We're about
> to implement a way to override global config paths via an environment
> variable which would require us to adjust both sites.
>
> Unify both code paths into a single `git_global_config()` function which
> returns both paths for `~/.gitconfig` and the XDG config file. This will
> make the subsequent patch which introduces the new envvar easier to
> implement.
Seems like a good step forward. There is one minor issue with the
implementation, though.
> diff --git a/builtin/config.c b/builtin/config.c
> index 02ed0b3fe7..604a0973a5 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -671,9 +671,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> }
>
> if (use_global_config) {
> - char *user_config = expand_user_path("~/.gitconfig", 0);
> - char *xdg_config = xdg_config_home("config");
> + const char *user_config, *xdg_config;
>
> + git_global_config(&user_config, &xdg_config);
The pointer out-parameters make sense here, since we need to return two
values. I notice they became const, so the function will hold on to
ownership of the memory.
> @@ -688,10 +688,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> if (access_or_warn(user_config, R_OK, 0) &&
> xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
> given_config_source.file = xdg_config;
> - free(user_config);
> } else {
> given_config_source.file = user_config;
> - free(xdg_config);
> }
...which is why we drop these free() calls. So far so good.
> +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 = user_config;
> + *xdg = xdg_config;
> +}
And here in the implementation we hold on to the static values forever.
I think your "did we initialize already" check isn't robust, though.
expand_user_path() can return NULL, in which case every call would
trigger a re-initialization (even leaking xdg_config if it was set in
the last round).
So I think you'd need a separate "static int initialized" variable.
That said, I wonder if we should just pass ownership of the memory to
the caller. It is a minor inconvenience that they will have to free()
the result, but we're already doing that. And it removes any possibility
of thread unsafety.
I guess it doesn't match git_system_config() as well, then. But arguably
it should also just pass ownership (it also has only a handful of
callers, and freeing the result would not be a big deal).
I'm OK with either solution, though.
-Peff
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 3/3] config: allow overriding of global and system configuration
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 13:43 ` [PATCH v2 2/3] config: unify code paths to get global config paths Patrick Steinhardt
@ 2021-04-09 13:43 ` Patrick Steinhardt
2021-04-09 15:38 ` Jeff King
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
4 siblings, 2 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-09 13:43 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 7692 bytes --]
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
turned out that this design is inflexible: we cannot test system-level
parsing of the git configuration in our test harness because there is no
way to change its location, so all tests run with `GIT_CONFIG_NOSYSTEM`
set.
Instead of doing the same mistake with `GIT_CONFIG_NOGLOBAL`, introduce
two new variables `GIT_CONFIG_GLOBAL` and `GIT_CONFIG_SYSTEM`:
- If unset, git continues to use the usual locations.
- If set to a specific path, we skip reading the normal
configuration files and instead take the path. This path must
exist and be readable to ensure that the user didn't typo.
- If set to `/dev/null`, we do not load either global- or
system-level configuration at all.
This implements the usecase where we want to execute code in a sanitized
environment without any potential misconfigurations via `/dev/null`, but
is more flexible and allows for more usecases than simply adding
`GIT_CONFIG_NOGLOBAL`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-config.txt | 5 +++
Documentation/git.txt | 10 +++++
config.c | 34 ++++++++++++++--
t/t1300-config.sh | 75 ++++++++++++++++++++++++++++++++++++
4 files changed, 120 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4b4cc5c5e8..5cddadafd2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -340,6 +340,11 @@ GIT_CONFIG::
Using the "--global" option forces this to ~/.gitconfig. Using the
"--system" option forces this to $(prefix)/etc/gitconfig.
+GIT_CONFIG_GLOBAL::
+GIT_CONFIG_SYSTEM::
+ Take the configuration from the given files instead from global or
+ system-level configuration. 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..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.
+
`GIT_CONFIG_NOSYSTEM`::
Whether to skip reading settings from the system-wide
`$(prefix)/etc/gitconfig` file. This environment variable can
diff --git a/config.c b/config.c
index 6af0244085..9dfc4a79f7 100644
--- a/config.c
+++ b/config.c
@@ -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"));
+ 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);
+ }
+ }
+
return system_wide;
}
@@ -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");
+ }
}
*user = user_config;
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e0dd5d65ce..5498ca32b0 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2059,6 +2059,81 @@ test_expect_success '--show-scope with --show-origin' '
test_cmp expect output
'
+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
+
+ 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
+'
+
+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
+'
+
+test_expect_success 'write to overridden global and system config' '
+ cat >expect <<EOF &&
+[config]
+ key = value
+EOF
+
+ 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 &&
+
+ test_must_fail env GIT_CONFIG_SYSTEM=write-to-system git config --system config.key value &&
+ touch write-to-system &&
+ GIT_CONFIG_SYSTEM=write-to-system git config --system config.key value &&
+ test_cmp expect write-to-system
+'
+
for opt in --local --worktree
do
test_expect_success "$opt requires a repo" '
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] config: allow overriding of global and system configuration
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
2021-04-12 14:04 ` Patrick Steinhardt
2021-04-09 22:18 ` Junio C Hamano
1 sibling, 1 reply; 52+ messages in thread
From: Jeff King @ 2021-04-09 15:38 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Eric Sunshine
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] config: allow overriding of global and system configuration
2021-04-09 15:38 ` Jeff King
@ 2021-04-12 14:04 ` Patrick Steinhardt
0 siblings, 0 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-12 14:04 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 9612 bytes --]
On Fri, Apr 09, 2021 at 11:38:31AM -0400, Jeff King wrote:
> 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
I do agree that for the writing side it's limiting to require the file
to exist. The question is really what's gained by it. The worst thing
that could happen is that the user writes to a file he didn't intend to
write to -- this can happen regardless of whether or not the file
exists, and in fact the worse case is where we overwrite values of a
file which the user didn't intend to overwrite. The case where we create
a new file by accident doesn't seem to be that interesting to me.
In any case, the user would probably use `git config -f` anyway. Which
raises the question whether it's sensible to allow writing to the file
in the first place. And for the sake of scripts, I lean towards "yes":
the dev can set up envvars once, and then use the config for both
reading and writing.
On the reading side, I can relate with Junio's argument that there's now
two possibilities for typos: once in the envvar, and once in the file
path. But given that git won't check the envvar's key for typos, we
cannot really reduce the chance for typos down to zero anyway.
So I do tend towards just allowing for the file to not exist: when
reading, we silentely ignore it, and when writing we create it.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] config: allow overriding of global and system configuration
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
@ 2021-04-09 22:18 ` Junio C Hamano
1 sibling, 0 replies; 52+ messages in thread
From: Junio C Hamano @ 2021-04-09 22:18 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason, Eric Sunshine
Patrick Steinhardt <ps@pks.im> writes:
> diff --git a/config.c b/config.c
> index 6af0244085..9dfc4a79f7 100644
> --- a/config.c
> +++ b/config.c
> @@ -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"));
> + 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);
> + }
> + }
> +
> return system_wide;
> }
I wrote the design to truly special case "/dev/null" at the pathname
string level, and we do not even try to open/read from a file when
"/dev/null" is given, so that we can even allow "/dev/null" to be
missing in a funny embedded or containerized environment, but that
does not seem to be what is going on here.
I do not think "access()" here is a good idea; the result we get
here may not match what happens when we actually try to open the
path. Just remembering if we got the system_wide value from the
GIT_CONFIG_SYSTEM, and change what happens when the open fails when
we try to use the system-wide configuration depending on that, would
be the right approach.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 0/3] config: allow overriding global/system config
2021-04-09 13:43 ` [PATCH v2 0/3] config: allow overriding global/system config Patrick Steinhardt
` (2 preceding siblings ...)
2021-04-09 13:43 ` [PATCH v2 3/3] config: allow overriding of global and system configuration Patrick Steinhardt
@ 2021-04-09 15:41 ` Jeff King
2021-04-12 14:46 ` [PATCH v3 " Patrick Steinhardt
4 siblings, 0 replies; 52+ messages in thread
From: Jeff King @ 2021-04-09 15:41 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Eric Sunshine
On Fri, Apr 09, 2021 at 03:43:16PM +0200, Patrick Steinhardt wrote:
> Instead of going for GIT_CONFIG_NOGLOBAL, I've adopted Junio's proposal
> of going with GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM, which allow a
> user to modify the locations of those files. Thanks for the discussion,
> this solution feels a lot nicer to me!
Thanks, I like this much better. I left a few comments on the patches
themselves. Mostly small suggestions, but design-wise I'm of the opinion
we should drop the "file must exist" requirement; see the response to
patch 3.
-Peff
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 0/3] config: allow overriding global/system config
2021-04-09 13:43 ` [PATCH v2 0/3] config: allow overriding global/system config Patrick Steinhardt
` (3 preceding siblings ...)
2021-04-09 15:41 ` [PATCH v2 0/3] config: allow overriding global/system config Jeff King
@ 2021-04-12 14:46 ` Patrick Steinhardt
2021-04-12 14:46 ` [PATCH v3 1/3] config: rename `git_etc_config()` Patrick Steinhardt
` (3 more replies)
4 siblings, 4 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-12 14:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 12176 bytes --]
Hi,
this is the third version of my patch series to provide a way of
overriding the global system configuration.
Changes compared to v2:
- `/dev/null` is now truly special-cased to skip reading any file
such that it's possible to use it e.g. in environments where this
special file does not exist.
- There is no explicit requirement for the given files to exist
anymore. On the reading side, it will in most cases just be
ignored (except for e.g. `git config --global --list` and similar,
where we'd also fail if the normal global config didn't exist). On
the writing site, we now create the config file it it's missing.
- Both `git_system_config()` and `git_global_config()` now pass
memory ownership to the caller.
Patrick
Patrick Steinhardt (3):
config: rename `git_etc_config()`
config: unify code paths to get global config paths
config: allow overriding of global and system configuration
Documentation/git-config.txt | 5 +++
Documentation/git.txt | 10 +++++
builtin/config.c | 12 ++++--
config.c | 48 ++++++++++++++++++------
config.h | 4 +-
t/t1300-config.sh | 71 ++++++++++++++++++++++++++++++++++++
6 files changed, 133 insertions(+), 17 deletions(-)
Range-diff against v2:
1: da0b8ce6f0 ! 1: 34bdbc27d6 config: rename `git_etc_config()`
@@ Commit message
would start to become misleading.
Rename the function to `git_system_config()` as a preparatory step.
+ While at it, the function is also refactored to pass memory ownership to
+ the caller. This is done to better match semantics of
+ `git_global_config()`, which is going to be introduced in the next
+ commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
@@ config.c: static int git_config_from_blob_ref(config_fn_t fn,
}
-const char *git_etc_gitconfig(void)
-+const char *git_system_config(void)
++char *git_system_config(void)
{
- static const char *system_wide;
- if (!system_wide)
+- static const char *system_wide;
+- if (!system_wide)
+- system_wide = system_path(ETC_GITCONFIG);
+- return system_wide;
++ return system_path(ETC_GITCONFIG);
+ }
+
+ /*
+@@ config.c: static int do_git_config_sequence(const struct config_options *opts,
+ config_fn_t fn, void *data)
+ {
+ int ret = 0;
++ char *system_config = git_system_config();
+ char *xdg_config = xdg_config_home("config");
+ char *user_config = expand_user_path("~/.gitconfig", 0);
+ char *repo_config;
@@ config.c: static int do_git_config_sequence(const struct config_options *opts,
repo_config = NULL;
current_parsing_scope = CONFIG_SCOPE_SYSTEM;
- if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
-+ if (git_config_system() && !access_or_die(git_system_config(), R_OK,
- opts->system_gently ?
- ACCESS_EACCES_OK : 0))
+- opts->system_gently ?
+- ACCESS_EACCES_OK : 0))
- ret += git_config_from_file(fn, git_etc_gitconfig(),
-+ ret += git_config_from_file(fn, git_system_config(),
- data);
+- data);
++ if (system_config && !access_or_die(system_config, R_OK,
++ opts->system_gently ?
++ ACCESS_EACCES_OK : 0))
++ ret += git_config_from_file(fn, system_config, data);
current_parsing_scope = CONFIG_SCOPE_GLOBAL;
+ if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
+@@ config.c: static int do_git_config_sequence(const struct config_options *opts,
+ die(_("unable to parse command-line config"));
+
+ current_parsing_scope = prev_parsing_scope;
++ free(system_config);
+ free(xdg_config);
+ free(user_config);
+ free(repo_config);
## config.h ##
@@ config.h: int git_config_rename_section(const char *, const char *);
@@ config.h: int config_error_nonbool(const char *);
#define config_error_nonbool(s) (config_error_nonbool(s), const_error())
#endif
-+const char *git_system_config(void);
++char *git_system_config(void);
+
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
2: dddc85bcf5 < -: ---------- config: unify code paths to get global config paths
-: ---------- > 2: 30f18679bd config: unify code paths to get global config paths
3: 272a3b31aa ! 3: af663640ae config: allow overriding of global and system configuration
@@ Commit message
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
+ The obvious way to fix this would be to introduce `GIT_CONFIG_NOGLOBAL`
+ as an equivalent to `GIT_CONFIG_NOSYSTEM`. But in the past, it has
turned out that this design is inflexible: we cannot test system-level
parsing of the git configuration in our test harness because there is no
way to change its location, so all tests run with `GIT_CONFIG_NOSYSTEM`
@@ Commit message
- If unset, git continues to use the usual locations.
- If set to a specific path, we skip reading the normal
- configuration files and instead take the path. This path must
- exist and be readable to ensure that the user didn't typo.
+ configuration files and instead take the path.
- If set to `/dev/null`, we do not load either global- or
system-level configuration at all.
@@ Documentation/git.txt: for further details.
+`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`
++ system-level configuration files. If `GIT_CONFIG_SYSTEM` is set, the
++ system config file defined at build time (usually `/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
@@ Documentation/git.txt: for further details.
Whether to skip reading settings from the system-wide
`$(prefix)/etc/gitconfig` file. This environment variable can
+ ## builtin/config.c ##
+@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
+ char *user_config, *xdg_config;
+
+ git_global_config(&user_config, &xdg_config);
+- if (!user_config)
++ if (!user_config) {
++ if (getenv("GIT_CONFIG_GLOBAL"))
++ die(_("GIT_CONFIG_GLOBAL=/dev/null set"));
++
+ /*
+ * It is unknown if HOME/.gitconfig exists, so
+ * we do not know if we should write to XDG
+@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
+ * is set and points at a sane location.
+ */
+ die(_("$HOME not set"));
++ }
+
+ given_config_source.scope = CONFIG_SCOPE_GLOBAL;
+
+
## config.c ##
@@ config.c: static int git_config_from_blob_ref(config_fn_t fn,
- const char *git_system_config(void)
+
+ 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"));
-+ 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);
-+ }
++ char *system_config = xstrdup_or_null(getenv("GIT_CONFIG_SYSTEM"));
++ if (system_config) {
++ if (!strcmp(system_config, "/dev/null"))
++ FREE_AND_NULL(system_config);
++ return system_config;
+ }
-+
- return system_wide;
+ return system_path(ETC_GITCONFIG);
}
-@@ config.c: void git_global_config(const char **user, const char **xdg)
- static const char *user_config, *xdg_config;
+-void git_global_config(char **user_config, char **xdg_config)
++void git_global_config(char **user_out, char **xdg_out)
+ {
+- *user_config = expand_user_path("~/.gitconfig", 0);
+- *xdg_config = xdg_config_home("config");
++ char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
++ char *xdg_config = NULL;
++
++ if (user_config) {
++ if (!strcmp(user_config, "/dev/null"))
++ FREE_AND_NULL(user_config);
++ xdg_config = NULL;
++ } else {
++ user_config = expand_user_path("~/.gitconfig", 0);
++ xdg_config = xdg_config_home("config");
++ }
++
++ *user_out = user_config;
++ *xdg_out = 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");
-+ }
- }
-
- *user = user_config;
+ /*
## t/t1300-config.sh ##
@@ t/t1300-config.sh: test_expect_success '--show-scope with --show-origin' '
@@ t/t1300-config.sh: test_expect_success '--show-scope with --show-origin' '
+
+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
++ 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
+'
+
+test_expect_success 'write to overridden global and system config' '
@@ t/t1300-config.sh: test_expect_success '--show-scope with --show-origin' '
+ key = value
+EOF
+
-+ 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 &&
+
-+ test_must_fail env GIT_CONFIG_SYSTEM=write-to-system git config --system config.key value &&
-+ touch write-to-system &&
+ GIT_CONFIG_SYSTEM=write-to-system git config --system config.key value &&
+ test_cmp expect write-to-system
+'
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 1/3] config: rename `git_etc_config()`
2021-04-12 14:46 ` [PATCH v3 " Patrick Steinhardt
@ 2021-04-12 14:46 ` Patrick Steinhardt
2021-04-12 14:46 ` [PATCH v3 2/3] config: unify code paths to get global config paths Patrick Steinhardt
` (2 subsequent siblings)
3 siblings, 0 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-12 14:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 3932 bytes --]
The `git_etc_gitconfig()` function retrieves the system-level path of
the configuration file. We're about to introduce a way to override it
via an environment variable, at which point the name of this function
would start to become misleading.
Rename the function to `git_system_config()` as a preparatory step.
While at it, the function is also refactored to pass memory ownership to
the caller. This is done to better match semantics of
`git_global_config()`, which is going to be introduced in the next
commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/config.c | 2 +-
config.c | 18 ++++++++----------
config.h | 3 ++-
3 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index f71fa39b38..02ed0b3fe7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -695,7 +695,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
}
else if (use_system_config) {
- given_config_source.file = git_etc_gitconfig();
+ given_config_source.file = git_system_config();
given_config_source.scope = CONFIG_SCOPE_SYSTEM;
} else if (use_local_config) {
given_config_source.file = git_pathdup("config");
diff --git a/config.c b/config.c
index 6428393a41..8c83669cce 100644
--- a/config.c
+++ b/config.c
@@ -1844,12 +1844,9 @@ static int git_config_from_blob_ref(config_fn_t fn,
return git_config_from_blob_oid(fn, name, &oid, data);
}
-const char *git_etc_gitconfig(void)
+char *git_system_config(void)
{
- static const char *system_wide;
- if (!system_wide)
- system_wide = system_path(ETC_GITCONFIG);
- return system_wide;
+ return system_path(ETC_GITCONFIG);
}
/*
@@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts,
config_fn_t fn, void *data)
{
int ret = 0;
+ char *system_config = git_system_config();
char *xdg_config = xdg_config_home("config");
char *user_config = expand_user_path("~/.gitconfig", 0);
char *repo_config;
@@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts,
repo_config = NULL;
current_parsing_scope = CONFIG_SCOPE_SYSTEM;
- if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
- opts->system_gently ?
- ACCESS_EACCES_OK : 0))
- ret += git_config_from_file(fn, git_etc_gitconfig(),
- data);
+ if (system_config && !access_or_die(system_config, R_OK,
+ opts->system_gently ?
+ ACCESS_EACCES_OK : 0))
+ ret += git_config_from_file(fn, system_config, data);
current_parsing_scope = CONFIG_SCOPE_GLOBAL;
if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
@@ -1927,6 +1924,7 @@ static int do_git_config_sequence(const struct config_options *opts,
die(_("unable to parse command-line config"));
current_parsing_scope = prev_parsing_scope;
+ free(system_config);
free(xdg_config);
free(user_config);
free(repo_config);
diff --git a/config.h b/config.h
index 19a9adbaa9..2be8fa1880 100644
--- a/config.h
+++ b/config.h
@@ -318,7 +318,6 @@ int git_config_rename_section(const char *, const char *);
int git_config_rename_section_in_file(const char *, const char *, const char *);
int git_config_copy_section(const char *, const char *);
int git_config_copy_section_in_file(const char *, const char *, const char *);
-const char *git_etc_gitconfig(void);
int git_env_bool(const char *, int);
unsigned long git_env_ulong(const char *, unsigned long);
int git_config_system(void);
@@ -327,6 +326,8 @@ int config_error_nonbool(const char *);
#define config_error_nonbool(s) (config_error_nonbool(s), const_error())
#endif
+char *git_system_config(void);
+
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
enum config_scope current_config_scope(void);
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v3 2/3] config: unify code paths to get global config paths
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 ` Patrick Steinhardt
2021-04-12 14:46 ` [PATCH v3 3/3] config: allow overriding of global and system configuration Patrick Steinhardt
2021-04-13 7:11 ` [PATCH v4 0/3] config: allow overriding global/system config Patrick Steinhardt
3 siblings, 0 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-12 14:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]
There's two callsites which assemble global config paths, once in the
config loading code and once in the git-config(1) builtin. We're about
to implement a way to override global config paths via an environment
variable which would require us to adjust both sites.
Unify both code paths into a single `git_global_config()` function which
returns both paths for `~/.gitconfig` and the XDG config file. This will
make the subsequent patch which introduces the new envvar easier to
implement.
No functional changes are expected from this patch.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/config.c | 4 ++--
config.c | 12 ++++++++++--
config.h | 1 +
3 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 02ed0b3fe7..865fddd6ce 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -671,9 +671,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
if (use_global_config) {
- char *user_config = expand_user_path("~/.gitconfig", 0);
- char *xdg_config = xdg_config_home("config");
+ char *user_config, *xdg_config;
+ git_global_config(&user_config, &xdg_config);
if (!user_config)
/*
* It is unknown if HOME/.gitconfig exists, so
diff --git a/config.c b/config.c
index 8c83669cce..ebff58aa57 100644
--- a/config.c
+++ b/config.c
@@ -1849,6 +1849,12 @@ char *git_system_config(void)
return system_path(ETC_GITCONFIG);
}
+void git_global_config(char **user_config, char **xdg_config)
+{
+ *user_config = expand_user_path("~/.gitconfig", 0);
+ *xdg_config = xdg_config_home("config");
+}
+
/*
* Parse environment variable 'k' as a boolean (in various
* possible spellings); if missing, use the default value 'def'.
@@ -1881,8 +1887,8 @@ static int do_git_config_sequence(const struct config_options *opts,
{
int ret = 0;
char *system_config = git_system_config();
- char *xdg_config = xdg_config_home("config");
- char *user_config = expand_user_path("~/.gitconfig", 0);
+ char *xdg_config = NULL;
+ char *user_config = NULL;
char *repo_config;
enum config_scope prev_parsing_scope = current_parsing_scope;
@@ -1900,6 +1906,8 @@ static int do_git_config_sequence(const struct config_options *opts,
ret += git_config_from_file(fn, system_config, data);
current_parsing_scope = CONFIG_SCOPE_GLOBAL;
+ git_global_config(&user_config, &xdg_config);
+
if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
ret += git_config_from_file(fn, xdg_config, data);
diff --git a/config.h b/config.h
index 2be8fa1880..9038538ffd 100644
--- a/config.h
+++ b/config.h
@@ -327,6 +327,7 @@ int config_error_nonbool(const char *);
#endif
char *git_system_config(void);
+void git_global_config(char **user, char **xdg);
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v3 3/3] config: allow overriding of global and system configuration
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 ` 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
3 siblings, 1 reply; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-12 14:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 7786 bytes --]
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_NOGLOBAL`
as an equivalent to `GIT_CONFIG_NOSYSTEM`. But in the past, it has
turned out that this design is inflexible: we cannot test system-level
parsing of the git configuration in our test harness because there is no
way to change its location, so all tests run with `GIT_CONFIG_NOSYSTEM`
set.
Instead of doing the same mistake with `GIT_CONFIG_NOGLOBAL`, introduce
two new variables `GIT_CONFIG_GLOBAL` and `GIT_CONFIG_SYSTEM`:
- If unset, git continues to use the usual locations.
- If set to a specific path, we skip reading the normal
configuration files and instead take the path.
- If set to `/dev/null`, we do not load either global- or
system-level configuration at all.
This implements the usecase where we want to execute code in a sanitized
environment without any potential misconfigurations via `/dev/null`, but
is more flexible and allows for more usecases than simply adding
`GIT_CONFIG_NOGLOBAL`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-config.txt | 5 +++
Documentation/git.txt | 10 +++++
builtin/config.c | 6 ++-
config.c | 24 ++++++++++--
t/t1300-config.sh | 71 ++++++++++++++++++++++++++++++++++++
5 files changed, 112 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4b4cc5c5e8..5cddadafd2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -340,6 +340,11 @@ GIT_CONFIG::
Using the "--global" option forces this to ~/.gitconfig. Using the
"--system" option forces this to $(prefix)/etc/gitconfig.
+GIT_CONFIG_GLOBAL::
+GIT_CONFIG_SYSTEM::
+ Take the configuration from the given files instead from global or
+ system-level configuration. 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..380422a6a9 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. If `GIT_CONFIG_SYSTEM` is set, the
+ system config file defined at build time (usually `/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.
+
`GIT_CONFIG_NOSYSTEM`::
Whether to skip reading settings from the system-wide
`$(prefix)/etc/gitconfig` file. This environment variable can
diff --git a/builtin/config.c b/builtin/config.c
index 865fddd6ce..9104816459 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -674,7 +674,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)
char *user_config, *xdg_config;
git_global_config(&user_config, &xdg_config);
- if (!user_config)
+ if (!user_config) {
+ if (getenv("GIT_CONFIG_GLOBAL"))
+ die(_("GIT_CONFIG_GLOBAL=/dev/null set"));
+
/*
* It is unknown if HOME/.gitconfig exists, so
* we do not know if we should write to XDG
@@ -682,6 +685,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
* is set and points at a sane location.
*/
die(_("$HOME not set"));
+ }
given_config_source.scope = CONFIG_SCOPE_GLOBAL;
diff --git a/config.c b/config.c
index ebff58aa57..ed46eda997 100644
--- a/config.c
+++ b/config.c
@@ -1846,13 +1846,31 @@ static int git_config_from_blob_ref(config_fn_t fn,
char *git_system_config(void)
{
+ char *system_config = xstrdup_or_null(getenv("GIT_CONFIG_SYSTEM"));
+ if (system_config) {
+ if (!strcmp(system_config, "/dev/null"))
+ FREE_AND_NULL(system_config);
+ return system_config;
+ }
return system_path(ETC_GITCONFIG);
}
-void git_global_config(char **user_config, char **xdg_config)
+void git_global_config(char **user_out, char **xdg_out)
{
- *user_config = expand_user_path("~/.gitconfig", 0);
- *xdg_config = xdg_config_home("config");
+ char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
+ char *xdg_config = NULL;
+
+ if (user_config) {
+ if (!strcmp(user_config, "/dev/null"))
+ FREE_AND_NULL(user_config);
+ xdg_config = NULL;
+ } else {
+ user_config = expand_user_path("~/.gitconfig", 0);
+ xdg_config = xdg_config_home("config");
+ }
+
+ *user_out = user_config;
+ *xdg_out = xdg_config;
}
/*
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e0dd5d65ce..17f1b78c01 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2059,6 +2059,77 @@ test_expect_success '--show-scope with --show-origin' '
test_cmp expect output
'
+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
+
+ 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
+'
+
+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
+'
+
+test_expect_success 'write to overridden global and system config' '
+ cat >expect <<EOF &&
+[config]
+ key = value
+EOF
+
+ GIT_CONFIG_GLOBAL=write-to-global git config --global config.key value &&
+ test_cmp expect write-to-global &&
+
+ GIT_CONFIG_SYSTEM=write-to-system git config --system config.key value &&
+ test_cmp expect write-to-system
+'
+
for opt in --local --worktree
do
test_expect_success "$opt requires a repo" '
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] config: allow overriding of global and system configuration
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
0 siblings, 0 replies; 52+ messages in thread
From: Junio C Hamano @ 2021-04-12 17:04 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason, Eric Sunshine
Patrick Steinhardt <ps@pks.im> writes:
> char *git_system_config(void)
> {
> + char *system_config = xstrdup_or_null(getenv("GIT_CONFIG_SYSTEM"));
> + if (system_config) {
> + if (!strcmp(system_config, "/dev/null"))
> + FREE_AND_NULL(system_config);
> + return system_config;
> + }
I am not sure if returning NULL from this function will always be
the same as returning /dev/null on a system with functioning
/dev/null. For example, when use_system_config is enabled,
builtin/config.c::cmd_config() assigns the NULL returned by this
function to given_config_source.file and then calls
config.c::config_with_options(), which notices that none of
use_stdin, file, or blob member of the config_source exists and
falls back to the config_sequence().
So, for the purpose of special casing "/dev/null" textually, the
above is not sufficient, I am afraid.
Let's rescind the "/dev/null gets turned into NULL" in the above
change for now. If we truly want to cater to an installation where
open("/dev/null") fails and emulate, that needs to be done at a much
lower layer, but we do not have to go there for the purpose of this
series.
Thanks.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v4 0/3] config: allow overriding global/system config
2021-04-12 14:46 ` [PATCH v3 " Patrick Steinhardt
` (2 preceding siblings ...)
2021-04-12 14:46 ` [PATCH v3 3/3] config: allow overriding of global and system configuration Patrick Steinhardt
@ 2021-04-13 7:11 ` Patrick Steinhardt
2021-04-13 7:11 ` [PATCH v4 1/3] config: rename `git_etc_config()` Patrick Steinhardt
` (5 more replies)
3 siblings, 6 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-13 7:11 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 3897 bytes --]
Hi,
this is the fourth version of my patch series to provide a way of
overriding the global system configuration.
Compared to v3, I only dropped the special-casing of `/dev/null`. As
Junio rightly pointed out, the special-casing was incomplete and would
have required more work to do the right thing for all cases. It can
still be re-added at a later point if the usecase actually comes up.
Patrick
Patrick Steinhardt (3):
config: rename `git_etc_config()`
config: unify code paths to get global config paths
config: allow overriding of global and system configuration
Documentation/git-config.txt | 5 +++
Documentation/git.txt | 10 +++++
builtin/config.c | 6 +--
config.c | 41 +++++++++++++++------
config.h | 4 +-
t/t1300-config.sh | 71 ++++++++++++++++++++++++++++++++++++
6 files changed, 121 insertions(+), 16 deletions(-)
Range-diff against v3:
1: 34bdbc27d6 = 1: 34bdbc27d6 config: rename `git_etc_config()`
2: 30f18679bd = 2: 30f18679bd config: unify code paths to get global config paths
3: af663640ae ! 3: d27efc0aa8 config: allow overriding of global and system configuration
@@ Commit message
- If unset, git continues to use the usual locations.
- If set to a specific path, we skip reading the normal
- configuration files and instead take the path.
-
- - If set to `/dev/null`, we do not load either global- or
- system-level configuration at all.
+ configuration files and instead take the path. By setting the path
+ to `/dev/null`, no configuration will be loaded for the respective
+ level.
This implements the usecase where we want to execute code in a sanitized
environment without any potential misconfigurations via `/dev/null`, but
@@ Documentation/git.txt: for further details.
Whether to skip reading settings from the system-wide
`$(prefix)/etc/gitconfig` file. This environment variable can
- ## builtin/config.c ##
-@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
- char *user_config, *xdg_config;
-
- git_global_config(&user_config, &xdg_config);
-- if (!user_config)
-+ if (!user_config) {
-+ if (getenv("GIT_CONFIG_GLOBAL"))
-+ die(_("GIT_CONFIG_GLOBAL=/dev/null set"));
-+
- /*
- * It is unknown if HOME/.gitconfig exists, so
- * we do not know if we should write to XDG
-@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
- * is set and points at a sane location.
- */
- die(_("$HOME not set"));
-+ }
-
- given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
-
## config.c ##
@@ config.c: static int git_config_from_blob_ref(config_fn_t fn,
char *git_system_config(void)
{
+ char *system_config = xstrdup_or_null(getenv("GIT_CONFIG_SYSTEM"));
-+ if (system_config) {
-+ if (!strcmp(system_config, "/dev/null"))
-+ FREE_AND_NULL(system_config);
++ if (system_config)
+ return system_config;
-+ }
return system_path(ETC_GITCONFIG);
}
@@ config.c: static int git_config_from_blob_ref(config_fn_t fn,
+ char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
+ char *xdg_config = NULL;
+
-+ if (user_config) {
-+ if (!strcmp(user_config, "/dev/null"))
-+ FREE_AND_NULL(user_config);
-+ xdg_config = NULL;
-+ } else {
++ if (!user_config) {
+ user_config = expand_user_path("~/.gitconfig", 0);
+ xdg_config = xdg_config_home("config");
+ }
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v4 1/3] config: rename `git_etc_config()`
2021-04-13 7:11 ` [PATCH v4 0/3] config: allow overriding global/system config Patrick Steinhardt
@ 2021-04-13 7:11 ` Patrick Steinhardt
2021-04-13 7:25 ` Jeff King
2021-04-16 21:14 ` SZEDER Gábor
2021-04-13 7:11 ` [PATCH v4 2/3] config: unify code paths to get global config paths Patrick Steinhardt
` (4 subsequent siblings)
5 siblings, 2 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-13 7:11 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 3932 bytes --]
The `git_etc_gitconfig()` function retrieves the system-level path of
the configuration file. We're about to introduce a way to override it
via an environment variable, at which point the name of this function
would start to become misleading.
Rename the function to `git_system_config()` as a preparatory step.
While at it, the function is also refactored to pass memory ownership to
the caller. This is done to better match semantics of
`git_global_config()`, which is going to be introduced in the next
commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/config.c | 2 +-
config.c | 18 ++++++++----------
config.h | 3 ++-
3 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index f71fa39b38..02ed0b3fe7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -695,7 +695,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
}
else if (use_system_config) {
- given_config_source.file = git_etc_gitconfig();
+ given_config_source.file = git_system_config();
given_config_source.scope = CONFIG_SCOPE_SYSTEM;
} else if (use_local_config) {
given_config_source.file = git_pathdup("config");
diff --git a/config.c b/config.c
index 6428393a41..8c83669cce 100644
--- a/config.c
+++ b/config.c
@@ -1844,12 +1844,9 @@ static int git_config_from_blob_ref(config_fn_t fn,
return git_config_from_blob_oid(fn, name, &oid, data);
}
-const char *git_etc_gitconfig(void)
+char *git_system_config(void)
{
- static const char *system_wide;
- if (!system_wide)
- system_wide = system_path(ETC_GITCONFIG);
- return system_wide;
+ return system_path(ETC_GITCONFIG);
}
/*
@@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts,
config_fn_t fn, void *data)
{
int ret = 0;
+ char *system_config = git_system_config();
char *xdg_config = xdg_config_home("config");
char *user_config = expand_user_path("~/.gitconfig", 0);
char *repo_config;
@@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts,
repo_config = NULL;
current_parsing_scope = CONFIG_SCOPE_SYSTEM;
- if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
- opts->system_gently ?
- ACCESS_EACCES_OK : 0))
- ret += git_config_from_file(fn, git_etc_gitconfig(),
- data);
+ if (system_config && !access_or_die(system_config, R_OK,
+ opts->system_gently ?
+ ACCESS_EACCES_OK : 0))
+ ret += git_config_from_file(fn, system_config, data);
current_parsing_scope = CONFIG_SCOPE_GLOBAL;
if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
@@ -1927,6 +1924,7 @@ static int do_git_config_sequence(const struct config_options *opts,
die(_("unable to parse command-line config"));
current_parsing_scope = prev_parsing_scope;
+ free(system_config);
free(xdg_config);
free(user_config);
free(repo_config);
diff --git a/config.h b/config.h
index 19a9adbaa9..2be8fa1880 100644
--- a/config.h
+++ b/config.h
@@ -318,7 +318,6 @@ int git_config_rename_section(const char *, const char *);
int git_config_rename_section_in_file(const char *, const char *, const char *);
int git_config_copy_section(const char *, const char *);
int git_config_copy_section_in_file(const char *, const char *, const char *);
-const char *git_etc_gitconfig(void);
int git_env_bool(const char *, int);
unsigned long git_env_ulong(const char *, unsigned long);
int git_config_system(void);
@@ -327,6 +326,8 @@ int config_error_nonbool(const char *);
#define config_error_nonbool(s) (config_error_nonbool(s), const_error())
#endif
+char *git_system_config(void);
+
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
enum config_scope current_config_scope(void);
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/3] config: rename `git_etc_config()`
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
1 sibling, 0 replies; 52+ messages in thread
From: Jeff King @ 2021-04-13 7:25 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Eric Sunshine
On Tue, Apr 13, 2021 at 09:11:44AM +0200, Patrick Steinhardt wrote:
> While at it, the function is also refactored to pass memory ownership to
> the caller. This is done to better match semantics of
> `git_global_config()`, which is going to be introduced in the next
> commit.
I think this turned out nicely. There are only two callers, one of which
is already handling freeing the global config. In the other:
> diff --git a/builtin/config.c b/builtin/config.c
> index f71fa39b38..02ed0b3fe7 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -695,7 +695,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> }
> }
> else if (use_system_config) {
> - given_config_source.file = git_etc_gitconfig();
> + given_config_source.file = git_system_config();
> given_config_source.scope = CONFIG_SCOPE_SYSTEM;
> } else if (use_local_config) {
> given_config_source.file = git_pathdup("config");
We "leak" the result, but I think it is actually an improvement. As you
can see in the context, we are sometimes allocating the field in other
code paths, so this is takes us closer to a world where we can actually
call free(given_config_source.file) to fix the leak in all of the other
code paths. :)
(I don't think any of that needs to be dealt with in this series, of
course).
-Peff
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/3] config: rename `git_etc_config()`
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
1 sibling, 1 reply; 52+ messages in thread
From: SZEDER Gábor @ 2021-04-16 21:14 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
On Tue, Apr 13, 2021 at 09:11:44AM +0200, Patrick Steinhardt wrote:
> The `git_etc_gitconfig()` function retrieves the system-level path of
> the configuration file. We're about to introduce a way to override it
> via an environment variable, at which point the name of this function
> would start to become misleading.
>
> Rename the function to `git_system_config()` as a preparatory step.
> While at it, the function is also refactored to pass memory ownership to
> the caller. This is done to better match semantics of
> `git_global_config()`, which is going to be introduced in the next
> commit.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/config.c | 2 +-
> config.c | 18 ++++++++----------
> config.h | 3 ++-
> 3 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index f71fa39b38..02ed0b3fe7 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -695,7 +695,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> }
> }
> else if (use_system_config) {
> - given_config_source.file = git_etc_gitconfig();
> + given_config_source.file = git_system_config();
> given_config_source.scope = CONFIG_SCOPE_SYSTEM;
> } else if (use_local_config) {
> given_config_source.file = git_pathdup("config");
> diff --git a/config.c b/config.c
> index 6428393a41..8c83669cce 100644
> --- a/config.c
> +++ b/config.c
> @@ -1844,12 +1844,9 @@ static int git_config_from_blob_ref(config_fn_t fn,
> return git_config_from_blob_oid(fn, name, &oid, data);
> }
>
> -const char *git_etc_gitconfig(void)
> +char *git_system_config(void)
> {
> - static const char *system_wide;
> - if (!system_wide)
> - system_wide = system_path(ETC_GITCONFIG);
> - return system_wide;
> + return system_path(ETC_GITCONFIG);
> }
>
> /*
> @@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts,
> config_fn_t fn, void *data)
> {
> int ret = 0;
> + char *system_config = git_system_config();
> char *xdg_config = xdg_config_home("config");
> char *user_config = expand_user_path("~/.gitconfig", 0);
> char *repo_config;
> @@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts,
> repo_config = NULL;
>
> current_parsing_scope = CONFIG_SCOPE_SYSTEM;
> - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
Removing git_config_system() from the condition breaks
GIT_CONFIG_NOSYSTEM:
expecting success of 9999.1 'test':
cat /usr/local/etc/gitconfig &&
git config --list --show-origin --show-scope
+ cat /usr/local/etc/gitconfig
[foo]
bar = baz
+ git config --list --show-origin --show-scope
system file:/usr/local/etc/gitconfig foo.bar=baz
local file:.git/config core.repositoryformatversion=0
local file:.git/config core.filemode=true
local file:.git/config core.bare=false
local file:.git/config core.logallrefupdates=true
ok 1 - test
And breaks just about everything the Linux32 job on Travis CI:
https://travis-ci.org/github/git/git/jobs/767207687#L1218
> - opts->system_gently ?
> - ACCESS_EACCES_OK : 0))
> - ret += git_config_from_file(fn, git_etc_gitconfig(),
> - data);
> + if (system_config && !access_or_die(system_config, R_OK,
> + opts->system_gently ?
> + ACCESS_EACCES_OK : 0))
> + ret += git_config_from_file(fn, system_config, data);
>
> current_parsing_scope = CONFIG_SCOPE_GLOBAL;
> if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
> @@ -1927,6 +1924,7 @@ static int do_git_config_sequence(const struct config_options *opts,
> die(_("unable to parse command-line config"));
>
> current_parsing_scope = prev_parsing_scope;
> + free(system_config);
> free(xdg_config);
> free(user_config);
> free(repo_config);
> diff --git a/config.h b/config.h
> index 19a9adbaa9..2be8fa1880 100644
> --- a/config.h
> +++ b/config.h
> @@ -318,7 +318,6 @@ int git_config_rename_section(const char *, const char *);
> int git_config_rename_section_in_file(const char *, const char *, const char *);
> int git_config_copy_section(const char *, const char *);
> int git_config_copy_section_in_file(const char *, const char *, const char *);
> -const char *git_etc_gitconfig(void);
> int git_env_bool(const char *, int);
> unsigned long git_env_ulong(const char *, unsigned long);
> int git_config_system(void);
> @@ -327,6 +326,8 @@ int config_error_nonbool(const char *);
> #define config_error_nonbool(s) (config_error_nonbool(s), const_error())
> #endif
>
> +char *git_system_config(void);
> +
> int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
>
> enum config_scope current_config_scope(void);
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/3] config: rename `git_etc_config()`
2021-04-16 21:14 ` SZEDER Gábor
@ 2021-04-17 8:44 ` Jeff King
2021-04-17 21:37 ` Junio C Hamano
0 siblings, 1 reply; 52+ messages in thread
From: Jeff King @ 2021-04-17 8:44 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Patrick Steinhardt, git, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Eric Sunshine
On Fri, Apr 16, 2021 at 11:14:51PM +0200, SZEDER Gábor wrote:
> > @@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts,
> > config_fn_t fn, void *data)
> > {
> > int ret = 0;
> > + char *system_config = git_system_config();
> > char *xdg_config = xdg_config_home("config");
> > char *user_config = expand_user_path("~/.gitconfig", 0);
> > char *repo_config;
> > @@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts,
> > repo_config = NULL;
> >
> > current_parsing_scope = CONFIG_SCOPE_SYSTEM;
> > - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
>
> Removing git_config_system() from the condition breaks
> GIT_CONFIG_NOSYSTEM:
Good catch. My gut feeling is that the new git_system_config() should
check NOSYSTEM and return NULL if it's set, and then we can get rid of
git_config_system() entirely.
That is slightly different than the old behavior; right now
GIT_CONFIG_NOSYSTEM only prevents reading during the normal sequence,
and not reading (or writing!) via "git config --system". But I think it
would be an improvement to prevent those (the whole point of the feature
was to avoid the test suite accidentally accessing the larger
environment).
-Peff
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/3] config: rename `git_etc_config()`
2021-04-17 8:44 ` Jeff King
@ 2021-04-17 21:37 ` Junio C Hamano
2021-04-18 5:39 ` Jeff King
0 siblings, 1 reply; 52+ messages in thread
From: Junio C Hamano @ 2021-04-17 21:37 UTC (permalink / raw)
To: Jeff King
Cc: SZEDER Gábor, Patrick Steinhardt, git,
Ævar Arnfjörð Bjarmason, Eric Sunshine
Jeff King <peff@peff.net> writes:
> On Fri, Apr 16, 2021 at 11:14:51PM +0200, SZEDER Gábor wrote:
>
>> > @@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts,
>> > config_fn_t fn, void *data)
>> > {
>> > int ret = 0;
>> > + char *system_config = git_system_config();
>> > char *xdg_config = xdg_config_home("config");
>> > char *user_config = expand_user_path("~/.gitconfig", 0);
>> > char *repo_config;
>> > @@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts,
>> > repo_config = NULL;
>> >
>> > current_parsing_scope = CONFIG_SCOPE_SYSTEM;
>> > - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
>>
>> Removing git_config_system() from the condition breaks
>> GIT_CONFIG_NOSYSTEM:
>
> Good catch. My gut feeling is that the new git_system_config() should
> check NOSYSTEM and return NULL if it's set, and then we can get rid of
> git_config_system() entirely.
NULL -> /dev/null I guess?
> That is slightly different than the old behavior; right now
> GIT_CONFIG_NOSYSTEM only prevents reading during the normal sequence,
> and not reading (or writing!) via "git config --system". But I think it
> would be an improvement to prevent those (the whole point of the feature
> was to avoid the test suite accidentally accessing the larger
> environment).
Yup.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/3] config: rename `git_etc_config()`
2021-04-17 21:37 ` Junio C Hamano
@ 2021-04-18 5:39 ` Jeff King
2021-04-19 11:03 ` Patrick Steinhardt
0 siblings, 1 reply; 52+ messages in thread
From: Jeff King @ 2021-04-18 5:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: SZEDER Gábor, Patrick Steinhardt, git,
Ævar Arnfjörð Bjarmason, Eric Sunshine
On Sat, Apr 17, 2021 at 02:37:39PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Fri, Apr 16, 2021 at 11:14:51PM +0200, SZEDER Gábor wrote:
> >
> >> > @@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts,
> >> > config_fn_t fn, void *data)
> >> > {
> >> > int ret = 0;
> >> > + char *system_config = git_system_config();
> >> > char *xdg_config = xdg_config_home("config");
> >> > char *user_config = expand_user_path("~/.gitconfig", 0);
> >> > char *repo_config;
> >> > @@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts,
> >> > repo_config = NULL;
> >> >
> >> > current_parsing_scope = CONFIG_SCOPE_SYSTEM;
> >> > - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
> >>
> >> Removing git_config_system() from the condition breaks
> >> GIT_CONFIG_NOSYSTEM:
> >
> > Good catch. My gut feeling is that the new git_system_config() should
> > check NOSYSTEM and return NULL if it's set, and then we can get rid of
> > git_config_system() entirely.
>
> NULL -> /dev/null I guess?
I was thinking NULL to catch this line from the post-image of Patrick's
series:
if (system_config && !access_or_die(system_config, R_OK,
opts->system_gently ?
ACCESS_EACCES_OK : 0))
ret += git_config_from_file(fn, system_config, data);
which would see that we have no file at all. But that may be unexpected
for other callers (right now git_etc_gitconfig() can never return NULL,
and I'm not sure what would happen with "git config --system"; I suspect
it would do the regular config sequence instead, which is wrong).
So yeah, probably returning /dev/null is more sensible (and makes it a
true alias for GIT_CONFIG_SYSTEM=/dev/null).
-Peff
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/3] config: rename `git_etc_config()`
2021-04-18 5:39 ` Jeff King
@ 2021-04-19 11:03 ` Patrick Steinhardt
2021-04-23 9:27 ` Jeff King
0 siblings, 1 reply; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-19 11:03 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, SZEDER Gábor, git,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]
On Sun, Apr 18, 2021 at 01:39:31AM -0400, Jeff King wrote:
> On Sat, Apr 17, 2021 at 02:37:39PM -0700, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > On Fri, Apr 16, 2021 at 11:14:51PM +0200, SZEDER Gábor wrote:
> > >
> > >> > @@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts,
> > >> > config_fn_t fn, void *data)
> > >> > {
> > >> > int ret = 0;
> > >> > + char *system_config = git_system_config();
> > >> > char *xdg_config = xdg_config_home("config");
> > >> > char *user_config = expand_user_path("~/.gitconfig", 0);
> > >> > char *repo_config;
> > >> > @@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts,
> > >> > repo_config = NULL;
> > >> >
> > >> > current_parsing_scope = CONFIG_SCOPE_SYSTEM;
> > >> > - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
> > >>
> > >> Removing git_config_system() from the condition breaks
> > >> GIT_CONFIG_NOSYSTEM:
> > >
> > > Good catch. My gut feeling is that the new git_system_config() should
> > > check NOSYSTEM and return NULL if it's set, and then we can get rid of
> > > git_config_system() entirely.
> >
> > NULL -> /dev/null I guess?
>
> I was thinking NULL to catch this line from the post-image of Patrick's
> series:
>
> if (system_config && !access_or_die(system_config, R_OK,
> opts->system_gently ?
> ACCESS_EACCES_OK : 0))
> ret += git_config_from_file(fn, system_config, data);
>
> which would see that we have no file at all. But that may be unexpected
> for other callers (right now git_etc_gitconfig() can never return NULL,
> and I'm not sure what would happen with "git config --system"; I suspect
> it would do the regular config sequence instead, which is wrong).
>
> So yeah, probably returning /dev/null is more sensible (and makes it a
> true alias for GIT_CONFIG_SYSTEM=/dev/null).
>
> -Peff
It's only by accident that I dropped the call to `git_config_system()`,
must've happened when resolving conflicts somewhere. The issue with just
returning `/dev/null` from `git_system_config()` is that git-config(1)
would be broken, as you hint at. We do not honor GIT_CONFIG_NOSYSTEM
there if the "--system" flag was given.
So yes, we could change it to return `/dev/null`, but that would change
semantics of GIT_CONFIG_NOSYSTEM. I'm not sure doing this in the same
series is a good idea. Even more so because with returning `/dev/null`,
the conversion would be silent: whereas previous versions simply wrote
to or read from the system-level config, we now pretend to have read or
written something even though we didn't.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/3] config: rename `git_etc_config()`
2021-04-19 11:03 ` Patrick Steinhardt
@ 2021-04-23 9:27 ` Jeff King
0 siblings, 0 replies; 52+ messages in thread
From: Jeff King @ 2021-04-23 9:27 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Junio C Hamano, SZEDER Gábor, git,
Ævar Arnfjörð Bjarmason, Eric Sunshine
On Mon, Apr 19, 2021 at 01:03:56PM +0200, Patrick Steinhardt wrote:
> It's only by accident that I dropped the call to `git_config_system()`,
> must've happened when resolving conflicts somewhere. The issue with just
> returning `/dev/null` from `git_system_config()` is that git-config(1)
> would be broken, as you hint at. We do not honor GIT_CONFIG_NOSYSTEM
> there if the "--system" flag was given.
>
> So yes, we could change it to return `/dev/null`, but that would change
> semantics of GIT_CONFIG_NOSYSTEM. I'm not sure doing this in the same
> series is a good idea. Even more so because with returning `/dev/null`,
> the conversion would be silent: whereas previous versions simply wrote
> to or read from the system-level config, we now pretend to have read or
> written something even though we didn't.
I'm OK being more cautious here, and leaving NOSYSTEM as it is.
My original thinking was that returning /dev/null would be an
improvement even for the existing call in "git config --system". And I
do think it would be for reading. But it actually gets pretty weird for
writing, because of our atomic-rename strategy.
And that is actually true of your new variable too. Doing this:
GIT_CONFIG_SYSTEM=/dev/null git config --system foo.bar
would return nothing, which makes sense to me. But this:
GIT_CONFIG_SYSTEM=/dev/null git config --system foo.bar value
would try to write /dev/null.lock, and then rename it into place over
the real /dev/null! That may be slightly surprising, but it is not
really any different than any other non-regular-file entry.
The stakes are slightly higher here, as it breaks all sorts of things
that can be hard to recover from (even if you know the correct mknod
incantation, important things like bash seem to get crabby if they can't
open /dev/null for writing). And it may be more likely to happen by
mistake (as opposed to "git config --file=/dev/null", which behaves the
same). But unless you are root, you are likely to just get an error in
the first place, so it seems like an unlikely mistake in practice.
So I think it may fall into "if it hurts, don't do it".
-Peff
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v4 2/3] config: unify code paths to get global config paths
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:11 ` Patrick Steinhardt
2021-04-13 7:11 ` [PATCH v4 3/3] config: allow overriding of global and system configuration Patrick Steinhardt
` (3 subsequent siblings)
5 siblings, 0 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-13 7:11 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]
There's two callsites which assemble global config paths, once in the
config loading code and once in the git-config(1) builtin. We're about
to implement a way to override global config paths via an environment
variable which would require us to adjust both sites.
Unify both code paths into a single `git_global_config()` function which
returns both paths for `~/.gitconfig` and the XDG config file. This will
make the subsequent patch which introduces the new envvar easier to
implement.
No functional changes are expected from this patch.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/config.c | 4 ++--
config.c | 12 ++++++++++--
config.h | 1 +
3 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 02ed0b3fe7..865fddd6ce 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -671,9 +671,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
if (use_global_config) {
- char *user_config = expand_user_path("~/.gitconfig", 0);
- char *xdg_config = xdg_config_home("config");
+ char *user_config, *xdg_config;
+ git_global_config(&user_config, &xdg_config);
if (!user_config)
/*
* It is unknown if HOME/.gitconfig exists, so
diff --git a/config.c b/config.c
index 8c83669cce..ebff58aa57 100644
--- a/config.c
+++ b/config.c
@@ -1849,6 +1849,12 @@ char *git_system_config(void)
return system_path(ETC_GITCONFIG);
}
+void git_global_config(char **user_config, char **xdg_config)
+{
+ *user_config = expand_user_path("~/.gitconfig", 0);
+ *xdg_config = xdg_config_home("config");
+}
+
/*
* Parse environment variable 'k' as a boolean (in various
* possible spellings); if missing, use the default value 'def'.
@@ -1881,8 +1887,8 @@ static int do_git_config_sequence(const struct config_options *opts,
{
int ret = 0;
char *system_config = git_system_config();
- char *xdg_config = xdg_config_home("config");
- char *user_config = expand_user_path("~/.gitconfig", 0);
+ char *xdg_config = NULL;
+ char *user_config = NULL;
char *repo_config;
enum config_scope prev_parsing_scope = current_parsing_scope;
@@ -1900,6 +1906,8 @@ static int do_git_config_sequence(const struct config_options *opts,
ret += git_config_from_file(fn, system_config, data);
current_parsing_scope = CONFIG_SCOPE_GLOBAL;
+ git_global_config(&user_config, &xdg_config);
+
if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
ret += git_config_from_file(fn, xdg_config, data);
diff --git a/config.h b/config.h
index 2be8fa1880..9038538ffd 100644
--- a/config.h
+++ b/config.h
@@ -327,6 +327,7 @@ int config_error_nonbool(const char *);
#endif
char *git_system_config(void);
+void git_global_config(char **user, char **xdg);
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v4 3/3] config: allow overriding of global and system configuration
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:11 ` [PATCH v4 2/3] config: unify code paths to get global config paths Patrick Steinhardt
@ 2021-04-13 7:11 ` Patrick Steinhardt
2021-04-13 7:33 ` Jeff King
2021-04-13 7:33 ` [PATCH v4 0/3] config: allow overriding global/system config Jeff King
` (2 subsequent siblings)
5 siblings, 1 reply; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-13 7:11 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 6757 bytes --]
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_NOGLOBAL`
as an equivalent to `GIT_CONFIG_NOSYSTEM`. But in the past, it has
turned out that this design is inflexible: we cannot test system-level
parsing of the git configuration in our test harness because there is no
way to change its location, so all tests run with `GIT_CONFIG_NOSYSTEM`
set.
Instead of doing the same mistake with `GIT_CONFIG_NOGLOBAL`, introduce
two new variables `GIT_CONFIG_GLOBAL` and `GIT_CONFIG_SYSTEM`:
- If unset, git continues to use the usual locations.
- If set to a specific path, we skip reading the normal
configuration files and instead take the path. By setting the path
to `/dev/null`, no configuration will be loaded for the respective
level.
This implements the usecase where we want to execute code in a sanitized
environment without any potential misconfigurations via `/dev/null`, but
is more flexible and allows for more usecases than simply adding
`GIT_CONFIG_NOGLOBAL`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-config.txt | 5 +++
Documentation/git.txt | 10 +++++
config.c | 17 +++++++--
t/t1300-config.sh | 71 ++++++++++++++++++++++++++++++++++++
4 files changed, 100 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4b4cc5c5e8..5cddadafd2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -340,6 +340,11 @@ GIT_CONFIG::
Using the "--global" option forces this to ~/.gitconfig. Using the
"--system" option forces this to $(prefix)/etc/gitconfig.
+GIT_CONFIG_GLOBAL::
+GIT_CONFIG_SYSTEM::
+ Take the configuration from the given files instead from global or
+ system-level configuration. 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..380422a6a9 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. If `GIT_CONFIG_SYSTEM` is set, the
+ system config file defined at build time (usually `/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.
+
`GIT_CONFIG_NOSYSTEM`::
Whether to skip reading settings from the system-wide
`$(prefix)/etc/gitconfig` file. This environment variable can
diff --git a/config.c b/config.c
index ebff58aa57..7caf6c1e53 100644
--- a/config.c
+++ b/config.c
@@ -1846,13 +1846,24 @@ static int git_config_from_blob_ref(config_fn_t fn,
char *git_system_config(void)
{
+ char *system_config = xstrdup_or_null(getenv("GIT_CONFIG_SYSTEM"));
+ if (system_config)
+ return system_config;
return system_path(ETC_GITCONFIG);
}
-void git_global_config(char **user_config, char **xdg_config)
+void git_global_config(char **user_out, char **xdg_out)
{
- *user_config = expand_user_path("~/.gitconfig", 0);
- *xdg_config = xdg_config_home("config");
+ char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
+ char *xdg_config = NULL;
+
+ if (!user_config) {
+ user_config = expand_user_path("~/.gitconfig", 0);
+ xdg_config = xdg_config_home("config");
+ }
+
+ *user_out = user_config;
+ *xdg_out = xdg_config;
}
/*
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e0dd5d65ce..17f1b78c01 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2059,6 +2059,77 @@ test_expect_success '--show-scope with --show-origin' '
test_cmp expect output
'
+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
+
+ 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
+'
+
+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
+'
+
+test_expect_success 'write to overridden global and system config' '
+ cat >expect <<EOF &&
+[config]
+ key = value
+EOF
+
+ GIT_CONFIG_GLOBAL=write-to-global git config --global config.key value &&
+ test_cmp expect write-to-global &&
+
+ GIT_CONFIG_SYSTEM=write-to-system git config --system config.key value &&
+ test_cmp expect write-to-system
+'
+
for opt in --local --worktree
do
test_expect_success "$opt requires a repo" '
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v4 3/3] config: allow overriding of global and system configuration
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
0 siblings, 1 reply; 52+ messages in thread
From: Jeff King @ 2021-04-13 7:33 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Eric Sunshine
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.
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 ;) ).
-Peff
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 3/3] config: allow overriding of global and system configuration
2021-04-13 7:33 ` Jeff King
@ 2021-04-13 7:54 ` Patrick Steinhardt
0 siblings, 0 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-13 7:54 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]
On Tue, Apr 13, 2021 at 03:33:08AM -0400, Jeff King wrote:
> 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.
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 0/3] config: allow overriding global/system config
2021-04-13 7:11 ` [PATCH v4 0/3] config: allow overriding global/system config Patrick Steinhardt
` (2 preceding siblings ...)
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 17:49 ` Junio C Hamano
2021-04-19 12:31 ` [PATCH v5 " Patrick Steinhardt
5 siblings, 0 replies; 52+ messages in thread
From: Jeff King @ 2021-04-13 7:33 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Eric Sunshine
On Tue, Apr 13, 2021 at 09:11:37AM +0200, Patrick Steinhardt wrote:
> this is the fourth version of my patch series to provide a way of
> overriding the global system configuration.
>
> Compared to v3, I only dropped the special-casing of `/dev/null`. As
> Junio rightly pointed out, the special-casing was incomplete and would
> have required more work to do the right thing for all cases. It can
> still be re-added at a later point if the usecase actually comes up.
This version looks good to me. Thanks for working on this!
-Peff
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 0/3] config: allow overriding global/system config
2021-04-13 7:11 ` [PATCH v4 0/3] config: allow overriding global/system config Patrick Steinhardt
` (3 preceding siblings ...)
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
5 siblings, 1 reply; 52+ messages in thread
From: Junio C Hamano @ 2021-04-13 17:49 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason, Eric Sunshine
Patrick Steinhardt <ps@pks.im> writes:
> Compared to v3, I only dropped the special-casing of `/dev/null`. As
> Junio rightly pointed out, the special-casing was incomplete and would
> have required more work to do the right thing for all cases. It can
> still be re-added at a later point if the usecase actually comes up.
>
> Patrick
>
> Patrick Steinhardt (3):
> config: rename `git_etc_config()`
> config: unify code paths to get global config paths
> config: allow overriding of global and system configuration
>
> Documentation/git-config.txt | 5 +++
> Documentation/git.txt | 10 +++++
> builtin/config.c | 6 +--
> config.c | 41 +++++++++++++++------
> config.h | 4 +-
> t/t1300-config.sh | 71 ++++++++++++++++++++++++++++++++++++
> 6 files changed, 121 insertions(+), 16 deletions(-)
This round looks good to me. Sorry for suggesting the "/dev/null"
thing in the first place to lead you into wild goose chase during
the last round.
Will queue. Thanks for working on it.
And thanks for reviewing, everybody.
>
> Range-diff against v3:
> 1: 34bdbc27d6 = 1: 34bdbc27d6 config: rename `git_etc_config()`
> 2: 30f18679bd = 2: 30f18679bd config: unify code paths to get global config paths
> 3: af663640ae ! 3: d27efc0aa8 config: allow overriding of global and system configuration
> @@ Commit message
> - If unset, git continues to use the usual locations.
>
> - If set to a specific path, we skip reading the normal
> - configuration files and instead take the path.
> -
> - - If set to `/dev/null`, we do not load either global- or
> - system-level configuration at all.
> + configuration files and instead take the path. By setting the path
> + to `/dev/null`, no configuration will be loaded for the respective
> + level.
>
> This implements the usecase where we want to execute code in a sanitized
> environment without any potential misconfigurations via `/dev/null`, but
> @@ Documentation/git.txt: for further details.
> Whether to skip reading settings from the system-wide
> `$(prefix)/etc/gitconfig` file. This environment variable can
>
> - ## builtin/config.c ##
> -@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
> - char *user_config, *xdg_config;
> -
> - git_global_config(&user_config, &xdg_config);
> -- if (!user_config)
> -+ if (!user_config) {
> -+ if (getenv("GIT_CONFIG_GLOBAL"))
> -+ die(_("GIT_CONFIG_GLOBAL=/dev/null set"));
> -+
> - /*
> - * It is unknown if HOME/.gitconfig exists, so
> - * we do not know if we should write to XDG
> -@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
> - * is set and points at a sane location.
> - */
> - die(_("$HOME not set"));
> -+ }
> -
> - given_config_source.scope = CONFIG_SCOPE_GLOBAL;
> -
> -
> ## config.c ##
> @@ config.c: static int git_config_from_blob_ref(config_fn_t fn,
>
> char *git_system_config(void)
> {
> + char *system_config = xstrdup_or_null(getenv("GIT_CONFIG_SYSTEM"));
> -+ if (system_config) {
> -+ if (!strcmp(system_config, "/dev/null"))
> -+ FREE_AND_NULL(system_config);
> ++ if (system_config)
> + return system_config;
> -+ }
> return system_path(ETC_GITCONFIG);
> }
>
> @@ config.c: static int git_config_from_blob_ref(config_fn_t fn,
> + char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
> + char *xdg_config = NULL;
> +
> -+ if (user_config) {
> -+ if (!strcmp(user_config, "/dev/null"))
> -+ FREE_AND_NULL(user_config);
> -+ xdg_config = NULL;
> -+ } else {
> ++ if (!user_config) {
> + user_config = expand_user_path("~/.gitconfig", 0);
> + xdg_config = xdg_config_home("config");
> + }
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 0/3] config: allow overriding global/system config
2021-04-13 17:49 ` Junio C Hamano
@ 2021-04-14 5:37 ` Patrick Steinhardt
0 siblings, 0 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-14 5:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]
On Tue, Apr 13, 2021 at 10:49:40AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Compared to v3, I only dropped the special-casing of `/dev/null`. As
> > Junio rightly pointed out, the special-casing was incomplete and would
> > have required more work to do the right thing for all cases. It can
> > still be re-added at a later point if the usecase actually comes up.
> >
> > Patrick
> >
> > Patrick Steinhardt (3):
> > config: rename `git_etc_config()`
> > config: unify code paths to get global config paths
> > config: allow overriding of global and system configuration
> >
> > Documentation/git-config.txt | 5 +++
> > Documentation/git.txt | 10 +++++
> > builtin/config.c | 6 +--
> > config.c | 41 +++++++++++++++------
> > config.h | 4 +-
> > t/t1300-config.sh | 71 ++++++++++++++++++++++++++++++++++++
> > 6 files changed, 121 insertions(+), 16 deletions(-)
>
> This round looks good to me. Sorry for suggesting the "/dev/null"
> thing in the first place to lead you into wild goose chase during
> the last round.
>
> Will queue. Thanks for working on it.
>
> And thanks for reviewing, everybody.
No worries, and thanks a lot for all the feedback!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v5 0/3] config: allow overriding global/system config
2021-04-13 7:11 ` [PATCH v4 0/3] config: allow overriding global/system config Patrick Steinhardt
` (4 preceding siblings ...)
2021-04-13 17:49 ` Junio C Hamano
@ 2021-04-19 12:31 ` Patrick Steinhardt
2021-04-19 12:31 ` [PATCH v5 1/3] config: rename `git_etc_config()` Patrick Steinhardt
` (4 more replies)
5 siblings, 5 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-19 12:31 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine,
SZEDER Gábor
[-- Attachment #1: Type: text/plain, Size: 3933 bytes --]
Hi,
this is the fifth version of my patch series to provide a way of
overriding the global system configuration.
Changes to v4:
- Readded the call to `git_config_system()`, which I've previously
dropped by accident. I didn't move it into the new
`git_system_config()` function as it would change semantics of
`git config --system`.
- Added a testcase which verifies that GIT_CONFIG_NOSYSTEM and
GIT_CONFIG_SYSTEM properly interact with each other: if
GIT_CONFIG_NOSYSTEM is set, no system-level configuration shall be
read. This is different than the tests for `git config --system`
which used to and still does ignore GIT_CONFIG_NOSYSTEM.
- Small fixups for another testcase to drop needless redirects and
the `sane_unset` of GIT_CONFIG_NOSYSTEM.
Patrick
Patrick Steinhardt (3):
config: rename `git_etc_config()`
config: unify code paths to get global config paths
config: allow overriding of global and system configuration
Documentation/git-config.txt | 5 +++
Documentation/git.txt | 10 +++++
builtin/config.c | 6 +--
config.c | 41 ++++++++++++-----
config.h | 4 +-
t/t1300-config.sh | 86 ++++++++++++++++++++++++++++++++++++
6 files changed, 136 insertions(+), 16 deletions(-)
Range-diff against v4:
1: 34bdbc27d6 ! 1: 1e8899408a config: rename `git_etc_config()`
@@ config.c: static int do_git_config_sequence(const struct config_options *opts,
- ACCESS_EACCES_OK : 0))
- ret += git_config_from_file(fn, git_etc_gitconfig(),
- data);
-+ if (system_config && !access_or_die(system_config, R_OK,
-+ opts->system_gently ?
-+ ACCESS_EACCES_OK : 0))
++ if (git_config_system() && system_config &&
++ !access_or_die(system_config, R_OK,
++ opts->system_gently ? ACCESS_EACCES_OK : 0))
+ ret += git_config_from_file(fn, system_config, data);
current_parsing_scope = CONFIG_SCOPE_GLOBAL;
2: 30f18679bd = 2: 39468f45d2 config: unify code paths to get global config paths
3: d27efc0aa8 ! 3: 7e7506217e config: allow overriding of global and system configuration
@@ t/t1300-config.sh: test_expect_success '--show-scope with --show-origin' '
+'
+
+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 &&
++ test_must_fail env GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=/dev/null git config --global --list &&
++ test_must_fail env GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=does-not-exist git config --system --list &&
+ GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=does-not-exist git version
+'
+
++test_expect_success 'system override has no effect with GIT_CONFIG_NOSYSTEM' '
++ # `git config --system` has different semantics compared to other
++ # commands as it ignores GIT_CONFIG_NOSYSTEM. We thus test whether the
++ # variable has an effect via a different proxy.
++ cat >alias-config <<-EOF &&
++ [alias]
++ hello-world = !echo "hello world"
++ EOF
++ test_must_fail env GIT_CONFIG_NOSYSTEM=true GIT_CONFIG_SYSTEM=alias-config \
++ git hello-world &&
++ GIT_CONFIG_NOSYSTEM=false GIT_CONFIG_SYSTEM=alias-config \
++ git hello-world >actual &&
++ echo "hello world" >expect &&
++ test_cmp expect actual
++'
++
+test_expect_success 'write to overridden global and system config' '
+ cat >expect <<EOF &&
+[config]
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v5 1/3] config: rename `git_etc_config()`
2021-04-19 12:31 ` [PATCH v5 " Patrick Steinhardt
@ 2021-04-19 12:31 ` Patrick Steinhardt
2021-04-19 12:31 ` [PATCH v5 2/3] config: unify code paths to get global config paths Patrick Steinhardt
` (3 subsequent siblings)
4 siblings, 0 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-19 12:31 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine,
SZEDER Gábor
[-- Attachment #1: Type: text/plain, Size: 3948 bytes --]
The `git_etc_gitconfig()` function retrieves the system-level path of
the configuration file. We're about to introduce a way to override it
via an environment variable, at which point the name of this function
would start to become misleading.
Rename the function to `git_system_config()` as a preparatory step.
While at it, the function is also refactored to pass memory ownership to
the caller. This is done to better match semantics of
`git_global_config()`, which is going to be introduced in the next
commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/config.c | 2 +-
config.c | 18 ++++++++----------
config.h | 3 ++-
3 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index f71fa39b38..02ed0b3fe7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -695,7 +695,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
}
else if (use_system_config) {
- given_config_source.file = git_etc_gitconfig();
+ given_config_source.file = git_system_config();
given_config_source.scope = CONFIG_SCOPE_SYSTEM;
} else if (use_local_config) {
given_config_source.file = git_pathdup("config");
diff --git a/config.c b/config.c
index 870d9534de..e62960b5a6 100644
--- a/config.c
+++ b/config.c
@@ -1830,12 +1830,9 @@ static int git_config_from_blob_ref(config_fn_t fn,
return git_config_from_blob_oid(fn, name, &oid, data);
}
-const char *git_etc_gitconfig(void)
+char *git_system_config(void)
{
- static const char *system_wide;
- if (!system_wide)
- system_wide = system_path(ETC_GITCONFIG);
- return system_wide;
+ return system_path(ETC_GITCONFIG);
}
/*
@@ -1869,6 +1866,7 @@ static int do_git_config_sequence(const struct config_options *opts,
config_fn_t fn, void *data)
{
int ret = 0;
+ char *system_config = git_system_config();
char *xdg_config = xdg_config_home("config");
char *user_config = expand_user_path("~/.gitconfig", 0);
char *repo_config;
@@ -1882,11 +1880,10 @@ static int do_git_config_sequence(const struct config_options *opts,
repo_config = NULL;
current_parsing_scope = CONFIG_SCOPE_SYSTEM;
- if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
- opts->system_gently ?
- ACCESS_EACCES_OK : 0))
- ret += git_config_from_file(fn, git_etc_gitconfig(),
- data);
+ if (git_config_system() && system_config &&
+ !access_or_die(system_config, R_OK,
+ opts->system_gently ? ACCESS_EACCES_OK : 0))
+ ret += git_config_from_file(fn, system_config, data);
current_parsing_scope = CONFIG_SCOPE_GLOBAL;
if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
@@ -1913,6 +1910,7 @@ static int do_git_config_sequence(const struct config_options *opts,
die(_("unable to parse command-line config"));
current_parsing_scope = prev_parsing_scope;
+ free(system_config);
free(xdg_config);
free(user_config);
free(repo_config);
diff --git a/config.h b/config.h
index 19a9adbaa9..2be8fa1880 100644
--- a/config.h
+++ b/config.h
@@ -318,7 +318,6 @@ int git_config_rename_section(const char *, const char *);
int git_config_rename_section_in_file(const char *, const char *, const char *);
int git_config_copy_section(const char *, const char *);
int git_config_copy_section_in_file(const char *, const char *, const char *);
-const char *git_etc_gitconfig(void);
int git_env_bool(const char *, int);
unsigned long git_env_ulong(const char *, unsigned long);
int git_config_system(void);
@@ -327,6 +326,8 @@ int config_error_nonbool(const char *);
#define config_error_nonbool(s) (config_error_nonbool(s), const_error())
#endif
+char *git_system_config(void);
+
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
enum config_scope current_config_scope(void);
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v5 2/3] config: unify code paths to get global config paths
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 ` Patrick Steinhardt
2021-04-19 12:31 ` [PATCH v5 3/3] config: allow overriding of global and system configuration Patrick Steinhardt
` (2 subsequent siblings)
4 siblings, 0 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-19 12:31 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine,
SZEDER Gábor
[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]
There's two callsites which assemble global config paths, once in the
config loading code and once in the git-config(1) builtin. We're about
to implement a way to override global config paths via an environment
variable which would require us to adjust both sites.
Unify both code paths into a single `git_global_config()` function which
returns both paths for `~/.gitconfig` and the XDG config file. This will
make the subsequent patch which introduces the new envvar easier to
implement.
No functional changes are expected from this patch.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/config.c | 4 ++--
config.c | 12 ++++++++++--
config.h | 1 +
3 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 02ed0b3fe7..865fddd6ce 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -671,9 +671,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
if (use_global_config) {
- char *user_config = expand_user_path("~/.gitconfig", 0);
- char *xdg_config = xdg_config_home("config");
+ char *user_config, *xdg_config;
+ git_global_config(&user_config, &xdg_config);
if (!user_config)
/*
* It is unknown if HOME/.gitconfig exists, so
diff --git a/config.c b/config.c
index e62960b5a6..161dfaa707 100644
--- a/config.c
+++ b/config.c
@@ -1835,6 +1835,12 @@ char *git_system_config(void)
return system_path(ETC_GITCONFIG);
}
+void git_global_config(char **user_config, char **xdg_config)
+{
+ *user_config = expand_user_path("~/.gitconfig", 0);
+ *xdg_config = xdg_config_home("config");
+}
+
/*
* Parse environment variable 'k' as a boolean (in various
* possible spellings); if missing, use the default value 'def'.
@@ -1867,8 +1873,8 @@ static int do_git_config_sequence(const struct config_options *opts,
{
int ret = 0;
char *system_config = git_system_config();
- char *xdg_config = xdg_config_home("config");
- char *user_config = expand_user_path("~/.gitconfig", 0);
+ char *xdg_config = NULL;
+ char *user_config = NULL;
char *repo_config;
enum config_scope prev_parsing_scope = current_parsing_scope;
@@ -1886,6 +1892,8 @@ static int do_git_config_sequence(const struct config_options *opts,
ret += git_config_from_file(fn, system_config, data);
current_parsing_scope = CONFIG_SCOPE_GLOBAL;
+ git_global_config(&user_config, &xdg_config);
+
if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
ret += git_config_from_file(fn, xdg_config, data);
diff --git a/config.h b/config.h
index 2be8fa1880..9038538ffd 100644
--- a/config.h
+++ b/config.h
@@ -327,6 +327,7 @@ int config_error_nonbool(const char *);
#endif
char *git_system_config(void);
+void git_global_config(char **user, char **xdg);
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v5 3/3] config: allow overriding of global and system configuration
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 ` Patrick Steinhardt
2021-04-21 20:46 ` SZEDER Gábor
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
4 siblings, 1 reply; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-19 12:31 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine,
SZEDER Gábor
[-- Attachment #1: Type: text/plain, Size: 7326 bytes --]
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_NOGLOBAL`
as an equivalent to `GIT_CONFIG_NOSYSTEM`. But in the past, it has
turned out that this design is inflexible: we cannot test system-level
parsing of the git configuration in our test harness because there is no
way to change its location, so all tests run with `GIT_CONFIG_NOSYSTEM`
set.
Instead of doing the same mistake with `GIT_CONFIG_NOGLOBAL`, introduce
two new variables `GIT_CONFIG_GLOBAL` and `GIT_CONFIG_SYSTEM`:
- If unset, git continues to use the usual locations.
- If set to a specific path, we skip reading the normal
configuration files and instead take the path. By setting the path
to `/dev/null`, no configuration will be loaded for the respective
level.
This implements the usecase where we want to execute code in a sanitized
environment without any potential misconfigurations via `/dev/null`, but
is more flexible and allows for more usecases than simply adding
`GIT_CONFIG_NOGLOBAL`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-config.txt | 5 +++
Documentation/git.txt | 10 +++++
config.c | 17 +++++--
t/t1300-config.sh | 86 ++++++++++++++++++++++++++++++++++++
4 files changed, 115 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4b4cc5c5e8..5cddadafd2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -340,6 +340,11 @@ GIT_CONFIG::
Using the "--global" option forces this to ~/.gitconfig. Using the
"--system" option forces this to $(prefix)/etc/gitconfig.
+GIT_CONFIG_GLOBAL::
+GIT_CONFIG_SYSTEM::
+ Take the configuration from the given files instead from global or
+ system-level configuration. 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..380422a6a9 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. If `GIT_CONFIG_SYSTEM` is set, the
+ system config file defined at build time (usually `/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.
+
`GIT_CONFIG_NOSYSTEM`::
Whether to skip reading settings from the system-wide
`$(prefix)/etc/gitconfig` file. This environment variable can
diff --git a/config.c b/config.c
index 161dfaa707..f9c400ad30 100644
--- a/config.c
+++ b/config.c
@@ -1832,13 +1832,24 @@ static int git_config_from_blob_ref(config_fn_t fn,
char *git_system_config(void)
{
+ char *system_config = xstrdup_or_null(getenv("GIT_CONFIG_SYSTEM"));
+ if (system_config)
+ return system_config;
return system_path(ETC_GITCONFIG);
}
-void git_global_config(char **user_config, char **xdg_config)
+void git_global_config(char **user_out, char **xdg_out)
{
- *user_config = expand_user_path("~/.gitconfig", 0);
- *xdg_config = xdg_config_home("config");
+ char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
+ char *xdg_config = NULL;
+
+ if (!user_config) {
+ user_config = expand_user_path("~/.gitconfig", 0);
+ xdg_config = xdg_config_home("config");
+ }
+
+ *user_out = user_config;
+ *xdg_out = xdg_config;
}
/*
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e0dd5d65ce..0f92dfe6fb 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2059,6 +2059,92 @@ test_expect_success '--show-scope with --show-origin' '
test_cmp expect output
'
+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
+
+ 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
+'
+
+test_expect_success 'override global and system config with missing file' '
+ test_must_fail env GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=/dev/null git config --global --list &&
+ test_must_fail env GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=does-not-exist git config --system --list &&
+ GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=does-not-exist git version
+'
+
+test_expect_success 'system override has no effect with GIT_CONFIG_NOSYSTEM' '
+ # `git config --system` has different semantics compared to other
+ # commands as it ignores GIT_CONFIG_NOSYSTEM. We thus test whether the
+ # variable has an effect via a different proxy.
+ cat >alias-config <<-EOF &&
+ [alias]
+ hello-world = !echo "hello world"
+ EOF
+ test_must_fail env GIT_CONFIG_NOSYSTEM=true GIT_CONFIG_SYSTEM=alias-config \
+ git hello-world &&
+ GIT_CONFIG_NOSYSTEM=false GIT_CONFIG_SYSTEM=alias-config \
+ git hello-world >actual &&
+ echo "hello world" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'write to overridden global and system config' '
+ cat >expect <<EOF &&
+[config]
+ key = value
+EOF
+
+ GIT_CONFIG_GLOBAL=write-to-global git config --global config.key value &&
+ test_cmp expect write-to-global &&
+
+ GIT_CONFIG_SYSTEM=write-to-system git config --system config.key value &&
+ test_cmp expect write-to-system
+'
+
for opt in --local --worktree
do
test_expect_success "$opt requires a repo" '
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v5 3/3] config: allow overriding of global and system configuration
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-23 5:47 ` [PATCH] t1300: fix unset of GIT_CONFIG_NOSYSTEM leaking into subsequent tests Patrick Steinhardt
0 siblings, 2 replies; 52+ messages in thread
From: SZEDER Gábor @ 2021-04-21 20:46 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
On Mon, Apr 19, 2021 at 02:31:16PM +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_NOGLOBAL`
> as an equivalent to `GIT_CONFIG_NOSYSTEM`. But in the past, it has
> turned out that this design is inflexible: we cannot test system-level
> parsing of the git configuration in our test harness because there is no
> way to change its location, so all tests run with `GIT_CONFIG_NOSYSTEM`
> set.
>
> Instead of doing the same mistake with `GIT_CONFIG_NOGLOBAL`, introduce
> two new variables `GIT_CONFIG_GLOBAL` and `GIT_CONFIG_SYSTEM`:
>
> - If unset, git continues to use the usual locations.
>
> - If set to a specific path, we skip reading the normal
> configuration files and instead take the path. By setting the path
> to `/dev/null`, no configuration will be loaded for the respective
> level.
>
> This implements the usecase where we want to execute code in a sanitized
> environment without any potential misconfigurations via `/dev/null`, but
> is more flexible and allows for more usecases than simply adding
> `GIT_CONFIG_NOGLOBAL`.
Something is still not right with this patch series, because:
> +test_expect_success 'write to overridden global and system config' '
> + cat >expect <<EOF &&
> +[config]
> + key = value
> +EOF
> +
> + GIT_CONFIG_GLOBAL=write-to-global git config --global config.key value &&
> + test_cmp expect write-to-global &&
> +
> + GIT_CONFIG_SYSTEM=write-to-system git config --system config.key value &&
> + test_cmp expect write-to-system
> +'
This test fails on Travis CI's Linux32 job:
expecting success of 1300.184 'write to overridden global and system config':
cat >expect <<EOF &&
[config]
key = value
EOF
GIT_CONFIG_GLOBAL=write-to-global git config --global config.key value &&
test_cmp expect write-to-global &&
GIT_CONFIG_SYSTEM=write-to-system git config --system config.key value &&
test_cmp expect write-to-system
+ cat
+ GIT_CONFIG_GLOBAL=write-to-global git config --global config.key value
fatal: unable to access '/root/etc/gitconfig': Permission denied
error: last command exited with $?=128
not ok 184 - write to overridden global and system config
Yeah, that job has a weird environment with Docker and 'su'
interacting in a way that ultimately builds Git with 'HOME=/root',
which in our build system means that 'sysconfdir=/root/etc'. To
reproduce at home just run:
make prefix=/root && cd t && ./t1300-config.sh -V -x -i
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v5 3/3] config: allow overriding of global and system configuration
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
1 sibling, 1 reply; 52+ messages in thread
From: SZEDER Gábor @ 2021-04-21 21:06 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
On Wed, Apr 21, 2021 at 10:46:37PM +0200, SZEDER Gábor wrote:
> On Mon, Apr 19, 2021 at 02:31:16PM +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_NOGLOBAL`
> > as an equivalent to `GIT_CONFIG_NOSYSTEM`. But in the past, it has
> > turned out that this design is inflexible: we cannot test system-level
> > parsing of the git configuration in our test harness because there is no
> > way to change its location, so all tests run with `GIT_CONFIG_NOSYSTEM`
> > set.
> >
> > Instead of doing the same mistake with `GIT_CONFIG_NOGLOBAL`, introduce
> > two new variables `GIT_CONFIG_GLOBAL` and `GIT_CONFIG_SYSTEM`:
> >
> > - If unset, git continues to use the usual locations.
> >
> > - If set to a specific path, we skip reading the normal
> > configuration files and instead take the path. By setting the path
> > to `/dev/null`, no configuration will be loaded for the respective
> > level.
> >
> > This implements the usecase where we want to execute code in a sanitized
> > environment without any potential misconfigurations via `/dev/null`, but
> > is more flexible and allows for more usecases than simply adding
> > `GIT_CONFIG_NOGLOBAL`.
>
> Something is still not right with this patch series, because:
>
> > +test_expect_success 'write to overridden global and system config' '
> > + cat >expect <<EOF &&
> > +[config]
> > + key = value
> > +EOF
> > +
> > + GIT_CONFIG_GLOBAL=write-to-global git config --global config.key value &&
> > + test_cmp expect write-to-global &&
> > +
> > + GIT_CONFIG_SYSTEM=write-to-system git config --system config.key value &&
> > + test_cmp expect write-to-system
> > +'
>
> This test fails on Travis CI's Linux32 job:
>
> expecting success of 1300.184 'write to overridden global and system config':
> cat >expect <<EOF &&
> [config]
> key = value
> EOF
> GIT_CONFIG_GLOBAL=write-to-global git config --global config.key value &&
> test_cmp expect write-to-global &&
> GIT_CONFIG_SYSTEM=write-to-system git config --system config.key value &&
> test_cmp expect write-to-system
> + cat
> + GIT_CONFIG_GLOBAL=write-to-global git config --global config.key value
> fatal: unable to access '/root/etc/gitconfig': Permission denied
> error: last command exited with $?=128
> not ok 184 - write to overridden global and system config
https://travis-ci.org/github/git/git/jobs/767898817#L6931
> Yeah, that job has a weird environment with Docker and 'su'
> interacting in a way that ultimately builds Git with 'HOME=/root',
> which in our build system means that 'sysconfdir=/root/etc'. To
> reproduce at home just run:
>
> make prefix=/root && cd t && ./t1300-config.sh -V -x -i
Hrm, that's not the only test that fails, but I only ran it with
'-i'... but in fact most subsequent tests fail with the same error.
I think the culprit is the previous test case which I too eagerly
snipped from my previous email, so here it is again (copy-pasted,
whitespace-damaged):
> 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
>
> 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 &&
Unsetting GIT_CONFIG_NOSYSTEM like this does affect the environment of
all subsequent tests and their git commands will then try to look at
the system config file.
Putting this 'sane_unset' and the rest of this test case in a subshell
seems to fix the issue.
> 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
> '
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v5 3/3] config: allow overriding of global and system configuration
2021-04-21 21:06 ` SZEDER Gábor
@ 2021-04-22 5:36 ` Patrick Steinhardt
0 siblings, 0 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-22 5:36 UTC (permalink / raw)
To: SZEDER Gábor
Cc: git, Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]
On Wed, Apr 21, 2021 at 11:06:14PM +0200, SZEDER Gábor wrote:
> On Wed, Apr 21, 2021 at 10:46:37PM +0200, SZEDER Gábor wrote:
> > On Mon, Apr 19, 2021 at 02:31:16PM +0200, Patrick Steinhardt wrote:
[snip]
> > 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
> >
> > 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 &&
>
> Unsetting GIT_CONFIG_NOSYSTEM like this does affect the environment of
> all subsequent tests and their git commands will then try to look at
> the system config file.
>
> Putting this 'sane_unset' and the rest of this test case in a subshell
> seems to fix the issue.
Ah, that makes sense. Thanks for digging into the issue, I'll send a fix
for this later today.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] t1300: fix unset of GIT_CONFIG_NOSYSTEM leaking into subsequent tests
2021-04-21 20:46 ` SZEDER Gábor
2021-04-21 21:06 ` SZEDER Gábor
@ 2021-04-23 5:47 ` Patrick Steinhardt
1 sibling, 0 replies; 52+ messages in thread
From: Patrick Steinhardt @ 2021-04-23 5:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 1921 bytes --]
In order to test whether the new GIT_CONFIG_SYSTEM environment variable
behaves as expected, we unset GIT_CONFIG_NOSYSTEM in one of our tests in
t1300. But because tests are not executed in a subshell, this unset
leaks into all subsequent tests and may thus cause them to fail in some
environments. These failures are easily reproducable with `make
prefix=/root test`.
Fix the issue by not using `sane_unset GIT_CONFIG_NOSYSTEM`, but instead
just manually add it to the environment of the two command invocations
which need it.
Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
This patch applies on top of 47e6f16901 (Sync with master, 2021-04-20),
which is the tip of next at the time of writing.
t/t1300-config.sh | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 0f92dfe6fb..ec599baeba 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2092,21 +2092,20 @@ test_expect_success 'override global and system config' '
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_NOSYSTEM=false 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 &&
+ GIT_CONFIG_NOSYSTEM=false GIT_CONFIG_SYSTEM=/dev/null GIT_CONFIG_GLOBAL=/dev/null \
+ git config --show-scope --list >output &&
test_cmp expect output
'
--
2.31.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v5 0/3] config: allow overriding global/system config
2021-04-19 12:31 ` [PATCH v5 " Patrick Steinhardt
` (2 preceding siblings ...)
2021-04-19 12:31 ` [PATCH v5 3/3] config: allow overriding of global and system configuration Patrick Steinhardt
@ 2021-04-19 21:55 ` Junio C Hamano
2021-04-23 9:32 ` Jeff King
4 siblings, 0 replies; 52+ messages in thread
From: Junio C Hamano @ 2021-04-19 21:55 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Eric Sunshine, SZEDER Gábor
Patrick Steinhardt <ps@pks.im> writes:
> this is the fifth version of my patch series to provide a way of
> overriding the global system configuration.
Hmm, this topic has been in next since April 15th. It is
preferrable to fix things with incremental updates (the reasoning
being that while in 'seen' the issues are "mistakes by the author
alone", which may not necessarily benefit others to learn from, but
what is in 'next' are supposed to have got enough review exposure,
so any mistakes found are both of the author and reviewers, i.e.
trickier than what is found before it hits 'next' and are more worth
documenting with incremental "oops that was wrong for such and such
reason, which even our reviews missed, and here is a fix").
But I'll have to go offline for a while soon, so let's make an
exception, revert the merge of the previous round out of 'next', and
queue this in 'seen'.
Thanks.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v5 0/3] config: allow overriding global/system config
2021-04-19 12:31 ` [PATCH v5 " Patrick Steinhardt
` (3 preceding siblings ...)
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
4 siblings, 0 replies; 52+ messages in thread
From: Jeff King @ 2021-04-23 9:32 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Eric Sunshine, SZEDER Gábor
On Mon, Apr 19, 2021 at 02:31:04PM +0200, Patrick Steinhardt wrote:
> this is the fifth version of my patch series to provide a way of
> overriding the global system configuration.
>
> Changes to v4:
>
> - Readded the call to `git_config_system()`, which I've previously
> dropped by accident. I didn't move it into the new
> `git_system_config()` function as it would change semantics of
> `git config --system`.
>
> - Added a testcase which verifies that GIT_CONFIG_NOSYSTEM and
> GIT_CONFIG_SYSTEM properly interact with each other: if
> GIT_CONFIG_NOSYSTEM is set, no system-level configuration shall be
> read. This is different than the tests for `git config --system`
> which used to and still does ignore GIT_CONFIG_NOSYSTEM.
>
> - Small fixups for another testcase to drop needless redirects and
> the `sane_unset` of GIT_CONFIG_NOSYSTEM.
With the extra fixup to avoid unsetting GIT_CONFIG_NOSYSTEM for the
whole test suite, this looks good to me.
-Peff
^ permalink raw reply [flat|nested] 52+ messages in thread