config: read both home and xdg files for --global
cover config: read both home and xdg files for --global
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:
Quoted 9 lines
OPTIONS
--global::
For writing options: write to global~/.gitconfigfile rather than the repository.git/config, write to$XDG_CONFIG_HOME/git/configfile if this file exists and the~/.gitconfigfile doesn't.For reading options: read only from global
~/.gitconfigand from$XDG_CONFIG_HOME/git/configrather 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/[email protected]/
[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
Signature · 1 line
gitgitgadget
PATCH 1/4 cleanup_path: force forward slashes on Windows
From: Delilah Ashley Wu <[email protected]>
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.
--- path.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
path.c +7 -3
@@ -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; }
Signature · 1 line
gitgitgadget
Re: [PATCH/RFC 1/4] cleanup_path: force forward slashes on Windows
"Delilah Ashley Wu via GitGitGadget" <[email protected]>
Quoted 2 lines · writes:
All existing callers of
cleanup_path()passchar *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?
Quoted 2 lines
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.
path.c static struct strbuf *get_pathname(void) · line 49
Quoted 9 lines
path.c +2
-3-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?
path.c static struct strbuf *get_pathname(void) · line 52
Quoted 3 lines
path.c +3
-0+#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"?
Quoted 2 lines
return path;
}
PATCH 2/4 config: test home and xdg files in `list --global`
In reply to Delilah Ashley Wu via GitGitGadgetFrom: Delilah Ashley Wu <[email protected]>
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.
--- t/t1300-config.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++ t/t1306-xdg-files.sh | 5 ++-- 2 files changed, 68 insertions(+), 2 deletions(-)
t/t1300-config.sh +65 -0
@@ -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 &&
t/t1306-xdg-files.sh +3 -2
@@ -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 '
Signature · 1 line
gitgitgadget
Re: [PATCH/RFC 2/4] config: test home and xdg files in `list --global`
"Delilah Ashley Wu via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Delilah Ashley Wu <[email protected]>
The
git config list --globaloutput 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.
Quoted 7 lines
Modify tests to check the following and expect a failure:
-git config list --globalshould include contents from both the home and XDG config locations (assuming they are readable), not just the former.-
--show-originshould 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.
Quoted 10 lines
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/.gitconfigand$XDG_CONFIG_HOME/git/configare unreadable. It should not bail if at least one of them is readable.
Good.
Quoted 18 lines
The next patch, config: read global scope via
config_sequence, will implement a fix to include both config files when--globalis specified.Reported-by Jade Lovelace <[email protected]>Helped-by Derrick Stolee <[email protected]>Signed-off-by Delilah Ashley Wu <[email protected]>Reviewed-by Johannes Schindelin <[email protected]>--- t/t1300-config.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++ t/t1306-xdg-files.sh | 5 ++-- 2 files changed, 68 insertions(+), 2 deletions(-)t/t1300-config.sh +0
-0
t/t1300-config.sh test_expect_success '--show-scope with --default' ' · line 2373
Quoted 7 lines
t/t1300-config.sh +4
-0@@ -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.
t/t1300-config.sh test_expect_success '--show-scope with --default' ' · line 2378
Quoted 4 lines
t/t1300-config.sh +4
-0+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.
t/t1300-config.sh test_expect_success '--show-scope with --default' ' · line 2381
Quoted 2 lines
t/t1300-config.sh +2
-0+test_expect_success 'list --global with only home' ' + rm -rf "$HOME"/.config/git/config &&
Lose "r" from "-rf" or lose "/config".
t/t1300-config.sh test_expect_success '--show-scope with --default' ' · line 2394
Quoted 12 lines
t/t1300-config.sh +12
-0+ 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.
Quoted 6 lines
t/t1300-config.sh +6
-0+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 &&
t/t1300-config.sh test_expect_success '--show-scope with --default' ' · line 2411
Quoted 10 lines
t/t1300-config.sh +10
-0+ [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.
Quoted 11 lines
t/t1300-config.sh +11
-0+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]
t/t1300-config.sh test_expect_success '--show-scope with --default' ' · line 2433
Quoted 10 lines
t/t1300-config.sh +10
-0+ 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 actualBut 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.
Quoted 5 lines
t/t1306-xdg-files.sh +0
-0
t/t1306-xdg-files.sh test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' · line 75
Quoted 10 lines
t/t1306-xdg-files.sh +3
-2@@ -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 3/4 config: read global scope via config_sequence
In reply to Delilah Ashley Wu via GitGitGadgetFrom: Delilah Ashley Wu <[email protected]>
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/[email protected].
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.
--- 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(-)
builtin/config.c +12 -0
@@ -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)/*
config.c +15 -11
@@ -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 );
config.h +2 -0
@@ -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 ;
t/t1300-config.sh +3 -3
@@ -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
t/t1306-xdg-files.sh +1 -1
@@ -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 '
Signature · 1 line
gitgitgadget
Re: [PATCH/RFC 3/4] config: read global scope via config_sequence
"Delilah Ashley Wu via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Delilah Ashley Wu <[email protected]>
The output of
git config list --globalshould 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".
Quoted 5 lines
This patch introduces a regression. If both global config files are unreadable, then
git config list --globalshould 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?
builtin/config.c static void location_options_init(struct config_location_options *opts, · line 781
Quoted 12 lines
builtin/config.c +11
-0if (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 4/4 config: keep bailing on unreadable global files
In reply to Delilah Ashley Wu via GitGitGadgetFrom: Delilah Ashley Wu <[email protected]>
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.
--- config.c | 40 +++++++++++++++++++++++++++++++--------- t/t1300-config.sh | 4 ++-- 2 files changed, 33 insertions(+), 11 deletions(-)
config.c +31 -9
@@ -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) {
t/t1300-config.sh +2 -2
@@ -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
Signature · 1 line
gitgitgadget
Quoted 3 lines · Delilah Ashley Wu via GitGitGadget
As reported in [1]:
$HOME/.gitconfigand$XDG_CONFIG_HOME/git/configare both valid global config locations, butgit config list --globalonly includes the former in its output.
Note only if both files exist.
Signature · 1 line
Kristoffer Haugsbakk
Quoted 6 lines · Kristoffer Haugsbakk
Quoted 3 lines · Delilah Ashley Wu via GitGitGadget
As reported in [1]:
$HOME/.gitconfigand$XDG_CONFIG_HOME/git/configare both valid global config locations, butgit config list --globalonly 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 =)
Quoted 10 lines · Delilah Ashley Wu
Quoted 6 lines · Kristoffer Haugsbakk
Quoted 3 lines · Delilah Ashley Wu via GitGitGadget
As reported in [1]:
$HOME/.gitconfigand$XDG_CONFIG_HOME/git/configare both valid global config locations, butgit config list --globalonly 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.
After this and [*] the discussion stopped and the topic has been dormant since then for full two months. I'd drop the topic from 'seen' soonish but that does not mean an improved version of this patch is unwelcome.
Thanks.
[References]
* https://lore.kernel.org/git/20251122020047.GB3947@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net/
Hi,
Quoted 53 lines · Delilah Ashley Wu via GitGitGadget
As reported in [1]:
$HOME/.gitconfigand$XDG_CONFIG_HOME/git/configare both valid global config locations, butgit config list --globalonly includes the former in its output.Suppose we have this config in
$HOME/.gitconfig:[home]
config = trueAnd this config in
$XDG_CONFIG_HOME/git/config:[xdg]
config = trueThen, to reproduce the issue that
--globalonly shows the home config:$ git config list --global --show-scope --show-origin global file:/Users/delilah/.gitconfig home.config=trueGit correctly applies the XDG config in its effective configuration, but it doesn't show up when
--globalis specified. We can confirm this by checking the output without the--globalflag:$ git config list --show-scope --show-origin global file:/Users/delilah/.config/git/config xdg.config=true global file:/Users/delilah/.gitconfig home.config=trueThe expected behaviour is both configs should be shown when
--globalis specified, so we'd expect its output to look the same as above. This was confirmed in [2], which quoted thegit configdocumentation:Quoted 9 lines
OPTIONS
--global::
For writing options: write to global~/.gitconfigfile rather than the repository.git/config, write to$XDG_CONFIG_HOME/git/configfile if this file exists and the~/.gitconfigfile doesn't.For reading options: read only from global
~/.gitconfigand from$XDG_CONFIG_HOME/git/configrather 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
--globalis 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.
Quoted 6 lines
[1]:
https://lore.kernel.org/git/CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/[email protected]/
[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
Quoted 3 lines · Johannes Schindelin
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
seenbranch.
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.
"Delilah Ashley Wu via GitGitGadget" <[email protected]>
Quoted 3 lines · writes:
As reported in [1]:
$HOME/.gitconfigand$XDG_CONFIG_HOME/git/configare both valid global config locations, butgit config list --globalonly 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.
Quoted 9 lines · Junio C Hamano
"Delilah Ashley Wu via GitGitGadget" <[email protected]>
Quoted 3 lines · writes:
As reported in [1]:
$HOME/.gitconfigand$XDG_CONFIG_HOME/git/configare both valid global config locations, butgit config list --globalonly 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.
Quoted 3 lines
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 :)
v2 cover config: read both home and xdg files for --global
In reply to Delilah Ashley Wu via GitGitGadgetHi all, thanks for your patience. Here's my reroll.
As reported in [1], $HOME/.gitconfig and $XDG_CONFIG_HOME/git/config are both valid global configuration locations. However, when both files exist, git config list --global only reads from the former location whereas git config list (without --global) reads from both. The same issue was reported for git config get in [2]. This inconsistency has no good justification and contradicts the documented behaviour.
Suppose that $HOME/.gitconfig contains:
[home]
config = true
and $XDG_CONFIG_HOME/git/config contains:
[xdg]
config = true
Then, listing with --global shows only the home config:
$ git config list --global --show-scope --show-origin
global file:/Users/delilah/.gitconfig home.config=trueand getting the XDG configuration entry with --global will fail:
$ git config get --global xdg.config; echo $?
1Git still reads the XDG config as part of its effective configuration, as shown by listing the configuration without --global:
$ git config list --show-scope --show-origin
global file:/Users/delilah/.config/git/config xdg.config=true
global file:/Users/delilah/.gitconfig home.config=trueThe documentation, quoted in [1] and [2], states that --global should read from both files, so its output should be the same as above. Here's the relevant excerpt:
Quoted 9 lines
OPTIONS
--global::
For writing options: write to global~/.gitconfigfile rather than the repository.git/config, write to$XDG_CONFIG_HOME/git/configfile if this file exists and the~/.gitconfigfile doesn't.For reading options: read only from global
~/.gitconfigand from$XDG_CONFIG_HOME/git/configrather than from all available files.
To be consistent with the documentation and the behaviour without --global, we should read both configuration files when --global is passed. We do this in a few steps:
- Patch 1 fixes slash normalisation on Windows paths. This is used for --show-origin assertions in patch 3 tests.
- Patch 2 modifies error handling when reading configuration files. This is used to prevent a regression in patch 3.
- Patch 3 reads both configuration files when --global is specified.
[1]: https://lore.kernel.org/git/CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAAdFe9yhBk-WecVzCTsjQ-4Z3AZAbpP+w+B076ouM3qX6d1WAg@mail.gmail.com/
Thanks again for your time!
Delilah
---
Changes in v2:
- Squash test-only patches into their corresponding implementation patches.
- Reorder patches to prevent a regression from being introduced and
then fixed in a later patch.
- Narrow the scope of slash conversion to xdg_config_home_for() and avoid modifying cleanup_path(), which could've broken callers that
do not expect normalised slashes.
- Clarify that some tests only check the return code of a git config command; we do not care about the output.
- Link to v1: https://patch.msgid.link/[email protected]/
---
Delilah Ashley Wu (3):
path: use forward slashes in XDG config on Windows
config: let sequence require a successful file
config: read global scope via config_sequence
builtin/config.c | 11 +++++ config.c | 76 +++++++++++++++++++++++----------- config.h | 2 + path.c | 16 ++++--- t/t1300-config.sh | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++ t/t1306-xdg-files.sh | 5 ++- 6 files changed, 194 insertions(+), 31 deletions(-)
Range-diff versus v1:
1: d9525d954e < -: ---------- config: read both home and xdg files for --global 2: c24ed49bac < -: ---------- cleanup_path: force forward slashes on Windows 3: 51293ee827 < -: ---------- config: test home and xdg files in `list --global` 4: 26f3c46598 < -: ---------- config: read global scope via config_sequence 5: b6ab7bfd67 < -: ---------- config: keep bailing on unreadable global files -: ---------- > 1: 2fa37d8aa7 path: use forward slashes in XDG config on Windows -: ---------- > 2: d90c9ae69f config: let sequence require a successful file -: ---------- > 3: a3b5599c8d config: read global scope via config_sequence
---
base-commit: 2c78326f810173a4f3aefd8021f1e07575412481
change-id: 20260808-fix-config-list-global-home-and-xdg-9bcaac093a1b
PATCH v2 1/3 path: use forward slashes in XDG config on Windows
From: Delilah Ashley Wu <[email protected]>
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 displays 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 construct 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=barThese mixed slashes occur because the $HOME and $XDG_CONFIG_HOME environment variables usually contain backslashes on Windows, and xdg_config_home_for() interpolates them into templates that use hardcoded forward slashes.
Since callers of xdg_config_home_for() handle mixed slashes correctly, it is reasonable to assume that they can handle paths with only forward slashes. Let's enforce forward slashes in xdg_config_home_for() by using convert_slashes() on Windows.
Also, there are no tests for the XDG path with --show-origin. Add a test for slash conversion and a confidence check for the default path.
--- path.c | 16 ++++++++++------ t/t1300-config.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 6 deletions(-)
path.c +10 -6
@@ -1544,19 +1544,23 @@ int looks_like_command_line_option(const char *str)char *xdg_config_home_for (const char *subdir,const char *filename) { +char *ret;const char *home, *config_home;assert (subdir);assert (filename); config_home =getenv ("XDG_CONFIG_HOME" );if (config_home && *config_home) -return mkpathdup ("%s/%s/%s" , config_home, subdir, filename); - - home =getenv ("HOME" ); -if (home) -return mkpathdup ("%s/.config/%s/%s" , home, subdir, filename); + ret =mkpathdup ("%s/%s/%s" , config_home, subdir, filename); +else if ((home =getenv ("HOME" ))) + ret =mkpathdup ("%s/.config/%s/%s" , home, subdir, filename); +else +return NULL ; -return NULL ; +#ifdef GIT_WINDOWS_NATIVE +convert_slashes (ret); +#endif +return ret; }char *xdg_config_home (const char *filename)
t/t1300-config.sh +32 -0
@@ -2350,6 +2350,38 @@ test_expect_success '--show-origin with --default' ' test_cmp expect actual ' +test_expect_success 'set up xdg config --show-origin tests' ' + mkdir -p "$HOME"/.config/git && + cat >"$HOME"/.config/git/config <<-EOF + [xdg] + config = true + EOF +' + +test_expect_success MINGW '--show-origin converts backslashes in xdg path to forward slashes on Windows' ' + backslash_home="$(echo "$HOME" | tr / \\\\)" && + echo "file:$HOME/.config/git/config true" >expect && + + ( + sane_unset XDG_CONFIG_HOME && + HOME="$backslash_home" git config ${mode_get} --show-origin xdg.config >actual + ) && + test_cmp expect actual && + + XDG_CONFIG_HOME="$backslash_home\\.config" git config ${mode_get} --show-origin xdg.config >actual && + test_cmp expect actual +' + +test_expect_success '--show-origin with default xdg path' ' + echo "file:$HOME/.config/git/config true" >expect && + git config ${mode_get} --show-origin xdg.config >actual && + test_cmp expect actual +' + +test_expect_success 'clean up xdg config --show-origin tests' ' + rm -rf "$HOME"/.config/git +' + test_expect_success '--show-scope with --list' ' cat >expect <<-EOF && global user.global=true
Signature · 1 line
2.54.0
Re: [PATCH v2 1/3] path: use forward slashes in XDG config on Windows
Quoted 26 lines · Delilah Ashley Wu
From: Delilah Ashley Wu <[email protected]>
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 displays 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()in5ca6b7bb47b(config --show-origin: report paths with forward slashes, 2016-03-23). However, other code paths may construct paths containing backslashes. For example,config --show-originprints 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=barThese mixed slashes occur because the
$HOMEand$XDG_CONFIG_HOMEenvironment variables usually contain backslashes on Windows, andxdg_config_home_for()interpolates them into templates that use hardcoded forward slashes.
Quoted 7 lines
Since callers of
xdg_config_home_for()handle mixed slashes correctly, it is reasonable to assume that they can handle paths with only forward slashes. Let's enforce forward slashes inxdg_config_home_for()by usingconvert_slashes()on Windows.Also, there are no tests for the XDG path with
--show-origin. Add a test for slash conversion and a confidence check for the default path.
Is this "force forwared slashes to Windows users" a required part of XDG/HOME global fix? If not, please leave it out of the topic.
Even if it is a good idea to always force forward slashes to Windows users (I have no strong opinions on the topic), and if it is very unlikely to break existing Windows users (I do not have any clue if that would be the case or not, as I do not do Windows), we would want to make sure if we can get the same effect without sprinkling "#ifdef" in the platform agnostic part of the codebase like "path.c" file.
Where would the slash in "ret" that is passed to convert_slashes() function come from? If they come from environment variables like XDG_CONFIG_HOME and HOME, that is end-user's preference and we have no business forcing them which forms of slashes to use. Does it come from "subdir" or "filename" parameters? It might be the job for the callers to standardize slashes in the value they send in, but as far as I can see, these do not have anything other than hardcoded constants that use no slashes (most of them) or one forward slash ("systemd/user").
Again, I do not see it explained why this change has to be part of this series in the proposed log message, so...?
Quoted 83 lines
Signed-off-by Delilah Ashley Wu <[email protected]>--- path.c | 16 ++++++++++------ t/t1300-config.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 6 deletions(-)path.c +10
-6@@ -1544,19 +1544,23 @@ int looks_like_command_line_option(const char *str)char *xdg_config_home_for (const char *subdir,const char *filename) { +char *ret;const char *home, *config_home;assert (subdir);assert (filename); config_home =getenv ("XDG_CONFIG_HOME" );if (config_home && *config_home) -return mkpathdup ("%s/%s/%s" , config_home, subdir, filename); - - home =getenv ("HOME" ); -if (home) -return mkpathdup ("%s/.config/%s/%s" , home, subdir, filename); + ret =mkpathdup ("%s/%s/%s" , config_home, subdir, filename); +else if ((home =getenv ("HOME" ))) + ret =mkpathdup ("%s/.config/%s/%s" , home, subdir, filename); +else +return NULL ; -return NULL ; +#ifdef GIT_WINDOWS_NATIVE +convert_slashes (ret); +#endif +return ret; }char *xdg_config_home (const char *filename)t/t1300-config.sh +32
-0@@ -2350,6 +2350,38 @@ test_expect_success '--show-origin with --default' ' test_cmp expect actual ' +test_expect_success 'set up xdg config --show-origin tests' ' + mkdir -p "$HOME"/.config/git && + cat >"$HOME"/.config/git/config <<-EOF + [xdg] + config = true + EOF +' + +test_expect_success MINGW '--show-origin converts backslashes in xdg path to forward slashes on Windows' ' + backslash_home="$(echo "$HOME" | tr / \\\\)" && + echo "file:$HOME/.config/git/config true" >expect && + + ( + sane_unset XDG_CONFIG_HOME && + HOME="$backslash_home" git config ${mode_get} --show-origin xdg.config >actual + ) && + test_cmp expect actual && + + XDG_CONFIG_HOME="$backslash_home\\.config" git config ${mode_get} --show-origin xdg.config >actual && + test_cmp expect actual +' + +test_expect_success '--show-origin with default xdg path' ' + echo "file:$HOME/.config/git/config true" >expect && + git config ${mode_get} --show-origin xdg.config >actual && + test_cmp expect actual +' + +test_expect_success 'clean up xdg config --show-origin tests' ' + rm -rf "$HOME"/.config/git +' + test_expect_success '--show-scope with --list' ' cat >expect <<-EOF && global user.global=true
Re: [PATCH v2 1/3] path: use forward slashes in XDG config on Windows
Thanks for the reviews! I'm still working through the feedback for v3.
Quoted 4 lines · Junio C Hamano
Is this "force forwared slashes to Windows users" a required part of XDG/HOME global fix? If not, please leave it out of the topic. [...] Again, I do not see it explained why this change has to be part of this series in the proposed log message, so...?
Sorry, I forgot to explain in the log message that this change is supposed to supplement the --show-origin tests added in patch 3 (config: read global scope via config_sequence). Without it, the --show-origin would output a path with mixed slashes on Windows:
file:"C:\\Users\\delilah/.config/git/config" xdg.foo=bar
The tests expect paths containing only forward slashes. So patch 3 modifies t1300-config.sh assertions to look like this:
echo "file:$HOME/.config/git/config xdg.config=xdg" >expect
git config list --global --show-origin >actual test_cmp expect actual
where $HOME has been normalised to contain forward slashes only, as seen at t1300-config.sh:2179, which was introduced in 45bf329 (t1300:
fix the new --show-origin tests on Windows):
HOME="$(pwd)" # convert to Windows path
Quoted 7 lines
Even if it is a good idea to always force forward slashes to Windows users (I have no strong opinions on the topic), and if it is very unlikely to break existing Windows users (I do not have any clue if that would be the case or not, as I do not do Windows), we would want to make sure if we can get the same effect without sprinkling "#ifdef" in the platform agnostic part of the codebase like "
path.c" file.
I followed an existing usage of #ifdef GIT_WINDOWS_NATIVE and convert_slashes() in path.c, but if it's no longer allowed in the platform agnostic part, we could do the slash conversion elsewhere. This assumes we want to keep converting the slashes, but we should reconsider from your points raised below.
Quoted 4 lines
Where would the slash in "ret" that is passed to
convert_slashes()function come from? If they come from environment variables likeXDG_CONFIG_HOMEand HOME, that is end-user's preference and we have no business forcing them which forms of slashes to use.
The slash in ret would come from the environment variables, so perhaps we should not modify the slashes in them at all. Instead, I could drop this patch and change the tests in patch 3 to export a XDG_CONFIG_HOME value containing only forward slashes. This would satisfy the assumption that paths in the tests will contain only forward slashes. What do you think?
Thanks!
Delilah =)
Re: [PATCH v2 1/3] path: use forward slashes in XDG config on Windows
Quoted 6 lines · Delilah Ashley Wu
Quoted 2 lines · Junio C Hamano
Is this "force forwared slashes to Windows users" a required part of XDG/HOME global fix? If not, please leave it out of the topic.
I could drop this patch and change the tests in patch 3 to export a
XDG_CONFIG_HOMEvalue containing only forward slashes.
I've chosen to drop this patch and change the tests as above in v3. Delilah =)
PATCH v2 2/3 config: let sequence require a successful file
In reply to Delilah Ashley WuFrom: Delilah Ashley Wu <[email protected]>
Teach do_git_config_sequence() to optionally report an error if no configuration files in the sequence were successfully processed. Gate this new behaviour with a flag and keep it disabled for now.
Add tests to record existing behaviour and prevent regressions in the next patch, "config: read global scope via config_sequence", which adds a code path that enables the flag. When no global configuration file exists, git config list succeeds whereas git config list --global fails. The command output is irrelevant, so only check the exit code.
--- config.c | 57 ++++++++++++++++++++++++++++++++++++++----------------- t/t1300-config.sh | 12 ++++++++++++ 2 files changed, 52 insertions(+), 17 deletions(-)
config.c +40 -17
@@ -1544,11 +1544,27 @@ int git_config_system(void)return !git_env_bool ("GIT_CONFIG_NOSYSTEM" ,0 ); } +static void attempt_git_config_from_file_with_options (config_fn_t fn, +const char *filename, +void *data, +enum config_scope scope, +const struct config_options *opts, +int *success_count, +int *cumulative_ret) +{ +int ret =git_config_from_file_with_options (fn, filename, data, + scope, opts); +if (!ret) + (*success_count)++; + *cumulative_ret += ret; +} +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,int require_successful_config) {int ret =0 ; +int success_count =0 ;char *system_config =git_system_config ();char *xdg_config =NULL ;char *user_config =NULL ;
@@ -1574,32 +1590,35 @@ static int do_git_config_sequence(const struct config_options *opts,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_with_options (fn, system_config, - data,CONFIG_SCOPE_SYSTEM , -NULL ); +attempt_git_config_from_file_with_options (fn, system_config, data, +CONFIG_SCOPE_SYSTEM ,NULL , + &success_count, &ret);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 ); +attempt_git_config_from_file_with_options (fn, xdg_config, + data, +CONFIG_SCOPE_GLOBAL , +NULL , &success_count, &ret);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 ); +attempt_git_config_from_file_with_options (fn, user_config, + data, +CONFIG_SCOPE_GLOBAL , +NULL , &success_count, &ret);if (!opts->ignore_repo && repo_config && !access_or_die (repo_config,R_OK ,0 )) - ret +=git_config_from_file_with_options (fn, repo_config, data, -CONFIG_SCOPE_LOCAL ,NULL ); +attempt_git_config_from_file_with_options (fn, repo_config, data, +CONFIG_SCOPE_LOCAL ,NULL , &success_count, &ret);if (!opts->ignore_worktree && worktree_config && repo && repo->repository_format_worktree_config && - !access_or_die (worktree_config,R_OK ,0 )) { - ret +=git_config_from_file_with_options (fn, worktree_config, data, -CONFIG_SCOPE_WORKTREE , -NULL ); - } + !access_or_die (worktree_config,R_OK ,0 )) +attempt_git_config_from_file_with_options (fn, worktree_config, data, +CONFIG_SCOPE_WORKTREE , +NULL , &success_count, &ret);if (!opts->ignore_cmdline &&git_config_from_parameters (fn, data) <0 )die (_ ("unable to parse command-line config" ));
@@ -1609,6 +1628,10 @@ static int do_git_config_sequence(const struct config_options *opts,free (user_config);free (repo_config);free (worktree_config); + +if (require_successful_config && !success_count && !ret) + ret =-1 ; +return ret; }
@@ -1644,7 +1667,7 @@ 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,0 ); }if (inc.remote_urls) {
t/t1300-config.sh +12 -0
@@ -2457,6 +2457,18 @@ test_expect_success '--show-scope with --default' ' test_cmp expect actual ' +test_expect_success 'list with nonexistent global config gracefully exits' ' + rm -f "$HOME"/.gitconfig "$HOME"/.config/git/config && + git config ${mode_prefix}list && + git config ${mode_prefix}list --show-scope +' + +test_expect_success 'list --global with nonexistent global config fails' ' + 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 +' + test_expect_success 'override global and system config' ' test_when_finished rm -f \"\$HOME\"/.gitconfig && cat >"$HOME"/.gitconfig <<-EOF &&
Signature · 1 line
2.54.0
Re: [PATCH v2 2/3] config: let sequence require a successful file
Quoted 11 lines · Delilah Ashley Wu
From: Delilah Ashley Wu <[email protected]>
Teach
do_git_config_sequence()to optionally report an error if no configuration files in the sequence were successfully processed. Gate this new behaviour with a flag and keep it disabled for now.Add tests to record existing behaviour and prevent regressions in the next patch, "config: read global scope via
config_sequence", which adds a code path that enables the flag. When no global configuration file exists,git config listsucceeds whereasgit config list --globalfails. The command output is irrelevant, so only check the exit code.
It is not exactly 'irrelevant' as that is how the user learns what caused the command to fail, e.g. "fatal: unable to read config file <path>".
What you meant was that you are not interested in the exact message, you only want to make sure it fails because of the missing file, and you thought that it is a good way to do so to check the exit code.
config.c int git_config_system(void) · line 1545
Quoted 13 lines
Signed-off-by Delilah Ashley Wu <[email protected]>--- config.c | 57 ++++++++++++++++++++++++++++++++++++++----------------- t/t1300-config.sh | 12 ++++++++++++ 2 files changed, 52 insertions(+), 17 deletions(-)config.c +0
-0@@ -1544,11 +1544,27 @@ int git_config_system(void)return !git_env_bool ("GIT_CONFIG_NOSYSTEM" ,0 ); }
Perhaps "attempt" -> "try" or something more clever can be used to make sure we won't have to type so many characters. "try_config()" should be decriptive enough for the purpose, for example.
File scope static helper functions do not have to be and should not be named with so many words. Shorter names would also help to keep your lines under ~70 column limit.
Quoted 27 lines
config.c +18
-2+static void attempt_git_config_from_file_with_options (config_fn_t fn, +const char *filename, +void *data, +enum config_scope scope, +const struct config_options *opts, +int *success_count, +int *cumulative_ret) +{ +int ret =git_config_from_file_with_options (fn, filename, data, + scope, opts); +if (!ret) + (*success_count)++; + *cumulative_ret += ret; +} +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,int require_successful_config) {int ret =0 ; +int success_count =0 ;char *system_config =git_system_config ();char *xdg_config =NULL ;char *user_config =NULL ;
config.c static int do_git_config_sequence(const struct config_options *opts, · line 1596
Quoted 10 lines
config.c +3
-3@@ -1574,32 +1590,35 @@ static int do_git_config_sequence(const struct config_options *opts,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_with_options (fn, system_config, - data,CONFIG_SCOPE_SYSTEM , -NULL ); +attempt_git_config_from_file_with_options (fn, system_config, data, +CONFIG_SCOPE_SYSTEM ,NULL , + &success_count, &ret);
If we are allowed to use system config, system_config is defined, and we can read the system config, we try to grab values from it, and record the fact that we did so successfully.
config.c static int do_git_config_sequence(const struct config_options *opts, · line 1597
Quoted 1 line
config.c +0
-0git_global_config_paths (&user_config, &xdg_config);
We grab paths to two files, as before.
config.c static int do_git_config_sequence(const struct config_options *opts, · line 1603
Quoted 7 lines
config.c +4
-2if (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 ); +attempt_git_config_from_file_with_options (fn, xdg_config, + data, +CONFIG_SCOPE_GLOBAL , +NULL , &success_count, &ret);
If xdg config is to be used (note: GIT_CONFIG_GLOBAL environment can disable the use of it) and xdg file is available, we read and record just like we saw is done for the system config above.
config.c static int do_git_config_sequence(const struct config_options *opts, · line 1609
Quoted 7 lines
config.c +4
-2if (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 ); +attempt_git_config_from_file_with_options (fn, user_config, + data, +CONFIG_SCOPE_GLOBAL , +NULL , &success_count, &ret);
Ditto fo user config.
config.c static int do_git_config_sequence(const struct config_options *opts, · line 1614
Quoted 6 lines
config.c +2
-2if (!opts->ignore_repo && repo_config && !access_or_die (repo_config,R_OK ,0 )) - ret +=git_config_from_file_with_options (fn, repo_config, data, -CONFIG_SCOPE_LOCAL ,NULL ); +attempt_git_config_from_file_with_options (fn, repo_config, data, +CONFIG_SCOPE_LOCAL ,NULL , &success_count, &ret);
And the local one.
config.c static int do_git_config_sequence(const struct config_options *opts, · line 1621
Quoted 11 lines
config.c +4
-5if (!opts->ignore_worktree && worktree_config && repo && repo->repository_format_worktree_config && - !access_or_die (worktree_config,R_OK ,0 )) { - ret +=git_config_from_file_with_options (fn, worktree_config, data, -CONFIG_SCOPE_WORKTREE , -NULL ); - } + !access_or_die (worktree_config,R_OK ,0 )) +attempt_git_config_from_file_with_options (fn, worktree_config, data, +CONFIG_SCOPE_WORKTREE , +NULL , &success_count, &ret);
And the per-worktree one.
config.c static int do_git_config_sequence(const struct config_options *opts, · line 1633
Quoted 9 lines
config.c +3
-0if (!opts->ignore_cmdline &&git_config_from_parameters (fn, data) <0 )die (_ ("unable to parse command-line config" ));@@ -1609,6 +1628,10 @@ static int do_git_config_sequence(const struct config_options *opts,free (user_config);free (repo_config);free (worktree_config); + +if (require_successful_config && !success_count && !ret) + ret =-1 ;
If we are asked to ensure that we successfully read at least one place and we didn't, we assign -1 to ret but we do so ONLY when we haven't seen any other errors (i.e., existing non-zero ret is preserved, which may not be -1). OK.
config.c static int do_git_config_sequence(const struct config_options *opts, · line 1636
Quoted 2 lines
config.c +0
-0return ret; }
I am not convinced 100% that we need "success_count", either, until we see how it is used in the later steps. But from the way the try_config() thing is used, I find it dubious that it now returns void. It should just keep returning the error code as before, and the caller should just keep accumulcating as the original code used to. I.e.,
ret += try_config(fn, frotz_config, data,
CONFIG_SCOPE_FROTZ, NULL,
&success);Re: [PATCH v2 2/3] config: let sequence require a successful file
Quoted 13 lines · Junio C Hamano
config.c int git_config_system(void) · line 1567
Quoted 9 lines · Delilah Ashley Wu
From: Delilah Ashley Wu <[email protected]>
config.c +4
-0+static void attempt_git_config_from_file_with_options (config_fn_t fn,static int do_git_config_sequence (const struct config_options *opts, +const struct repository *repo,config_fn_t fn, + void *data, int require_successful_config) {int ret =0 ; +int success_count =0 ;I am not convinced 100% that we need "
success_count", either, until we see how it is used in the later steps.
Good point!! I realised that v2 overcomplicated the error handling. We want to let do_git_config_sequence() optionally bail when both global configuration files could not be read. We can simply set a boolean flag when a global configuration file is successfully read, rather than adding a helper function that tracks the number of successful reads. I've corrected this in v3.
PATCH v2 3/3 config: read global scope via config_sequence
In reply to Delilah Ashley WuFrom: Delilah Ashley Wu <[email protected]>
When both $HOME/.gitconfig and $XDG_CONFIG_HOME/git/config exist, git config list --global and git config get --global read the home configuration file but ignore the XDG file. Bug reporters expected these --global scoped commands to read both files [1][2], which would be consistent with the documentation and the behaviour of the unscoped variants. For example, git config list and git config get (without --global) read from both files (in addition to system-wide and repository-specific entries). We should address this inconsistency by respecting both files during --global read operations.
The implementation assumes that each configuration scope corresponds to a single file. So during --global read operations, Git selects one file path to pass to git_config_from_file_with_options(file). Because the global scope can come from more than one file, we should use another method to read the global configuration.
Since git config list --show-scope --show-origin reads both the home and XDG files, there must be existing code that respects both locations, namely do_git_config_sequence() which reads from all scopes. Introduce flags 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 [3] in the original bug report [1].
Modify tests to check that both configuration files are respected during --global read operations. Also, add additional tests to supplement the regression tests from the previous patch, "config: let sequence require a successful file". The expected behaviour of git config list is:
- 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 when one or more of them is readable.
Implementation notes:
- The ignore_global flag is not set anywhere, so the if (!opts->ignore_global) condition is always met. Include the flag for completeness, but we can remove it if desired.
- 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 clarify this.
[1] https://lore.kernel.org/git/CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com/
[2] https://lore.kernel.org/git/CAAdFe9yhBk-WecVzCTsjQ-4Z3AZAbpP+w+B076ouM3qX6d1WAg@mail.gmail.com/
[3] https://lore.kernel.org/git/[email protected]
--- builtin/config.c | 11 ++++++++ config.c | 37 +++++++++++++++------------ config.h | 2 ++ t/t1300-config.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++ t/t1306-xdg-files.sh | 5 +++- 5 files changed, 109 insertions(+), 17 deletions(-)
builtin/config.c +11 -0
@@ -957,6 +957,17 @@ 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, +* read it using `do_git_config_sequence()` with other scopes +* ignored. 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.file = opts->file_to_free =git_global_config ();if (!opts->source.file)/*
config.c +21 -16
@@ -1587,26 +1587,31 @@ 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 ))attempt_git_config_from_file_with_options (fn, system_config, data,CONFIG_SCOPE_SYSTEM ,NULL , &success_count, &ret); -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 )) -attempt_git_config_from_file_with_options (fn, xdg_config, - data, -CONFIG_SCOPE_GLOBAL , -NULL , &success_count, &ret); +if (xdg_config && !access_or_die (xdg_config,R_OK ,ACCESS_EACCES_OK )) +attempt_git_config_from_file_with_options (fn, xdg_config, + data, +CONFIG_SCOPE_GLOBAL , +NULL , &success_count, &ret); -if (user_config && !access_or_die (user_config,R_OK ,ACCESS_EACCES_OK )) -attempt_git_config_from_file_with_options (fn, user_config, - data, -CONFIG_SCOPE_GLOBAL , -NULL , &success_count, &ret); +if (user_config && !access_or_die (user_config,R_OK ,ACCESS_EACCES_OK )) +attempt_git_config_from_file_with_options (fn, user_config, + data, +CONFIG_SCOPE_GLOBAL , +NULL , &success_count, &ret); + +free (xdg_config); +free (user_config); + }if (!opts->ignore_repo && repo_config && !access_or_die (repo_config,R_OK ,0 ))
@@ -1624,8 +1629,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);
@@ -1659,7 +1662,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 );
@@ -1667,7 +1671,8 @@ 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,0 ); + ret =do_git_config_sequence (opts, repo, fn, data, + config_source && config_source->scope ==CONFIG_SCOPE_GLOBAL ); }if (inc.remote_urls) {
config.h +2 -0
@@ -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 ;
t/t1300-config.sh +71 -0
77 lines not on this page · raw message
t/t1306-xdg-files.sh +4 -1
@@ -52,6 +52,8 @@ test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' ' echo " name = read_gitconfig" >>.gitconfig && echo read_gitconfig >expected && git config --get user.name >actual && + test_cmp expected actual && + git config --global --get user.name >actual && test_cmp expected actual '
@@ -68,7 +70,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 '
Signature · 1 line
2.54.0
Re: [PATCH v2 3/3] config: read global scope via config_sequence
Review
Quoted 11 lines · Delilah Ashley Wu
(quoted diff) +10
-0if (opts->use_global_config) { + /* + * Since global config is sourced from more than one location, + * read it using `do_git_config_sequence()` with other scopes + * ignored. 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;
We used to use ignore_repo, ignore_worktree, and ignore_cmdline members in the config_options, but to ignore system configuration, we relied on git_config_system() that checks GIT_CONFIG_NOSYSTEM environment variable, and there was no way to ignore per-user configuration. From that point of view, I find it sensible to make config_options the primary way to configure which parts of the configuration sequence is disabled.
But then we should go one step further, shouldn't we? Either teach git_config_system() to take config_options struct and pay attention to .ignore_system member in it, or get rid of git_config_system() and have the current users of that function take config_options and pay attention to its .ignore_system member, so that we do not have to write an ugly conditional like this one:
Review
Quoted 5 lines
(quoted diff) +2
-1- if (git_config_system() && system_config && + if (!opts->ignore_system && git_config_system() && system_config && + if (!opts->ignore_global) {
It is a bit misleading that this conditional is always taken. No caller will tell this function to skip the per-user configuration.
Quoted 36 lines
(quoted diff) +17
-3+ git_global_config_paths(&user_config, &xdg_config); + if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) + attempt_git_config_from_file_with_options(fn, xdg_config, + data, + CONFIG_SCOPE_GLOBAL, + NULL, &success_count, &ret); + if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) + attempt_git_config_from_file_with_options(fn, user_config, + data, + CONFIG_SCOPE_GLOBAL, + NULL, &success_count, &ret); + + free(xdg_config); + free(user_config); + }@@ -1624,8 +1629,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);@@ -1659,7 +1662,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);
Review int config_with_options(config_fn_t fn, void *data, · line 1676
Quoted 7 lines
(quoted diff) +2
-1@@ -1667,7 +1671,8 @@ 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, 0); + ret = do_git_config_sequence(opts, repo, fn, data, + config_source && config_source->scope == CONFIG_SCOPE_GLOBAL); }
+100 column wide columns? Please don't.
This sequence is a bit hard to read. Instead of piggybacking on the existing call to do the READL sequencing, add a new "else if" clause to deal specifically with the global case to the cascade would make the result easier to follow, I suspect. Something like this fix-up on top of this patch, perhaps.
config.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
config.c +4 -3
@@ -1663,7 +1663,9 @@ 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 && - config_source->scope !=CONFIG_SCOPE_GLOBAL ) { + config_source->scope ==CONFIG_SCOPE_GLOBAL ) { + ret =do_git_config_sequence (opts, repo, fn, data,1 ); + }else if (config_source && config_source->file) { ret =git_config_from_file_with_options (fn, config_source->file, data, config_source->scope,NULL );
@@ -1671,8 +1673,7 @@ 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, - config_source && config_source->scope ==CONFIG_SCOPE_GLOBAL ); + ret =do_git_config_sequence (opts, repo, fn, data,0 ); }if (inc.remote_urls) {
Quoted 26 lines · Delilah Ashley Wu
Hi all, thanks for your patience. Here's my reroll.
As reported in [1],
$HOME/.gitconfigand$XDG_CONFIG_HOME/git/configare both valid global configuration locations. However, when both files exist,git config list --globalonly reads from the former location whereasgit config list(without--global) reads from both. The same issue was reported forgit config getin [2]. This inconsistency has no good justification and contradicts the documented behaviour.Suppose that
$HOME/.gitconfigcontains:
[home]
config = trueand
$XDG_CONFIG_HOME/git/configcontains:
[xdg]
config = trueThen, listing with
--globalshows only the home config:$ git config list --global --show-scope --show-origin global file:/Users/delilah/.gitconfig home.config=trueand getting the XDG configuration entry with
--globalwill fail:$ git config get --global xdg.config; echo $? 1
Quoted 8 lines
Git still reads the XDG config as part of its effective configuration, as shown by listing the configuration without
--global:$ git config list --show-scope --show-origin global file:/Users/delilah/.config/git/config xdg.config=true global file:/Users/delilah/.gitconfig home.config=trueThe documentation, quoted in [1] and [2], states that
--globalshould read from both files ...
I have a related question: which of the global file(s) does
git config --global --edit
edit? Which one(s) should it edit?
Chris
Quoted 6 lines · Chris Torek
Quoted 6 lines
Git still reads the XDG config as part of its effective configuration, as shown by listing the configuration without
--global:$ 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
Quoted 8 lines
Quoted 2 lines
The documentation, quoted in [1] and [2], states that
--globalshould read from both files ...I have a related question: which of the global file(s) does
git config --global --editedit? Which one(s) should it edit?
I _know_ that having git-config read per-user configuration from both places was a deliberate design choice to help those who choose to migrate away from ~/.gitconfig to the XDG layout, while making sure we do not disrupt those who choose not to migrate.
For the write-out path of "git config --global set var val", we also chose accordingly, knowing that the majority of users back then had their per-user configuration in ~/.gitconfig and some, but not necessarily all, wanted to migrate to the XDG layout, while avoiding writing the same thing twice to different places. Therefore, "git config --global --edit" should follow the choice in the same spirit as the existing write-out code path (and no, I do not think we want to open two files in users' editors).
As to the primary focus of this topic, I think "git config --global" for the read path was not designed as carefully as the write-out code path or the general "git config" sequence when we introduced optional support for the XDG layout. Any discrepancy between "git config" when reading per-user values (to be overridden further by per-repository settings) and what "git config --global" reads from per-user files is very likely not due to any deliberate design choice, but merely bugs caused by a slip of the mind.
v3 cover config: read both home and xdg files for --global
In reply to Delilah Ashley Wu via GitGitGadgetHi all, thanks again for your patience. I think I've addressed all the feedback from Junio's review.
As reported in [1], $HOME/.gitconfig and $XDG_CONFIG_HOME/git/config are both valid global configuration locations. However, when both files exist, git config list --global only reads from the former location whereas git config list (without --global) reads from both. The same issue was reported for git config get in [2]. This inconsistency has no good justification and contradicts the documented behaviour.
Suppose that $HOME/.gitconfig contains:
[home]
config = true
and $XDG_CONFIG_HOME/git/config contains:
[xdg]
config = true
Then, listing with --global shows only the home config:
$ git config list --global --show-scope --show-origin
global file:/Users/delilah/.gitconfig home.config=trueand getting the XDG configuration entry with --global will fail:
$ git config get --global xdg.config; echo $?
1Git still reads the XDG config as part of its effective configuration, as shown by listing the configuration without --global:
$ git config list --show-scope --show-origin
global file:/Users/delilah/.config/git/config xdg.config=true
global file:/Users/delilah/.gitconfig home.config=trueThe documentation, quoted in [1] and [2], states that --global should read from both files, so its output should be the same as above. Here's the relevant excerpt:
Quoted 9 lines
OPTIONS
--global::
For writing options: write to global~/.gitconfigfile rather than the repository.git/config, write to$XDG_CONFIG_HOME/git/configfile if this file exists and the~/.gitconfigfile doesn't.For reading options: read only from global
~/.gitconfigand from$XDG_CONFIG_HOME/git/configrather than from all available files.
To be consistent with the documentation and the behaviour without --global, we should read both configuration files when --global is passed. Writing or editing the global configuration remains unchanged and retains the current destinations as discussed in [3].
[1]: https://lore.kernel.org/git/CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAAdFe9yhBk-WecVzCTsjQ-4Z3AZAbpP+w+B076ouM3qX6d1WAg@mail.gmail.com/
[3]: https://lore.kernel.org/git/[email protected]/
Thanks for your time!
Delilah
---
Changes in v3:
- Drop patch 1 ("path: use forward slashes in XDG config on Windows"),
then set XDG_CONFIG_HOME during a --show-origin test so the path
assertions still pass on Windows.
- Drop implementation changes from v2 patch 2 ("config: let sequence
require a successful file"), leaving its tests as v3 patch 1 ("t1300:
test list with missing global config"). This removes attempt_git_config_from_file_with_options() and success_count in
favour of a new global_had_success flag in v3 patch 2.
- Teach git_config_system() to respect the new ignore_system flag.
- Refactor config_with_options() changes for shorter lines.
- Remove the unused ignore_global flag.
- Test that other scopes are ignored when --global is specified.
- Link to v2: https://patch.msgid.link/20260823-fix-config-list-global-home-and-xdg-v2-0-b29cc63f017b@microsoft.com/
Changes in v2:
- Squash test-only patches into their corresponding implementation patches.
- Reorder patches to prevent a regression from being introduced and
then fixed in a later patch.
- Narrow the scope of slash conversion to xdg_config_home_for() and avoid modifying cleanup_path(), which could've broken callers that
do not expect normalised slashes.
- Clarify that some tests only check the return code of a git config command; we do not care about the output.
- Link to v1: https://patch.msgid.link/[email protected]/
---
Delilah Ashley Wu (2):
t1300: test list with missing global config
config: read global scope via config_sequence
builtin/config.c | 11 +++++ builtin/var.c | 4 +- config.c | 50 +++++++++++++++------- config.h | 3 +- t/t1300-config.sh | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++ t/t1306-xdg-files.sh | 5 ++- 6 files changed, 172 insertions(+), 17 deletions(-)
Range-diff versus v2:
1: 5a2a98c38b < -: ---------- path: use forward slashes in XDG config on Windows
2: 75e1deab27 < -: ---------- config: let sequence require a successful file
-: ---------- > 1: 283725f0c4 t1300: test list with missing global config
3: 4a6f67fdf4 ! 2: 482d05e24d config: read global scope via config_sequence
@@ Commit message
Since `git config list --show-scope --show-origin` reads both the home
and XDG files, there must be existing code that respects both locations,
namely `do_git_config_sequence()` which reads from all scopes. Introduce
- flags 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 [3]
- in the original bug report [1].
+ an `ignore_system` flag and modify `git_config_system()` to respect it.
+ This makes `config_options` the primary way to disable parts of the
+ configuration sequence and allows callers of `do_git_config_sequence()`
+ to ignore all but the global scope (i.e. ignore system, local, worktree,
+ and cmdline). Reuse the function to read only the global scope when
+ `--global` is specified. This was the suggested solution [3] in the
+ original bug report [1].
- Modify tests to check that both configuration files are respected during
- `--global` read operations. Also, add additional tests to supplement the
- regression tests from the previous patch, "config: let sequence require
- a successful file". The expected behaviour of `git config list` is:
- - Without `--global`, it should not bail on unreadable/non-existent
- global config files.
+ The previous patch, "t1300: test list with missing global config",
+ recorded existing behaviour: when no global configuration file exists,
+ `git config list` should succeed whereas `git config list --global`
+ should fail. Since the configuration sequence gracefully ignores missing
+ global configuration files, track whether we successfully read at least
+ one of them and adjust the return code accordingly.
- - 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.
+ Keep populating `opts->source.file` in `builtin/config.c` as the
+ destination for write operations. Read operations use the configuration
+ sequence instead, which can be confusing. Add a comment to explain this
+ distinction.
- Implementation notes:
- - The `ignore_global` flag is not set anywhere, so the
- `if (!opts->ignore_global)` condition is always met. Include the
- flag for completeness, but we can remove it if desired.
-
- - 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 clarify this.
+ Lastly, modify tests to check that both home and XDG configuration files
+ are respected during `--global` read operations.
[1] https://lore.kernel.org/git/CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com/
[2] https://lore.kernel.org/git/CAAdFe9yhBk-WecVzCTsjQ-4Z3AZAbpP+w+B076ouM3qX6d1WAg@mail.gmail.com/
- [3] https://lore.kernel.org/git/[email protected]
+ [3] https://lore.kernel.org/git/[email protected]/
Reported-by: Jade Lovelace <[email protected]>
Reported-by: Nils Fahldieck <[email protected]>
@@ builtin/config.c: static void location_options_init(struct config_location_optio
if (!opts->source.file)
/*
+ ## builtin/var.c ##
+@@ builtin/var.c: static char *git_attr_val_global(int ident_flag UNUSED)
+
+ static char *git_config_val_system(int ident_flag UNUSED)
+ {
+- if (git_config_system()) {
++ const struct config_options opts = { 0 };
++
++ if (git_config_system(&opts)) {
+ char *file = git_system_config();
+ normalize_path_copy(file, file);
+ return file;
+
## config.c ##
+@@ config.c: void git_global_config_paths(char **user_out, char **xdg_out)
+ *xdg_out = xdg_config;
+ }
+
+-int git_config_system(void)
++int git_config_system(const struct config_options *opts)
+ {
+- return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
++ return !opts->ignore_system && !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
+ }
+
+ 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, int require_global_scope_success)
+ {
++ int tmp_ret;
+ int ret = 0;
++ int global_had_success = 0;
+ char *system_config = git_system_config();
+ char *xdg_config = NULL;
+ char *user_config = NULL;
@@ config.c: 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 &&
++ if (git_config_system(opts) && system_config &&
!access_or_die(system_config, R_OK,
opts->system_gently ? ACCESS_EACCES_OK : 0))
- attempt_git_config_from_file_with_options(fn, system_config, data,
- CONFIG_SCOPE_SYSTEM, NULL,
- &success_count, &ret);
+ ret += git_config_from_file_with_options(fn, system_config,
+@@ config.c: static int do_git_config_sequence(const struct config_options *opts,
-- git_global_config_paths(&user_config, &xdg_config);
-+ if (!opts->ignore_global) {
-+ git_global_config_paths(&user_config, &xdg_config);
+ git_global_config_paths(&user_config, &xdg_config);
- if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
-- attempt_git_config_from_file_with_options(fn, xdg_config,
-- data,
-- CONFIG_SCOPE_GLOBAL,
-- NULL, &success_count, &ret);
-+ if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
-+ attempt_git_config_from_file_with_options(fn, xdg_config,
-+ data,
-+ CONFIG_SCOPE_GLOBAL,
-+ NULL, &success_count, &ret);
+- 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)) {
++ tmp_ret = git_config_from_file_with_options(fn, xdg_config,
++ data, CONFIG_SCOPE_GLOBAL,
++ NULL);
++ ret += tmp_ret;
++ if (!tmp_ret)
++ global_had_success = 1;
++ }
- if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
-- attempt_git_config_from_file_with_options(fn, user_config,
-- data,
-- CONFIG_SCOPE_GLOBAL,
-- NULL, &success_count, &ret);
-+ if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
-+ attempt_git_config_from_file_with_options(fn, user_config,
-+ data,
-+ CONFIG_SCOPE_GLOBAL,
-+ NULL, &success_count, &ret);
-+
-+ free(xdg_config);
-+ free(user_config);
+- 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)) {
++ tmp_ret = git_config_from_file_with_options(fn, user_config,
++ data, CONFIG_SCOPE_GLOBAL,
++ NULL);
++ ret += tmp_ret;
++ if (!tmp_ret)
++ global_had_success = 1;
+ }
if (!opts->ignore_repo && repo_config &&
!access_or_die(repo_config, R_OK, 0))
@@ config.c: 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(user_config);
free(repo_config);
free(worktree_config);
++
++ if (require_global_scope_success && !global_had_success && !ret)
++ ret = -1;
++
+ return ret;
+ }
@@ config.c: int config_with_options(config_fn_t fn, void *data,
+ }
+
+ /*
+- * If we have a specific filename, use it. Otherwise, follow the
+- * regular lookup sequence.
++ * Use the specified file when provided, except for the global scope,
++ * which can come from more than one file. With no explicit source,
++ * follow the regular lookup sequence.
*/
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) {
++ config_source->scope == CONFIG_SCOPE_GLOBAL) {
++ ret = do_git_config_sequence(opts, repo, fn, data, 1);
+ } else if (config_source && config_source->file) {
ret = git_config_from_file_with_options(fn, config_source->file,
data, config_source->scope,
- NULL);
@@ config.c: 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, 0);
-+ ret = do_git_config_sequence(opts, repo, fn, data,
-+ config_source && config_source->scope == CONFIG_SCOPE_GLOBAL);
+- ret = do_git_config_sequence(opts, repo, fn, data);
++ ret = do_git_config_sequence(opts, repo, fn, data, 0);
}
if (inc.remote_urls) {
@@ config.h: 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;
+@@ config.h: int repo_config_rename_section(struct repository *, const char *, const char *);
+ int repo_config_rename_section_in_file(struct repository *, const char *, const char *, const char *);
+ int repo_config_copy_section(struct repository *, const char *, const char *);
+ int repo_config_copy_section_in_file(struct repository *, const char *, const char *, const char *);
+-int git_config_system(void);
++int git_config_system(const struct config_options *opts);
+ int config_error_nonbool(const char *);
+ #if defined(__GNUC__)
+ #define config_error_nonbool(s) (config_error_nonbool(s), const_error())
## t/t1300-config.sh ##
@@ t/t1300-config.sh: test_expect_success 'list --global with nonexistent global config fails' '
@@ t/t1300-config.sh: test_expect_success 'list --global with nonexistent global co
+ global file:$HOME/.config/git/config xdg.config=xdg
+ global file:$HOME/.gitconfig home.config=home
+ EOF
-+ git config ${mode_prefix}list --global --show-scope --show-origin >actual &&
++ # Git uses HOME and XDG_CONFIG_HOME to determine the location of the
++ # XDG config file. On Windows, the default values lead to mixed forward
++ # and backward slashes in the output --show-origin path for XDG config.
++ # Explicitly set XDG_CONFIG_HOME with only forward slashes, allowing us
++ # to use the same expected path across all platforms.
++ XDG_CONFIG_HOME="$HOME/.config" \
++ git config ${mode_prefix}list --global \
++ --show-scope --show-origin >actual &&
+ test_cmp expect actual &&
+
+ echo xdg >expect &&
@@ t/t1300-config.sh: test_expect_success 'list --global with nonexistent global co
+ git config ${mode_get} --global home.config >actual &&
+ test_cmp expect actual
+'
++
++test_expect_success 'list --global ignores other scopes' '
++ test_when_finished rm -f system_config &&
++ cat >system_config <<-EOF &&
++ [system]
++ config = true
++ EOF
++
++ test_config core.repositoryformatversion 1 &&
++ test_config extensions.worktreeConfig true &&
++ test_config --worktree worktree.config true &&
++
++ test_when_finished rm -f \"\$HOME\"/.gitconfig &&
++ cat >"$HOME"/.gitconfig <<-EOF &&
++ [home]
++ config = home
++ EOF
++
++ test_config repo.config true &&
++
++ echo "global home.config=home" >expect &&
++ GIT_CONFIG_NOSYSTEM=false GIT_CONFIG_SYSTEM=system_config \
++ git -c cmdline.config=true config ${mode_prefix}list \
++ --global --show-scope >actual &&
++ test_cmp expect actual
++'
+
test_expect_success 'override global and system config' '
test_when_finished rm -f \"\$HOME\"/.gitconfig &&---
base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
change-id: 20260808-fix-config-list-global-home-and-xdg-9bcaac093a1b
PATCH v3 1/2 t1300: test list with missing global config
From: Delilah Ashley Wu <[email protected]>
Record behaviour in two new tests: when no global configuration file exists, git config list succeeds whereas git config list --global fails. Only check the exit code, as we're not interested in the exact error message.
This prevents regressions in the next patch, "config: read global scope via config_sequence", which modifies do_git_config_sequence() to optionally return a failure if no global configuration files were successfully read (i.e. when the --global flag is specified).
--- t/t1300-config.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)
t/t1300-config.sh +12 -0
@@ -2425,6 +2425,18 @@ test_expect_success '--show-scope with --default' ' test_cmp expect actual ' +test_expect_success 'list with nonexistent global config gracefully exits' ' + rm -f "$HOME"/.gitconfig "$HOME"/.config/git/config && + git config ${mode_prefix}list && + git config ${mode_prefix}list --show-scope +' + +test_expect_success 'list --global with nonexistent global config fails' ' + 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 +' + test_expect_success 'override global and system config' ' test_when_finished rm -f \"\$HOME\"/.gitconfig && cat >"$HOME"/.gitconfig <<-EOF &&
Signature · 1 line
2.54.0
Re: [PATCH v3 1/2] t1300: test list with missing global config
Quoted 12 lines · Delilah Ashley Wu
From: Delilah Ashley Wu <[email protected]>
Record behaviour in two new tests: when no global configuration file exists,
git config listsucceeds whereasgit config list --globalfails. Only check the exit code, as we're not interested in the exact error message.This prevents regressions in the next patch, "config: read global scope via
config_sequence", which modifiesdo_git_config_sequence()to optionally return a failure if no global configuration files were successfully read (i.e. when the--globalflag is specified).
Quoted 4 lines
Signed-off-by Delilah Ashley Wu <[email protected]>--- t/t1300-config.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)
OK.
Quoted 9 lines
t/t1300-config.sh +1
-0@@ -2425,6 +2425,18 @@ test_expect_success '--show-scope with --default' ' test_cmp expect actual ' +test_expect_success 'list with nonexistent global config gracefully exits' '
t/t1300-config.sh test_expect_success '--show-scope with --default' ' · line 2438
Quoted 10 lines
t/t1300-config.sh +10
-0+ rm -f "$HOME"/.gitconfig "$HOME"/.config/git/config && + git config ${mode_prefix}list && + git config ${mode_prefix}list --show-scope +' + +test_expect_success 'list --global with nonexistent global config fails' ' + 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 +'
Do we know that XDG stuff is always ~/.config/git/config? Other tests that deal with XDG config files like t0007 seem to make sure XDG_CONFIG_HOME is at a place known to the tests by setting and exporting XDG_CONFIG_HOME; perhaps it does not matter?
Quoted 3 lines
t/t1300-config.sh +0
-0test_expect_success 'override global and system config' ' test_when_finished rm -f \"\$HOME\"/.gitconfig && cat >"$HOME"/.gitconfig <<-EOF &&
PATCH v3 2/2 config: read global scope via config_sequence
In reply to Delilah Ashley WuFrom: Delilah Ashley Wu <[email protected]>
When both $HOME/.gitconfig and $XDG_CONFIG_HOME/git/config exist, git config list --global and git config get --global read the home configuration file but ignore the XDG file. Bug reporters expected these --global scoped commands to read both files [1][2], which would be consistent with the documentation and the behaviour of the unscoped variants. For example, git config list and git config get (without --global) read from both files (in addition to system-wide and repository-specific entries). We should address this inconsistency by respecting both files during --global read operations.
The implementation assumes that each configuration scope corresponds to a single file. So during --global read operations, Git selects one file path to pass to git_config_from_file_with_options(file). Because the global scope can come from more than one file, we should use another method to read the global configuration.
Since git config list --show-scope --show-origin reads both the home and XDG files, there must be existing code that respects both locations, namely do_git_config_sequence() which reads from all scopes. Introduce an ignore_system flag and modify git_config_system() to respect it. This makes config_options the primary way to disable parts of the configuration sequence and allows callers of do_git_config_sequence() to ignore all but the global scope (i.e. ignore system, local, worktree, and cmdline). Reuse the function to read only the global scope when --global is specified. This was the suggested solution [3] in the original bug report [1].
The previous patch, "t1300: test list with missing global config", recorded existing behaviour: when no global configuration file exists, git config list should succeed whereas git config list --global should fail. Since the configuration sequence gracefully ignores missing global configuration files, track whether we successfully read at least one of them and adjust the return code accordingly.
Keep populating opts->source.file in builtin/config.c as the destination for write operations. Read operations use the configuration sequence instead, which can be confusing. Add a comment to explain this distinction.
Lastly, modify tests to check that both home and XDG configuration files are respected during --global read operations.
[1] https://lore.kernel.org/git/CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com/
[2] https://lore.kernel.org/git/CAAdFe9yhBk-WecVzCTsjQ-4Z3AZAbpP+w+B076ouM3qX6d1WAg@mail.gmail.com/
[3] https://lore.kernel.org/git/[email protected]/
--- builtin/config.c | 11 ++++++ builtin/var.c | 4 +- config.c | 50 ++++++++++++++++++------- config.h | 3 +- t/t1300-config.sh | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++ t/t1306-xdg-files.sh | 5 ++- 6 files changed, 160 insertions(+), 17 deletions(-)
builtin/config.c +11 -0
@@ -957,6 +957,17 @@ 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, +* read it using `do_git_config_sequence()` with other scopes +* ignored. 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.file = opts->file_to_free =git_global_config ();if (!opts->source.file)/*
builtin/var.c +3 -1
@@ -82,7 +82,9 @@ static char *git_attr_val_global(int ident_flag UNUSED) static char *git_config_val_system(int ident_flag UNUSED) { -if (git_config_system ()) { +const struct config_options opts = {0 }; + +if (git_config_system (&opts)) {char *file =git_system_config ();normalize_path_copy (file, file);return file;
config.c +36 -14
96 lines not on this page · raw message
config.h +2 -1
@@ -88,6 +88,7 @@ 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_repo :1 ;unsigned int ignore_worktree :1 ;unsigned int ignore_cmdline :1 ;
@@ -423,7 +424,7 @@ int repo_config_rename_section(struct repository *, const char *, const char *);int repo_config_rename_section_in_file (struct repository *,const char *,const char *,const char *);int repo_config_copy_section (struct repository *,const char *,const char *);int repo_config_copy_section_in_file (struct repository *,const char *,const char *,const char *); -int git_config_system (void ); +int git_config_system (const struct config_options *opts);int config_error_nonbool (const char *);#if defined(__GNUC__)#define config_error_nonbool (s) (config_error_nonbool(s), const_error())
t/t1300-config.sh +104 -0
110 lines not on this page · raw message
t/t1306-xdg-files.sh +4 -1
@@ -52,6 +52,8 @@ test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' ' echo " name = read_gitconfig" >>.gitconfig && echo read_gitconfig >expected && git config --get user.name >actual && + test_cmp expected actual && + git config --global --get user.name >actual && test_cmp expected actual '
@@ -68,7 +70,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 '
Signature · 1 line
2.54.0
Re: [PATCH v3 2/2] config: read global scope via config_sequence
Quoted 10 lines · Delilah Ashley Wu
The implementation assumes that each configuration scope corresponds to a single file. So during
--globalread operations, Git selects one file path to pass togit_config_from_file_with_options(file). Because the global scope can come from more than one file, we should use another method to read the global configuration.Since
git config list --show-scope --show-originreads both the home and XDG files, there must be existing code that respects both locations, namelydo_git_config_sequence()which reads from all scopes. Introduce anignore_systemflag and modifygit_config_system()to respect it.
We do not really need to listen to your story of discovery. If you wrote this via LLM, perhaps you can ask the same LLM to tone down the verbosity a bit and shorten it by say a quarter or a half?
Quoted 20 lines
builtin/config.c +10
-0@@ -957,6 +957,17 @@ 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, +* read it using `do_git_config_sequence()` with other scopes +* ignored. 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 ;
builtin/config.c int config_with_options(config_fn_t fn, void *data, · line 1685
Quoted 6 lines
builtin/config.c +1
-1@@ -1659,7 +1681,7 @@ 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,0 ); }
THe idea to let the config sequence machinery to read only those two per-user configuration files in turn does look sensible.
builtin/config.c int config_with_options(config_fn_t fn, void *data,
Quoted 6 lines
builtin/config.c +2
-2-int git_config_system (void ) +int git_config_system (const struct config_options *opts) { -return !git_env_bool ("GIT_CONFIG_NOSYSTEM" ,0 ); +return !opts->ignore_system && !git_env_bool ("GIT_CONFIG_NOSYSTEM" ,0 ); }
Existing callers of this function have all been changed to take a non-NULL opts argument. I am wondering if we want
if (!opts) BUG(...);
to make sure callers don't pass NULL by mistake.