From 8d98751fb10bea3953ea1354e88884b74216ce52 Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 16:32:15 +1000 Subject: [PATCH 1/7] lilah(config): add tracing Do not submit this commit; it's only intended for debugging. Signed-off-by: Delilah Ashley Wu --- builtin/config.c | 5 +++++ config.c | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/builtin/config.c b/builtin/config.c index f70d6354772259..cb78105bf81240 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -14,6 +14,7 @@ #include "setup.h" #include "strbuf.h" #include "worktree.h" +#include "../trace2.h" // todo(delilahwu): Clean up traces static const char *const builtin_config_usage[] = { N_("git config list [] [] [--includes]"), @@ -960,6 +961,8 @@ static int cmd_config_set(int argc, const char **argv, const char *prefix, comment = git_config_prepare_comment_string(comment_arg); + trace2_printf("config", "set", "key=%s, value=%s, comment=%s\n", + argv[0], argv[1], comment); location_options_init(&location_opts, prefix); check_write(&location_opts.source); @@ -1293,6 +1296,8 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) ret = show_editor(&location_opts); } else if (actions == ACTION_SET) { + trace2_printf("action set key=%s, value=%s, comment=%s\n", + argv[0], argv[1], comment); check_write(&location_opts.source); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1], display_opts.type, &default_kvi); diff --git a/config.c b/config.c index b18b5617fcd05d..94ca5f1584d29b 100644 --- a/config.c +++ b/config.c @@ -2083,6 +2083,8 @@ static int do_git_config_sequence(const struct config_options *opts, free(user_config); free(repo_config); free(worktree_config); + + trace2_printf("do_git_config_sequence=%d\n", ret); return ret; } @@ -2104,6 +2106,10 @@ int config_with_options(config_fn_t fn, void *data, data = &inc; } + trace2_printf("config_source->file=%s, config_source->blob=%s\n", + config_source ? config_source->file : NULL, + config_source ? config_source->blob : NULL); + /* * If we have a specific filename, use it. Otherwise, follow the * regular lookup sequence. From 99e712a1b79b3e5d69deb9c6a5fea7660a3d6365 Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 16:32:15 +1000 Subject: [PATCH 2/7] config: refactor file error handling into function Not actually required for the fix. I just wanted to do it hehe. Could always split it out to another patch or keep the current code. Signed-off-by: Delilah Ashley Wu --- builtin/config.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index cb78105bf81240..5e1c42d0d7fb0b 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -834,6 +834,22 @@ static void display_options_init(struct config_display_options *opts) } } +static void handle_nonzero_config_with_options(const struct config_location_options + *location_opts) +{ + // todo(delilahwu): Either un-refactor this back to its original state from + // `master` or clean it up. + const char *err_file = location_opts->source.file; /* ? + location_opts->source.file : + location_opts->source.user_config ? + location_opts->source.user_config : + NULL; */ + if (err_file) + die_errno(_("unable to read config file '%s'"), err_file); + else + die(_("error processing config file(s)")); +} + static int cmd_config_list(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) { @@ -859,11 +875,7 @@ static int cmd_config_list(int argc, const char **argv, const char *prefix, if (config_with_options(show_all_config, &display_opts, &location_opts.source, the_repository, &location_opts.options) < 0) { - if (location_opts.source.file) - die_errno(_("unable to read config file '%s'"), - location_opts.source.file); - else - die(_("error processing config file(s)")); + handle_nonzero_config_with_options(&location_opts); } location_options_release(&location_opts); @@ -1285,11 +1297,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) if (config_with_options(show_all_config, &display_opts, &location_opts.source, the_repository, &location_opts.options) < 0) { - if (location_opts.source.file) - die_errno(_("unable to read config file '%s'"), - location_opts.source.file); - else - die(_("error processing config file(s)")); + handle_nonzero_config_with_options(&location_opts); } } else if (actions == ACTION_EDIT) { From b85f0ed0388fedbf51f0e444d1780c08133362fe Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 16:32:15 +1000 Subject: [PATCH 3/7] config: read from config sequence for global scope Based on https://lore.kernel.org/git/kl6ly1oze7wb.fsf@chooglen-macbookpro.roam.corp.google.com. We assumed that git config is always read from a single file, but that is not the case for the global scope, which takes inputs from both `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config`. The existing `do_git_config_sequence()` function respects both locations, in addition to other scopes. Hence, modify the function to support reading only the global scope, by introducing new flags that exclude everything else (i.e. `ignore_system` and `ignore_global`). I've assumed that `config_source->scope == CONFIG_SCOPE_GLOBAL` iff `--global` is specified. This allows us to call the modified `do_git_config_sequence()` function when reading only the the global scope, rather than calling `git_config_from_file_with_options(..., "~/.gitconfig", ...)`. This fixes the issue where `XDG_CONFIG_HOME` location is ignored in `git config list --global`. NB: The `ignore_global` flag is not set to truthy anywhere, so the `if (!opts->ignore_global)` condition is always met. We can actually remove this unused flag if we want. The `opts->source.file` is still set in `builtin/config.c` because it is required for write operations. I thought that this would be misleading because there is no single source of truth for the source files in the global scope. So I added comments that will (hopefully) clarify this. Reported-by: Jade Lovelace Suggested-by: Glen Choo Signed-off-by: Delilah Ashley Wu --- builtin/config.c | 9 +++++++++ config.c | 26 +++++++++++++++----------- config.h | 2 ++ 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 5e1c42d0d7fb0b..72895e7227b525 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -769,7 +769,16 @@ static void location_options_init(struct config_location_options *opts, } if (opts->use_global_config) { + // Used for reading config: + 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; + + // Used for writing config: opts->source.file = opts->file_to_free = git_global_config(); + // todo(delilahwu): Do we need to move this error handling somewhere else? if (!opts->source.file) /* * It is unknown if HOME/.gitconfig exists, so diff --git a/config.c b/config.c index 94ca5f1584d29b..e594a72f48e66f 100644 --- a/config.c +++ b/config.c @@ -2045,22 +2045,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)) @@ -2079,8 +2084,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); @@ -2116,7 +2119,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..d7e0c4430d8e04 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; // From /etc/gitconfig + unsigned int ignore_global : 1; // From a combination of `~/.gitconfig` and `XDG_CONFIG_HOME/git/config` unsigned int ignore_repo : 1; unsigned int ignore_worktree : 1; unsigned int ignore_cmdline : 1; From 97664f2f0a546b7abd5efea31017d14572ca51c0 Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 16:32:15 +1000 Subject: [PATCH 4/7] config: keep bailing on unreadable global files The expected existing 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 after encountering an unreadable global config file, e.g. `GIT_CONFIG_GLOBAL=does-not-exist git config list --global` would exit with status code 0. Given that `config_source->scope == CONFIG_SCOPE_GLOBAL` iff `--global` is specified, we can use this to determine whether to bail or not. When both global config files are unreadable and we are restricted to reading only the global scope, adjust the return code to be non-zero. Signed-off-by: Delilah Ashley Wu --- config.c | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/config.c b/config.c index e594a72f48e66f..0bb77b0f5be72f 100644 --- a/config.c +++ b/config.c @@ -2019,11 +2019,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 access_success_count = 0; + int nonzero_ret_on_global_access_error = scope == CONFIG_SCOPE_GLOBAL; char *xdg_config = NULL; char *user_config = NULL; char *repo_config; @@ -2055,13 +2057,31 @@ 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) { + if (!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_access_error && !ret) { + ++access_success_count; + } + } + } + + if (user_config) { + if (!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_access_error && !ret) { + ++access_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_access_error && !access_success_count) { + --ret; + } free(xdg_config); free(user_config); @@ -2128,7 +2148,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) { From ace72c40dabfd3415d379d573a894e4d1e7ca31f Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 17:32:50 +1000 Subject: [PATCH 5/7] config: test home and xdg files in `list --global` Checks for regressions in two existing behaviours of `git config list`,as described in the previous commit message: 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. Also adds a test for the issue that this contribution solves. C. `git config list --global` should include contents from both `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config` (assuming they are readable), not just the former. Signed-off-by: Delilah Ashley Wu --- t/t1300-config.sh | 70 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 51a85e83c27b13..524c2e5ff6fef3 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -2367,6 +2367,76 @@ test_expect_success '--show-scope with --default' ' test_cmp expect actual ' +test_expect_success 'list with nonexistent global config' ' + rm -rf "$HOME"/.config/git/config && + rm -f .git/config && + git config ${mode_prefix}list --show-scope +' + +test_expect_success 'list --global with nonexistent global config' ' + rm -rf "$HOME"/.config/git/config && + rm -f .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 && + rm -f .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 .git/config && + + 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' ' + rm -f .git/config && + + 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 && From 8c209c6d45f494b6b2515cca2a12aced0de23826 Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Thu, 22 May 2025 17:57:17 +1000 Subject: [PATCH 6/7] lilah: Add nix flakes for my hot girl git-scm workspace Intentional --no-signoff; this commit should not be included in the final patch series. --- flake.lock | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ flake.nix | 41 ++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+) create mode 100644 flake.lock create mode 100644 flake.nix diff --git a/flake.lock b/flake.lock new file mode 100644 index 00000000000000..065e5e50fcb221 --- /dev/null +++ b/flake.lock @@ -0,0 +1,61 @@ +{ + "nodes": { + "flake-utils": { + "inputs": { + "systems": "systems" + }, + "locked": { + "lastModified": 1731533236, + "narHash": "sha256-l0KFg5HjrsfsO/JpG+r7fRrqm12kzFHyUHqHCVpMMbI=", + "owner": "numtide", + "repo": "flake-utils", + "rev": "11707dc2f618dd54ca8739b309ec4fc024de578b", + "type": "github" + }, + "original": { + "owner": "numtide", + "repo": "flake-utils", + "type": "github" + } + }, + "nixpkgs": { + "locked": { + "lastModified": 1747676747, + "narHash": "sha256-LXkWBVqilgx7Pohwqu/ABxDVw+Cmi5/Mj2S2mpUH0Fw=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "72841a4a8761d1aed92ef6169a636872c986c76d", + "type": "github" + }, + "original": { + "owner": "NixOS", + "ref": "nixos-24.11", + "repo": "nixpkgs", + "type": "github" + } + }, + "root": { + "inputs": { + "flake-utils": "flake-utils", + "nixpkgs": "nixpkgs" + } + }, + "systems": { + "locked": { + "lastModified": 1681028828, + "narHash": "sha256-Vy1rq5AaRuLzOxct8nz4T6wlgyUR7zLU309k9mBC768=", + "owner": "nix-systems", + "repo": "default", + "rev": "da67096a3b9bf56a91d16901293e51ba5b49a27e", + "type": "github" + }, + "original": { + "owner": "nix-systems", + "repo": "default", + "type": "github" + } + } + }, + "root": "root", + "version": 7 +} diff --git a/flake.nix b/flake.nix new file mode 100644 index 00000000000000..ac196bb9d0c389 --- /dev/null +++ b/flake.nix @@ -0,0 +1,41 @@ +{ + inputs = { + nixpkgs.url = "github:NixOS/nixpkgs/nixos-24.11"; + flake-utils.url = "github:numtide/flake-utils"; + }; + outputs = + { + self, + nixpkgs, + flake-utils, + }: + flake-utils.lib.eachDefaultSystem ( + system: + let + overlays = [ ]; + pkgs = import nixpkgs { + inherit system overlays; + }; + in + with pkgs; + { + devShell = mkShell { + buildInputs = [ + # Build tools + autoconf + clang-tools_19 + clang_19 + cmake + ninja + perl + lldb_19 + + # Deps + curl + zlib + expat + ]; + }; + } + ); +} From 936fe976bdc2afcdc2347d4b331578400ba8c40e Mon Sep 17 00:00:00 2001 From: Delilah Ashley Wu Date: Fri, 23 May 2025 10:24:37 +1000 Subject: [PATCH 7/7] Revert "lilah(config): add tracing" This reverts commit 8d98751fb10bea3953ea1354e88884b74216ce52. --- builtin/config.c | 5 ----- config.c | 6 ------ 2 files changed, 11 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 72895e7227b525..24b5cf61827f09 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -14,7 +14,6 @@ #include "setup.h" #include "strbuf.h" #include "worktree.h" -#include "../trace2.h" // todo(delilahwu): Clean up traces static const char *const builtin_config_usage[] = { N_("git config list [] [] [--includes]"), @@ -982,8 +981,6 @@ static int cmd_config_set(int argc, const char **argv, const char *prefix, comment = git_config_prepare_comment_string(comment_arg); - trace2_printf("config", "set", "key=%s, value=%s, comment=%s\n", - argv[0], argv[1], comment); location_options_init(&location_opts, prefix); check_write(&location_opts.source); @@ -1313,8 +1310,6 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) ret = show_editor(&location_opts); } else if (actions == ACTION_SET) { - trace2_printf("action set key=%s, value=%s, comment=%s\n", - argv[0], argv[1], comment); check_write(&location_opts.source); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1], display_opts.type, &default_kvi); diff --git a/config.c b/config.c index 0bb77b0f5be72f..dbf06bc3d3cda2 100644 --- a/config.c +++ b/config.c @@ -2106,8 +2106,6 @@ static int do_git_config_sequence(const struct config_options *opts, free(system_config); free(repo_config); free(worktree_config); - - trace2_printf("do_git_config_sequence=%d\n", ret); return ret; } @@ -2129,10 +2127,6 @@ int config_with_options(config_fn_t fn, void *data, data = &inc; } - trace2_printf("config_source->file=%s, config_source->blob=%s\n", - config_source ? config_source->file : NULL, - config_source ? config_source->blob : NULL); - /* * If we have a specific filename, use it. Otherwise, follow the * regular lookup sequence.