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