{"thread":{"id":"59228","subject":"BUG: git config --global --get ITEM ignores ~/.config/git/config when ~/.gitconfig is present","startedAt":"2023-02-11T00:39:32Z","lastAt":"2023-02-15T16:20:10Z","messageCount":7,"participants":["Jade Lovelace","Junio C Hamano","Glen Choo"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"471953","messageId":"CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com","threadId":"59228","inReplyTo":null,"subject":"BUG: git config --global --get ITEM ignores ~/.config/git/config when ~/.gitconfig is present","fromName":"Jade Lovelace","fromEmail":"lists@jade.fyi","sentAt":"2023-02-11T00:39:17Z","receivedAt":"2023-02-11T00:39:32Z","isPatch":false,"sender":{"key":"lists@jade.fyi","avatar":null},"body":"Dear maintainers,\n\nI have found what I think is a bug or at least a documentation flaw in\ngit-config: when both ~/.gitconfig and ~/.config/git/config exist,\n`git config` can retrieve items set in the latter, but not with the\n`--global` flag set. For context, I use ~/.gitconfig as a\nnon-checked-in machine-specific config, and ~/.config/git/config as my\nchecked-in all-machines configuration file.\n\nThe documentation states:\n> 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”).\n\n> FILES\n>        $XDG_CONFIG_HOME/git/config, ~/.gitconfig\n>           User-specific configuration files. When the XDG_CONFIG_HOME environment variable is not set or empty, $HOME/.config/ is used as $XDG_CONFIG_HOME.\n>\n>           These are also called \"global\" configuration files. If both files exist, both files are read in the order given above.\n\nBased on this documentation, I would expect `--global` to consider\nboth global configuration files, but it does not.\n\nReproduction:\n\nSet some setting in ~/.config/git/config, say:\n[pull]\nff = only\n\nEnsure that ~/.gitconfig exists.\n\nThen:\n\n » git config --global --get user.name\n » git config --show-scope --show-origin --get user.name\nglobal  file:/home/jade/.config/git/config      Jade Lovelace\n\n» git --version\ngit version 2.39.1\n\nRegards,\nJade\n"},{"id":"471954","messageId":"xmqqsffdf0ji.fsf@gitster.g","threadId":"59228","inReplyTo":"CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com","subject":"Re: BUG: git config --global --get ITEM ignores ~/.config/git/config when ~/.gitconfig is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-11T01:33:53Z","receivedAt":"2023-02-11T01:33:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jade Lovelace <lists@jade.fyi> writes:\n\n> Then:\n>\n>  » git config --global --get user.name\n>  » git config --show-scope --show-origin --get user.name\n> global  file:/home/jade/.config/git/config      Jade Lovelace\n\nWith \"--get\" replaced with \"--get-all\", what do you see?\n\n"},{"id":"471956","messageId":"CAFA9we---t=uMy3LmxnU0Hz_9XAR-YFsDqDX16CoNpkiPitFnA@mail.gmail.com","threadId":"59228","inReplyTo":"xmqqsffdf0ji.fsf@gitster.g","subject":"Re: BUG: git config --global --get ITEM ignores ~/.config/git/config when ~/.gitconfig is present","fromName":"Jade Lovelace","fromEmail":"jade@jade.fyi","sentAt":"2023-02-11T01:44:13Z","receivedAt":"2023-02-11T01:44:28Z","isPatch":false,"sender":{"key":"jade@jade.fyi","avatar":null},"body":"Thanks for the reply.\n\n» git config --get-all --global user.name ; echo $#\n0\n\nSame result, no output, exit code zero.\n\nJade\n\nOn Fri, Feb 10, 2023 at 5:33 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jade Lovelace <lists@jade.fyi> writes:\n>\n> > Then:\n> >\n> >  » git config --global --get user.name\n> >  » git config --show-scope --show-origin --get user.name\n> > global  file:/home/jade/.config/git/config      Jade Lovelace\n>\n> With \"--get\" replaced with \"--get-all\", what do you see?\n>\n"},{"id":"471957","messageId":"xmqqmt5lezi3.fsf@gitster.g","threadId":"59228","inReplyTo":"xmqqsffdf0ji.fsf@gitster.g","subject":"Re: BUG: git config --global --get ITEM ignores ~/.config/git/config when ~/.gitconfig is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-11T01:56:20Z","receivedAt":"2023-02-11T01:56:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jade Lovelace <lists@jade.fyi> writes:\n>\n>> Then:\n>>\n>>  » git config --global --get user.name\n>>  » git config --show-scope --show-origin --get user.name\n>> global  file:/home/jade/.config/git/config      Jade Lovelace\n>\n> With \"--get\" replaced with \"--get-all\", what do you see?\n\nAh, nevermind.  With \"--global\", we seem to read from only one,\ngiving the ~/.gitconfig precedence over $XDG/git/config, even though\nwithout \"--global\", we will read from both.\n\nThe code has been behaving like so since its inception at 21cf3227\n(config: read (but not write) from $XDG_CONFIG_HOME/git/config file,\n2012-06-22).  I am not sure if this was designed to behave like so\nfor a reason (which unfortunately is not clear in the log message of\nthe commit), or a bug that was caused by the authors who were too\nfocused on the writing side of the equation (which must pick just\none file to write to).\n\nMy gut feeling is that this is merely a bug that we can fix without\nworrying too much about users screaming at us complaining that they\nrelied on the current behaviour.  Without --global we do read from\nboth, so with with \"--global\" the behaviour is inconsistent.\n\nThanks for a report.\n"},{"id":"471969","messageId":"xmqqzg9kew1q.fsf@gitster.g","threadId":"59228","inReplyTo":"xmqqmt5lezi3.fsf@gitster.g","subject":"Re: BUG: git config --global --get ITEM ignores ~/.config/git/config when ~/.gitconfig is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-11T03:10:57Z","receivedAt":"2023-02-11T03:11:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> My gut feeling is that this is merely a bug that we can fix without\n> worrying too much about users screaming at us complaining that they\n> relied on the current behaviour.  Without --global we do read from\n> both, so with with \"--global\" the behaviour is inconsistent.\n\nSo, here is what I think happens, if anybody wants to get their\nhands dirty.\n\nbuiltin/config.c::cmd_config() notices \"--global\", and\ntries to choose between user_config and xdg_config and\npicks one.\n\nThe choice is stored in given_config_source.file\n\nEventually, \"--get\", \"--get-all\", etc. are handled by calling\nbuiltin/config.c::get_value() and that function eventually calls\nconfig.c::config_with_options().\n\nconfig.c::config_with_options(), when config_source.file exists,\nuses only that file.  There is no facility to say \"read from this\none, and also that one\".\n\nWhen the command is called without \"--global\",\ngiven_config_source.file is not set and in that case,\nconfig.c::config_with_options() does the \"config sequence\".\nThis is implemented in config.c::do_git_config_sequence().\n\nWhat is disturbing about this function is that it knows about two\nglobal configuration files, and finds out about these two files by\ncalling the same git_global_config() helper function\nbuiltin/config.c::cmd_config() uses.  However, the logic used to\ndecide if the file(s) are actually attempted to be read (e.g.  it is\nnot a configuration error to lack ~/.gitconfig) is slightly\ndifferent.  Ideally, it would be very nice if the high level caller\nin cmd_config() loses the duplicated logic and instead just sets a\nsingle \"we are dealing with --global\" bit in given_config_source\nstructure, and config_with_options() is taught to reuse the \"we need\nto read both of them\" logic in do_git_config_sequence() when the bit\nis set.\n\n\n"},{"id":"472126","messageId":"kl6ly1oze7wb.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59228","inReplyTo":"xmqqzg9kew1q.fsf@gitster.g","subject":"Re: BUG: git config --global --get ITEM ignores ~/.config/git/config when ~/.gitconfig is present","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-02-15T06:53:56Z","receivedAt":"2023-02-15T06:54:02Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> My gut feeling is that this is merely a bug that we can fix without\n>> worrying too much about users screaming at us complaining that they\n>> relied on the current behaviour.  Without --global we do read from\n>> both, so with with \"--global\" the behaviour is inconsistent.\n>\n> So, here is what I think happens, if anybody wants to get their\n> hands dirty.\n>\n> builtin/config.c::cmd_config() notices \"--global\", and\n> tries to choose between user_config and xdg_config and\n> picks one.\n>\n> The choice is stored in given_config_source.file\n\nYeah, I think this is the crux of the problem. builtin/config.c assumes\nthat there should be a unique \"given_config_source\", but in the case of\nreading global config, there are actually two \"config_source\"s.\n\n>            Ideally, it would be very nice if the high level caller\n> in cmd_config() loses the duplicated logic and instead just sets a\n> single \"we are dealing with --global\" bit in given_config_source\n> structure, and config_with_options() is taught to reuse the \"we need\n> to read both of them\" logic in do_git_config_sequence() when the bit\n> is set.\n\nI mostly agree, except that I think we should leverage the existing\n\"ignore\" flags in \"config_options\". \"do_git_config_sequence()\" already\nuses some of them to mean \"read from X but not from Y\" (e.g. to\nimplement very early config, protected config), so we could add the\nmissing ignore flags like so...\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\n  diff --git a/config.c b/config.c\n  index e9d52ee7b2..6734d16ce1 100644\n  --- a/config.c\n  +++ b/config.c\n  @@ -2184,19 +2184,23 @@ static int do_git_config_sequence(const struct config_options *opts,\n      repo_config = NULL;\n\n    current_parsing_scope = CONFIG_SCOPE_SYSTEM;\n  -\tif (git_config_system() && system_config &&\n  +\tif (!opts->ignore_system && git_config_system() && system_config &&\n        !access_or_die(system_config, R_OK,\n          opts->system_gently ? ACCESS_EACCES_OK : 0))\n      ret += git_config_from_file(fn, system_config, data);\n\n    current_parsing_scope = CONFIG_SCOPE_GLOBAL;\n  -\tgit_global_config(&user_config, &xdg_config);\n  +\tif (!opts->ignore_global) {\n  +\t\tgit_global_config(&user_config, &xdg_config);\n\n  -\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))\n  -\t\tret += git_config_from_file(fn, xdg_config, data);\n  +\t\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))\n  +\t\t\tret += git_config_from_file(fn, xdg_config, data);\n\n  -\tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))\n  -\t\tret += git_config_from_file(fn, user_config, data);\n  +\t\tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))\n  +\t\t\tret += git_config_from_file(fn, user_config, data);\n  +\t\tfree(xdg_config);\n  +\t\tfree(user_config);\n  +\t}\n\n    current_parsing_scope = CONFIG_SCOPE_LOCAL;\n    if (!opts->ignore_repo && repo_config &&\n  @@ -2217,8 +2221,6 @@ static int do_git_config_sequence(const struct config_options *opts,\n\n    current_parsing_scope = prev_parsing_scope;\n    free(system_config);\n  -\tfree(xdg_config);\n  -\tfree(user_config);\n    free(repo_config);\n    return ret;\n  }\n  diff --git a/config.h b/config.h\n  index 8fb7a89875..be7b7ddd6c 100644\n  --- a/config.h\n  +++ b/config.h\n  @@ -86,6 +86,8 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type,\n\n  struct config_options {\n    unsigned int respect_includes : 1;\n  +\tunsigned int ignore_system : 1;\n  +\tunsigned int ignore_global : 1;\n    unsigned int ignore_repo : 1;\n    unsigned int ignore_worktree : 1;\n    unsigned int ignore_cmdline : 1;\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\nAnd then builtin/config.c can leave the \"config_source\" arg unset, and\npass those flags, e.g. here's an untested, dirty hack:\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\n  diff --git a/builtin/config.c b/builtin/config.c\n  index 060cf9f3e0..495d56274d 100644\n  --- a/builtin/config.c\n  +++ b/builtin/config.c\n  @@ -312,6 +312,20 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n    return format_config(&values->items[values->nr++], key_, value_);\n  }\n\n  +static int read_scoped_config(config_fn_t cb, void *data)\n  +{\n  +\tif (use_global_config) {\n  +\t\tconfig_options.ignore_repo = 1;\n  +\t\tconfig_options.ignore_cmdline = 1;\n  +\t\tconfig_options.ignore_worktree = 1;\n  +\t\tconfig_options.ignore_system = 1;\n  +\t\treturn config_with_options(cb, data,\n  +\t\t\t\tNULL, &config_options);\n  +\n  +\t}\n  +\treturn config_with_options(cb, data, &given_config_source, &config_options);\n  +}\n  +\n  static int get_value(const char *key_, const char *regex_, unsigned flags)\n  {\n    int ret = CONFIG_GENERIC_ERROR;\n  @@ -366,8 +380,7 @@ static int get_value(const char *key_, const char *regex_, unsigned flags)\n      }\n    }\n\n  -\tconfig_with_options(collect_config, &values,\n  -\t\t\t    &given_config_source, &config_options);\n  +\tread_scoped_config(collect_config, &values);\n\n    if (!values.nr && default_value) {\n      struct strbuf *item;\n  @@ -477,8 +490,7 @@ static void get_color(const char *var, const char *def_color)\n    get_color_slot = var;\n    get_color_found = 0;\n    parsed_color[0] = '\\0';\n  -\tconfig_with_options(git_get_color_config, NULL,\n  -\t\t\t    &given_config_source, &config_options);\n  +\tread_scoped_config(git_get_color_config, NULL);\n\n    if (!get_color_found && def_color) {\n      if (color_parse(def_color, parsed_color) < 0)\n  @@ -509,8 +521,7 @@ static int get_colorbool(const char *var, int print)\n    get_colorbool_found = -1;\n    get_diff_color_found = -1;\n    get_color_ui_found = -1;\n  -\tconfig_with_options(git_get_colorbool_config, NULL,\n  -\t\t\t    &given_config_source, &config_options);\n  +\tread_scoped_config(git_get_colorbool_config, NULL);\n\n    if (get_colorbool_found < 0) {\n      if (!strcmp(get_colorbool_slot, \"color.diff\"))\n  @@ -598,8 +609,7 @@ static int get_urlmatch(const char *var, const char *url)\n      show_keys = 1;\n    }\n\n  -\tconfig_with_options(urlmatch_config_entry, &config,\n  -\t\t\t    &given_config_source, &config_options);\n  +\tread_scoped_config(urlmatch_config_entry, &config);\n\n    ret = !values.nr;\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\nI call it dirty because they're plastering over the real problem, which\nis that \"given_config_source\" has stopped being a good mental model once\nwe added the second global file, so the better long term fix would be to\noverhaul builtin/config.c to reflect that (probably distinguishing\nbetween config sources for reading and writing).\n"},{"id":"472140","messageId":"xmqqedqq296u.fsf@gitster.g","threadId":"59228","inReplyTo":"kl6ly1oze7wb.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: BUG: git config --global --get ITEM ignores ~/.config/git/config when ~/.gitconfig is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-15T16:19:05Z","receivedAt":"2023-02-15T16:20:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo <chooglen@google.com> writes:\n\n> I mostly agree, except that I think we should leverage the existing\n> \"ignore\" flags in \"config_options\".\n\nExcellent.\n\nThe option, e.g. --system, does not have to translate to \"let's see\nwhich file does the option mean, OK, it is /etc/gitconfig so do the\nsame thing as --file /etc/gitconfig\".  Instead, we can treat it as\n\"do the usual config sequence but ignore everything but the system\nconfig\".  And that would handle a class like --global that can have\nmore than one input source in a lot more natural way.\n\nVery good.\n"}]}