Re: [PATCH v2 3/3] config: read global scope via config_sequence

Delilah Ashley Wu [off-list ref] 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);
+	}

+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) {