* includeIf breaks calling dashed externals
@ 2017-04-14 17:04 Bert Wesarg
2017-04-14 17:43 ` Jeff King
0 siblings, 1 reply; 24+ messages in thread
From: Bert Wesarg @ 2017-04-14 17:04 UTC (permalink / raw)
To: Nguyễn Thái Ngọc; +Cc: Git Mailing List
Dear Duy,
heaving an includeIf in a git config file breaks calling external git
commands, most prominently git-gui.
$ git --version
git version 2.12.2.599.gcf11a6797
$ git rev-parse --is-inside-work-tree
true
$ git echo
git: 'echo' is not a git command. See 'git --help'.
Did you mean this?
fetch
$ echo '[includeIf "gitdir:does-not-exists"]path = does-not-exists'
>>.git/config
$ git rev-parse --is-inside-work-tree
true
$ git echo
fatal: BUG: setup_git_env called without repository
Best,
Bert
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: includeIf breaks calling dashed externals 2017-04-14 17:04 includeIf breaks calling dashed externals Bert Wesarg @ 2017-04-14 17:43 ` Jeff King 2017-04-15 11:49 ` Duy Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Jeff King @ 2017-04-14 17:43 UTC (permalink / raw) To: Bert Wesarg; +Cc: Nguyễn Thái Ngọc, Git Mailing List On Fri, Apr 14, 2017 at 07:04:23PM +0200, Bert Wesarg wrote: > Dear Duy, > > heaving an includeIf in a git config file breaks calling external git > commands, most prominently git-gui. > > $ git --version > git version 2.12.2.599.gcf11a6797 > $ git rev-parse --is-inside-work-tree > true > $ git echo > git: 'echo' is not a git command. See 'git --help'. > > Did you mean this? > fetch > $ echo '[includeIf "gitdir:does-not-exists"]path = does-not-exists' > >>.git/config > $ git rev-parse --is-inside-work-tree > true > $ git echo > fatal: BUG: setup_git_env called without repository Probably this fixes it: diff --git a/config.c b/config.c index b6e4a57b9..8d66bdf56 100644 --- a/config.c +++ b/config.c @@ -213,6 +213,9 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase) struct strbuf pattern = STRBUF_INIT; int ret = 0, prefix; + if (!have_git_dir()) + return 0; + strbuf_add_absolute_path(&text, get_git_dir()); strbuf_add(&pattern, cond, cond_len); prefix = prepare_include_condition_pattern(&pattern); But it does raise a question of reading config before/after repository setup, since those will give different answers. I guess they do anyway because of $GIT_DIR/config. -Peff ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: includeIf breaks calling dashed externals 2017-04-14 17:43 ` Jeff King @ 2017-04-15 11:49 ` Duy Nguyen 2017-04-16 4:50 ` Jeff King 0 siblings, 1 reply; 24+ messages in thread From: Duy Nguyen @ 2017-04-15 11:49 UTC (permalink / raw) To: Jeff King; +Cc: Bert Wesarg, Git Mailing List On Fri, Apr 14, 2017 at 01:43:37PM -0400, Jeff King wrote: > On Fri, Apr 14, 2017 at 07:04:23PM +0200, Bert Wesarg wrote: > > > Dear Duy, > > > > heaving an includeIf in a git config file breaks calling external git > > commands, most prominently git-gui. > > > > $ git --version > > git version 2.12.2.599.gcf11a6797 > > $ git rev-parse --is-inside-work-tree > > true > > $ git echo > > git: 'echo' is not a git command. See 'git --help'. > > > > Did you mean this? > > fetch > > $ echo '[includeIf "gitdir:does-not-exists"]path = does-not-exists' > > >>.git/config > > $ git rev-parse --is-inside-work-tree > > true > > $ git echo > > fatal: BUG: setup_git_env called without repository > > Probably this fixes it: > > diff --git a/config.c b/config.c > index b6e4a57b9..8d66bdf56 100644 > --- a/config.c > +++ b/config.c > @@ -213,6 +213,9 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase) > struct strbuf pattern = STRBUF_INIT; > int ret = 0, prefix; > > + if (!have_git_dir()) > + return 0; > + > strbuf_add_absolute_path(&text, get_git_dir()); > strbuf_add(&pattern, cond, cond_len); > prefix = prepare_include_condition_pattern(&pattern); > > But it does raise a question of reading config before/after repository > setup, since those will give different answers. I guess they do anyway > because of $GIT_DIR/config. This happens in execv_dased_external() -> check_pager_config() -> read_early_config(). We probably could use the same discover_git_directory trick to get .git dir (because we should find it). Maybe something like this instead? diff --git a/config.c b/config.c index 1a4d85537b..4f540ae578 100644 --- a/config.c +++ b/config.c @@ -212,8 +212,14 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase) struct strbuf text = STRBUF_INIT; struct strbuf pattern = STRBUF_INIT; int ret = 0, prefix; + struct strbuf gitdir = STRBUF_INIT; - strbuf_add_absolute_path(&text, get_git_dir()); + if (have_git_dir()) + strbuf_addstr(&gitdir, get_git_dir()); + else if (!discover_git_directory(&gitdir)) + goto done; + + strbuf_add_absolute_path(&text, gitdir.buf); strbuf_add(&pattern, cond, cond_len); prefix = prepare_include_condition_pattern(&pattern); @@ -237,6 +243,7 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase) icase ? WM_CASEFOLD : 0, NULL); done: + strbuf_release(&gitdir); strbuf_release(&pattern); strbuf_release(&text); return ret; -- Duy ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: includeIf breaks calling dashed externals 2017-04-15 11:49 ` Duy Nguyen @ 2017-04-16 4:50 ` Jeff King 2017-04-16 10:41 ` [PATCH 1/2] config: prepare to pass more info in git_config_with_options() Nguyễn Thái Ngọc Duy 0 siblings, 1 reply; 24+ messages in thread From: Jeff King @ 2017-04-16 4:50 UTC (permalink / raw) To: Duy Nguyen; +Cc: Bert Wesarg, Git Mailing List On Sat, Apr 15, 2017 at 06:49:01PM +0700, Duy Nguyen wrote: > > Probably this fixes it: > > > > diff --git a/config.c b/config.c > > index b6e4a57b9..8d66bdf56 100644 > > --- a/config.c > > +++ b/config.c > > @@ -213,6 +213,9 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase) > > struct strbuf pattern = STRBUF_INIT; > > int ret = 0, prefix; > > > > + if (!have_git_dir()) > > + return 0; > > + > > strbuf_add_absolute_path(&text, get_git_dir()); > > strbuf_add(&pattern, cond, cond_len); > > prefix = prepare_include_condition_pattern(&pattern); > > > > But it does raise a question of reading config before/after repository > > setup, since those will give different answers. I guess they do anyway > > because of $GIT_DIR/config. > > This happens in execv_dased_external() -> check_pager_config() -> > read_early_config(). We probably could use the same discover_git_directory > trick to get .git dir (because we should find it). Maybe something > like this instead? I know that we only kick in discover_git_directory() in certain cases when we're reading config (for the "early config"). Would it makes sense to respect the same rules here? I.e., if we have a program that wants to look at config but explicitly _doesn't_ setup the git repository, is it right to say that we are inside that repository? So instead of lazily doing the discovery here: > diff --git a/config.c b/config.c > index 1a4d85537b..4f540ae578 100644 > --- a/config.c > +++ b/config.c > @@ -212,8 +212,14 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase) > struct strbuf text = STRBUF_INIT; > struct strbuf pattern = STRBUF_INIT; > int ret = 0, prefix; > + struct strbuf gitdir = STRBUF_INIT; > > - strbuf_add_absolute_path(&text, get_git_dir()); > + if (have_git_dir()) > + strbuf_addstr(&gitdir, get_git_dir()); > + else if (!discover_git_directory(&gitdir)) > + goto done; ..should we record the discovered git dir in read_early_config(), and use it only if available? And if not, then we know the program is explicitly trying not to be in a repo (or we know we tried to discover one already and it didn't exist). -Peff ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/2] config: prepare to pass more info in git_config_with_options() 2017-04-16 4:50 ` Jeff King @ 2017-04-16 10:41 ` Nguyễn Thái Ngọc Duy 2017-04-16 10:41 ` [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up Nguyễn Thái Ngọc Duy 2017-04-16 15:31 ` [PATCH 1/2] config: prepare to pass more info in git_config_with_options() Jeff King 0 siblings, 2 replies; 24+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2017-04-16 10:41 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, bert.wesarg, Nguyễn Thái Ngọc Duy So far we can only pass one flag, respect_includes, to thie function. We need to pass some more (non-flag even), so let's make it accept a struct instead of an integer. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- Prep patch for the bug fix in 2/2. builtin/config.c | 21 ++++++++++++--------- cache.h | 7 ++++++- config.c | 16 +++++++++++----- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 4f49a0edb9..5de4a36146 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -26,7 +26,8 @@ static int use_global_config, use_system_config, use_local_config; static struct git_config_source given_config_source; static int actions, types; static int end_null; -static int respect_includes = -1; +static int respect_includes_opt; +static struct config_options config_options; static int show_origin; #define ACTION_GET (1<<0) @@ -81,7 +82,7 @@ static struct option builtin_config_options[] = { OPT_GROUP(N_("Other")), OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")), OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")), - OPT_BOOL(0, "includes", &respect_includes, N_("respect include directives on lookup")), + OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")), OPT_END(), }; @@ -242,7 +243,7 @@ static int get_value(const char *key_, const char *regex_) } git_config_with_options(collect_config, &values, - &given_config_source, respect_includes); + &given_config_source, &config_options); ret = !values.nr; @@ -320,7 +321,7 @@ static void get_color(const char *var, const char *def_color) get_color_found = 0; parsed_color[0] = '\0'; git_config_with_options(git_get_color_config, NULL, - &given_config_source, respect_includes); + &given_config_source, &config_options); if (!get_color_found && def_color) { if (color_parse(def_color, parsed_color) < 0) @@ -352,7 +353,7 @@ static int get_colorbool(const char *var, int print) get_diff_color_found = -1; get_color_ui_found = -1; git_config_with_options(git_get_colorbool_config, NULL, - &given_config_source, respect_includes); + &given_config_source, &config_options); if (get_colorbool_found < 0) { if (!strcmp(get_colorbool_slot, "color.diff")) @@ -441,7 +442,7 @@ static int get_urlmatch(const char *var, const char *url) } git_config_with_options(urlmatch_config_entry, &config, - &given_config_source, respect_includes); + &given_config_source, &config_options); ret = !values.nr; @@ -530,8 +531,10 @@ int cmd_config(int argc, const char **argv, const char *prefix) prefix_filename(prefix, given_config_source.file); } - if (respect_includes == -1) - respect_includes = !given_config_source.file; + if (respect_includes_opt == -1) + config_options.respect_includes = !given_config_source.file; + else + config_options.respect_includes = respect_includes_opt; if (end_null) { term = '\0'; @@ -578,7 +581,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_argc(argc, 0, 0); if (git_config_with_options(show_all_config, NULL, &given_config_source, - respect_includes) < 0) { + &config_options) < 0) { if (given_config_source.file) die_errno("unable to read config file '%s'", given_config_source.file); diff --git a/cache.h b/cache.h index 5c8078291c..e29a093839 100644 --- a/cache.h +++ b/cache.h @@ -1882,6 +1882,10 @@ enum config_origin_type { CONFIG_ORIGIN_CMDLINE }; +struct config_options { + unsigned int respect_includes : 1; +}; + typedef int (*config_fn_t)(const char *, const char *, void *); extern int git_default_config(const char *, const char *, void *); extern int git_config_from_file(config_fn_t fn, const char *, void *); @@ -1895,7 +1899,7 @@ extern void read_early_config(config_fn_t cb, void *data); extern void git_config(config_fn_t fn, void *); extern int git_config_with_options(config_fn_t fn, void *, struct git_config_source *config_source, - int respect_includes); + const struct config_options *opts); extern int git_parse_ulong(const char *, unsigned long *); extern int git_parse_maybe_bool(const char *); extern int git_config_int(const char *, const char *); @@ -1947,6 +1951,7 @@ struct config_include_data { int depth; config_fn_t fn; void *data; + const struct config_options *opts; }; #define CONFIG_INCLUDE_INIT { 0 } extern int git_config_include(const char *name, const char *value, void *data); diff --git a/config.c b/config.c index 1a4d85537b..042321a3a0 100644 --- a/config.c +++ b/config.c @@ -1530,13 +1530,14 @@ static int do_git_config_sequence(config_fn_t fn, void *data) int git_config_with_options(config_fn_t fn, void *data, struct git_config_source *config_source, - int respect_includes) + const struct config_options *opts) { struct config_include_data inc = CONFIG_INCLUDE_INIT; - if (respect_includes) { + if (opts->respect_includes) { inc.fn = fn; inc.data = data; + inc.opts = opts; fn = git_config_include; data = &inc; } @@ -1557,7 +1558,10 @@ int git_config_with_options(config_fn_t fn, void *data, static void git_config_raw(config_fn_t fn, void *data) { - if (git_config_with_options(fn, data, NULL, 1) < 0) + struct config_options opts = {0}; + + opts.respect_includes = 1; + if (git_config_with_options(fn, data, NULL, &opts) < 0) /* * git_config_with_options() normally returns only * zero, as most errors are fatal, and @@ -1597,9 +1601,11 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data) void read_early_config(config_fn_t cb, void *data) { + struct config_options opts = {0}; struct strbuf buf = STRBUF_INIT; - git_config_with_options(cb, data, NULL, 1); + opts.respect_includes = 1; + git_config_with_options(cb, data, NULL, &opts); /* * When setup_git_directory() was not yet asked to discover the @@ -1615,7 +1621,7 @@ void read_early_config(config_fn_t cb, void *data) memset(&repo_config, 0, sizeof(repo_config)); strbuf_addstr(&buf, "/config"); repo_config.file = buf.buf; - git_config_with_options(cb, data, &repo_config, 1); + git_config_with_options(cb, data, &repo_config, &opts); } strbuf_release(&buf); } -- 2.11.0.157.gd943d85 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up 2017-04-16 10:41 ` [PATCH 1/2] config: prepare to pass more info in git_config_with_options() Nguyễn Thái Ngọc Duy @ 2017-04-16 10:41 ` Nguyễn Thái Ngọc Duy 2017-04-16 15:51 ` Jeff King 2017-04-16 15:31 ` [PATCH 1/2] config: prepare to pass more info in git_config_with_options() Jeff King 1 sibling, 1 reply; 24+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2017-04-16 10:41 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, bert.wesarg, Nguyễn Thái Ngọc Duy If setup_git_directory() and friends have not been called, get_git_dir() (because of includeIf.gitdir:XXX) would lead to die("BUG: setup_git_env called without repository"); There are two cases when a config file could be read before $GIT_DIR is located. The first one is check_repository_format(), where we read just the one file $GIT_DIR/config to check if we could understand this repository. This case should be safe. The concerned variables should never be hidden away behind includes anyway. The second one is triggered in check_pager_config() when we're about to run an external git command. We might be able to find $GIT_DIR in this case, which is exactly what read_early_config() does (and also is the what check_pager_config() uses). Conditional includes and get_git_dir() could be triggered by the first git_config_with_options() call there, before discover_git_directory() is used as a fallback $GIT_DIR detection. Detect this special "early reading" case, pass down the $GIT_DIR, either from previous setup or detected by discover_git_directory(), and make conditional include use it. Noticed-by: Bert Wesarg <bert.wesarg@googlemail.com> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- cache.h | 2 ++ config.c | 35 ++++++++++++++++++++++++++--------- t/t1305-config-include.sh | 11 +++++++++++ 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/cache.h b/cache.h index e29a093839..27b7286f99 100644 --- a/cache.h +++ b/cache.h @@ -1884,6 +1884,8 @@ enum config_origin_type { struct config_options { unsigned int respect_includes : 1; + unsigned int early_config : 1; + const char *git_dir; /* only valid when early_config is true */ }; typedef int (*config_fn_t)(const char *, const char *, void *); diff --git a/config.c b/config.c index 042321a3a0..f323b96283 100644 --- a/config.c +++ b/config.c @@ -207,13 +207,20 @@ static int prepare_include_condition_pattern(struct strbuf *pat) return prefix; } -static int include_by_gitdir(const char *cond, size_t cond_len, int icase) +static int include_by_gitdir(const struct config_options *opts, + const char *cond, size_t cond_len, int icase) { struct strbuf text = STRBUF_INIT; struct strbuf pattern = STRBUF_INIT; int ret = 0, prefix; - strbuf_add_absolute_path(&text, get_git_dir()); + if (!opts->early_config) + strbuf_add_absolute_path(&text, get_git_dir()); + else if (opts->git_dir) + strbuf_add_absolute_path(&text, opts->git_dir); + else + goto done; + strbuf_add(&pattern, cond, cond_len); prefix = prepare_include_condition_pattern(&pattern); @@ -242,13 +249,14 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase) return ret; } -static int include_condition_is_true(const char *cond, size_t cond_len) +static int include_condition_is_true(const struct config_options *opts, + const char *cond, size_t cond_len) { if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len)) - return include_by_gitdir(cond, cond_len, 0); + return include_by_gitdir(opts, cond, cond_len, 0); else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len)) - return include_by_gitdir(cond, cond_len, 1); + return include_by_gitdir(opts, cond, cond_len, 1); /* unknown conditionals are always false */ return 0; @@ -273,7 +281,7 @@ int git_config_include(const char *var, const char *value, void *data) ret = handle_path_include(value, inc); if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) && - (cond && include_condition_is_true(cond, cond_len)) && + (cond && include_condition_is_true(inc->opts, cond, cond_len)) && !strcmp(key, "path")) ret = handle_path_include(value, inc); @@ -1603,10 +1611,13 @@ void read_early_config(config_fn_t cb, void *data) { struct config_options opts = {0}; struct strbuf buf = STRBUF_INIT; + char *to_free = NULL; opts.respect_includes = 1; - git_config_with_options(cb, data, NULL, &opts); + opts.early_config = 1; + if (have_git_dir()) + opts.git_dir = get_git_dir(); /* * When setup_git_directory() was not yet asked to discover the * GIT_DIR, we ask discover_git_directory() to figure out whether there @@ -1615,15 +1626,21 @@ void read_early_config(config_fn_t cb, void *data) * notably, the current working directory is still the same after the * call). */ - if (!have_git_dir() && discover_git_directory(&buf)) { + else if (discover_git_directory(&buf)) + opts.git_dir = to_free = strbuf_detach(&buf, NULL); + + git_config_with_options(cb, data, NULL, &opts); + + if (opts.git_dir) { struct git_config_source repo_config; memset(&repo_config, 0, sizeof(repo_config)); - strbuf_addstr(&buf, "/config"); + strbuf_addf(&buf, "%s/config", opts.git_dir); repo_config.file = buf.buf; git_config_with_options(cb, data, &repo_config, &opts); } strbuf_release(&buf); + free(to_free); } static void git_config_check_init(void); diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh index e833939320..fddb47bafa 100755 --- a/t/t1305-config-include.sh +++ b/t/t1305-config-include.sh @@ -208,6 +208,17 @@ test_expect_success 'conditional include, both unanchored, icase' ' ) ' +test_expect_success 'conditional include, early config reading' ' + ( + cd foo && + echo "[includeIf \"gitdir:foo/\"]path=bar6" >>.git/config && + echo "[test]six=6" >.git/bar6 && + echo 6 >expect && + test-config read_early_config test.six >actual && + test_cmp expect actual + ) +' + test_expect_success 'include cycles are detected' ' cat >.gitconfig <<-\EOF && [test]value = gitconfig -- 2.11.0.157.gd943d85 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up 2017-04-16 10:41 ` [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up Nguyễn Thái Ngọc Duy @ 2017-04-16 15:51 ` Jeff King 2017-04-17 2:13 ` Duy Nguyen 2017-04-17 10:07 ` Duy Nguyen 0 siblings, 2 replies; 24+ messages in thread From: Jeff King @ 2017-04-16 15:51 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, bert.wesarg On Sun, Apr 16, 2017 at 05:41:25PM +0700, Nguyễn Thái Ngọc Duy wrote: > If setup_git_directory() and friends have not been called, > get_git_dir() (because of includeIf.gitdir:XXX) would lead to > > die("BUG: setup_git_env called without repository"); > > There are two cases when a config file could be read before $GIT_DIR is > located. The first one is check_repository_format(), where we read just > the one file $GIT_DIR/config to check if we could understand this > repository. This case should be safe. The concerned variables should > never be hidden away behind includes anyway. Right, we should not even have respect_includes turned on for that case. > The second one is triggered in check_pager_config() when we're about to > run an external git command. We might be able to find $GIT_DIR in this > case, which is exactly what read_early_config() does (and also is the > what check_pager_config() uses). Conditional includes and s/the what/what/ > diff --git a/cache.h b/cache.h > index e29a093839..27b7286f99 100644 > --- a/cache.h > +++ b/cache.h > @@ -1884,6 +1884,8 @@ enum config_origin_type { > > struct config_options { > unsigned int respect_includes : 1; > + unsigned int early_config : 1; > + const char *git_dir; /* only valid when early_config is true */ > }; Why do we need both the flag and the string? Later, you do: > -static int include_by_gitdir(const char *cond, size_t cond_len, int icase) > +static int include_by_gitdir(const struct config_options *opts, > + const char *cond, size_t cond_len, int icase) > { > struct strbuf text = STRBUF_INIT; > struct strbuf pattern = STRBUF_INIT; > int ret = 0, prefix; > > - strbuf_add_absolute_path(&text, get_git_dir()); > + if (!opts->early_config) > + strbuf_add_absolute_path(&text, get_git_dir()); > + else if (opts->git_dir) > + strbuf_add_absolute_path(&text, opts->git_dir); > + else > + goto done; So we call get_git_dir() always when we're not in early config. Even if we don't have a git dir! Doesn't this mean that programs operating outside of a repo will still hit the BUG? I.e.: git config --global includeif.gitdir:/whatever.path foo cd /not/a/git/dir git diff --no-index foo bar ? I think instead the logic should be: if (opts->git_dir) strbuf_add_absolute_path(&text, opts->git_dir); else if (have_git_dir()) strbuf_add_absolute_path(&text, get_git_dir()); else goto done; I'd also be tempted to call the option field "override_git_dir" or something to indicate that it takes precedence over the normal one. With the current code it doesn't matter, because we set it only to the result of the discovered dir. > @@ -1615,15 +1626,21 @@ void read_early_config(config_fn_t cb, void *data) > * notably, the current working directory is still the same after the > * call). > */ > - if (!have_git_dir() && discover_git_directory(&buf)) { > + else if (discover_git_directory(&buf)) > + opts.git_dir = to_free = strbuf_detach(&buf, NULL); This to_free seemed redundant to me at first; why not just hold on to the strbuf and release it later? The answer is that we reuse the strbuf to generate the config-file path later. However, by detaching, we clear the strbuf. So later when we use it: > + if (opts.git_dir) { > struct git_config_source repo_config; > > memset(&repo_config, 0, sizeof(repo_config)); > - strbuf_addstr(&buf, "/config"); > + strbuf_addf(&buf, "%s/config", opts.git_dir); > repo_config.file = buf.buf; > git_config_with_options(cb, data, &repo_config, &opts); > } ...we have to re-add the git_dir. Might it be simpler to just xstrdup() to opts.git_dir, and then leave this later code alone? That said, I actually think in the long run that git_config_with_options() should compute the repo_config itself from our git_dir parameter. That lets us fix a very subtle bug in read_early_config(). The problem is that the function works like this: 1. Run git_config_with_options(), hitting the usual sources in order 2. If we didn't have a git-dir, run it again just for our discovered repo-config That has two implications: - the config callback will see keys from ~/.gitconfig twice, once for each run. This is usually OK because the only early config we care about uses last-one-wins semantics (so no list-appending). - the repo-level config is read last, so by last-one-wins it takes precedence over ~/.gitconfig. Good. But it should have _less_ precedence than command-line config, and it doesn't. You can see the second problem with: # random external cat >git-foo <<-\EOF #!/bin/sh echo foo EOF chmod +x git-foo PATH=$PWD:$PATH git init git config pager.foo 'sed s/^/repo:/' git -c pager.foo='sed s/^/cmdline:/' foo That command should prefer the cmdline config, but it doesn't. The fix is something like what's below, which is easy on top of your new options struct. I can wrap it up with a config message and test on top of your series. diff --git a/config.c b/config.c index f323b9628..5dda6e8ca 100644 --- a/config.c +++ b/config.c @@ -1502,12 +1502,20 @@ int git_config_system(void) return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0); } -static int do_git_config_sequence(config_fn_t fn, void *data) +static int do_git_config_sequence(config_fn_t fn, void *data, + const char *override_repo_dir) { int ret = 0; char *xdg_config = xdg_config_home("config"); char *user_config = expand_user_path("~/.gitconfig"); - char *repo_config = have_git_dir() ? git_pathdup("config") : NULL; + char *repo_config; + + if (override_repo_dir) + repo_config = mkpathdup("%s/config", override_repo_dir); + else if (have_git_dir()) + repo_config = git_pathdup("config"); + else + repo_config = NULL; current_parsing_scope = CONFIG_SCOPE_SYSTEM; if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) @@ -1561,7 +1569,7 @@ int git_config_with_options(config_fn_t fn, void *data, else if (config_source && config_source->blob) return git_config_from_blob_ref(fn, config_source->blob, data); - return do_git_config_sequence(fn, data); + return do_git_config_sequence(fn, data, opts->git_dir); } static void git_config_raw(config_fn_t fn, void *data) @@ -1631,14 +1639,6 @@ void read_early_config(config_fn_t cb, void *data) git_config_with_options(cb, data, NULL, &opts); - if (opts.git_dir) { - struct git_config_source repo_config; - - memset(&repo_config, 0, sizeof(repo_config)); - strbuf_addf(&buf, "%s/config", opts.git_dir); - repo_config.file = buf.buf; - git_config_with_options(cb, data, &repo_config, &opts); - } strbuf_release(&buf); free(to_free); } ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up 2017-04-16 15:51 ` Jeff King @ 2017-04-17 2:13 ` Duy Nguyen 2017-04-18 3:56 ` Jeff King 2017-04-17 10:07 ` Duy Nguyen 1 sibling, 1 reply; 24+ messages in thread From: Duy Nguyen @ 2017-04-17 2:13 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, bert.wesarg On Sun, Apr 16, 2017 at 11:51:32AM -0400, Jeff King wrote: > > diff --git a/cache.h b/cache.h > > index e29a093839..27b7286f99 100644 > > --- a/cache.h > > +++ b/cache.h > > @@ -1884,6 +1884,8 @@ enum config_origin_type { > > > > struct config_options { > > unsigned int respect_includes : 1; > > + unsigned int early_config : 1; > > + const char *git_dir; /* only valid when early_config is true */ > > }; > > Why do we need both the flag and the string? Later, you do: > > > -static int include_by_gitdir(const char *cond, size_t cond_len, int icase) > > +static int include_by_gitdir(const struct config_options *opts, > > + const char *cond, size_t cond_len, int icase) > > { > > struct strbuf text = STRBUF_INIT; > > struct strbuf pattern = STRBUF_INIT; > > int ret = 0, prefix; > > > > - strbuf_add_absolute_path(&text, get_git_dir()); > > + if (!opts->early_config) > > + strbuf_add_absolute_path(&text, get_git_dir()); > > + else if (opts->git_dir) > > + strbuf_add_absolute_path(&text, opts->git_dir); > > + else > > + goto done; > > So we call get_git_dir() always when we're not in early config. Even if > we don't have a git dir! Doesn't this mean that programs operating > outside of a repo will still hit the BUG? I.e.: > > git config --global includeif.gitdir:/whatever.path foo > cd /not/a/git/dir > git diff --no-index foo bar > > ? > > I think instead the logic should be: > > if (opts->git_dir) > strbuf_add_absolute_path(&text, opts->git_dir); > else if (have_git_dir()) > strbuf_add_absolute_path(&text, get_git_dir()); > else > goto done; Do we care about the case when the override instruction is "we don't have $GIT_DIR, act as if it does not exist, even though have_git_dir() returns true"? I'm guessing no, we won't run into that situation (and am inclined to restructure the code as you suggested). Just throwing it out there if I'm mistaken. -- Duy ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up 2017-04-17 2:13 ` Duy Nguyen @ 2017-04-18 3:56 ` Jeff King 0 siblings, 0 replies; 24+ messages in thread From: Jeff King @ 2017-04-18 3:56 UTC (permalink / raw) To: Duy Nguyen; +Cc: git, Junio C Hamano, bert.wesarg On Mon, Apr 17, 2017 at 09:13:35AM +0700, Duy Nguyen wrote: > > So we call get_git_dir() always when we're not in early config. Even if > > we don't have a git dir! Doesn't this mean that programs operating > > outside of a repo will still hit the BUG? I.e.: > > > > git config --global includeif.gitdir:/whatever.path foo > > cd /not/a/git/dir > > git diff --no-index foo bar > > > > ? > > > > I think instead the logic should be: > > > > if (opts->git_dir) > > strbuf_add_absolute_path(&text, opts->git_dir); > > else if (have_git_dir()) > > strbuf_add_absolute_path(&text, get_git_dir()); > > else > > goto done; > > Do we care about the case when the override instruction is "we don't > have $GIT_DIR, act as if it does not exist, even though have_git_dir() > returns true"? > > I'm guessing no, we won't run into that situation (and am inclined to > restructure the code as you suggested). Just throwing it out there if > I'm mistaken. I can't think of a case where that would matter. We're always prodding the function to look at something it might not, and I don't think there would be any need to convince it _not_ to look at something. -Peff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up 2017-04-16 15:51 ` Jeff King 2017-04-17 2:13 ` Duy Nguyen @ 2017-04-17 10:07 ` Duy Nguyen 2017-04-17 10:10 ` [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() Nguyễn Thái Ngọc Duy ` (2 more replies) 1 sibling, 3 replies; 24+ messages in thread From: Duy Nguyen @ 2017-04-17 10:07 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Bert Wesarg (To Junio, this series conflicts slightly with nd/conditional-config-include, let me know if you want me to rebase this on top of that) On Sun, Apr 16, 2017 at 10:51 PM, Jeff King <peff@peff.net> wrote: >> + if (opts.git_dir) { >> struct git_config_source repo_config; >> >> memset(&repo_config, 0, sizeof(repo_config)); >> - strbuf_addstr(&buf, "/config"); >> + strbuf_addf(&buf, "%s/config", opts.git_dir); >> repo_config.file = buf.buf; >> git_config_with_options(cb, data, &repo_config, &opts); >> } > > ...we have to re-add the git_dir. > > Might it be simpler to just xstrdup() to opts.git_dir, and then leave > this later code alone? Sure thing. But we need to restore the "if" expression too. Otherwise, if have_git_dir() is true, we may come here with an empty "buf" before that strbuf_addstr(&buf, "/config") is called. It does not matter much anyway because this block will be removed. > You can see the second problem with: > > # random external > cat >git-foo <<-\EOF > #!/bin/sh > echo foo > EOF > chmod +x git-foo > PATH=$PWD:$PATH > > git init > git config pager.foo 'sed s/^/repo:/' > git -c pager.foo='sed s/^/cmdline:/' foo > > That command should prefer the cmdline config, but it doesn't. I actually had problems seeing the problem, for some reason it didn't work for me. I guess I made a mistake somewhere. > The fix is something like what's below, which is easy on top of your new > options struct. I can wrap it up with a config message and test on top > of your series. ... anyway I read this last sentence too late and spent too much time wrapping your changes in a patch (well most of the time was spent on writing a new test actually), so I'm sending it out too. My test uses test-config though (I have given up on dealing with pager and tty). Off topic. Back to the pager.foo thing (because I had to fight it a bit before giving up). I find it a bit unintuitive that "--paginate" forces the pager back to less (or $PAGER) when I say "pager.foo = my-favorite-pager". Back when pager.foo is a boolean thing, it makes sense for --paginate to override the "to page or not to page" decision. But then you added a new meaning too pager.foo (what command to run). "--paginate" should respect the command pager.foo specifies when its value is a command, I think. -- Duy ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() 2017-04-17 10:07 ` Duy Nguyen @ 2017-04-17 10:10 ` Nguyễn Thái Ngọc Duy 2017-04-17 10:10 ` [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up Nguyễn Thái Ngọc Duy ` (2 more replies) 2017-04-18 3:17 ` [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up Junio C Hamano 2017-04-18 4:03 ` Jeff King 2 siblings, 3 replies; 24+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2017-04-17 10:10 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, bert.wesarg, Nguyễn Thái Ngọc Duy So far we can only pass one flag, respect_includes, to thie function. We need to pass some more (non-flag even), so let's make it accept a struct instead of an integer. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/config.c | 21 ++++++++++++--------- cache.h | 7 ++++++- config.c | 16 +++++++++++----- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 4f49a0edb9..b937d175a9 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -26,7 +26,8 @@ static int use_global_config, use_system_config, use_local_config; static struct git_config_source given_config_source; static int actions, types; static int end_null; -static int respect_includes = -1; +static int respect_includes_opt = -1; +static struct config_options config_options; static int show_origin; #define ACTION_GET (1<<0) @@ -81,7 +82,7 @@ static struct option builtin_config_options[] = { OPT_GROUP(N_("Other")), OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")), OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")), - OPT_BOOL(0, "includes", &respect_includes, N_("respect include directives on lookup")), + OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")), OPT_END(), }; @@ -242,7 +243,7 @@ static int get_value(const char *key_, const char *regex_) } git_config_with_options(collect_config, &values, - &given_config_source, respect_includes); + &given_config_source, &config_options); ret = !values.nr; @@ -320,7 +321,7 @@ static void get_color(const char *var, const char *def_color) get_color_found = 0; parsed_color[0] = '\0'; git_config_with_options(git_get_color_config, NULL, - &given_config_source, respect_includes); + &given_config_source, &config_options); if (!get_color_found && def_color) { if (color_parse(def_color, parsed_color) < 0) @@ -352,7 +353,7 @@ static int get_colorbool(const char *var, int print) get_diff_color_found = -1; get_color_ui_found = -1; git_config_with_options(git_get_colorbool_config, NULL, - &given_config_source, respect_includes); + &given_config_source, &config_options); if (get_colorbool_found < 0) { if (!strcmp(get_colorbool_slot, "color.diff")) @@ -441,7 +442,7 @@ static int get_urlmatch(const char *var, const char *url) } git_config_with_options(urlmatch_config_entry, &config, - &given_config_source, respect_includes); + &given_config_source, &config_options); ret = !values.nr; @@ -530,8 +531,10 @@ int cmd_config(int argc, const char **argv, const char *prefix) prefix_filename(prefix, given_config_source.file); } - if (respect_includes == -1) - respect_includes = !given_config_source.file; + if (respect_includes_opt == -1) + config_options.respect_includes = !given_config_source.file; + else + config_options.respect_includes = respect_includes_opt; if (end_null) { term = '\0'; @@ -578,7 +581,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_argc(argc, 0, 0); if (git_config_with_options(show_all_config, NULL, &given_config_source, - respect_includes) < 0) { + &config_options) < 0) { if (given_config_source.file) die_errno("unable to read config file '%s'", given_config_source.file); diff --git a/cache.h b/cache.h index 556468c25b..a6294d2573 100644 --- a/cache.h +++ b/cache.h @@ -1885,6 +1885,10 @@ enum config_origin_type { CONFIG_ORIGIN_CMDLINE }; +struct config_options { + unsigned int respect_includes : 1; +}; + typedef int (*config_fn_t)(const char *, const char *, void *); extern int git_default_config(const char *, const char *, void *); extern int git_config_from_file(config_fn_t fn, const char *, void *); @@ -1898,7 +1902,7 @@ extern void read_early_config(config_fn_t cb, void *data); extern void git_config(config_fn_t fn, void *); extern int git_config_with_options(config_fn_t fn, void *, struct git_config_source *config_source, - int respect_includes); + const struct config_options *opts); extern int git_parse_ulong(const char *, unsigned long *); extern int git_parse_maybe_bool(const char *); extern int git_config_int(const char *, const char *); @@ -1950,6 +1954,7 @@ struct config_include_data { int depth; config_fn_t fn; void *data; + const struct config_options *opts; }; #define CONFIG_INCLUDE_INIT { 0 } extern int git_config_include(const char *name, const char *value, void *data); diff --git a/config.c b/config.c index 1a4d85537b..042321a3a0 100644 --- a/config.c +++ b/config.c @@ -1530,13 +1530,14 @@ static int do_git_config_sequence(config_fn_t fn, void *data) int git_config_with_options(config_fn_t fn, void *data, struct git_config_source *config_source, - int respect_includes) + const struct config_options *opts) { struct config_include_data inc = CONFIG_INCLUDE_INIT; - if (respect_includes) { + if (opts->respect_includes) { inc.fn = fn; inc.data = data; + inc.opts = opts; fn = git_config_include; data = &inc; } @@ -1557,7 +1558,10 @@ int git_config_with_options(config_fn_t fn, void *data, static void git_config_raw(config_fn_t fn, void *data) { - if (git_config_with_options(fn, data, NULL, 1) < 0) + struct config_options opts = {0}; + + opts.respect_includes = 1; + if (git_config_with_options(fn, data, NULL, &opts) < 0) /* * git_config_with_options() normally returns only * zero, as most errors are fatal, and @@ -1597,9 +1601,11 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data) void read_early_config(config_fn_t cb, void *data) { + struct config_options opts = {0}; struct strbuf buf = STRBUF_INIT; - git_config_with_options(cb, data, NULL, 1); + opts.respect_includes = 1; + git_config_with_options(cb, data, NULL, &opts); /* * When setup_git_directory() was not yet asked to discover the @@ -1615,7 +1621,7 @@ void read_early_config(config_fn_t cb, void *data) memset(&repo_config, 0, sizeof(repo_config)); strbuf_addstr(&buf, "/config"); repo_config.file = buf.buf; - git_config_with_options(cb, data, &repo_config, 1); + git_config_with_options(cb, data, &repo_config, &opts); } strbuf_release(&buf); } -- 2.11.0.157.gd943d85 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up 2017-04-17 10:10 ` [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() Nguyễn Thái Ngọc Duy @ 2017-04-17 10:10 ` Nguyễn Thái Ngọc Duy 2017-04-18 2:49 ` Junio C Hamano 2017-04-17 10:10 ` [PATCH v2 3/3] config: correct file reading order in read_early_config() Nguyễn Thái Ngọc Duy 2017-04-18 2:27 ` [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() Junio C Hamano 2 siblings, 1 reply; 24+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2017-04-17 10:10 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, bert.wesarg, Nguyễn Thái Ngọc Duy If setup_git_directory() and friends have not been called, get_git_dir() (because of includeIf.gitdir:XXX) would lead to die("BUG: setup_git_env called without repository"); There are two cases when a config file could be read before $GIT_DIR is located. The first one is check_repository_format(), where we read just the one file $GIT_DIR/config to check if we could understand this repository. This case should be safe. We do not parse include directives, which can only be triggered from git_config_with_options, but setup code uses a lower-level function. The concerned variables should never be hidden away behind includes anyway. The second one is triggered in check_pager_config() when we're about to run an external git command. We might be able to find $GIT_DIR in this case, which is exactly what read_early_config() does (and also is what check_pager_config() uses). Conditional includes and get_git_dir() could be triggered by the first git_config_with_options() call there, before discover_git_directory() is used as a fallback $GIT_DIR detection. Detect this special "early reading" case, pass down the $GIT_DIR, either from previous setup or detected by discover_git_directory(), and make conditional include use it. Noticed-by: Bert Wesarg <bert.wesarg@googlemail.com> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- cache.h | 1 + config.c | 34 ++++++++++++++++++++++++++-------- t/t1305-config-include.sh | 11 +++++++++++ 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/cache.h b/cache.h index a6294d2573..878e1d441f 100644 --- a/cache.h +++ b/cache.h @@ -1887,6 +1887,7 @@ enum config_origin_type { struct config_options { unsigned int respect_includes : 1; + const char *git_dir; }; typedef int (*config_fn_t)(const char *, const char *, void *); diff --git a/config.c b/config.c index 042321a3a0..14f0417460 100644 --- a/config.c +++ b/config.c @@ -207,13 +207,22 @@ static int prepare_include_condition_pattern(struct strbuf *pat) return prefix; } -static int include_by_gitdir(const char *cond, size_t cond_len, int icase) +static int include_by_gitdir(const struct config_options *opts, + const char *cond, size_t cond_len, int icase) { struct strbuf text = STRBUF_INIT; struct strbuf pattern = STRBUF_INIT; int ret = 0, prefix; + const char *git_dir; - strbuf_add_absolute_path(&text, get_git_dir()); + if (opts->git_dir) + git_dir = opts->git_dir; + else if (have_git_dir()) + git_dir = get_git_dir(); + else + goto done; + + strbuf_add_absolute_path(&text, git_dir); strbuf_add(&pattern, cond, cond_len); prefix = prepare_include_condition_pattern(&pattern); @@ -242,13 +251,14 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase) return ret; } -static int include_condition_is_true(const char *cond, size_t cond_len) +static int include_condition_is_true(const struct config_options *opts, + const char *cond, size_t cond_len) { if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len)) - return include_by_gitdir(cond, cond_len, 0); + return include_by_gitdir(opts, cond, cond_len, 0); else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len)) - return include_by_gitdir(cond, cond_len, 1); + return include_by_gitdir(opts, cond, cond_len, 1); /* unknown conditionals are always false */ return 0; @@ -273,7 +283,7 @@ int git_config_include(const char *var, const char *value, void *data) ret = handle_path_include(value, inc); if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) && - (cond && include_condition_is_true(cond, cond_len)) && + (cond && include_condition_is_true(inc->opts, cond, cond_len)) && !strcmp(key, "path")) ret = handle_path_include(value, inc); @@ -1603,10 +1613,12 @@ void read_early_config(config_fn_t cb, void *data) { struct config_options opts = {0}; struct strbuf buf = STRBUF_INIT; + char *to_free = NULL; opts.respect_includes = 1; - git_config_with_options(cb, data, NULL, &opts); + if (have_git_dir()) + opts.git_dir = get_git_dir(); /* * When setup_git_directory() was not yet asked to discover the * GIT_DIR, we ask discover_git_directory() to figure out whether there @@ -1615,7 +1627,12 @@ void read_early_config(config_fn_t cb, void *data) * notably, the current working directory is still the same after the * call). */ - if (!have_git_dir() && discover_git_directory(&buf)) { + else if (discover_git_directory(&buf)) + opts.git_dir = to_free = xstrdup(buf.buf); + + git_config_with_options(cb, data, NULL, &opts); + + if (!have_git_dir() && opts.git_dir) { struct git_config_source repo_config; memset(&repo_config, 0, sizeof(repo_config)); @@ -1624,6 +1641,7 @@ void read_early_config(config_fn_t cb, void *data) git_config_with_options(cb, data, &repo_config, &opts); } strbuf_release(&buf); + free(to_free); } static void git_config_check_init(void); diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh index e833939320..fddb47bafa 100755 --- a/t/t1305-config-include.sh +++ b/t/t1305-config-include.sh @@ -208,6 +208,17 @@ test_expect_success 'conditional include, both unanchored, icase' ' ) ' +test_expect_success 'conditional include, early config reading' ' + ( + cd foo && + echo "[includeIf \"gitdir:foo/\"]path=bar6" >>.git/config && + echo "[test]six=6" >.git/bar6 && + echo 6 >expect && + test-config read_early_config test.six >actual && + test_cmp expect actual + ) +' + test_expect_success 'include cycles are detected' ' cat >.gitconfig <<-\EOF && [test]value = gitconfig -- 2.11.0.157.gd943d85 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up 2017-04-17 10:10 ` [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up Nguyễn Thái Ngọc Duy @ 2017-04-18 2:49 ` Junio C Hamano 2017-04-18 2:56 ` Junio C Hamano 0 siblings, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2017-04-18 2:49 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, bert.wesarg Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > @@ -1603,10 +1613,12 @@ void read_early_config(config_fn_t cb, void *data) > { > struct config_options opts = {0}; > struct strbuf buf = STRBUF_INIT; > + char *to_free = NULL; > > opts.respect_includes = 1; > - git_config_with_options(cb, data, NULL, &opts); > > + if (have_git_dir()) > + opts.git_dir = get_git_dir(); > /* > * When setup_git_directory() was not yet asked to discover the > * GIT_DIR, we ask discover_git_directory() to figure out whether there > @@ -1615,7 +1627,12 @@ void read_early_config(config_fn_t cb, void *data) > * notably, the current working directory is still the same after the > * call). > */ > - if (!have_git_dir() && discover_git_directory(&buf)) { > + else if (discover_git_directory(&buf)) > + opts.git_dir = to_free = xstrdup(buf.buf); > + > + git_config_with_options(cb, data, NULL, &opts); This one I can understand. By having NULL for config_source, this does the usual do_git_config_sequence() dance, which knows to treat opts.git_dir is the "repository config" without necessarily being able to do git_pathdup("config"). > + if (!have_git_dir() && opts.git_dir) { > struct git_config_source repo_config; > > memset(&repo_config, 0, sizeof(repo_config)); But this one I do not quite understand. When have_git_dir() was false and asked discover_git_directory() to set opts.git_dir, we enter the body of this block and then end up doing git_config_with_options(cb, data &repo_config, &opts); with repo_config set to the discovered git directory plus "/config"; we'd read the repository configuration twice, in other words. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up 2017-04-18 2:49 ` Junio C Hamano @ 2017-04-18 2:56 ` Junio C Hamano 2017-04-18 3:46 ` Jeff King 0 siblings, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2017-04-18 2:56 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, bert.wesarg Junio C Hamano <gitster@pobox.com> writes: >> + if (!have_git_dir() && opts.git_dir) { >> struct git_config_source repo_config; >> >> memset(&repo_config, 0, sizeof(repo_config)); > > But this one I do not quite understand. When have_git_dir() was > false and asked discover_git_directory() to set opts.git_dir, we > enter the body of this block and then end up doing > > git_config_with_options(cb, data &repo_config, &opts); > > with repo_config set to the discovered git directory plus "/config"; > we'd read the repository configuration twice, in other words. Ahh, nevermind. The fix to make the "usual sequence" pay attention to the opts->git_dir comes in 3/3 and in that step this redundant reading is also removed, so all is well at the end. Thanks. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up 2017-04-18 2:56 ` Junio C Hamano @ 2017-04-18 3:46 ` Jeff King 0 siblings, 0 replies; 24+ messages in thread From: Jeff King @ 2017-04-18 3:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git, bert.wesarg On Mon, Apr 17, 2017 at 07:56:47PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > >> + if (!have_git_dir() && opts.git_dir) { > >> struct git_config_source repo_config; > >> > >> memset(&repo_config, 0, sizeof(repo_config)); > > > > But this one I do not quite understand. When have_git_dir() was > > false and asked discover_git_directory() to set opts.git_dir, we > > enter the body of this block and then end up doing > > > > git_config_with_options(cb, data &repo_config, &opts); > > > > with repo_config set to the discovered git directory plus "/config"; > > we'd read the repository configuration twice, in other words. > > Ahh, nevermind. The fix to make the "usual sequence" pay attention > to the opts->git_dir comes in 3/3 and in that step this redundant > reading is also removed, so all is well at the end. I think 2/3 is actually OK as-is. We know we didn't read repo config in the first call to git_config_with_options() because have_git_dir() is unset. So 2/3 doesn't make anything worse; the bug already existed before Duy's series (it goes back to my original read_early_config() hack). -Peff ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 3/3] config: correct file reading order in read_early_config() 2017-04-17 10:10 ` [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() Nguyễn Thái Ngọc Duy 2017-04-17 10:10 ` [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up Nguyễn Thái Ngọc Duy @ 2017-04-17 10:10 ` Nguyễn Thái Ngọc Duy 2017-04-18 3:53 ` Jeff King 2017-04-18 2:27 ` [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() Junio C Hamano 2 siblings, 1 reply; 24+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2017-04-17 10:10 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, bert.wesarg, Nguyễn Thái Ngọc Duy Config file reading order is important because each file can override values in the previous files and this is expected behavior. Normally we read in this order, all in do_git_config_sequence(): 1. $HOME/.gitconfig 2. $GIT_DIR/config 3. config from command line However in read_early_config() the order may be swapped a bit if setup_git_directory() has not been called: 1. $HOME/.gitconfig 2. $GIT_DIR/config is NOT read because .git dir is not found _yet_ 3. config from command line 4. $GIT_DIR/config is now READ (after discover_git_directory() call) The reading at step 4 could override config at step 3, which is not the expectation. Now that we could pass the .git dir around, we could feed discover_git_directory() back to step 2, so that it works again, and remove step 4. Noticed-by: Jeff King <peff@peff.net> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- config.c | 26 ++++++++++++-------------- t/helper/test-config.c | 4 ++++ t/t1309-early-config.sh | 17 +++++++++++++++++ 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/config.c b/config.c index 14f0417460..fffda653e3 100644 --- a/config.c +++ b/config.c @@ -1504,12 +1504,20 @@ int git_config_system(void) return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0); } -static int do_git_config_sequence(config_fn_t fn, void *data) +static int do_git_config_sequence(const struct config_options *opts, + config_fn_t fn, void *data) { int ret = 0; char *xdg_config = xdg_config_home("config"); char *user_config = expand_user_path("~/.gitconfig"); - char *repo_config = have_git_dir() ? git_pathdup("config") : NULL; + char *repo_config; + + if (opts->git_dir) + repo_config = mkpathdup("%s/config", opts->git_dir); + else if (have_git_dir()) + repo_config = git_pathdup("config"); + else + repo_config = NULL; current_parsing_scope = CONFIG_SCOPE_SYSTEM; if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) @@ -1563,7 +1571,7 @@ int git_config_with_options(config_fn_t fn, void *data, else if (config_source && config_source->blob) return git_config_from_blob_ref(fn, config_source->blob, data); - return do_git_config_sequence(fn, data); + return do_git_config_sequence(opts, fn, data); } static void git_config_raw(config_fn_t fn, void *data) @@ -1613,7 +1621,6 @@ void read_early_config(config_fn_t cb, void *data) { struct config_options opts = {0}; struct strbuf buf = STRBUF_INIT; - char *to_free = NULL; opts.respect_includes = 1; @@ -1628,20 +1635,11 @@ void read_early_config(config_fn_t cb, void *data) * call). */ else if (discover_git_directory(&buf)) - opts.git_dir = to_free = xstrdup(buf.buf); + opts.git_dir = buf.buf; git_config_with_options(cb, data, NULL, &opts); - if (!have_git_dir() && opts.git_dir) { - struct git_config_source repo_config; - - memset(&repo_config, 0, sizeof(repo_config)); - strbuf_addstr(&buf, "/config"); - repo_config.file = buf.buf; - git_config_with_options(cb, data, &repo_config, &opts); - } strbuf_release(&buf); - free(to_free); } static void git_config_check_init(void); diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 8e3ed6a76c..696d0a52fd 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -84,6 +84,10 @@ int cmd_main(int argc, const char **argv) struct config_set cs; if (argc == 3 && !strcmp(argv[1], "read_early_config")) { + const char *cmdline_config = getenv("CMDL_CFG"); + + if (cmdline_config) + git_config_push_parameter(cmdline_config); read_early_config(early_config_cb, (void *)argv[2]); return 0; } diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh index b97357b8ab..c15f18dd96 100755 --- a/t/t1309-early-config.sh +++ b/t/t1309-early-config.sh @@ -47,6 +47,23 @@ test_expect_success 'ceiling #2' ' test xdg = "$(cat output)" ' +test_expect_success 'read config file in right order' ' + echo "[test]source = home" >>.gitconfig && + git init foo && + ( + cd foo && + echo "[test]source = repo" >>.git/config && + CMDL_CFG=test.source=cmdline test-config \ + read_early_config test.source >actual && + cat >expected <<-\EOF && + home + repo + cmdline + EOF + test_cmp expected actual + ) +' + test_with_config () { rm -rf throwaway && git init throwaway && -- 2.11.0.157.gd943d85 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/3] config: correct file reading order in read_early_config() 2017-04-17 10:10 ` [PATCH v2 3/3] config: correct file reading order in read_early_config() Nguyễn Thái Ngọc Duy @ 2017-04-18 3:53 ` Jeff King 0 siblings, 0 replies; 24+ messages in thread From: Jeff King @ 2017-04-18 3:53 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, bert.wesarg On Mon, Apr 17, 2017 at 05:10:02PM +0700, Nguyễn Thái Ngọc Duy wrote: > diff --git a/t/helper/test-config.c b/t/helper/test-config.c > index 8e3ed6a76c..696d0a52fd 100644 > --- a/t/helper/test-config.c > +++ b/t/helper/test-config.c > @@ -84,6 +84,10 @@ int cmd_main(int argc, const char **argv) > struct config_set cs; > > if (argc == 3 && !strcmp(argv[1], "read_early_config")) { > + const char *cmdline_config = getenv("CMDL_CFG"); > + > + if (cmdline_config) > + git_config_push_parameter(cmdline_config); I think you can do without this hunk by just setting: GIT_CONFIG_PARAMETERS="'foo.bar=from-cmdline'" (note the single-quotes which must be there). See how t1308 does it. > +test_expect_success 'read config file in right order' ' > + echo "[test]source = home" >>.gitconfig && > + git init foo && > + ( > + cd foo && > + echo "[test]source = repo" >>.git/config && > + CMDL_CFG=test.source=cmdline test-config \ > + read_early_config test.source >actual && > + cat >expected <<-\EOF && > + home > + repo > + cmdline > + EOF > + test_cmp expected actual > + ) > +' This looks good (modulo the CMDL_CFG above). If we wanted to trigger it in a real-world test, we'd have to use pager config (since it's the only thing that uses early-config; alias lookup probably should do, but that's for another time). But I think this synthetic test is fine; it makes the output easy to verify. -Peff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() 2017-04-17 10:10 ` [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() Nguyễn Thái Ngọc Duy 2017-04-17 10:10 ` [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up Nguyễn Thái Ngọc Duy 2017-04-17 10:10 ` [PATCH v2 3/3] config: correct file reading order in read_early_config() Nguyễn Thái Ngọc Duy @ 2017-04-18 2:27 ` Junio C Hamano 2017-04-18 3:55 ` Jeff King 2 siblings, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2017-04-18 2:27 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, bert.wesarg Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > So far we can only pass one flag, respect_includes, to thie function. We s/thie/this/ > need to pass some more (non-flag even), so let's make it accept a struct > instead of an integer. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > builtin/config.c | 21 ++++++++++++--------- > cache.h | 7 ++++++- > config.c | 16 +++++++++++----- > 3 files changed, 29 insertions(+), 15 deletions(-) > > diff --git a/builtin/config.c b/builtin/config.c > index 4f49a0edb9..b937d175a9 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -26,7 +26,8 @@ static int use_global_config, use_system_config, use_local_config; > static struct git_config_source given_config_source; > static int actions, types; > static int end_null; > -static int respect_includes = -1; > +static int respect_includes_opt = -1; > +static struct config_options config_options; > static int show_origin; > > #define ACTION_GET (1<<0) > @@ -81,7 +82,7 @@ static struct option builtin_config_options[] = { > OPT_GROUP(N_("Other")), > OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")), > OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")), > - OPT_BOOL(0, "includes", &respect_includes, N_("respect include directives on lookup")), > + OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), It would be more in line with what the log message advertised if you did static struct config_options config_options = { -1, /* .respect_includes: unspecified */ }; OPT_BOOL(0, "includes", &config_options.respect_includes, N_("...")), no? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() 2017-04-18 2:27 ` [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() Junio C Hamano @ 2017-04-18 3:55 ` Jeff King 2017-04-18 4:51 ` Junio C Hamano 0 siblings, 1 reply; 24+ messages in thread From: Jeff King @ 2017-04-18 3:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git, bert.wesarg On Mon, Apr 17, 2017 at 07:27:16PM -0700, Junio C Hamano wrote: > > @@ -81,7 +82,7 @@ static struct option builtin_config_options[] = { > > OPT_GROUP(N_("Other")), > > OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")), > > OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")), > > - OPT_BOOL(0, "includes", &respect_includes, N_("respect include directives on lookup")), > > + OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), > > It would be more in line with what the log message advertised if you > did > > static struct config_options config_options = { > -1, /* .respect_includes: unspecified */ > }; > > OPT_BOOL(0, "includes", &config_options.respect_includes, N_("...")), > > no? I think I like the split between the option-value here and the "final" value that goes into config_options.respect_includes. Because we actually munge it later based on the given-config value anyway. So I agree this makes the diff larger than it might need to be, but I think the end result is a bit nicer. -Peff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() 2017-04-18 3:55 ` Jeff King @ 2017-04-18 4:51 ` Junio C Hamano 0 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2017-04-18 4:51 UTC (permalink / raw) To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git, bert.wesarg Jeff King <peff@peff.net> writes: > On Mon, Apr 17, 2017 at 07:27:16PM -0700, Junio C Hamano wrote: > >> > @@ -81,7 +82,7 @@ static struct option builtin_config_options[] = { >> > OPT_GROUP(N_("Other")), >> > OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")), >> > OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")), >> > - OPT_BOOL(0, "includes", &respect_includes, N_("respect include directives on lookup")), >> > + OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), >> >> It would be more in line with what the log message advertised if you >> did >> >> static struct config_options config_options = { >> -1, /* .respect_includes: unspecified */ >> }; >> >> OPT_BOOL(0, "includes", &config_options.respect_includes, N_("...")), >> >> no? > > I think I like the split between the option-value here and the "final" > value that goes into config_options.respect_includes. Because we > actually munge it later based on the given-config value anyway. > > So I agree this makes the diff larger than it might need to be, but I > think the end result is a bit nicer. Yeah, I didn't see the end result was a single bit (unsigned :1). This separation is OK. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up 2017-04-17 10:07 ` Duy Nguyen 2017-04-17 10:10 ` [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() Nguyễn Thái Ngọc Duy @ 2017-04-18 3:17 ` Junio C Hamano 2017-04-18 4:03 ` Jeff King 2 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2017-04-18 3:17 UTC (permalink / raw) To: Duy Nguyen; +Cc: Jeff King, Git Mailing List, Bert Wesarg Duy Nguyen <pclouds@gmail.com> writes: > (To Junio, this series conflicts slightly with > nd/conditional-config-include, let me know if you want me to rebase > this on top of that) I think I can manage---having to resolve inter-topic conflict every once in a while is a good sanity check for me anyway ;-). Please holler if my resolution is faulty when I push things out tonight my time (note: I'll be operating in GMT+9 for coming weeks). Thanks. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up 2017-04-17 10:07 ` Duy Nguyen 2017-04-17 10:10 ` [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() Nguyễn Thái Ngọc Duy 2017-04-18 3:17 ` [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up Junio C Hamano @ 2017-04-18 4:03 ` Jeff King 2 siblings, 0 replies; 24+ messages in thread From: Jeff King @ 2017-04-18 4:03 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Bert Wesarg On Mon, Apr 17, 2017 at 05:07:52PM +0700, Duy Nguyen wrote: > > You can see the second problem with: > > > > # random external > > cat >git-foo <<-\EOF > > #!/bin/sh > > echo foo > > EOF > > chmod +x git-foo > > PATH=$PWD:$PATH > > > > git init > > git config pager.foo 'sed s/^/repo:/' > > git -c pager.foo='sed s/^/cmdline:/' foo > > > > That command should prefer the cmdline config, but it doesn't. > > I actually had problems seeing the problem, for some reason it didn't > work for me. I guess I made a mistake somewhere. I just double-checked it, and the above works for me. <shrug> > > The fix is something like what's below, which is easy on top of your new > > options struct. I can wrap it up with a config message and test on top > > of your series. > > ... anyway I read this last sentence too late and spent too much time > wrapping your changes in a patch (well most of the time was spent on > writing a new test actually), so I'm sending it out too. My test uses > test-config though (I have given up on dealing with pager and tty). Thanks for doing that. I had planned to do it myself to avoid making work for you, but what you posted looks good. I hadn't thought about tty issues. You'd have to run it under test_terminal (which isn't even available everywhere). So your test-config version is probably the best bet. > Off topic. Back to the pager.foo thing (because I had to fight it a > bit before giving up). I find it a bit unintuitive that "--paginate" > forces the pager back to less (or $PAGER) when I say "pager.foo = > my-favorite-pager". Back when pager.foo is a boolean thing, it makes > sense for --paginate to override the "to page or not to page" > decision. But then you added a new meaning too pager.foo (what command > to run). "--paginate" should respect the command pager.foo specifies > when its value is a command, I think. I never noticed that before, and I agree that it's nonsense. Using "-p" should be unnecessary if you have pager.foo defined (because it already implies "yes, use the pager"). But if you were to do it anyway, I agree it should not ignore the configured pager. One might use an extra "-p" hoping that it would override the tty check, but it doesn't. It's too late to change that now, I think, but I have been tempted to add a --paginate-me-harder option. It would be handy for tests. -Peff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] config: prepare to pass more info in git_config_with_options() 2017-04-16 10:41 ` [PATCH 1/2] config: prepare to pass more info in git_config_with_options() Nguyễn Thái Ngọc Duy 2017-04-16 10:41 ` [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up Nguyễn Thái Ngọc Duy @ 2017-04-16 15:31 ` Jeff King 2017-04-17 1:42 ` Duy Nguyen 1 sibling, 1 reply; 24+ messages in thread From: Jeff King @ 2017-04-16 15:31 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, bert.wesarg On Sun, Apr 16, 2017 at 05:41:24PM +0700, Nguyễn Thái Ngọc Duy wrote: > So far we can only pass one flag, respect_includes, to thie function. We > need to pass some more (non-flag even), so let's make it accept a struct > instead of an integer. Yeah, this makes sense. But... > diff --git a/builtin/config.c b/builtin/config.c > index 4f49a0edb9..5de4a36146 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -26,7 +26,8 @@ static int use_global_config, use_system_config, use_local_config; > static struct git_config_source given_config_source; > static int actions, types; > static int end_null; > -static int respect_includes = -1; > +static int respect_includes_opt; > +static struct config_options config_options; It fails all the git-config "include" tests because respect_includes_opt is missing the initialization to "-1". -Peff ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] config: prepare to pass more info in git_config_with_options() 2017-04-16 15:31 ` [PATCH 1/2] config: prepare to pass more info in git_config_with_options() Jeff King @ 2017-04-17 1:42 ` Duy Nguyen 0 siblings, 0 replies; 24+ messages in thread From: Duy Nguyen @ 2017-04-17 1:42 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, bert.wesarg On Sun, Apr 16, 2017 at 11:31:28AM -0400, Jeff King wrote: > On Sun, Apr 16, 2017 at 05:41:24PM +0700, Nguyễn Thái Ngọc Duy wrote: > > > So far we can only pass one flag, respect_includes, to thie function. We > > need to pass some more (non-flag even), so let's make it accept a struct > > instead of an integer. > > Yeah, this makes sense. But... > > > diff --git a/builtin/config.c b/builtin/config.c > > index 4f49a0edb9..5de4a36146 100644 > > --- a/builtin/config.c > > +++ b/builtin/config.c > > @@ -26,7 +26,8 @@ static int use_global_config, use_system_config, use_local_config; > > static struct git_config_source given_config_source; > > static int actions, types; > > static int end_null; > > -static int respect_includes = -1; > > +static int respect_includes_opt; > > +static struct config_options config_options; > > It fails all the git-config "include" tests because respect_includes_opt > is missing the initialization to "-1". Serves me right for doing last-minute "harmless" refactoring without rerunning the test suite :( > > -Peff ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-04-18 4:51 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-14 17:04 includeIf breaks calling dashed externals Bert Wesarg 2017-04-14 17:43 ` Jeff King 2017-04-15 11:49 ` Duy Nguyen 2017-04-16 4:50 ` Jeff King 2017-04-16 10:41 ` [PATCH 1/2] config: prepare to pass more info in git_config_with_options() Nguyễn Thái Ngọc Duy 2017-04-16 10:41 ` [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up Nguyễn Thái Ngọc Duy 2017-04-16 15:51 ` Jeff King 2017-04-17 2:13 ` Duy Nguyen 2017-04-18 3:56 ` Jeff King 2017-04-17 10:07 ` Duy Nguyen 2017-04-17 10:10 ` [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() Nguyễn Thái Ngọc Duy 2017-04-17 10:10 ` [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up Nguyễn Thái Ngọc Duy 2017-04-18 2:49 ` Junio C Hamano 2017-04-18 2:56 ` Junio C Hamano 2017-04-18 3:46 ` Jeff King 2017-04-17 10:10 ` [PATCH v2 3/3] config: correct file reading order in read_early_config() Nguyễn Thái Ngọc Duy 2017-04-18 3:53 ` Jeff King 2017-04-18 2:27 ` [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options() Junio C Hamano 2017-04-18 3:55 ` Jeff King 2017-04-18 4:51 ` Junio C Hamano 2017-04-18 3:17 ` [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up Junio C Hamano 2017-04-18 4:03 ` Jeff King 2017-04-16 15:31 ` [PATCH 1/2] config: prepare to pass more info in git_config_with_options() Jeff King 2017-04-17 1:42 ` Duy Nguyen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.