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

[Junio C Hamano <gitster@pobox.com>]

Delilah Ashley Wu <delilahwu@linux.microsoft.com> 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) {