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

[Glen Choo <chooglen@google.com>]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.
>
> 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

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.

>            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.

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).