BUG: git config --global --get ITEM ignores ~/.config/git/config when ~/.gitconfig is present

Dear maintainers,

I have found what I think is a bug or at least a documentation flaw in git-config: when both ~/.gitconfig and ~/.config/git/config exist, `git config` can retrieve items set in the latter, but not with the `--global` flag set. For context, I use ~/.gitconfig as a non-checked-in machine-specific config, and ~/.config/git/config as my checked-in all-machines configuration file.

The documentation states:

Based on this documentation, I would expect `--global` to consider both global configuration files, but it does not.

Reproduction:

Set some setting in ~/.config/git/config, say: [pull] ff = only

Ensure that ~/.gitconfig exists.

Then:

 » git config --global --get user.name
 » git config --show-scope --show-origin --get user.name
global  file:/home/jade/.config/git/config      Jade Lovelace

» git --version git version 2.39.1

Regards, Jade

Jade Lovelace <lists@jade.fyi> writes:

With "--get" replaced with "--get-all", what do you see?

Thanks for the reply.

» git config --get-all --global user.name ; echo $# 0

Same result, no output, exit code zero.

Jade

On Fri, Feb 10, 2023 at 5:33 PM Junio C Hamano <gitster@pobox.com> wrote:

Junio C Hamano <gitster@pobox.com> writes:

Ah, nevermind. With "--global", we seem to read from only one, giving the ~/.gitconfig precedence over $XDG/git/config, even though without "--global", we will read from both.

The code has been behaving like so since its inception at 21cf3227 (config: read (but not write) from $XDG_CONFIG_HOME/git/config file, 2012-06-22). I am not sure if this was designed to behave like so for a reason (which unfortunately is not clear in the log message of the commit), or a bug that was caused by the authors who were too focused on the writing side of the equation (which must pick just one file to write to).

My gut feeling is that this is merely a bug that we can fix without worrying too much about users screaming at us complaining that they relied on the current behaviour. Without --global we do read from both, so with with "--global" the behaviour is inconsistent.

Thanks for a report.

Junio C Hamano <gitster@pobox.com> writes:

So, here is what I think happens, if anybody wants to get their hands dirty.

builtin/config.c::cmd_config() notices "--global", and tries to choose between user_config and xdg_config and picks one.

The choice is stored in given_config_source.file

Eventually, "--get", "--get-all", etc. are handled by calling builtin/config.c::get_value() and that function eventually calls config.c::config_with_options().

config.c::config_with_options(), when config_source.file exists, uses only that file. There is no facility to say "read from this one, and also that one".

When the command is called without "--global", given_config_source.file is not set and in that case, config.c::config_with_options() does the "config sequence". This is implemented in config.c::do_git_config_sequence().

What is disturbing about this function is that it knows about two global configuration files, and finds out about these two files by calling the same git_global_config() helper function builtin/config.c::cmd_config() uses. However, the logic used to decide if the file(s) are actually attempted to be read (e.g. it is not a configuration error to lack ~/.gitconfig) is slightly different. Ideally, it would be very nice if the high level caller in cmd_config() loses the duplicated logic and instead just sets a single "we are dealing with --global" bit in given_config_source structure, and config_with_options() is taught to reuse the "we need to read both of them" logic in do_git_config_sequence() when the bit is set.

Junio C Hamano <gitster@pobox.com> writes:

Yeah, I think this is the crux of the problem. builtin/config.c assumes that there should be a unique "given_config_source", but in the case of reading global config, there are actually two "config_source"s.

