From c8df6a042b9e971f392b2fd2d09a9c3c655dbceb Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Wed, 17 Sep 2025 12:41:51 +1000 Subject: [PATCH 1/4] cleanup_path: force forward slashes on Windows Git prefers forward slashes as directory separators across all platforms. On Windows, the backslash is the native directory separator, but all Windows versions supported by Git also accept the forward slash in all but rare circumstances. Our tests expect forward slashes. Git generates relative paths with forward slashes. Forward slashes are more convenient to use in shell scripts. For these reasons, we enforced forward slashes in `interpolate_path()` in 5ca6b7bb47b (config --show-origin: report paths with forward slashes, 2016-03-23). However, other code paths may generate paths containing backslashes. For example, `config --show-origin` prints the XDG config path with mixed slashes on Windows: $ git config --list --show-origin file:C:/Program Files/Git/etc/gitconfig system.foo=bar file:"C:\\Users\\delilah/.config/git/config" xdg.foo=bar file:C:/Users/delilah/.gitconfig home.foo=bar file:.git/config local.foo=bar Let's enforce forward slashes in all code paths that directly or indirectly call `cleanup_path()` by modifying it to use `convert_slashes()` on Windows. Since `convert_slashes()` modifies the path in-place, change the argument and return type of `cleanup_path()` from `const char *` to `char *`. All existing callers of `cleanup_path()` pass `char *` anyways, so this change is compatible. The next patch, config: test home and xdg files in `list --global`, will assert that the XDG config path uses forward slashes. Suggested-by: Johannes Schindelin Signed-off-by: Delilah Ashley Wu Reviewed-by: Johannes Schindelin --- path.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/path.c b/path.c index 7f56eaf9930374..db7b94fcda9401 100644 --- a/path.c +++ b/path.c @@ -40,13 +40,17 @@ static struct strbuf *get_pathname(void) return sb; } -static const char *cleanup_path(const char *path) +static char *cleanup_path(char *path) { /* Clean it up */ - if (skip_prefix(path, "./", &path)) { + if (skip_prefix(path, "./", (const char **)&path)) while (*path == '/') path++; - } + +#ifdef GIT_WINDOWS_NATIVE + convert_slashes(path); +#endif + return path; } From d2167a81d31defddbcdda06726b004e44a192f8d Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 17:32:50 +1000 Subject: [PATCH 2/4] config: test home and xdg files in `list --global` The `git config list --global` output includes `$HOME/.gitconfig` (home config), but ignores `$XDG_CONFIG_HOME/git/config` (XDG config). It should include both files. Modify tests to check the following and expect a failure: - `git config list --global` should include contents from both the home and XDG config locations (assuming they are readable), not just the former. - `--show-origin` should print correct paths to both config files, assuming they exist. Also, add tests to ensure subsequent patches do not introduce regressions to `git config list`. Specifically, check that: - The home config should take precedence over the XDG config. - Without `--global`, it should not bail on unreadable/non-existent global config files. - With `--global`, it should bail when both `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config` are unreadable. It should not bail if at least one of them is readable. The next patch, config: read global scope via config_sequence, will implement a fix to include both config files when `--global` is specified. Reported-by: Jade Lovelace Helped-by: Derrick Stolee Signed-off-by: Delilah Ashley Wu Reviewed-by: Johannes Schindelin --- t/t1300-config.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++ t/t1306-xdg-files.sh | 5 ++-- 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index f856821839247e..5fa0111bd95b11 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -2367,6 +2367,71 @@ test_expect_success '--show-scope with --default' ' test_cmp expect actual ' +test_expect_success 'list with nonexistent global config' ' + rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config && + git config ${mode_prefix}list --show-scope +' + +test_expect_success 'list --global with nonexistent global config' ' + rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config && + test_must_fail git config ${mode_prefix}list --global --show-scope +' + +test_expect_success 'list --global with only home' ' + rm -rf "$HOME"/.config/git/config && + + test_when_finished rm -f \"\$HOME\"/.gitconfig && + cat >"$HOME"/.gitconfig <<-EOF && + [home] + config = true + EOF + + cat >expect <<-EOF && + global home.config=true + EOF + git config ${mode_prefix}list --global --show-scope >output && + test_cmp expect output +' + +test_expect_success 'list --global with only xdg' ' + rm -f "$HOME"/.gitconfig && + + test_when_finished rm -rf \"\$HOME\"/.config/git && + mkdir -p "$HOME"/.config/git && + cat >"$HOME"/.config/git/config <<-EOF && + [xdg] + config = true + EOF + + cat >expect <<-EOF && + global xdg.config=true + EOF + git config ${mode_prefix}list --global --show-scope >output && + test_cmp expect output +' + +test_expect_success 'list --global with both home and xdg' ' + test_when_finished rm -f \"\$HOME\"/.gitconfig && + cat >"$HOME"/.gitconfig <<-EOF && + [home] + config = true + EOF + + test_when_finished rm -rf \"\$HOME\"/.config/git && + mkdir -p "$HOME"/.config/git && + cat >"$HOME"/.config/git/config <<-EOF && + [xdg] + config = true + EOF + + cat >expect <<-EOF && + global file:$HOME/.config/git/config xdg.config=true + global file:$HOME/.gitconfig home.config=true + EOF + git config ${mode_prefix}list --global --show-scope --show-origin >output && + ! test_cmp expect output +' + test_expect_success 'override global and system config' ' test_when_finished rm -f \"\$HOME\"/.gitconfig && cat >"$HOME"/.gitconfig <<-EOF && diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh index 40d3c42618c04f..03187557990b36 100755 --- a/t/t1306-xdg-files.sh +++ b/t/t1306-xdg-files.sh @@ -68,9 +68,10 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' >.gitconfig && echo "[user]" >.gitconfig && echo " name = read_gitconfig" >>.gitconfig && - echo user.name=read_gitconfig >expected && + echo user.name=read_config >expected && + echo user.name=read_gitconfig >>expected && git config --global --list >actual && - test_cmp expected actual + ! test_cmp expected actual ' From 9d8af4e6164002b8096fc03fa8189a670133bc77 Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 16:32:15 +1000 Subject: [PATCH 3/4] config: read global scope via config_sequence The output of `git config list --global` should include both the home (`$HOME/.gitconfig`) and XDG (`$XDG_CONFIG_HOME/git/config`) configs, but it only reads from the former. We assumed each config scope corresponds to a single config file. Under this assumption, `git config list --global` reads the global config by calling `git_config_from_file_with_options(...,"~/.gitconfig", ...)`. This function usage restricts us to a single config file. Because the global scope includes two files, we should read the configs via another method. The output of `git config list --show-scope --show-origin` (without `--global`) correctly includes both the home and XDG config files. So there's existing code that respects both locations, namely the `do_git_config_sequence()` function which reads from all scopes. Introduce flags to make it possible to ignore all but the global scope (i.e. ignore system, local, worktree, and cmdline). Then, reuse the function to read only the global scope when `--global` is specified. This was the suggested solution in the bug report: https://lore.kernel.org/git/kl6ly1oze7wb.fsf@chooglen-macbookpro.roam.corp.google.com. Then, modify the tests to check that `git config list --global` includes both home and XDG configs. This patch introduces a regression. If both global config files are unreadable, then `git config list --global` should exit non-zero. This is no longer the case, so mark the corresponding test as a "TODO known breakage" and address the issue in the next patch, config: keep bailing on unreadable global files. Implementation notes: 1. The `ignore_global` flag is not set anywhere, so the `if (!opts->ignore_global)` condition is always met. We can remove this flag if desired. 2. I've assumed that `config_source->scope == CONFIG_SCOPE_GLOBAL` iff `--global` is specified. This comparison determines whether to call `do_git_config_sequence()` for the global scope, or to keep calling `git_config_from_file_with_options()` for other scopes. 3. Keep populating `opts->source.file` in `builtin/config.c` because it is used as the destination config file for write operations. The proposed changes could convolute the code because there is no single source of truth for the config file locations in the global scope. Add a comment to help clarify this. Please let me know if it's unclear. Reported-by: Jade Lovelace Suggested-by: Glen Choo Helped-by: Derrick Stolee Signed-off-by: Delilah Ashley Wu Reviewed-by: Johannes Schindelin --- builtin/config.c | 12 ++++++++++++ config.c | 26 +++++++++++++++----------- config.h | 2 ++ t/t1300-config.sh | 6 +++--- t/t1306-xdg-files.sh | 2 +- 5 files changed, 33 insertions(+), 15 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 59fb113b073926..3fd1bd7f8d7076 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -768,6 +768,18 @@ static void location_options_init(struct config_location_options *opts, } if (opts->use_global_config) { + /* + * Since global config is sourced from more than one location, + * use `config.c#do_git_config_sequence()` with `opts->options` + * to read it. However, writing global config should point to a + * single destination, set in `opts->source.file`. + */ + opts->options.ignore_repo = 1; + opts->options.ignore_cmdline= 1; + opts->options.ignore_worktree = 1; + opts->options.ignore_system = 1; + opts->source.scope = CONFIG_SCOPE_GLOBAL; + opts->source.file = opts->file_to_free = git_global_config(); if (!opts->source.file) /* diff --git a/config.c b/config.c index 74bf76a97e4ede..4b9f3831b14c16 100644 --- a/config.c +++ b/config.c @@ -1526,22 +1526,27 @@ static int do_git_config_sequence(const struct config_options *opts, worktree_config = NULL; } - if (git_config_system() && system_config && + if (!opts->ignore_system && git_config_system() && system_config && !access_or_die(system_config, R_OK, opts->system_gently ? ACCESS_EACCES_OK : 0)) ret += git_config_from_file_with_options(fn, system_config, data, CONFIG_SCOPE_SYSTEM, NULL); - git_global_config_paths(&user_config, &xdg_config); + if (!opts->ignore_global) { + git_global_config_paths(&user_config, &xdg_config); + + if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) + ret += git_config_from_file_with_options(fn, xdg_config, data, + CONFIG_SCOPE_GLOBAL, NULL); - if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) - ret += git_config_from_file_with_options(fn, xdg_config, data, - CONFIG_SCOPE_GLOBAL, NULL); + if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) + ret += git_config_from_file_with_options(fn, user_config, data, + CONFIG_SCOPE_GLOBAL, NULL); - if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) - ret += git_config_from_file_with_options(fn, user_config, data, - CONFIG_SCOPE_GLOBAL, NULL); + free(xdg_config); + free(user_config); + } if (!opts->ignore_repo && repo_config && !access_or_die(repo_config, R_OK, 0)) @@ -1560,8 +1565,6 @@ static int do_git_config_sequence(const struct config_options *opts, die(_("unable to parse command-line config")); free(system_config); - free(xdg_config); - free(user_config); free(repo_config); free(worktree_config); return ret; @@ -1591,7 +1594,8 @@ int config_with_options(config_fn_t fn, void *data, */ if (config_source && config_source->use_stdin) { ret = git_config_from_stdin(fn, data, config_source->scope); - } else if (config_source && config_source->file) { + } else if (config_source && config_source->file && + config_source->scope != CONFIG_SCOPE_GLOBAL) { ret = git_config_from_file_with_options(fn, config_source->file, data, config_source->scope, NULL); diff --git a/config.h b/config.h index 19c87fc0bc1a2a..9425fe115d9863 100644 --- a/config.h +++ b/config.h @@ -87,6 +87,8 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type, struct config_options { unsigned int respect_includes : 1; + unsigned int ignore_system : 1; + unsigned int ignore_global : 1; unsigned int ignore_repo : 1; unsigned int ignore_worktree : 1; unsigned int ignore_cmdline : 1; diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 5fa0111bd95b11..42f256e122e9da 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -2372,7 +2372,7 @@ test_expect_success 'list with nonexistent global config' ' git config ${mode_prefix}list --show-scope ' -test_expect_success 'list --global with nonexistent global config' ' +test_expect_failure 'list --global with nonexistent global config' ' rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config && test_must_fail git config ${mode_prefix}list --global --show-scope ' @@ -2429,7 +2429,7 @@ test_expect_success 'list --global with both home and xdg' ' global file:$HOME/.gitconfig home.config=true EOF git config ${mode_prefix}list --global --show-scope --show-origin >output && - ! test_cmp expect output + test_cmp expect output ' test_expect_success 'override global and system config' ' @@ -2483,7 +2483,7 @@ test_expect_success 'override global and system config' ' test_cmp expect output ' -test_expect_success 'override global and system config with missing file' ' +test_expect_failure 'override global and system config with missing file' ' test_must_fail env GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=/dev/null git config ${mode_prefix}list --global && test_must_fail env GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=does-not-exist git config ${mode_prefix}list --system && GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=does-not-exist git version diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh index 03187557990b36..475bd26abaaa81 100755 --- a/t/t1306-xdg-files.sh +++ b/t/t1306-xdg-files.sh @@ -71,7 +71,7 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' echo user.name=read_config >expected && echo user.name=read_gitconfig >>expected && git config --global --list >actual && - ! test_cmp expected actual + test_cmp expected actual ' From 6119cee0c6557e67f3eb4e2f9d488e8684a63c99 Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 16:32:15 +1000 Subject: [PATCH 4/4] config: keep bailing on unreadable global files The expected behaviour for `git config list` is: A. Without `--global`, it should not bail on unreadable/non-existent global config files. B. With `--global`, it should bail when both `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config` are unreadable. It should not bail when one or more of them is readable. The previous patch, config: read global scope via config_sequence, introduced a regression in scenario B. When both global config files are unreadable, running `git config list --global` would not fail. For example, `GIT_CONFIG_GLOBAL=does-not-exist git config list --global` exits with status code 0. Assuming that `config_source->scope == CONFIG_SCOPE_GLOBAL` iff the `--global` argument is specified, use this to determine whether to bail. When reading only the global scope and both config files are unreadable, then adjust the return code to be non-zero. Note: When bailing, the exit code is not determined by sum of the return codes of the underlying operations. Instead, the exit code is modified via a single decrement. If this is undesirable, we can change it to sum the return codes of the underlying operations instead. Lastly, modify the tests to remove the known breakage/regression. The tests for scenario B will now pass. Helped-by: Derrick Stolee Signed-off-by: Delilah Ashley Wu Reviewed-by: Johannes Schindelin --- config.c | 40 +++++++++++++++++++++++++++++++--------- t/t1300-config.sh | 4 ++-- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/config.c b/config.c index 4b9f3831b14c16..3057c16f59edf0 100644 --- a/config.c +++ b/config.c @@ -1500,8 +1500,8 @@ int git_config_system(void) } static int do_git_config_sequence(const struct config_options *opts, - const struct repository *repo, - config_fn_t fn, void *data) + const struct repository *repo, config_fn_t fn, + void *data, enum config_scope scope) { int ret = 0; char *system_config = git_system_config(); @@ -1534,15 +1534,34 @@ static int do_git_config_sequence(const struct config_options *opts, NULL); if (!opts->ignore_global) { + int global_config_success_count = 0; + int nonzero_ret_on_global_config_error = scope == CONFIG_SCOPE_GLOBAL; + git_global_config_paths(&user_config, &xdg_config); - if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) - ret += git_config_from_file_with_options(fn, xdg_config, data, - CONFIG_SCOPE_GLOBAL, NULL); + if (xdg_config && + !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) { + ret += git_config_from_file_with_options(fn, xdg_config, + data, + CONFIG_SCOPE_GLOBAL, + NULL); + if (!ret) + global_config_success_count++; + } + + if (user_config && + !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) { + ret += git_config_from_file_with_options(fn, user_config, + data, + CONFIG_SCOPE_GLOBAL, + NULL); + if (!ret) + global_config_success_count++; + } - if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) - ret += git_config_from_file_with_options(fn, user_config, data, - CONFIG_SCOPE_GLOBAL, NULL); + if (nonzero_ret_on_global_config_error && + !global_config_success_count) + --ret; free(xdg_config); free(user_config); @@ -1603,7 +1622,10 @@ int config_with_options(config_fn_t fn, void *data, ret = git_config_from_blob_ref(fn, repo, config_source->blob, data, config_source->scope); } else { - ret = do_git_config_sequence(opts, repo, fn, data); + ret = do_git_config_sequence(opts, repo, fn, data, + config_source ? + config_source->scope : + CONFIG_SCOPE_UNKNOWN); } if (inc.remote_urls) { diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 42f256e122e9da..0c3911183cad9f 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -2372,7 +2372,7 @@ test_expect_success 'list with nonexistent global config' ' git config ${mode_prefix}list --show-scope ' -test_expect_failure 'list --global with nonexistent global config' ' +test_expect_success 'list --global with nonexistent global config' ' rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config && test_must_fail git config ${mode_prefix}list --global --show-scope ' @@ -2483,7 +2483,7 @@ test_expect_success 'override global and system config' ' test_cmp expect output ' -test_expect_failure 'override global and system config with missing file' ' +test_expect_success 'override global and system config with missing file' ' test_must_fail env GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=/dev/null git config ${mode_prefix}list --global && test_must_fail env GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=does-not-exist git config ${mode_prefix}list --system && GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=does-not-exist git version