[PATCH/RFC 0/4] config: read both home and xdg files for --global

[PATCH/RFC 0/4] config: read both home and xdg files for --global

[Delilah Ashley Wu via GitGitGadget]

Hi!

As reported in [1]: `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config` are
both valid global config locations, but `git config list --global` only
includes the former in its output.

Suppose we have this config in `$HOME/.gitconfig`:

[home]
    config = true


And this config in `$XDG_CONFIG_HOME/git/config`:

[xdg]
    config = true


Then, to reproduce the issue that `--global` only shows the home config:

$ git config list --global --show-scope --show-origin
global  file:/Users/delilah/.gitconfig    home.config=true


Git correctly applies the XDG config in its effective configuration, but it
doesn't show up when `--global` is specified. We can confirm this by
checking the output without the `--global` flag:

$ git config list --show-scope --show-origin
global  file:/Users/delilah/.config/git/config    xdg.config=true
global  file:/Users/delilah/.gitconfig            home.config=true


The expected behaviour is both configs should be shown when `--global` is
specified, so we'd expect its output to look the same as above. This was
confirmed in [2], which quoted the `git config` documentation:

> OPTIONS
>     --global::
>         For writing options: write to global `~/.gitconfig` file
>         rather than the repository `.git/config`, write to
>         `$XDG_CONFIG_HOME/git/config` file if this file exists and the
>         `~/.gitconfig` file doesn't.
>
>         For reading options: read only from global `~/.gitconfig` and from
>         `$XDG_CONFIG_HOME/git/config` rather than from all available files.


The first patch fixes forward slash normalisation on Windows paths. The
second patch introduces tests and regression checks. The third and fourth
patches implement the fix to include both config files when `--global` is
specified. Johannes has kindly pre-reviewed this patch series via GitHub on
GitGitGadget #1938 [3]. You'll notice some force-pushes after the review,
but I only changed commit messages.

[1]:
https://lore.kernel.org/git/CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/xmqqmt5lezi3.fsf@gitster.g/
[3]: https://github.com/gitgitgadget/git/pull/1938/

Thank you all for your time!
Delilah

Delilah Ashley Wu (4):
  cleanup_path: force forward slashes on Windows
  config: test home and xdg files in `list --global`
  config: read global scope via config_sequence
  config: keep bailing on unreadable global files

 builtin/config.c     | 12 ++++++++
 config.c             | 54 ++++++++++++++++++++++++++----------
 config.h             |  2 ++
 path.c               | 10 +++++--
 t/t1300-config.sh    | 65 ++++++++++++++++++++++++++++++++++++++++++++
 t/t1306-xdg-files.sh |  3 +-
 6 files changed, 128 insertions(+), 18 deletions(-)


base-commit: ca2559c1d630eb4f04cdee2328aaf1c768907a9e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1938%2Fdelilahw%2Flilah%2Ffix-config-list-global-home-and-xdg%2Fpatchset-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1938/delilahw/lilah/fix-config-list-global-home-and-xdg/patchset-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1938
-- 
gitgitgadget

[PATCH/RFC 1/4] cleanup_path: force forward slashes on Windows

[Delilah Ashley Wu via GitGitGadget]

From: Delilah Ashley Wu <delilahwu@microsoft.com>

Git prefers forward slashes as directory separators across all
platforms. On Windows, the backslash is the native directory separator,
but all Windows versions supported by Git also accept the forward slash
in all but rare circumstances. Our tests expect forward slashes. Git
generates relative paths with forward slashes. Forward slashes are more
convenient to use in shell scripts.

For these reasons, we enforced forward slashes in `interpolate_path()`
in 5ca6b7bb47b (config --show-origin: report paths with forward slashes,
2016-03-23). However, other code paths may generate paths containing
backslashes. For example, `config --show-origin` prints the XDG config
path with mixed slashes on Windows:

$ git config --list --show-origin
file:C:/Program Files/Git/etc/gitconfig         system.foo=bar
file:"C:\\Users\\delilah/.config/git/config"    xdg.foo=bar
file:C:/Users/delilah/.gitconfig                home.foo=bar
file:.git/config                                local.foo=bar

Let's enforce forward slashes in all code paths that directly or
indirectly call `cleanup_path()` by modifying it to use
`convert_slashes()` on Windows. Since `convert_slashes()` modifies the
path in-place, change the argument and return type of `cleanup_path()`
from `const char *` to `char *`. All existing callers of
`cleanup_path()` pass `char *` anyways, so this change is compatible.

The next patch, config: test home and xdg files in `list --global`, will
assert that the XDG config path uses forward slashes.

Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Delilah Ashley Wu <delilahwu@microsoft.com>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 path.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/path.c b/path.c
index 7f56eaf993..db7b94fcda 100644
--- a/path.c
+++ b/path.c
@@ -40,13 +40,17 @@ static struct strbuf *get_pathname(void)
 	return sb;
 }
 
