Skip to content

config: read both home and xdg files for --global #1938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

config: read both home and xdg files for --global #1938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8df6a0
cleanup_path: force forward slashes on Windows
delilahw Sep 17, 2025
d2167a8
config: test home and xdg files in `list --global`
delilahw May 22, 2025
9d8af4e
config: read global scope via config_sequence
delilahw May 22, 2025
6119cee
config: keep bailing on unreadable global files
delilahw May 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions builtin/config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,18 @@ static void location_options_init(struct config_location_options *opts,
}
Copy link

@gitgitgadget gitgitgadget bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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.


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)
/*
Expand Down
54 changes: 40 additions & 14 deletions config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -1526,22 +1526,46 @@ 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) {
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 (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 (nonzero_ret_on_global_config_error &&
!global_config_success_count)
--ret;

free(xdg_config);
free(user_config);
}

if (!opts->ignore_repo && repo_config &&
!access_or_die(repo_config, R_OK, 0))
Expand All @@ -1560,8 +1584,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;
Expand Down Expand Up @@ -1591,15 +1613,19 @@ 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);
} else if (config_source && config_source->blob) {
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) {
Expand Down
2 changes: 2 additions & 0 deletions config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
10 changes: 7 additions & 3 deletions path.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,17 @@ static struct strbuf *get_pathname(void)
return sb;
Copy link

@gitgitgadget gitgitgadget bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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;
>  }

}

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;
}

Expand Down
65 changes: 65 additions & 0 deletions t/t1300-config.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2367,6 +2367,71 @@ test_expect_success '--show-scope with --default' '
test_cmp expect actual
Copy link

@gitgitgadget gitgitgadget bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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.

'

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 &&
Expand Down
3 changes: 2 additions & 1 deletion t/t1306-xdg-files.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ 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
'
Expand Down
Loading