config: read global scope via config_sequence

The output of `git config list --global` should include both the home
(`$HOME/.gitconfig`) and XDG (`$XDG_CONFIG_HOME/git/config`) configs,
but it only reads from the former.

We assumed each config scope corresponds to a single config file. Under
this assumption, `git config list --global` reads the global config by
calling `git_config_from_file_with_options(...,"~/.gitconfig", ...)`.
This function usage restricts us to a single config file. Because the
global scope includes two files, we should read the configs via another
method.

The output of `git config list --show-scope --show-origin` (without
`--global`) correctly includes both the home and XDG config files. So
there's existing code that respects both locations, namely the
`do_git_config_sequence()` function which reads from all scopes.
Introduce flags to make it possible to ignore all but the global scope
(i.e. ignore system, local, worktree, and cmdline). Then, reuse the
function to read only the global scope when `--global` is specified.
This was the suggested solution in the bug report:
https://lore.kernel.org/git/kl6ly1oze7wb.fsf@chooglen-macbookpro.roam.corp.google.com.

Then, modify the tests to check that `git config list --global` includes
both home and XDG configs.

This patch introduces a regression. If both global config files are
unreadable, then `git config list --global` should exit non-zero. This
is no longer the case, so mark the corresponding test as a "TODO known
breakage" and address the issue in the next patch, config: keep bailing
on unreadable global files.

Implementation notes:
  1. The `ignore_global` flag is not set anywhere, so the
     `if (!opts->ignore_global)` condition is always met. We can remove
     this flag if desired.

  2. I've assumed that `config_source->scope == CONFIG_SCOPE_GLOBAL` iff
     `--global` is specified. This comparison determines whether to call
     `do_git_config_sequence()` for the global scope, or to keep calling
     `git_config_from_file_with_options()` for other scopes.

  3. Keep populating `opts->source.file` in `builtin/config.c` because
     it is used as the destination config file for write operations.
     The proposed changes could convolute the code because there is no
     single source of truth for the config file locations in the global
     scope. Add a comment to help clarify this. Please let me know if
     it's unclear.

Reported-by: Jade Lovelace <lists@jade.fyi>
Suggested-by: Glen Choo <glencbz@gmail.com>
Helped-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Delilah Ashley Wu <delilahwu@microsoft.com>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/config.c b/builtin/config.c
index 75852bd79d505983fb109b194143effcf64c8468..166568420bb3d7980384b8cdee02f47386f9be4c 100644 (file)
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -776,6 +776,18 @@ 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,
+                * use `config.c#do_git_config_sequence()` with `opts->options`
+                * to read it. 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;
+               opts->source.scope = CONFIG_SCOPE_GLOBAL;
+
                opts->source.file = opts->file_to_free = git_global_config();
                if (!opts->source.file)
                        /*

diff --git a/config.c b/config.c
index f1def0dcfbacba5fb4f915fbf5c244ffd6e40009..084c62c82db56d7259e841918fe88c5add8d3fde 100644 (file)
--- a/config.c
+++ b/config.c
@@ -1538,22 +1538,27 @@ static int do_git_config_sequence(const struct config_options *opts,
                worktree_config = NULL;
        }
 
-       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_with_options(fn, system_config,
                                                         data, CONFIG_SCOPE_SYSTEM,
                                                         NULL);
 
-       git_global_config_paths(&user_config, &xdg_config);
+       if (!opts->ignore_global) {
+               git_global_config_paths(&user_config, &xdg_config);
+
+               if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
+                       ret += git_config_from_file_with_options(fn, xdg_config, data,
+                                               CONFIG_SCOPE_GLOBAL, NULL);
 
-       if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
-               ret += git_config_from_file_with_options(fn, xdg_config, data,
-                                                        CONFIG_SCOPE_GLOBAL, NULL);
+               if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
+                       ret += git_config_from_file_with_options(fn, user_config, data,
+                                               CONFIG_SCOPE_GLOBAL, NULL);
 
-       if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
-               ret += git_config_from_file_with_options(fn, user_config, data,
-                                                        CONFIG_SCOPE_GLOBAL, NULL);
+               free(xdg_config);
+               free(user_config);
+       }
 
        if (!opts->ignore_repo && repo_config &&
            !access_or_die(repo_config, R_OK, 0))
@@ -1572,8 +1577,6 @@ static int do_git_config_sequence(const struct config_options *opts,
                die(_("unable to parse command-line config"));
 
        free(system_config);
-       free(xdg_config);
-       free(user_config);
        free(repo_config);
        free(worktree_config);
        return ret;
@@ -1603,7 +1606,8 @@ int config_with_options(config_fn_t fn, void *data,
         */
        if (config_source && config_source->use_stdin) {
                ret = git_config_from_stdin(fn, data, config_source->scope);
-       } else if (config_source && config_source->file) {
+       } else if (config_source && config_source->file &&
+                  config_source->scope != CONFIG_SCOPE_GLOBAL) {
                ret = git_config_from_file_with_options(fn, config_source->file,
                                                        data, config_source->scope,
                                                        NULL);

diff --git a/config.h b/config.h
index 19c87fc0bc1a2a8a6a1a7ad6c61c6fab50b6ce44..9425fe115d98630ed2191c82447184e66ad8c45d 100644 (file)
--- a/config.h
+++ b/config.h
@@ -87,6 +87,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;

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 61e44027bca0c098efba4dfbcb62cd36d1a25b48..6eaed6d62cdcf21bbf2d8e58802be75256de028c 100755 (executable)
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2367,7 +2367,7 @@ test_expect_success 'list with nonexistent global config' '
        git config ${mode_prefix}list --show-scope
 '
 
-test_expect_success 'list --global with nonexistent global config' '
+test_expect_failure 'list --global with nonexistent global config' '
        rm -rf "$HOME"/.gitconfig "$HOME"/.config/git/config &&
        test_must_fail git config ${mode_prefix}list --global --show-scope
 '
@@ -2424,7 +2424,7 @@ test_expect_success 'list --global with both home and xdg' '
        global  file:$HOME/.gitconfig   home.config=true
        EOF
        git config ${mode_prefix}list --global --show-scope --show-origin >output &&
-       ! test_cmp expect output
+       test_cmp expect output
 '
 
 test_expect_success 'override global and system config' '
@@ -2478,7 +2478,7 @@ test_expect_success 'override global and system config' '
        test_cmp expect output
 '
 
-test_expect_success 'override global and system config with missing file' '
+test_expect_failure 'override global and system config with missing file' '
        test_must_fail env GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=/dev/null git config ${mode_prefix}list --global &&
        test_must_fail env GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=does-not-exist git config ${mode_prefix}list --system &&
        GIT_CONFIG_GLOBAL=does-not-exist GIT_CONFIG_SYSTEM=does-not-exist git version

diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 03187557990b36c01e158bd329ae24373dedba2b..475bd26abaaa81e8590b7478ae530034176dd18d 100755 (executable)
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -71,7 +71,7 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists'
        echo user.name=read_config >expected &&
        echo user.name=read_gitconfig >>expected &&
        git config --global --list >actual &&
-       ! test_cmp expected actual
+       test_cmp expected actual
 '