-static const char *cleanup_path(const char *path)
+static char *cleanup_path(char *path)
 {
 	/* Clean it up */
-	if (skip_prefix(path, "./", &path)) {
+	if (skip_prefix(path, "./", (const char **)&path))
 		while (*path == '/')
 			path++;
-	}
+
+#ifdef GIT_WINDOWS_NATIVE
+	convert_slashes(path);
+#endif
+
 	return path;
 }
 
-- 
gitgitgadget

Re: [PATCH/RFC 1/4] cleanup_path: force forward slashes on Windows

[Junio C Hamano]

"Delilah Ashley Wu via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> All existing callers of
> `cleanup_path()` pass `char *` anyways, so this change is compatible.

Not just compatible ;-).  If there is a caller that wants
cleanup_path() not to munge what it passes, this change will
introduce a bug for them.  Have you made sure that none of these
callers mind that backslashes are converted into forward slashes?

> The next patch, config: test home and xdg files in `list --global`, will
> assert that the XDG config path uses forward slashes.

The path to the leaf-level blobs is always slash separated in the
index, a tree object sorts an entry that points at a subtree as if
its path component has terminating slash, etc., and only when these
paths are externalized, they are converted to filesystem dependent
hierarchy separator (by system call like creat(2) even on platforms
like Windows whose filesystem uses backslashes as the pathname
separator).  Canonicalizing end-user supplied path early at a
central place does make sense.

> -static const char *cleanup_path(const char *path)
> +static char *cleanup_path(char *path)
>  {
>  	/* Clean it up */
> -	if (skip_prefix(path, "./", &path)) {
> +	if (skip_prefix(path, "./", (const char **)&path))
>  		while (*path == '/')
>  			path++;
> -	}

Hmph, the need for cast is a bit annoying, but more importantly, why
don't we have to worry about leading ".\\\\" instead of ".////"?
Shouldn't we be stripping backslashes the same way on Windows?

> +#ifdef GIT_WINDOWS_NATIVE
> +	convert_slashes(path);
> +#endif

In other words, why do it here, not _before_ the loop that says "If
the path begins with dot (i.e. the thing is relative to the current
directory) followed by a directory separator, remove it together
with any extra directory separators that come immediately after it"?

>  	return path;
>  }

[PATCH/RFC 2/4] config: test home and xdg files in `list --global`

[Delilah Ashley Wu via GitGitGadget]

From: Delilah Ashley Wu <delilahwu@microsoft.com>

The `git config list --global` output includes `$HOME/.gitconfig` (home
config), but ignores `$XDG_CONFIG_HOME/git/config` (XDG config). It
should include both files.

Modify tests to check the following and expect a failure:
  - `git config list --global` should include contents from both the
     home and XDG config locations (assuming they are readable), not
     just the former.

  - `--show-origin` should print correct paths to both config files,
    assuming they exist.

Also, add tests to ensure subsequent patches do not introduce
regressions to `git config list`. Specifically, check that:
  - The home config should take precedence over the XDG config.

  - Without `--global`, it should not bail on unreadable/non-existent
    global config files.

  - With `--global`, it should bail when both `$HOME/.gitconfig` and
    `$XDG_CONFIG_HOME/git/config` are unreadable. It should not bail if
    at least one of them is readable.

The next patch, config: read global scope via config_sequence, will
implement a fix to include both config files when `--global` is
specified.

Reported-by: Jade Lovelace <lists@jade.fyi>
Helped-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Delilah Ashley Wu <delilahwu@microsoft.com>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1300-config.sh    | 65 ++++++++++++++++++++++++++++++++++++++++++++
 t/t1306-xdg-files.sh |  5 ++--
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index f856821839..5fa0111bd9 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2367,6 +2367,71 @@ test_expect_success '--show-scope with --default' '
 	test_cmp expect actual
 '
 