I mostly agree, except that I think we should leverage the existing "ignore" flags in "config_options". "do_git_config_sequence()" already uses some of them to mean "read from X but not from Y" (e.g. to implement very early config, protected config), so we could add the missing ignore flags like so...

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----

  diff --git a/config.c b/config.c
  index e9d52ee7b2..6734d16ce1 100644
  --- a/config.c
  +++ b/config.c
  @@ -2184,19 +2184,23 @@ static int do_git_config_sequence(const struct config_options *opts,
      repo_config = NULL;

    current_parsing_scope = CONFIG_SCOPE_SYSTEM;
  -	if (git_config_system() && system_config &&
  +	if (!opts->ignore_system && git_config_system() && system_config &&
        !access_or_die(system_config, R_OK,
          opts->system_gently ? ACCESS_EACCES_OK : 0))
      ret += git_config_from_file(fn, system_config, data);

    current_parsing_scope = CONFIG_SCOPE_GLOBAL;
  -	git_global_config(&user_config, &xdg_config);
  +	if (!opts->ignore_global) {
  +		git_global_config(&user_config, &xdg_config);

  -	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
  -		ret += git_config_from_file(fn, xdg_config, data);
  +		if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
  +			ret += git_config_from_file(fn, xdg_config, data);

  -	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
  -		ret += git_config_from_file(fn, user_config, data);
  +		if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
  +			ret += git_config_from_file(fn, user_config, data);
  +		free(xdg_config);
  +		free(user_config);
  +	}

    current_parsing_scope = CONFIG_SCOPE_LOCAL;
    if (!opts->ignore_repo && repo_config &&
  @@ -2217,8 +2221,6 @@ static int do_git_config_sequence(const struct config_options *opts,

    current_parsing_scope = prev_parsing_scope;
    free(system_config);
  -	free(xdg_config);
  -	free(user_config);
    free(repo_config);
    return ret;
  }
  diff --git a/config.h b/config.h
  index 8fb7a89875..be7b7ddd6c 100644
  --- a/config.h
  +++ b/config.h
  @@ -86,6 +86,8 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type,

  struct config_options {
    unsigned int respect_includes : 1;
  +	unsigned int ignore_system : 1;
  +	unsigned int ignore_global : 1;
    unsigned int ignore_repo : 1;
    unsigned int ignore_worktree : 1;
    unsigned int ignore_cmdline : 1;

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----

And then builtin/config.c can leave the "config_source" arg unset, and pass those flags, e.g. here's an untested, dirty hack:

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----

  diff --git a/builtin/config.c b/builtin/config.c
  index 060cf9f3e0..495d56274d 100644
  --- a/builtin/config.c
  +++ b/builtin/config.c
  @@ -312,6 +312,20 @@ static int collect_config(const char *key_, const char *value_, void *cb)
    return format_config(&values->items[values->nr++], key_, value_);
  }

  +static int read_scoped_config(config_fn_t cb, void *data)
  +{
  +	if (use_global_config) {
  +		config_options.ignore_repo = 1;
  +		config_options.ignore_cmdline = 1;
  +		config_options.ignore_worktree = 1;
  +		config_options.ignore_system = 1;
  +		return config_with_options(cb, data,
  +				NULL, &config_options);
  +
  +	}
  +	return config_with_options(cb, data, &given_config_source, &config_options);
  +}
  +
  static int get_value(const char *key_, const char *regex_, unsigned flags)
  {
    int ret = CONFIG_GENERIC_ERROR;
  @@ -366,8 +380,7 @@ static int get_value(const char *key_, const char *regex_, unsigned flags)
      }
    }

  -	config_with_options(collect_config, &values,
  -			    &given_config_source, &config_options);
  +	read_scoped_config(collect_config, &values);

    if (!values.nr && default_value) {
      struct strbuf *item;
  @@ -477,8 +490,7 @@ static void get_color(const char *var, const char *def_color)
    get_color_slot = var;
    get_color_found = 0;
    parsed_color[0] = '\0';
  -	config_with_options(git_get_color_config, NULL,
  -			    &given_config_source, &config_options);
  +	read_scoped_config(git_get_color_config, NULL);

    if (!get_color_found && def_color) {
      if (color_parse(def_color, parsed_color) < 0)
  @@ -509,8 +521,7 @@ static int get_colorbool(const char *var, int print)
    get_colorbool_found = -1;
    get_diff_color_found = -1;
    get_color_ui_found = -1;
  -	config_with_options(git_get_colorbool_config, NULL,
  -			    &given_config_source, &config_options);
  +	read_scoped_config(git_get_colorbool_config, NULL);

    if (get_colorbool_found < 0) {
      if (!strcmp(get_colorbool_slot, "color.diff"))
  @@ -598,8 +609,7 @@ static int get_urlmatch(const char *var, const char *url)
      show_keys = 1;
    }

  -	config_with_options(urlmatch_config_entry, &config,
  -			    &given_config_source, &config_options);
  +	read_scoped_config(urlmatch_config_entry, &config);

    ret = !values.nr;

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----

I call it dirty because they're plastering over the real problem, which is that "given_config_source" has stopped being a good mental model once we added the second global file, so the better long term fix would be to overhaul builtin/config.c to reflect that (probably distinguishing between config sources for reading and writing).

Glen Choo <chooglen@google.com> writes:

Excellent.

The option, e.g. --system, does not have to translate to "let's see which file does the option mean, OK, it is /etc/gitconfig so do the same thing as --file /etc/gitconfig". Instead, we can treat it as "do the usual config sequence but ignore everything but the system config". And that would handle a class like --global that can have more than one input source in a lot more natural way.

Very good.