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

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

[Jade Lovelace]

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:
> When reading, the values are read from the system, global and repository local configuration files by default, and options --system, --global, --local, --worktree and --file can be used to tell the command to read from only that location (see the section called “FILES”).

> FILES
>        $XDG_CONFIG_HOME/git/config, ~/.gitconfig
>           User-specific configuration files. When the XDG_CONFIG_HOME environment variable is not set or empty, $HOME/.config/ is used as $XDG_CONFIG_HOME.
>
>           These are also called "global" configuration files. If both files exist, both files are read in the order given above.

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

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

[Junio C Hamano]

Jade Lovelace <lists@jade.fyi> writes:

> 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

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

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

[Jade Lovelace]

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:
>
> Jade Lovelace <lists@jade.fyi> writes:
>
> > 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
>
> With "--get" replaced with "--get-all", what do you see?
>

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

[Junio C Hamano]

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

> Jade Lovelace <lists@jade.fyi> writes:
>
>> 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
>
> With "--get" replaced with "--get-all", what do you see?

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.

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

[Junio C Hamano]

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

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.

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

[Glen Choo]

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

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

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

> I mostly agree, except that I think we should leverage the existing
> "ignore" flags in "config_options".

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.