+test_expect_success 'list with nonexistent global config' '
+	rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config &&
+	git config ${mode_prefix}list --show-scope
+'
+
+test_expect_success 'list --global with nonexistent global config' '
+	rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config &&
+	test_must_fail git config ${mode_prefix}list --global --show-scope
+'
+
+test_expect_success 'list --global with only home' '
+	rm -rf "$HOME"/.config/git/config &&
+
+	test_when_finished rm -f \"\$HOME\"/.gitconfig &&
+	cat >"$HOME"/.gitconfig <<-EOF &&
+	[home]
+		config = true
+	EOF
+
+	cat >expect <<-EOF &&
+	global	home.config=true
+	EOF
+	git config ${mode_prefix}list --global --show-scope >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'list --global with only xdg' '
+	rm -f "$HOME"/.gitconfig &&
+
+	test_when_finished rm -rf \"\$HOME\"/.config/git &&
+	mkdir -p "$HOME"/.config/git &&
+	cat >"$HOME"/.config/git/config <<-EOF &&
+	[xdg]
+		config = true
+	EOF
+
+	cat >expect <<-EOF &&
+	global	xdg.config=true
+	EOF
+	git config ${mode_prefix}list --global --show-scope >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'list --global with both home and xdg' '
+	test_when_finished rm -f \"\$HOME\"/.gitconfig &&
+	cat >"$HOME"/.gitconfig <<-EOF &&
+	[home]
+		config = true
+	EOF
+
+	test_when_finished rm -rf \"\$HOME\"/.config/git &&
+	mkdir -p "$HOME"/.config/git &&
+	cat >"$HOME"/.config/git/config <<-EOF &&
+	[xdg]
+		config = true
+	EOF
+
+	cat >expect <<-EOF &&
+	global	file:$HOME/.config/git/config	xdg.config=true
+	global	file:$HOME/.gitconfig	home.config=true
+	EOF
+	git config ${mode_prefix}list --global --show-scope --show-origin >output &&
+	! test_cmp expect output
+'
+
 test_expect_success 'override global and system config' '
 	test_when_finished rm -f \"\$HOME\"/.gitconfig &&
 	cat >"$HOME"/.gitconfig <<-EOF &&
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 40d3c42618..0318755799 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -68,9 +68,10 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists'
 	>.gitconfig &&
 	echo "[user]" >.gitconfig &&
 	echo "	name = read_gitconfig" >>.gitconfig &&
-	echo user.name=read_gitconfig >expected &&
+	echo user.name=read_config >expected &&
+	echo user.name=read_gitconfig >>expected &&
 	git config --global --list >actual &&
-	test_cmp expected actual
+	! test_cmp expected actual
 '
 
 
-- 
gitgitgadget

Re: [PATCH/RFC 2/4] config: test home and xdg files in `list --global`

[Junio C Hamano]

"Delilah Ashley Wu via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Delilah Ashley Wu <delilahwu@microsoft.com>
>
> The `git config list --global` output includes `$HOME/.gitconfig` (home
> config), but ignores `$XDG_CONFIG_HOME/git/config` (XDG config). It
> should include both files.

Please be gentle to future readers of "git log" and help them with a
bit more explanation on the "should" here.  E.g., 

    should include both files, to be consistent with the output from
    `git config list` (not limited to `--global`) that lists entries
    from both files (in addition to system-wide and repository-specific
    entries, of course).

or something.

> Modify tests to check the following and expect a failure:
>   - `git config list --global` should include contents from both the
>      home and XDG config locations (assuming they are readable), not
>      just the former.
>
>   - `--show-origin` should print correct paths to both config files,
>     assuming they exist.

Testing these two combinations is a good thing, but "expect a
failure"?  There doesn't seem to be any test that is marked as
"test_expect_failure" in this patch.  Confused?

    Side note: we generally do not want test_expect_failure tests in
    one patch, followed by a code fix with changes to tests that
    flip s/test_expect_failure/test_expect_success/' in another
    patch, though.  The reason is primarily that such a two-patch
    series makes it harder to review the step that has the fix, by
    hiding the body of the test whose earlier failure gets fixed by
    the code change.

> Also, add tests to ensure subsequent patches do not introduce
> regressions to `git config list`. Specifically, check that:
>   - The home config should take precedence over the XDG config.
>
>   - Without `--global`, it should not bail on unreadable/non-existent
>     global config files.
>
>   - With `--global`, it should bail when both `$HOME/.gitconfig` and
>     `$XDG_CONFIG_HOME/git/config` are unreadable. It should not bail if
>     at least one of them is readable.

Good.

> The next patch, config: read global scope via config_sequence, will
> implement a fix to include both config files when `--global` is
> specified.
>
> Reported-by: Jade Lovelace <lists@jade.fyi>
> Helped-by: Derrick Stolee <stolee@gmail.com>
> Signed-off-by: Delilah Ashley Wu <delilahwu@microsoft.com>
> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t1300-config.sh    | 65 ++++++++++++++++++++++++++++++++++++++++++++
>  t/t1306-xdg-files.sh |  5 ++--
>  2 files changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index f856821839..5fa0111bd9 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -2367,6 +2367,71 @@ test_expect_success '--show-scope with --default' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'list with nonexistent global config' '
> +	rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config &&
> +	git config ${mode_prefix}list --show-scope
> +'

Do we expect an empty output, or are we happy as long as "git
config" does not segfault, even if it spews anything?  I guess that
at this late point in the test we have per-repository or system-wide
configuration files with something in them to test, so there would
be some output but we do not care?  If that is the case, not
checking the output, like this patch does, is the right thing.

> +test_expect_success 'list --global with nonexistent global config' '
> +	rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config &&
> +	test_must_fail git config ${mode_prefix}list --global --show-scope
> +'

