From: Junio C Hamano Date: Wed, 26 Aug 2026 18:38:03 GMT Subject: Re: [PATCH v2 3/3] config: read global scope via config_sequence Message-ID: In-Reply-To: <20260823-fix-config-list-global-home-and-xdg-v2-3-b29cc63f017b@microsoft.com> Delilah Ashley Wu writes: > 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; 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: > - 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. > + 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); > @@ -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(-) diff --git c/config.c w/config.c index acad89102d..bf77f847c3 100644 --- c/config.c +++ w/config.c @@ -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) {