Re: [PATCH v3 2/2] config: read global scope via config_sequence

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

Delilah Ashley Wu <delilahwu@linux.microsoft.com> writes:

> The implementation assumes that each configuration scope corresponds to
> a single file. So during `--global` read operations, Git selects one
> file path to pass to `git_config_from_file_with_options(file)`. Because
> the global scope can come from more than one file, we should use another
> method to read the global configuration.
>
> Since `git config list --show-scope --show-origin` reads both the home
> and XDG files, there must be existing code that respects both locations,
> namely `do_git_config_sequence()` which reads from all scopes. Introduce
> an `ignore_system` flag and modify `git_config_system()` to respect it.

We do not really need to listen to your story of discovery.  If you
wrote this via LLM, perhaps you can ask the same LLM to tone down
the verbosity a bit and shorten it by say a quarter or a half?

> diff --git a/builtin/config.c b/builtin/config.c
> index 2554322317..cdef5ebb00 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -957,6 +957,17 @@ static void location_options_init(struct config_location_options *opts,
>  	}
>  
>  	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;
> ...
> @@ -1659,7 +1681,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);
> +		ret = do_git_config_sequence(opts, repo, fn, data, 0);
>  	}


THe idea to let the config sequence machinery to read only those two
per-user configuration files in turn does look sensible.

> -int git_config_system(void)
> +int git_config_system(const struct config_options *opts)
>  {
> -	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
> +	return !opts->ignore_system && !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
>  }

Existing callers of this function have all been changed to take a
non-NULL opts argument.  I am wondering if we want

	if (!opts)
		BUG(...);

to make sure callers don't pass NULL by mistake.