OK.  Do we require --show-scope to fail this, or do we fail with and
without --show-scope as long as --global is in effect?  If the latter,
test both ...

	rm -f "$HOME/.gitconfig" "$HOME/.config/git/config" &&
	test_must_fail git config ${mode_prefix}list --global &&
	test_must_fail git config ${mode_prefix}list --global --show-scope

... like this, perhaps?  Also, don't overuse '-r' with 'rm' (applies
other tests in this patch) when you know what you are removing
should not be a directory.

> +test_expect_success 'list --global with only home' '
> +	rm -rf "$HOME"/.config/git/config &&

Lose "r" from "-rf" or lose "/config".

> +	test_when_finished rm -f \"\$HOME\"/.gitconfig &&
> +	cat >"$HOME"/.gitconfig <<-EOF &&
> +	[home]
> +		config = true
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	global	home.config=true
> +	EOF
> +	git config ${mode_prefix}list --global --show-scope >output &&
> +	test_cmp expect output
> +'

OK.

> +test_expect_success 'list --global with only xdg' '
> +	rm -f "$HOME"/.gitconfig &&
> +
> +	test_when_finished rm -rf \"\$HOME\"/.config/git &&
> +	mkdir -p "$HOME"/.config/git &&
> +	cat >"$HOME"/.config/git/config <<-EOF &&
> +	[xdg]
> +		config = true
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	global	xdg.config=true
> +	EOF
> +	git config ${mode_prefix}list --global --show-scope >output &&
> +	test_cmp expect output
> +'

OK.

> +test_expect_success 'list --global with both home and xdg' '
> +	test_when_finished rm -f \"\$HOME\"/.gitconfig &&
> +	cat >"$HOME"/.gitconfig <<-EOF &&
> +	[home]
> +		config = true
> +	EOF
> +
> +	test_when_finished rm -rf \"\$HOME\"/.config/git &&
> +	mkdir -p "$HOME"/.config/git &&
> +	cat >"$HOME"/.config/git/config <<-EOF &&
> +	[xdg]
> +		config = true
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	global	file:$HOME/.config/git/config	xdg.config=true
> +	global	file:$HOME/.gitconfig	home.config=true
> +	EOF
> +	git config ${mode_prefix}list --global --show-scope --show-origin >output &&
> +	! test_cmp expect output
> +'

Do not write a test this way.  If you want to document an existing
and unfixed breakage, instead of saying "we do want to see what is
in this expect file, but we know output does not unfortunately match
it", which is how the above test expresses it, start the whole thing
with "test_expect_failure" (instead of "test_expect_success"), and
have the body of the test express what you really want to see.  I.e.
the last steps should say

	git config ${mode_prefix}list --global --show-scope --show-origin >actual &&
	test_cmp expect actual

But an earier side note applies.  If "git config list --global" gets
corrected, this test will see update to turn "! test_cmp" into
"test_cmp" (or "test_expect_success" to "test_expect_failure"), and
such a patch that comes with the code fix will not show what is
being tested and forcing the reviewer to go back to the previous
step to see what the change is really about.  A test that
demonstrates and protects the behaviour corrected by the code change
is best added in the same patch as the code change.

> diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
> index 40d3c42618..0318755799 100755
> --- a/t/t1306-xdg-files.sh
> +++ b/t/t1306-xdg-files.sh
> @@ -68,9 +68,10 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists'
>  	>.gitconfig &&
>  	echo "[user]" >.gitconfig &&
>  	echo "	name = read_gitconfig" >>.gitconfig &&
> -	echo user.name=read_gitconfig >expected &&
> +	echo user.name=read_config >expected &&
> +	echo user.name=read_gitconfig >>expected &&
>  	git config --global --list >actual &&
> -	test_cmp expected actual
> +	! test_cmp expected actual
>  '

I cannot quite tell from only half the test, but I suspect that this
shares exactly the same problem with the last one in the other file
I commented above?

Thanks.

[PATCH/RFC 3/4] config: read global scope via config_sequence

[Delilah Ashley Wu via GitGitGadget]

From: Delilah Ashley Wu <delilahwu@microsoft.com>

The output of `git config list --global` should include both the home
(`$HOME/.gitconfig`) and XDG (`$XDG_CONFIG_HOME/git/config`) configs,
but it only reads from the former.

We assumed each config scope corresponds to a single config file. Under
this assumption, `git config list --global` reads the global config by
calling `git_config_from_file_with_options(...,"~/.gitconfig", ...)`.
This function usage restricts us to a single config file. Because the
global scope includes two files, we should read the configs via another
method.

The output of `git config list --show-scope --show-origin` (without
`--global`) correctly includes both the home and XDG config files. So
there's existing code that respects both locations, namely the
`do_git_config_sequence()` function which reads from all scopes.
Introduce flags to make it possible to ignore all but the global scope
(i.e. ignore system, local, worktree, and cmdline). Then, reuse the
function to read only the global scope when `--global` is specified.
This was the suggested solution in the bug report:
https://lore.kernel.org/git/kl6ly1oze7wb.fsf@chooglen-macbookpro.roam.corp.google.com.

