From f6819185fa758da4a26e587f3b790ed1ace12836 Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 17:32:50 +1000 Subject: [PATCH 1/3] config: test home and xdg files in `list --global` The `git config list --global` output includes `$HOME/.gitconfig`, but ignores `$XDG_CONFIG_HOME/git/config`. It should include both files. Modify tests to check for this expected behavior: A. `git config list --global` should include contents from both the home and XDG config locations (assuming they are readable), not just the former. Also, add tests to ensure that the subsequent patches do not introduce regressions on the behavior of `git config list`: B. The home config should take precedence over the XDG config. C. Without `--global`, it should not bail on unreadable/non-existent global config files. D. 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. Signed-off-by: Delilah Ashley Wu --- t/t1300-config.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++ t/t1306-xdg-files.sh | 3 +- 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 51a85e83c27b13..1aafa5b5cb29ea 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 xdg.config=true + global home.config=true + EOF + git config ${mode_prefix}list --global --show-scope >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..475bd26abaaa81 100755 --- a/t/t1306-xdg-files.sh +++ b/t/t1306-xdg-files.sh @@ -68,7 +68,8 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' >.gitconfig && echo "[user]" >.gitconfig && echo " name = read_gitconfig" >>.gitconfig && - echo user.name=read_gitconfig >expected && + echo user.name=read_config >expected && + echo user.name=read_gitconfig >>expected && git config --global --list >actual && test_cmp expected actual ' From c97026c5034ec2b80973bd1ae53b1ab34eade1de Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 16:32:15 +1000 Subject: [PATCH 2/3] config: read from config sequence for global scope The output of `git config list --global` should include both `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config`, but it only reads from the former. We've assumed each config scope corresponds to a single config file location. Because of 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 source file. Since the global scope includes more than one file, we should use another method to read the global config. The output of `git config list --show-scope --show-origin` 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 that make it possible to ignore all but the global scope (i.e. ignore system, local, worktree, cmdline). Then, reuse this function for reading only the global scope. This was the suggested solution in the original bug report thread [1]. Note 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. Note 2: It's 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. Note 3: Keep populating `opts->source.file` in `builtin/config.c` because it is used as the destination 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. [1]: https://lore.kernel.org/git/kl6ly1oze7wb.fsf@chooglen-macbookpro.roam.corp.google.com. Reported-by: Jade Lovelace Suggested-by: Glen Choo Signed-off-by: Delilah Ashley Wu --- builtin/config.c | 12 ++++++++++++ config.c | 26 +++++++++++++++----------- config.h | 2 ++ 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index f70d6354772259..0b449674fc58e2 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) { + /* + * Reading config is handled by `config.c#do_git_config_sequence()` + * because the global config can be sourced from multiple files. + * Writing config should point to a single destination as set in + * `opts->source.file` below. + */ + 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 eb60c293ab3133..3f2c8385ce0b20 100644 --- a/config.c +++ b/config.c @@ -2040,22 +2040,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)) @@ -2074,8 +2079,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; @@ -2105,7 +2108,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 29a027748375f1..e83b34c0ebe6a3 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; From a44ef7cc15a9a1aa80007cd04569dde8c15e0d7a Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 16:32:15 +1000 Subject: [PATCH 3/3] config: keep bailing on unreadable global files The 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 introduced a regression in scenario B: running `git config list --global` would not fail when both global config files are unreadable. For example, `GIT_CONFIG_GLOBAL=does-not-exist git config list --global` would exit 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: The logic to determine the exit code does not actually sum the return codes of the underlying operations. Instead, it uses a single decrement operation. If this is undesirable, we can change it to sum the return codes of the underlying operations instead. Signed-off-by: Delilah Ashley Wu --- config.c | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/config.c b/config.c index 3f2c8385ce0b20..9d201ed0d56f19 100644 --- a/config.c +++ b/config.c @@ -2014,11 +2014,13 @@ 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(); + int global_config_success_count = 0; + int nonzero_ret_on_global_config_error = scope == CONFIG_SCOPE_GLOBAL; char *xdg_config = NULL; char *user_config = NULL; char *repo_config; @@ -2050,13 +2052,29 @@ static int do_git_config_sequence(const struct config_options *opts, 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 (nonzero_ret_on_global_config_error && !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 && !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); @@ -2117,7 +2135,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) {