Re: [PATCH v2 3/3] config: read global scope via config_sequence
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 26, 2026, 18:38 UTC
- Message-ID
- <xmqqse40g22c.fsf@gitster.g>
- In-Reply-To
- <20260823-fix-config-list-global-home-and-xdg-v2-3-b29cc63f017b@microsoft.com>
Delilah Ashley Wu <delilahwu@linux.microsoft.com> writes:
Show 11 quoted lines
> 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.
Show 15 quoted lines
> + 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); > + }
Show 27 quoted lines
> @@ -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) {