Then, modify the tests to check that `git config list --global` includes
both home and XDG configs.

This patch introduces a regression. If both global config files are
unreadable, then `git config list --global` should exit non-zero. This
is no longer the case, so mark the corresponding test as a "TODO known
breakage" and address the issue in the next patch, config: keep bailing
on unreadable global files.

Implementation notes:
  1. The `ignore_global` flag is not set anywhere, so the
     `if (!opts->ignore_global)` condition is always met. We can remove
     this flag if desired.

  2. I've assumed that `config_source->scope == CONFIG_SCOPE_GLOBAL` iff
     `--global` is specified. This comparison determines whether to call
     `do_git_config_sequence()` for the global scope, or to keep calling
     `git_config_from_file_with_options()` for other scopes.

  3. Keep populating `opts->source.file` in `builtin/config.c` because
     it is used as the destination config file for write operations.
     The proposed changes could convolute the code because there is no
     single source of truth for the config file locations in the global
     scope. Add a comment to help clarify this. Please let me know if
     it's unclear.

Reported-by: Jade Lovelace <lists@jade.fyi>
Suggested-by: Glen Choo <glencbz@gmail.com>
Helped-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Delilah Ashley Wu <delilahwu@microsoft.com>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/config.c     | 12 ++++++++++++
 config.c             | 26 +++++++++++++++-----------
 config.h             |  2 ++
 t/t1300-config.sh    |  6 +++---
 t/t1306-xdg-files.sh |  2 +-
 5 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 59fb113b07..3fd1bd7f8d 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -768,6 +768,18 @@ static void location_options_init(struct config_location_options *opts,
 	}
 
 	if (opts->use_global_config) {
+		/*
+		 * Since global config is sourced from more than one location,
+		 * use `config.c#do_git_config_sequence()` with `opts->options`
+		 * to read it. However, writing global config should point to a
+		 * single destination, set in `opts->source.file`.
+		 */
+		opts->options.ignore_repo = 1;
+		opts->options.ignore_cmdline= 1;
+		opts->options.ignore_worktree = 1;
+		opts->options.ignore_system = 1;
+		opts->source.scope = CONFIG_SCOPE_GLOBAL;
+
 		opts->source.file = opts->file_to_free = git_global_config();
 		if (!opts->source.file)
 			/*
diff --git a/config.c b/config.c
index 74bf76a97e..4b9f3831b1 100644
--- a/config.c
+++ b/config.c
@@ -1526,22 +1526,27 @@ static int do_git_config_sequence(const struct config_options *opts,
 		worktree_config = NULL;
 	}
 
-	if (git_config_system() && system_config &&
+	if (!opts->ignore_system && git_config_system() && system_config &&
 	    !access_or_die(system_config, R_OK,
 			   opts->system_gently ? ACCESS_EACCES_OK : 0))
 		ret += git_config_from_file_with_options(fn, system_config,
 							 data, CONFIG_SCOPE_SYSTEM,
 							 NULL);
 
-	git_global_config_paths(&user_config, &xdg_config);
+	if (!opts->ignore_global) {
+		git_global_config_paths(&user_config, &xdg_config);
+
+		if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
+			ret += git_config_from_file_with_options(fn, xdg_config, data,
+						CONFIG_SCOPE_GLOBAL, NULL);
 
-	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
-		ret += git_config_from_file_with_options(fn, xdg_config, data,
-							 CONFIG_SCOPE_GLOBAL, NULL);
+		if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
+			ret += git_config_from_file_with_options(fn, user_config, data,
+						CONFIG_SCOPE_GLOBAL, NULL);
 
-	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
-		ret += git_config_from_file_with_options(fn, user_config, data,
-							 CONFIG_SCOPE_GLOBAL, NULL);
+		free(xdg_config);
+		free(user_config);
+	}
 
 	if (!opts->ignore_repo && repo_config &&
 	    !access_or_die(repo_config, R_OK, 0))
@@ -1560,8 +1565,6 @@ static int do_git_config_sequence(const struct config_options *opts,
 		die(_("unable to parse command-line config"));
 
 	free(system_config);
-	free(xdg_config);
-	free(user_config);
 	free(repo_config);
 	free(worktree_config);
 	return ret;
@@ -1591,7 +1594,8 @@ int config_with_options(config_fn_t fn, void *data,
 	 */
 	if (config_source && config_source->use_stdin) {
 		ret = git_config_from_stdin(fn, data, config_source->scope);
-	} else if (config_source && config_source->file) {
+	} else if (config_source && config_source->file &&
+		   config_source->scope != CONFIG_SCOPE_GLOBAL) {
 		ret = git_config_from_file_with_options(fn, config_source->file,
 							data, config_source->scope,
 							NULL);
diff --git a/config.h b/config.h
index 19c87fc0bc..9425fe115d 100644
--- a/config.h
+++ b/config.h
@@ -87,6 +87,8 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type,
 
 struct config_options {
 	unsigned int respect_includes : 1;
+	unsigned int ignore_system : 1;
+	unsigned int ignore_global : 1;
 	unsigned int ignore_repo : 1;
 	unsigned int ignore_worktree : 1;
 	unsigned int ignore_cmdline : 1;
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 5fa0111bd9..42f256e122 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2372,7 +2372,7 @@ test_expect_success 'list with nonexistent global config' '
 	git config ${mode_prefix}list --show-scope
 '
 
-test_expect_success 'list --global with nonexistent global config' '
+test_expect_failure 'list --global with nonexistent global config' '
 	rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config &&
 	test_must_fail git config ${mode_prefix}list --global --show-scope
 '
@@ -2429,7 +2429,7 @@ test_expect_success 'list --global with both home and xdg' '
 	global	file:$HOME/.gitconfig	home.config=true
 	EOF
 	git config ${mode_prefix}list --global --show-scope --show-origin >output &&
-	! test_cmp expect output
+	test_cmp expect output
 '
 
 test_expect_success 'override global and system config' '
@@ -2483,7 +2483,7 @@ test_expect_success 'override global and system config' '
 	test_cmp expect output
 '
 
-test_expect_success 'override global and system config with missing file' '
+test_expect_failure '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 ${mode_prefix}list --global &&
 	test_must_fail env GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=does-not-exist git config ${mode_prefix}list --system &&
 	GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=does-not-exist git version
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 0318755799..475bd26aba 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -71,7 +71,7 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists'
 	echo user.name=read_config >expected &&
 	echo user.name=read_gitconfig >>expected &&
 	git config --global --list >actual &&
-	! test_cmp expected actual
+	test_cmp expected actual
 '
 
 
-- 
gitgitgadget

Re: [PATCH/RFC 3/4] config: read global scope via config_sequence

[Junio C Hamano]

"Delilah Ashley Wu via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Delilah Ashley Wu <delilahwu@microsoft.com>
>
> The output of `git config list --global` should include both the home
> (`$HOME/.gitconfig`) and XDG (`$XDG_CONFIG_HOME/git/config`) configs,
> but it only reads from the former.

", but" -> "to match the information given by the command without
--global, but".

> This patch introduces a regression. If both global config files are
> unreadable, then `git config list --global` should exit non-zero. This
> is no longer the case, so mark the corresponding test as a "TODO known
> breakage" and address the issue in the next patch, config: keep bailing
> on unreadable global files.

That is rather unfortunate, as we do try hard to avoid deliberate
regressions in our history.  The reason why this step cannot be done
without first introducing a regression is...?

If the reason is "it would make a single patch too big", perhaps we
can do it in two steps, one preliminary "git_config_sequence() learns
an extra barf-if-no-input parameter that causes it to return error if
no files in the specified sequence exists" step, followed by this
change that starts using git_config_sequence() to handle "--global",
which uses that new flag to ensure that there won't be a regression?

>  	if (opts->use_global_config) {
> +		/*
> +		 * Since global config is sourced from more than one location,
> +		 * use `config.c#do_git_config_sequence()` with `opts->options`
> +		 * to read it. However, writing global config should point to a
> +		 * single destination, set in `opts->source.file`.
> +		 */
> +		opts->options.ignore_repo = 1;
> +		opts->options.ignore_cmdline= 1;
> +		opts->options.ignore_worktree = 1;
> +		opts->options.ignore_system = 1;
> +		opts->source.scope = CONFIG_SCOPE_GLOBAL;

Very nicely done.

Thanks.

[PATCH/RFC 4/4] config: keep bailing on unreadable global files

[Delilah Ashley Wu via GitGitGadget]

From: Delilah Ashley Wu <delilahwu@microsoft.com>

The expected behaviour for `git config list` is:
  A. Without `--global`, it should not bail on unreadable/non-existent
     global config files.

  B. With `--global`, it should bail when both `$HOME/.gitconfig` and
     `$XDG_CONFIG_HOME/git/config` are unreadable. It should not bail
     when one or more of them is readable.

The previous patch, config: read global scope via config_sequence,
introduced a regression in scenario B. When both global config files are
unreadable, running `git config list --global` would not fail. For
example, `GIT_CONFIG_GLOBAL=does-not-exist git config list --global`
exits with status code 0.

Assuming that `config_source->scope == CONFIG_SCOPE_GLOBAL` iff the
`--global` argument is specified, use this to determine whether to bail.
When reading only the global scope and both config files are unreadable,
then adjust the return code to be non-zero.

Note: When bailing, the exit code is not determined by sum of the return
codes of the underlying operations. Instead, the exit code is modified
via a single decrement. If this is undesirable, we can change it to sum
the return codes of the underlying operations instead.

Lastly, modify the tests to remove the known breakage/regression. The
tests for scenario B will now pass.

Helped-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Delilah Ashley Wu <delilahwu@microsoft.com>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.c          | 40 +++++++++++++++++++++++++++++++---------
 t/t1300-config.sh |  4 ++--
 2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 4b9f3831b1..3057c16f59 100644
--- a/config.c
+++ b/config.c
@@ -1500,8 +1500,8 @@ int git_config_system(void)
 }
 
 static int do_git_config_sequence(const struct config_options *opts,
-				  const struct repository *repo,
-				  config_fn_t fn, void *data)
+				  const struct repository *repo, config_fn_t fn,
+				  void *data, enum config_scope scope)
 {
 	int ret = 0;
 	char *system_config = git_system_config();
@@ -1534,15 +1534,34 @@ static int do_git_config_sequence(const struct config_options *opts,
 							 NULL);
 
 	if (!opts->ignore_global) {
+		int global_config_success_count = 0;
+		int nonzero_ret_on_global_config_error = scope == CONFIG_SCOPE_GLOBAL;
+
 		git_global_config_paths(&user_config, &xdg_config);
 
-		if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
-			ret += git_config_from_file_with_options(fn, xdg_config, data,
-						CONFIG_SCOPE_GLOBAL, NULL);
+		if (xdg_config &&
+		    !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
+			ret += git_config_from_file_with_options(fn, xdg_config,
+								 data,
+								 CONFIG_SCOPE_GLOBAL,
+								 NULL);
+			if (!ret)
+				global_config_success_count++;
+		}
+
+		if (user_config &&
+		    !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {
+			ret += git_config_from_file_with_options(fn, user_config,
+								 data,
+								 CONFIG_SCOPE_GLOBAL,
+								 NULL);
+			if (!ret)
+				global_config_success_count++;
+		}
 
-		if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
-			ret += git_config_from_file_with_options(fn, user_config, data,
-						CONFIG_SCOPE_GLOBAL, NULL);
+		if (nonzero_ret_on_global_config_error &&
+		    !global_config_success_count)
+			--ret;
 
 		free(xdg_config);
 		free(user_config);
@@ -1603,7 +1622,10 @@ int config_with_options(config_fn_t fn, void *data,
 		ret = git_config_from_blob_ref(fn, repo, config_source->blob,
 					       data, config_source->scope);
 	} else {
-		ret = do_git_config_sequence(opts, repo, fn, data);
+		ret = do_git_config_sequence(opts, repo, fn, data,
+					     config_source ?
+						     config_source->scope :
+						     CONFIG_SCOPE_UNKNOWN);
 	}
 
 	if (inc.remote_urls) {
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 42f256e122..0c3911183c 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2372,7 +2372,7 @@ test_expect_success 'list with nonexistent global config' '
 	git config ${mode_prefix}list --show-scope
 '
 
-test_expect_failure 'list --global with nonexistent global config' '
+test_expect_success 'list --global with nonexistent global config' '
 	rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config &&
 	test_must_fail git config ${mode_prefix}list --global --show-scope
 '
@@ -2483,7 +2483,7 @@ test_expect_success 'override global and system config' '
 	test_cmp expect output
 '
 
-test_expect_failure 'override global and system config with missing file' '
+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 ${mode_prefix}list --global &&
 	test_must_fail env GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=does-not-exist git config ${mode_prefix}list --system &&
 	GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=does-not-exist git version
-- 
gitgitgadget

Re: [PATCH/RFC 0/4] config: read both home and xdg files for --global

[Kristoffer Haugsbakk]

On Fri, Oct 10, 2025, at 03:14, Delilah Ashley Wu via GitGitGadget wrote:
> As reported in [1]: `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config` are
> both valid global config locations, but `git config list --global` only
> includes the former in its output.

Note only if both files exist.

-- 
Kristoffer Haugsbakk

Re: [PATCH/RFC 0/4] config: read both home and xdg files for --global

[Delilah Ashley Wu]

On Fri, Oct 10, 2025 at 03:27:24AM +0200, Kristoffer Haugsbakk wrote:
> On Fri, Oct 10, 2025, at 03:14, Delilah Ashley Wu via GitGitGadget wrote:
> > As reported in [1]: `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config` are
> > both valid global config locations, but `git config list --global` only
> > includes the former in its output.
> 
> Note only if both files exist.

Thanks for the clarification, I'll be sure to note this in my v2 cover
letter and commit messages.

Delilah =)

Re: [PATCH/RFC 0/4] config: read both home and xdg files for --global

[Johannes Schindelin]

Hi,

On Fri, 10 Oct 2025, Delilah Ashley Wu via GitGitGadget wrote:

> As reported in [1]: `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config` are
> both valid global config locations, but `git config list --global` only
> includes the former in its output.
> 
> Suppose we have this config in `$HOME/.gitconfig`:
> 
> [home]
>     config = true
> 
> 
> And this config in `$XDG_CONFIG_HOME/git/config`:
> 
> [xdg]
>     config = true
> 
> 
> Then, to reproduce the issue that `--global` only shows the home config:
> 
> $ git config list --global --show-scope --show-origin
> global  file:/Users/delilah/.gitconfig    home.config=true
> 
> 
> Git correctly applies the XDG config in its effective configuration, but it
> doesn't show up when `--global` is specified. We can confirm this by
> checking the output without the `--global` flag:
> 
> $ git config list --show-scope --show-origin
> global  file:/Users/delilah/.config/git/config    xdg.config=true
> global  file:/Users/delilah/.gitconfig            home.config=true
> 
> 
> The expected behaviour is both configs should be shown when `--global` is
> specified, so we'd expect its output to look the same as above. This was
> confirmed in [2], which quoted the `git config` documentation:
> 
> > OPTIONS
> >     --global::
> >         For writing options: write to global `~/.gitconfig` file
> >         rather than the repository `.git/config`, write to
> >         `$XDG_CONFIG_HOME/git/config` file if this file exists and the
> >         `~/.gitconfig` file doesn't.
> >
> >         For reading options: read only from global `~/.gitconfig` and from
> >         `$XDG_CONFIG_HOME/git/config` rather than from all available files.
> 
> 
> The first patch fixes forward slash normalisation on Windows paths. The
> second patch introduces tests and regression checks. The third and fourth
> patches implement the fix to include both config files when `--global` is
> specified. Johannes has kindly pre-reviewed this patch series via GitHub on
> GitGitGadget #1938 [3]. You'll notice some force-pushes after the review,
> but I only changed commit messages.
> 
> [1]:
> https://lore.kernel.org/git/CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com/
> [2]: https://lore.kernel.org/git/xmqqmt5lezi3.fsf@gitster.g/
> [3]: https://github.com/gitgitgadget/git/pull/1938/
> 
> Thank you all for your time!

For the record, my "Reviewed-by:" still stands, if lack of reviews should
be the reason why this patch series has not even entered the `seen`
branch.

Ciao,
Johannes

Re: [PATCH/RFC 0/4] config: read both home and xdg files for --global

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> For the record, my "Reviewed-by:" still stands, if lack of reviews should
> be the reason why this patch series has not even entered the `seen`
> branch.

Thanks for pinging.

"Why is it not in 'next'" is a legitimate question.  I think that is
because the topic has no discussion on the list in the thread.

"Why is it in 'seen'" is a question with no answer.  As I often say,
'seen' is merely what I happened to have seen and found it promising
but is not ready for 'next', and people should not read anything
more into it.

I didn't look at it primarily because nobody, not even one on a
handful of experienced contributors whose opinions are well regarded
in the community on the CC: list, responded to the thread at all.
Before the message I am responding to, that is ;-)

I wanted to see how well people receive the motivation behind the
proposed change, as I vaguely recalled that not using both at the
same time was deliberate to help those who migrate from historical
location to XDG layout, but did not have time and energy to do the
digging myself to become knowledgeable again to give any comment
worth reading.

Re: [PATCH/RFC 0/4] config: read both home and xdg files for --global

[Junio C Hamano]

"Delilah Ashley Wu via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> As reported in [1]: `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config` are
> both valid global config locations, but `git config list --global` only
> includes the former in its output.

... while "git config list" includes both, which is an inconsistency
without good reason.

Thanks for addressing this issue.  I haven't had a chance to look at
these patches yet, but both analysis and Glen's outline for the best
approach presented in the thread [1] do look very sensible.

What is the reason behind [RFC] in the title?  Are there things that
are iffy yourself in the patches that reviewers want to pay special
attention to?

Thanks.

Re: [PATCH/RFC 0/4] config: read both home and xdg files for --global

[Delilah Ashley Wu]

On Wed, Nov 19, 2025 at 06:44:25AM -0800, Junio C Hamano wrote:
> "Delilah Ashley Wu via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > As reported in [1]: `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config` are
> > both valid global config locations, but `git config list --global` only
> > includes the former in its output.
> 
> ... while "git config list" includes both, which is an inconsistency
> without good reason.

Good point! Will add above to the v2 cover letter.

> What is the reason behind [RFC] in the title?  Are there things that
> are iffy yourself in the patches that reviewers want to pay special
> attention to?

There wasn't any reason; I accidentally left the GitHub PR in draft
mode when I submitted it. I'll drop the [RFC] in v2.

And thanks for the review! You covered the points that I also felt
iffy about, e.g. introducing a regression in the middle of the patch
series. I'll address your feedback in v2.

Delilah :)