{"thread":{"id":"45690","subject":"includeIf breaks calling dashed externals","startedAt":"2017-04-14T17:04:47Z","lastAt":"2017-04-18T04:51:50Z","messageCount":24,"participants":["Bert Wesarg","Jeff King","Duy Nguyen","Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"316832","messageId":"CAKPyHN1W175wm1doQHF71vB3PHpU2o=xtEAcZMqZtR=U_Kv0_g@mail.gmail.com","threadId":"45690","inReplyTo":null,"subject":"includeIf breaks calling dashed externals","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2017-04-14T17:04:23Z","receivedAt":"2017-04-14T17:04:47Z","isPatch":false,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"Dear Duy,\n\nheaving an includeIf in a git config file breaks calling external git\ncommands, most prominently git-gui.\n\n$ git --version\ngit version 2.12.2.599.gcf11a6797\n$ git rev-parse --is-inside-work-tree\ntrue\n$ git echo\ngit: 'echo' is not a git command. See 'git --help'.\n\nDid you mean this?\n    fetch\n$ echo '[includeIf \"gitdir:does-not-exists\"]path = does-not-exists'\n>>.git/config\n$ git rev-parse --is-inside-work-tree\ntrue\n$ git echo\nfatal: BUG: setup_git_env called without repository\n\nBest,\nBert\n"},{"id":"316833","messageId":"20170414174337.n6qksfvugkrexnsi@sigill.intra.peff.net","threadId":"45690","inReplyTo":"CAKPyHN1W175wm1doQHF71vB3PHpU2o=xtEAcZMqZtR=U_Kv0_g@mail.gmail.com","subject":"Re: includeIf breaks calling dashed externals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-14T17:43:37Z","receivedAt":"2017-04-14T17:43:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 14, 2017 at 07:04:23PM +0200, Bert Wesarg wrote:\n\n> Dear Duy,\n> \n> heaving an includeIf in a git config file breaks calling external git\n> commands, most prominently git-gui.\n> \n> $ git --version\n> git version 2.12.2.599.gcf11a6797\n> $ git rev-parse --is-inside-work-tree\n> true\n> $ git echo\n> git: 'echo' is not a git command. See 'git --help'.\n> \n> Did you mean this?\n>     fetch\n> $ echo '[includeIf \"gitdir:does-not-exists\"]path = does-not-exists'\n> >>.git/config\n> $ git rev-parse --is-inside-work-tree\n> true\n> $ git echo\n> fatal: BUG: setup_git_env called without repository\n\nProbably this fixes it:\n\ndiff --git a/config.c b/config.c\nindex b6e4a57b9..8d66bdf56 100644\n--- a/config.c\n+++ b/config.c\n@@ -213,6 +213,9 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n \tstruct strbuf pattern = STRBUF_INIT;\n \tint ret = 0, prefix;\n \n+\tif (!have_git_dir())\n+\t\treturn 0;\n+\n \tstrbuf_add_absolute_path(&text, get_git_dir());\n \tstrbuf_add(&pattern, cond, cond_len);\n \tprefix = prepare_include_condition_pattern(&pattern);\n\nBut it does raise a question of reading config before/after repository\nsetup, since those will give different answers. I guess they do anyway\nbecause of $GIT_DIR/config.\n\n-Peff\n"},{"id":"316887","messageId":"20170415114901.GA5727@ash","threadId":"45690","inReplyTo":"20170414174337.n6qksfvugkrexnsi@sigill.intra.peff.net","subject":"Re: includeIf breaks calling dashed externals","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-15T11:49:01Z","receivedAt":"2017-04-15T11:49:24Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Apr 14, 2017 at 01:43:37PM -0400, Jeff King wrote:\n> On Fri, Apr 14, 2017 at 07:04:23PM +0200, Bert Wesarg wrote:\n> \n> > Dear Duy,\n> > \n> > heaving an includeIf in a git config file breaks calling external git\n> > commands, most prominently git-gui.\n> > \n> > $ git --version\n> > git version 2.12.2.599.gcf11a6797\n> > $ git rev-parse --is-inside-work-tree\n> > true\n> > $ git echo\n> > git: 'echo' is not a git command. See 'git --help'.\n> > \n> > Did you mean this?\n> >     fetch\n> > $ echo '[includeIf \"gitdir:does-not-exists\"]path = does-not-exists'\n> > >>.git/config\n> > $ git rev-parse --is-inside-work-tree\n> > true\n> > $ git echo\n> > fatal: BUG: setup_git_env called without repository\n> \n> Probably this fixes it:\n> \n> diff --git a/config.c b/config.c\n> index b6e4a57b9..8d66bdf56 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -213,6 +213,9 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n>  \tstruct strbuf pattern = STRBUF_INIT;\n>  \tint ret = 0, prefix;\n>  \n> +\tif (!have_git_dir())\n> +\t\treturn 0;\n> +\n>  \tstrbuf_add_absolute_path(&text, get_git_dir());\n>  \tstrbuf_add(&pattern, cond, cond_len);\n>  \tprefix = prepare_include_condition_pattern(&pattern);\n> \n> But it does raise a question of reading config before/after repository\n> setup, since those will give different answers. I guess they do anyway\n> because of $GIT_DIR/config.\n\nThis happens in execv_dased_external() -> check_pager_config() ->\nread_early_config(). We probably could use the same discover_git_directory\ntrick to get .git dir (because we should find it). Maybe something\nlike this instead?\n\ndiff --git a/config.c b/config.c\nindex 1a4d85537b..4f540ae578 100644\n--- a/config.c\n+++ b/config.c\n@@ -212,8 +212,14 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n \tstruct strbuf text = STRBUF_INIT;\n \tstruct strbuf pattern = STRBUF_INIT;\n \tint ret = 0, prefix;\n+\tstruct strbuf gitdir = STRBUF_INIT;\n \n-\tstrbuf_add_absolute_path(&text, get_git_dir());\n+\tif (have_git_dir())\n+\t\tstrbuf_addstr(&gitdir, get_git_dir());\n+\telse if (!discover_git_directory(&gitdir))\n+\t\tgoto done;\n+\n+\tstrbuf_add_absolute_path(&text, gitdir.buf);\n \tstrbuf_add(&pattern, cond, cond_len);\n \tprefix = prepare_include_condition_pattern(&pattern);\n \n@@ -237,6 +243,7 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n \t\t\t icase ? WM_CASEFOLD : 0, NULL);\n \n done:\n+\tstrbuf_release(&gitdir);\n \tstrbuf_release(&pattern);\n \tstrbuf_release(&text);\n \treturn ret;\n\n--\nDuy\n"},{"id":"316912","messageId":"20170416045004.2ghhiv7jzgroejgw@sigill.intra.peff.net","threadId":"45690","inReplyTo":"20170415114901.GA5727@ash","subject":"Re: includeIf breaks calling dashed externals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-16T04:50:04Z","receivedAt":"2017-04-16T04:50:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 15, 2017 at 06:49:01PM +0700, Duy Nguyen wrote:\n\n> > Probably this fixes it:\n> > \n> > diff --git a/config.c b/config.c\n> > index b6e4a57b9..8d66bdf56 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -213,6 +213,9 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n> >  \tstruct strbuf pattern = STRBUF_INIT;\n> >  \tint ret = 0, prefix;\n> >  \n> > +\tif (!have_git_dir())\n> > +\t\treturn 0;\n> > +\n> >  \tstrbuf_add_absolute_path(&text, get_git_dir());\n> >  \tstrbuf_add(&pattern, cond, cond_len);\n> >  \tprefix = prepare_include_condition_pattern(&pattern);\n> > \n> > But it does raise a question of reading config before/after repository\n> > setup, since those will give different answers. I guess they do anyway\n> > because of $GIT_DIR/config.\n> \n> This happens in execv_dased_external() -> check_pager_config() ->\n> read_early_config(). We probably could use the same discover_git_directory\n> trick to get .git dir (because we should find it). Maybe something\n> like this instead?\n\nI know that we only kick in discover_git_directory() in certain cases\nwhen we're reading config (for the \"early config\"). Would it makes sense\nto respect the same rules here?\n\nI.e., if we have a program that wants to look at config but explicitly\n_doesn't_ setup the git repository, is it right to say that we are\ninside that repository?\n\nSo instead of lazily doing the discovery here:\n\n> diff --git a/config.c b/config.c\n> index 1a4d85537b..4f540ae578 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -212,8 +212,14 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n>  \tstruct strbuf text = STRBUF_INIT;\n>  \tstruct strbuf pattern = STRBUF_INIT;\n>  \tint ret = 0, prefix;\n> +\tstruct strbuf gitdir = STRBUF_INIT;\n>  \n> -\tstrbuf_add_absolute_path(&text, get_git_dir());\n> +\tif (have_git_dir())\n> +\t\tstrbuf_addstr(&gitdir, get_git_dir());\n> +\telse if (!discover_git_directory(&gitdir))\n> +\t\tgoto done;\n\n..should we record the discovered git dir in read_early_config(), and\nuse it only if available? And if not, then we know the program is\nexplicitly trying not to be in a repo (or we know we tried to discover\none already and it didn't exist).\n\n-Peff\n"},{"id":"316941","messageId":"20170416104125.15300-1-pclouds@gmail.com","threadId":"45690","inReplyTo":"20170416045004.2ghhiv7jzgroejgw@sigill.intra.peff.net","subject":"[PATCH 1/2] config: prepare to pass more info in git_config_with_options()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-16T10:41:24Z","receivedAt":"2017-04-16T10:41:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"So far we can only pass one flag, respect_includes, to thie function. We\nneed to pass some more (non-flag even), so let's make it accept a struct\ninstead of an integer.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Prep patch for the bug fix in 2/2.\n\n builtin/config.c | 21 ++++++++++++---------\n cache.h          |  7 ++++++-\n config.c         | 16 +++++++++++-----\n 3 files changed, 29 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 4f49a0edb9..5de4a36146 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -26,7 +26,8 @@ static int use_global_config, use_system_config, use_local_config;\n static struct git_config_source given_config_source;\n static int actions, types;\n static int end_null;\n-static int respect_includes = -1;\n+static int respect_includes_opt;\n+static struct config_options config_options;\n static int show_origin;\n \n #define ACTION_GET (1<<0)\n@@ -81,7 +82,7 @@ static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Other\")),\n \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n-\tOPT_BOOL(0, \"includes\", &respect_includes, N_(\"respect include directives on lookup\")),\n+\tOPT_BOOL(0, \"includes\", &respect_includes_opt, N_(\"respect include directives on lookup\")),\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n \tOPT_END(),\n };\n@@ -242,7 +243,7 @@ static int get_value(const char *key_, const char *regex_)\n \t}\n \n \tgit_config_with_options(collect_config, &values,\n-\t\t\t\t&given_config_source, respect_includes);\n+\t\t\t\t&given_config_source, &config_options);\n \n \tret = !values.nr;\n \n@@ -320,7 +321,7 @@ static void get_color(const char *var, const char *def_color)\n \tget_color_found = 0;\n \tparsed_color[0] = '\\0';\n \tgit_config_with_options(git_get_color_config, NULL,\n-\t\t\t\t&given_config_source, respect_includes);\n+\t\t\t\t&given_config_source, &config_options);\n \n \tif (!get_color_found && def_color) {\n \t\tif (color_parse(def_color, parsed_color) < 0)\n@@ -352,7 +353,7 @@ static int get_colorbool(const char *var, int print)\n \tget_diff_color_found = -1;\n \tget_color_ui_found = -1;\n \tgit_config_with_options(git_get_colorbool_config, NULL,\n-\t\t\t\t&given_config_source, respect_includes);\n+\t\t\t\t&given_config_source, &config_options);\n \n \tif (get_colorbool_found < 0) {\n \t\tif (!strcmp(get_colorbool_slot, \"color.diff\"))\n@@ -441,7 +442,7 @@ static int get_urlmatch(const char *var, const char *url)\n \t}\n \n \tgit_config_with_options(urlmatch_config_entry, &config,\n-\t\t\t\t&given_config_source, respect_includes);\n+\t\t\t\t&given_config_source, &config_options);\n \n \tret = !values.nr;\n \n@@ -530,8 +531,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t\tprefix_filename(prefix, given_config_source.file);\n \t}\n \n-\tif (respect_includes == -1)\n-\t\trespect_includes = !given_config_source.file;\n+\tif (respect_includes_opt == -1)\n+\t\tconfig_options.respect_includes = !given_config_source.file;\n+\telse\n+\t\tconfig_options.respect_includes = respect_includes_opt;\n \n \tif (end_null) {\n \t\tterm = '\\0';\n@@ -578,7 +581,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (git_config_with_options(show_all_config, NULL,\n \t\t\t\t\t    &given_config_source,\n-\t\t\t\t\t    respect_includes) < 0) {\n+\t\t\t\t\t    &config_options) < 0) {\n \t\t\tif (given_config_source.file)\n \t\t\t\tdie_errno(\"unable to read config file '%s'\",\n \t\t\t\t\t  given_config_source.file);\ndiff --git a/cache.h b/cache.h\nindex 5c8078291c..e29a093839 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1882,6 +1882,10 @@ enum config_origin_type {\n \tCONFIG_ORIGIN_CMDLINE\n };\n \n+struct config_options {\n+\tunsigned int respect_includes : 1;\n+};\n+\n typedef int (*config_fn_t)(const char *, const char *, void *);\n extern int git_default_config(const char *, const char *, void *);\n extern int git_config_from_file(config_fn_t fn, const char *, void *);\n@@ -1895,7 +1899,7 @@ extern void read_early_config(config_fn_t cb, void *data);\n extern void git_config(config_fn_t fn, void *);\n extern int git_config_with_options(config_fn_t fn, void *,\n \t\t\t\t   struct git_config_source *config_source,\n-\t\t\t\t   int respect_includes);\n+\t\t\t\t   const struct config_options *opts);\n extern int git_parse_ulong(const char *, unsigned long *);\n extern int git_parse_maybe_bool(const char *);\n extern int git_config_int(const char *, const char *);\n@@ -1947,6 +1951,7 @@ struct config_include_data {\n \tint depth;\n \tconfig_fn_t fn;\n \tvoid *data;\n+\tconst struct config_options *opts;\n };\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\ndiff --git a/config.c b/config.c\nindex 1a4d85537b..042321a3a0 100644\n--- a/config.c\n+++ b/config.c\n@@ -1530,13 +1530,14 @@ static int do_git_config_sequence(config_fn_t fn, void *data)\n \n int git_config_with_options(config_fn_t fn, void *data,\n \t\t\t    struct git_config_source *config_source,\n-\t\t\t    int respect_includes)\n+\t\t\t    const struct config_options *opts)\n {\n \tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n \n-\tif (respect_includes) {\n+\tif (opts->respect_includes) {\n \t\tinc.fn = fn;\n \t\tinc.data = data;\n+\t\tinc.opts = opts;\n \t\tfn = git_config_include;\n \t\tdata = &inc;\n \t}\n@@ -1557,7 +1558,10 @@ int git_config_with_options(config_fn_t fn, void *data,\n \n static void git_config_raw(config_fn_t fn, void *data)\n {\n-\tif (git_config_with_options(fn, data, NULL, 1) < 0)\n+\tstruct config_options opts = {0};\n+\n+\topts.respect_includes = 1;\n+\tif (git_config_with_options(fn, data, NULL, &opts) < 0)\n \t\t/*\n \t\t * git_config_with_options() normally returns only\n \t\t * zero, as most errors are fatal, and\n@@ -1597,9 +1601,11 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n \n void read_early_config(config_fn_t cb, void *data)\n {\n+\tstruct config_options opts = {0};\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tgit_config_with_options(cb, data, NULL, 1);\n+\topts.respect_includes = 1;\n+\tgit_config_with_options(cb, data, NULL, &opts);\n \n \t/*\n \t * When setup_git_directory() was not yet asked to discover the\n@@ -1615,7 +1621,7 @@ void read_early_config(config_fn_t cb, void *data)\n \t\tmemset(&repo_config, 0, sizeof(repo_config));\n \t\tstrbuf_addstr(&buf, \"/config\");\n \t\trepo_config.file = buf.buf;\n-\t\tgit_config_with_options(cb, data, &repo_config, 1);\n+\t\tgit_config_with_options(cb, data, &repo_config, &opts);\n \t}\n \tstrbuf_release(&buf);\n }\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"316942","messageId":"20170416104125.15300-2-pclouds@gmail.com","threadId":"45690","inReplyTo":"20170416104125.15300-1-pclouds@gmail.com","subject":"[PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-16T10:41:25Z","receivedAt":"2017-04-16T10:41:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If setup_git_directory() and friends have not been called,\nget_git_dir() (because of includeIf.gitdir:XXX) would lead to\n\n    die(\"BUG: setup_git_env called without repository\");\n\nThere are two cases when a config file could be read before $GIT_DIR is\nlocated. The first one is check_repository_format(), where we read just\nthe one file $GIT_DIR/config to check if we could understand this\nrepository. This case should be safe. The concerned variables should\nnever be hidden away behind includes anyway.\n\nThe second one is triggered in check_pager_config() when we're about to\nrun an external git command. We might be able to find $GIT_DIR in this\ncase, which is exactly what read_early_config() does (and also is the\nwhat check_pager_config() uses). Conditional includes and\nget_git_dir() could be triggered by the first git_config_with_options()\ncall there, before discover_git_directory() is used as a fallback\n$GIT_DIR detection.\n\nDetect this special \"early reading\" case, pass down the $GIT_DIR,\neither from previous setup or detected by discover_git_directory(),\nand make conditional include use it.\n\nNoticed-by: Bert Wesarg <bert.wesarg@googlemail.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n cache.h                   |  2 ++\n config.c                  | 35 ++++++++++++++++++++++++++---------\n t/t1305-config-include.sh | 11 +++++++++++\n 3 files changed, 39 insertions(+), 9 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e29a093839..27b7286f99 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1884,6 +1884,8 @@ enum config_origin_type {\n \n struct config_options {\n \tunsigned int respect_includes : 1;\n+\tunsigned int early_config : 1;\n+\tconst char *git_dir; /* only valid when early_config is true */\n };\n \n typedef int (*config_fn_t)(const char *, const char *, void *);\ndiff --git a/config.c b/config.c\nindex 042321a3a0..f323b96283 100644\n--- a/config.c\n+++ b/config.c\n@@ -207,13 +207,20 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n \treturn prefix;\n }\n \n-static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n+static int include_by_gitdir(const struct config_options *opts,\n+\t\t\t     const char *cond, size_t cond_len, int icase)\n {\n \tstruct strbuf text = STRBUF_INIT;\n \tstruct strbuf pattern = STRBUF_INIT;\n \tint ret = 0, prefix;\n \n-\tstrbuf_add_absolute_path(&text, get_git_dir());\n+\tif (!opts->early_config)\n+\t\tstrbuf_add_absolute_path(&text, get_git_dir());\n+\telse if (opts->git_dir)\n+\t\tstrbuf_add_absolute_path(&text, opts->git_dir);\n+\telse\n+\t\tgoto done;\n+\n \tstrbuf_add(&pattern, cond, cond_len);\n \tprefix = prepare_include_condition_pattern(&pattern);\n \n@@ -242,13 +249,14 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n \treturn ret;\n }\n \n-static int include_condition_is_true(const char *cond, size_t cond_len)\n+static int include_condition_is_true(const struct config_options *opts,\n+\t\t\t\t     const char *cond, size_t cond_len)\n {\n \n \tif (skip_prefix_mem(cond, cond_len, \"gitdir:\", &cond, &cond_len))\n-\t\treturn include_by_gitdir(cond, cond_len, 0);\n+\t\treturn include_by_gitdir(opts, cond, cond_len, 0);\n \telse if (skip_prefix_mem(cond, cond_len, \"gitdir/i:\", &cond, &cond_len))\n-\t\treturn include_by_gitdir(cond, cond_len, 1);\n+\t\treturn include_by_gitdir(opts, cond, cond_len, 1);\n \n \t/* unknown conditionals are always false */\n \treturn 0;\n@@ -273,7 +281,7 @@ int git_config_include(const char *var, const char *value, void *data)\n \t\tret = handle_path_include(value, inc);\n \n \tif (!parse_config_key(var, \"includeif\", &cond, &cond_len, &key) &&\n-\t    (cond && include_condition_is_true(cond, cond_len)) &&\n+\t    (cond && include_condition_is_true(inc->opts, cond, cond_len)) &&\n \t    !strcmp(key, \"path\"))\n \t\tret = handle_path_include(value, inc);\n \n@@ -1603,10 +1611,13 @@ void read_early_config(config_fn_t cb, void *data)\n {\n \tstruct config_options opts = {0};\n \tstruct strbuf buf = STRBUF_INIT;\n+\tchar *to_free = NULL;\n \n \topts.respect_includes = 1;\n-\tgit_config_with_options(cb, data, NULL, &opts);\n+\topts.early_config = 1;\n \n+\tif (have_git_dir())\n+\t\topts.git_dir = get_git_dir();\n \t/*\n \t * When setup_git_directory() was not yet asked to discover the\n \t * GIT_DIR, we ask discover_git_directory() to figure out whether there\n@@ -1615,15 +1626,21 @@ void read_early_config(config_fn_t cb, void *data)\n \t * notably, the current working directory is still the same after the\n \t * call).\n \t */\n-\tif (!have_git_dir() && discover_git_directory(&buf)) {\n+\telse if (discover_git_directory(&buf))\n+\t\topts.git_dir = to_free = strbuf_detach(&buf, NULL);\n+\n+\tgit_config_with_options(cb, data, NULL, &opts);\n+\n+\tif (opts.git_dir) {\n \t\tstruct git_config_source repo_config;\n \n \t\tmemset(&repo_config, 0, sizeof(repo_config));\n-\t\tstrbuf_addstr(&buf, \"/config\");\n+\t\tstrbuf_addf(&buf, \"%s/config\", opts.git_dir);\n \t\trepo_config.file = buf.buf;\n \t\tgit_config_with_options(cb, data, &repo_config, &opts);\n \t}\n \tstrbuf_release(&buf);\n+\tfree(to_free);\n }\n \n static void git_config_check_init(void);\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex e833939320..fddb47bafa 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -208,6 +208,17 @@ test_expect_success 'conditional include, both unanchored, icase' '\n \t)\n '\n \n+test_expect_success 'conditional include, early config reading' '\n+\t(\n+\t\tcd foo &&\n+\t\techo \"[includeIf \\\"gitdir:foo/\\\"]path=bar6\" >>.git/config &&\n+\t\techo \"[test]six=6\" >.git/bar6 &&\n+\t\techo 6 >expect &&\n+\t\ttest-config read_early_config test.six >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'include cycles are detected' '\n \tcat >.gitconfig <<-\\EOF &&\n \t[test]value = gitconfig\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"316948","messageId":"20170416153127.f227kdbfctgrumh4@sigill.intra.peff.net","threadId":"45690","inReplyTo":"20170416104125.15300-1-pclouds@gmail.com","subject":"Re: [PATCH 1/2] config: prepare to pass more info in git_config_with_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-16T15:31:28Z","receivedAt":"2017-04-16T15:31:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 16, 2017 at 05:41:24PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> So far we can only pass one flag, respect_includes, to thie function. We\n> need to pass some more (non-flag even), so let's make it accept a struct\n> instead of an integer.\n\nYeah, this makes sense. But...\n\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 4f49a0edb9..5de4a36146 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -26,7 +26,8 @@ static int use_global_config, use_system_config, use_local_config;\n>  static struct git_config_source given_config_source;\n>  static int actions, types;\n>  static int end_null;\n> -static int respect_includes = -1;\n> +static int respect_includes_opt;\n> +static struct config_options config_options;\n\nIt fails all the git-config \"include\" tests because respect_includes_opt\nis missing the initialization to \"-1\".\n\n-Peff\n"},{"id":"316949","messageId":"20170416155131.ppp5iakohiiphzmk@sigill.intra.peff.net","threadId":"45690","inReplyTo":"20170416104125.15300-2-pclouds@gmail.com","subject":"Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-16T15:51:32Z","receivedAt":"2017-04-16T15:51:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 16, 2017 at 05:41:25PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> If setup_git_directory() and friends have not been called,\n> get_git_dir() (because of includeIf.gitdir:XXX) would lead to\n> \n>     die(\"BUG: setup_git_env called without repository\");\n> \n> There are two cases when a config file could be read before $GIT_DIR is\n> located. The first one is check_repository_format(), where we read just\n> the one file $GIT_DIR/config to check if we could understand this\n> repository. This case should be safe. The concerned variables should\n> never be hidden away behind includes anyway.\n\nRight, we should not even have respect_includes turned on for that case.\n\n> The second one is triggered in check_pager_config() when we're about to\n> run an external git command. We might be able to find $GIT_DIR in this\n> case, which is exactly what read_early_config() does (and also is the\n> what check_pager_config() uses). Conditional includes and\n\ns/the what/what/\n\n> diff --git a/cache.h b/cache.h\n> index e29a093839..27b7286f99 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1884,6 +1884,8 @@ enum config_origin_type {\n>  \n>  struct config_options {\n>  \tunsigned int respect_includes : 1;\n> +\tunsigned int early_config : 1;\n> +\tconst char *git_dir; /* only valid when early_config is true */\n>  };\n\nWhy do we need both the flag and the string? Later, you do:\n\n> -static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n> +static int include_by_gitdir(const struct config_options *opts,\n> +\t\t\t     const char *cond, size_t cond_len, int icase)\n>  {\n>  \tstruct strbuf text = STRBUF_INIT;\n>  \tstruct strbuf pattern = STRBUF_INIT;\n>  \tint ret = 0, prefix;\n>  \n> -\tstrbuf_add_absolute_path(&text, get_git_dir());\n> +\tif (!opts->early_config)\n> +\t\tstrbuf_add_absolute_path(&text, get_git_dir());\n> +\telse if (opts->git_dir)\n> +\t\tstrbuf_add_absolute_path(&text, opts->git_dir);\n> +\telse\n> +\t\tgoto done;\n\nSo we call get_git_dir() always when we're not in early config. Even if\nwe don't have a git dir! Doesn't this mean that programs operating\noutside of a repo will still hit the BUG? I.e.:\n\n  git config --global includeif.gitdir:/whatever.path foo\n  cd /not/a/git/dir\n  git diff --no-index foo bar\n\n?\n\nI think instead the logic should be:\n\n  if (opts->git_dir)\n\tstrbuf_add_absolute_path(&text, opts->git_dir);\n  else if (have_git_dir())\n\tstrbuf_add_absolute_path(&text, get_git_dir());\n  else\n\tgoto done;\n\nI'd also be tempted to call the option field \"override_git_dir\" or\nsomething to indicate that it takes precedence over the normal one. With\nthe current code it doesn't matter, because we set it only to the result\nof the discovered dir.\n\n> @@ -1615,15 +1626,21 @@ void read_early_config(config_fn_t cb, void *data)\n>  \t * notably, the current working directory is still the same after the\n>  \t * call).\n>  \t */\n> -\tif (!have_git_dir() && discover_git_directory(&buf)) {\n> +\telse if (discover_git_directory(&buf))\n> +\t\topts.git_dir = to_free = strbuf_detach(&buf, NULL);\n\nThis to_free seemed redundant to me at first; why not just hold on to\nthe strbuf and release it later?\n\nThe answer is that we reuse the strbuf to generate the config-file path\nlater. However, by detaching, we clear the strbuf. So later when we\nuse it:\n\n\n> +\tif (opts.git_dir) {\n>  \t\tstruct git_config_source repo_config;\n>  \n>  \t\tmemset(&repo_config, 0, sizeof(repo_config));\n> -\t\tstrbuf_addstr(&buf, \"/config\");\n> +\t\tstrbuf_addf(&buf, \"%s/config\", opts.git_dir);\n>  \t\trepo_config.file = buf.buf;\n>  \t\tgit_config_with_options(cb, data, &repo_config, &opts);\n>  \t}\n\n...we have to re-add the git_dir.\n\nMight it be simpler to just xstrdup() to opts.git_dir, and then leave\nthis later code alone?\n\n\nThat said, I actually think in the long run that\ngit_config_with_options() should compute the repo_config itself from our\ngit_dir parameter. That lets us fix a very subtle bug in\nread_early_config(). The problem is that the function works like this:\n\n  1. Run git_config_with_options(), hitting the usual sources in order\n\n  2. If we didn't have a git-dir, run it again just for our discovered\n     repo-config\n\nThat has two implications:\n\n  - the config callback will see keys from ~/.gitconfig twice, once for\n    each run. This is usually OK because the only early config we care\n    about uses last-one-wins semantics (so no list-appending).\n\n  - the repo-level config is read last, so by last-one-wins it takes\n    precedence over ~/.gitconfig. Good. But it should have _less_\n    precedence than command-line config, and it doesn't.\n\nYou can see the second problem with:\n\n  # random external\n  cat >git-foo <<-\\EOF\n  #!/bin/sh\n  echo foo\n  EOF\n  chmod +x git-foo\n  PATH=$PWD:$PATH\n\n  git init\n  git config pager.foo 'sed s/^/repo:/'\n  git -c pager.foo='sed s/^/cmdline:/' foo\n\nThat command should prefer the cmdline config, but it doesn't.\n\nThe fix is something like what's below, which is easy on top of your new\noptions struct. I can wrap it up with a config message and test on top\nof your series.\n\ndiff --git a/config.c b/config.c\nindex f323b9628..5dda6e8ca 100644\n--- a/config.c\n+++ b/config.c\n@@ -1502,12 +1502,20 @@ int git_config_system(void)\n \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\n }\n \n-static int do_git_config_sequence(config_fn_t fn, void *data)\n+static int do_git_config_sequence(config_fn_t fn, void *data,\n+\t\t\t\t  const char *override_repo_dir)\n {\n \tint ret = 0;\n \tchar *xdg_config = xdg_config_home(\"config\");\n \tchar *user_config = expand_user_path(\"~/.gitconfig\");\n-\tchar *repo_config = have_git_dir() ? git_pathdup(\"config\") : NULL;\n+\tchar *repo_config;\n+\n+\tif (override_repo_dir)\n+\t\trepo_config = mkpathdup(\"%s/config\", override_repo_dir);\n+\telse if (have_git_dir())\n+\t\trepo_config = git_pathdup(\"config\");\n+\telse\n+\t\trepo_config = NULL;\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_SYSTEM;\n \tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))\n@@ -1561,7 +1569,7 @@ int git_config_with_options(config_fn_t fn, void *data,\n \telse if (config_source && config_source->blob)\n \t\treturn git_config_from_blob_ref(fn, config_source->blob, data);\n \n-\treturn do_git_config_sequence(fn, data);\n+\treturn do_git_config_sequence(fn, data, opts->git_dir);\n }\n \n static void git_config_raw(config_fn_t fn, void *data)\n@@ -1631,14 +1639,6 @@ void read_early_config(config_fn_t cb, void *data)\n \n \tgit_config_with_options(cb, data, NULL, &opts);\n \n-\tif (opts.git_dir) {\n-\t\tstruct git_config_source repo_config;\n-\n-\t\tmemset(&repo_config, 0, sizeof(repo_config));\n-\t\tstrbuf_addf(&buf, \"%s/config\", opts.git_dir);\n-\t\trepo_config.file = buf.buf;\n-\t\tgit_config_with_options(cb, data, &repo_config, &opts);\n-\t}\n \tstrbuf_release(&buf);\n \tfree(to_free);\n }\n\n"},{"id":"316978","messageId":"20170417014255.GA8987@duynguyen.vn.dektech.internal","threadId":"45690","inReplyTo":"20170416153127.f227kdbfctgrumh4@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] config: prepare to pass more info in git_config_with_options()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-17T01:42:55Z","receivedAt":"2017-04-17T01:43:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Apr 16, 2017 at 11:31:28AM -0400, Jeff King wrote:\n> On Sun, Apr 16, 2017 at 05:41:24PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> \n> > So far we can only pass one flag, respect_includes, to thie function. We\n> > need to pass some more (non-flag even), so let's make it accept a struct\n> > instead of an integer.\n> \n> Yeah, this makes sense. But...\n> \n> > diff --git a/builtin/config.c b/builtin/config.c\n> > index 4f49a0edb9..5de4a36146 100644\n> > --- a/builtin/config.c\n> > +++ b/builtin/config.c\n> > @@ -26,7 +26,8 @@ static int use_global_config, use_system_config, use_local_config;\n> >  static struct git_config_source given_config_source;\n> >  static int actions, types;\n> >  static int end_null;\n> > -static int respect_includes = -1;\n> > +static int respect_includes_opt;\n> > +static struct config_options config_options;\n> \n> It fails all the git-config \"include\" tests because respect_includes_opt\n> is missing the initialization to \"-1\".\n\nServes me right for doing last-minute \"harmless\" refactoring without\nrerunning the test suite :(\n\n> \n> -Peff\n"},{"id":"316980","messageId":"20170417021335.GB8987@duynguyen.vn.dektech.internal","threadId":"45690","inReplyTo":"20170416155131.ppp5iakohiiphzmk@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-17T02:13:35Z","receivedAt":"2017-04-17T02:13:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Apr 16, 2017 at 11:51:32AM -0400, Jeff King wrote:\n> > diff --git a/cache.h b/cache.h\n> > index e29a093839..27b7286f99 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -1884,6 +1884,8 @@ enum config_origin_type {\n> >  \n> >  struct config_options {\n> >  \tunsigned int respect_includes : 1;\n> > +\tunsigned int early_config : 1;\n> > +\tconst char *git_dir; /* only valid when early_config is true */\n> >  };\n> \n> Why do we need both the flag and the string? Later, you do:\n> \n> > -static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n> > +static int include_by_gitdir(const struct config_options *opts,\n> > +\t\t\t     const char *cond, size_t cond_len, int icase)\n> >  {\n> >  \tstruct strbuf text = STRBUF_INIT;\n> >  \tstruct strbuf pattern = STRBUF_INIT;\n> >  \tint ret = 0, prefix;\n> >  \n> > -\tstrbuf_add_absolute_path(&text, get_git_dir());\n> > +\tif (!opts->early_config)\n> > +\t\tstrbuf_add_absolute_path(&text, get_git_dir());\n> > +\telse if (opts->git_dir)\n> > +\t\tstrbuf_add_absolute_path(&text, opts->git_dir);\n> > +\telse\n> > +\t\tgoto done;\n> \n> So we call get_git_dir() always when we're not in early config. Even if\n> we don't have a git dir! Doesn't this mean that programs operating\n> outside of a repo will still hit the BUG? I.e.:\n> \n>   git config --global includeif.gitdir:/whatever.path foo\n>   cd /not/a/git/dir\n>   git diff --no-index foo bar\n> \n> ?\n> \n> I think instead the logic should be:\n> \n>   if (opts->git_dir)\n> \tstrbuf_add_absolute_path(&text, opts->git_dir);\n>   else if (have_git_dir())\n> \tstrbuf_add_absolute_path(&text, get_git_dir());\n>   else\n> \tgoto done;\n\nDo we care about the case when the override instruction is \"we don't\nhave $GIT_DIR, act as if it does not exist, even though have_git_dir()\nreturns true\"?\n\nI'm guessing no, we won't run into that situation (and am inclined to\nrestructure the code as you suggested). Just throwing it out there if\nI'm mistaken.\n--\nDuy\n"},{"id":"316998","messageId":"CACsJy8D-=3qPbEO-dq2+Mr-WZ+5RrivkSw9NX7_FhEJFy+59zA@mail.gmail.com","threadId":"45690","inReplyTo":"20170416155131.ppp5iakohiiphzmk@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-17T10:07:52Z","receivedAt":"2017-04-17T10:08:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"(To Junio, this series conflicts slightly with\nnd/conditional-config-include, let me know if you want me to rebase\nthis on top of that)\n\nOn Sun, Apr 16, 2017 at 10:51 PM, Jeff King <peff@peff.net> wrote:\n>> +     if (opts.git_dir) {\n>>               struct git_config_source repo_config;\n>>\n>>               memset(&repo_config, 0, sizeof(repo_config));\n>> -             strbuf_addstr(&buf, \"/config\");\n>> +             strbuf_addf(&buf, \"%s/config\", opts.git_dir);\n>>               repo_config.file = buf.buf;\n>>               git_config_with_options(cb, data, &repo_config, &opts);\n>>       }\n>\n> ...we have to re-add the git_dir.\n>\n> Might it be simpler to just xstrdup() to opts.git_dir, and then leave\n> this later code alone?\n\nSure thing. But we need to restore the \"if\" expression too. Otherwise,\nif have_git_dir() is true, we may come here with an empty \"buf\" before\nthat strbuf_addstr(&buf, \"/config\") is called. It does not matter much\nanyway because this block will be removed.\n\n> You can see the second problem with:\n>\n>   # random external\n>   cat >git-foo <<-\\EOF\n>   #!/bin/sh\n>   echo foo\n>   EOF\n>   chmod +x git-foo\n>   PATH=$PWD:$PATH\n>\n>   git init\n>   git config pager.foo 'sed s/^/repo:/'\n>   git -c pager.foo='sed s/^/cmdline:/' foo\n>\n> That command should prefer the cmdline config, but it doesn't.\n\nI actually had problems seeing the problem, for some reason it didn't\nwork for me. I guess I made a mistake somewhere.\n\n> The fix is something like what's below, which is easy on top of your new\n> options struct. I can wrap it up with a config message and test on top\n> of your series.\n\n... anyway I read this last sentence too late and spent too much time\nwrapping your changes in a patch (well most of the time was spent on\nwriting a new test actually), so I'm  sending it out too. My test uses\ntest-config though (I have given up on dealing with pager and tty).\n\nOff topic. Back to the pager.foo thing (because I had to fight it a\nbit before giving up). I find it a bit unintuitive that \"--paginate\"\nforces the pager back to less (or $PAGER) when I say \"pager.foo =\nmy-favorite-pager\". Back when pager.foo is a boolean thing, it makes\nsense for --paginate to override the \"to page or not to page\"\ndecision. But then you added a new meaning too pager.foo (what command\nto run). \"--paginate\" should respect the command pager.foo specifies\nwhen its value is a command, I think.\n-- \nDuy\n"},{"id":"316999","messageId":"20170417101002.18272-1-pclouds@gmail.com","threadId":"45690","inReplyTo":"CACsJy8D-=3qPbEO-dq2+Mr-WZ+5RrivkSw9NX7_FhEJFy+59zA@mail.gmail.com","subject":"[PATCH v2 1/3] config: prepare to pass more info in git_config_with_options()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-17T10:10:00Z","receivedAt":"2017-04-17T10:10:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"So far we can only pass one flag, respect_includes, to thie function. We\nneed to pass some more (non-flag even), so let's make it accept a struct\ninstead of an integer.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/config.c | 21 ++++++++++++---------\n cache.h          |  7 ++++++-\n config.c         | 16 +++++++++++-----\n 3 files changed, 29 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 4f49a0edb9..b937d175a9 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -26,7 +26,8 @@ static int use_global_config, use_system_config, use_local_config;\n static struct git_config_source given_config_source;\n static int actions, types;\n static int end_null;\n-static int respect_includes = -1;\n+static int respect_includes_opt = -1;\n+static struct config_options config_options;\n static int show_origin;\n \n #define ACTION_GET (1<<0)\n@@ -81,7 +82,7 @@ static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Other\")),\n \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n-\tOPT_BOOL(0, \"includes\", &respect_includes, N_(\"respect include directives on lookup\")),\n+\tOPT_BOOL(0, \"includes\", &respect_includes_opt, N_(\"respect include directives on lookup\")),\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n \tOPT_END(),\n };\n@@ -242,7 +243,7 @@ static int get_value(const char *key_, const char *regex_)\n \t}\n \n \tgit_config_with_options(collect_config, &values,\n-\t\t\t\t&given_config_source, respect_includes);\n+\t\t\t\t&given_config_source, &config_options);\n \n \tret = !values.nr;\n \n@@ -320,7 +321,7 @@ static void get_color(const char *var, const char *def_color)\n \tget_color_found = 0;\n \tparsed_color[0] = '\\0';\n \tgit_config_with_options(git_get_color_config, NULL,\n-\t\t\t\t&given_config_source, respect_includes);\n+\t\t\t\t&given_config_source, &config_options);\n \n \tif (!get_color_found && def_color) {\n \t\tif (color_parse(def_color, parsed_color) < 0)\n@@ -352,7 +353,7 @@ static int get_colorbool(const char *var, int print)\n \tget_diff_color_found = -1;\n \tget_color_ui_found = -1;\n \tgit_config_with_options(git_get_colorbool_config, NULL,\n-\t\t\t\t&given_config_source, respect_includes);\n+\t\t\t\t&given_config_source, &config_options);\n \n \tif (get_colorbool_found < 0) {\n \t\tif (!strcmp(get_colorbool_slot, \"color.diff\"))\n@@ -441,7 +442,7 @@ static int get_urlmatch(const char *var, const char *url)\n \t}\n \n \tgit_config_with_options(urlmatch_config_entry, &config,\n-\t\t\t\t&given_config_source, respect_includes);\n+\t\t\t\t&given_config_source, &config_options);\n \n \tret = !values.nr;\n \n@@ -530,8 +531,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t\tprefix_filename(prefix, given_config_source.file);\n \t}\n \n-\tif (respect_includes == -1)\n-\t\trespect_includes = !given_config_source.file;\n+\tif (respect_includes_opt == -1)\n+\t\tconfig_options.respect_includes = !given_config_source.file;\n+\telse\n+\t\tconfig_options.respect_includes = respect_includes_opt;\n \n \tif (end_null) {\n \t\tterm = '\\0';\n@@ -578,7 +581,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (git_config_with_options(show_all_config, NULL,\n \t\t\t\t\t    &given_config_source,\n-\t\t\t\t\t    respect_includes) < 0) {\n+\t\t\t\t\t    &config_options) < 0) {\n \t\t\tif (given_config_source.file)\n \t\t\t\tdie_errno(\"unable to read config file '%s'\",\n \t\t\t\t\t  given_config_source.file);\ndiff --git a/cache.h b/cache.h\nindex 556468c25b..a6294d2573 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1885,6 +1885,10 @@ enum config_origin_type {\n \tCONFIG_ORIGIN_CMDLINE\n };\n \n+struct config_options {\n+\tunsigned int respect_includes : 1;\n+};\n+\n typedef int (*config_fn_t)(const char *, const char *, void *);\n extern int git_default_config(const char *, const char *, void *);\n extern int git_config_from_file(config_fn_t fn, const char *, void *);\n@@ -1898,7 +1902,7 @@ extern void read_early_config(config_fn_t cb, void *data);\n extern void git_config(config_fn_t fn, void *);\n extern int git_config_with_options(config_fn_t fn, void *,\n \t\t\t\t   struct git_config_source *config_source,\n-\t\t\t\t   int respect_includes);\n+\t\t\t\t   const struct config_options *opts);\n extern int git_parse_ulong(const char *, unsigned long *);\n extern int git_parse_maybe_bool(const char *);\n extern int git_config_int(const char *, const char *);\n@@ -1950,6 +1954,7 @@ struct config_include_data {\n \tint depth;\n \tconfig_fn_t fn;\n \tvoid *data;\n+\tconst struct config_options *opts;\n };\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\ndiff --git a/config.c b/config.c\nindex 1a4d85537b..042321a3a0 100644\n--- a/config.c\n+++ b/config.c\n@@ -1530,13 +1530,14 @@ static int do_git_config_sequence(config_fn_t fn, void *data)\n \n int git_config_with_options(config_fn_t fn, void *data,\n \t\t\t    struct git_config_source *config_source,\n-\t\t\t    int respect_includes)\n+\t\t\t    const struct config_options *opts)\n {\n \tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n \n-\tif (respect_includes) {\n+\tif (opts->respect_includes) {\n \t\tinc.fn = fn;\n \t\tinc.data = data;\n+\t\tinc.opts = opts;\n \t\tfn = git_config_include;\n \t\tdata = &inc;\n \t}\n@@ -1557,7 +1558,10 @@ int git_config_with_options(config_fn_t fn, void *data,\n \n static void git_config_raw(config_fn_t fn, void *data)\n {\n-\tif (git_config_with_options(fn, data, NULL, 1) < 0)\n+\tstruct config_options opts = {0};\n+\n+\topts.respect_includes = 1;\n+\tif (git_config_with_options(fn, data, NULL, &opts) < 0)\n \t\t/*\n \t\t * git_config_with_options() normally returns only\n \t\t * zero, as most errors are fatal, and\n@@ -1597,9 +1601,11 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n \n void read_early_config(config_fn_t cb, void *data)\n {\n+\tstruct config_options opts = {0};\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tgit_config_with_options(cb, data, NULL, 1);\n+\topts.respect_includes = 1;\n+\tgit_config_with_options(cb, data, NULL, &opts);\n \n \t/*\n \t * When setup_git_directory() was not yet asked to discover the\n@@ -1615,7 +1621,7 @@ void read_early_config(config_fn_t cb, void *data)\n \t\tmemset(&repo_config, 0, sizeof(repo_config));\n \t\tstrbuf_addstr(&buf, \"/config\");\n \t\trepo_config.file = buf.buf;\n-\t\tgit_config_with_options(cb, data, &repo_config, 1);\n+\t\tgit_config_with_options(cb, data, &repo_config, &opts);\n \t}\n \tstrbuf_release(&buf);\n }\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317000","messageId":"20170417101002.18272-2-pclouds@gmail.com","threadId":"45690","inReplyTo":"20170417101002.18272-1-pclouds@gmail.com","subject":"[PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-17T10:10:01Z","receivedAt":"2017-04-17T10:10:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If setup_git_directory() and friends have not been called,\nget_git_dir() (because of includeIf.gitdir:XXX) would lead to\n\n    die(\"BUG: setup_git_env called without repository\");\n\nThere are two cases when a config file could be read before $GIT_DIR\nis located.\n\nThe first one is check_repository_format(), where we read just the one\nfile $GIT_DIR/config to check if we could understand this\nrepository. This case should be safe. We do not parse include\ndirectives, which can only be triggered from git_config_with_options,\nbut setup code uses a lower-level function. The concerned variables\nshould never be hidden away behind includes anyway.\n\nThe second one is triggered in check_pager_config() when we're about\nto run an external git command. We might be able to find $GIT_DIR in\nthis case, which is exactly what read_early_config() does (and also is\nwhat check_pager_config() uses). Conditional includes and\nget_git_dir() could be triggered by the first\ngit_config_with_options() call there, before discover_git_directory()\nis used as a fallback $GIT_DIR detection.\n\nDetect this special \"early reading\" case, pass down the $GIT_DIR,\neither from previous setup or detected by discover_git_directory(),\nand make conditional include use it.\n\nNoticed-by: Bert Wesarg <bert.wesarg@googlemail.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n cache.h                   |  1 +\n config.c                  | 34 ++++++++++++++++++++++++++--------\n t/t1305-config-include.sh | 11 +++++++++++\n 3 files changed, 38 insertions(+), 8 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a6294d2573..878e1d441f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1887,6 +1887,7 @@ enum config_origin_type {\n \n struct config_options {\n \tunsigned int respect_includes : 1;\n+\tconst char *git_dir;\n };\n \n typedef int (*config_fn_t)(const char *, const char *, void *);\ndiff --git a/config.c b/config.c\nindex 042321a3a0..14f0417460 100644\n--- a/config.c\n+++ b/config.c\n@@ -207,13 +207,22 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n \treturn prefix;\n }\n \n-static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n+static int include_by_gitdir(const struct config_options *opts,\n+\t\t\t     const char *cond, size_t cond_len, int icase)\n {\n \tstruct strbuf text = STRBUF_INIT;\n \tstruct strbuf pattern = STRBUF_INIT;\n \tint ret = 0, prefix;\n+\tconst char *git_dir;\n \n-\tstrbuf_add_absolute_path(&text, get_git_dir());\n+\tif (opts->git_dir)\n+\t\tgit_dir = opts->git_dir;\n+\telse if (have_git_dir())\n+\t\tgit_dir = get_git_dir();\n+\telse\n+\t\tgoto done;\n+\n+\tstrbuf_add_absolute_path(&text, git_dir);\n \tstrbuf_add(&pattern, cond, cond_len);\n \tprefix = prepare_include_condition_pattern(&pattern);\n \n@@ -242,13 +251,14 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)\n \treturn ret;\n }\n \n-static int include_condition_is_true(const char *cond, size_t cond_len)\n+static int include_condition_is_true(const struct config_options *opts,\n+\t\t\t\t     const char *cond, size_t cond_len)\n {\n \n \tif (skip_prefix_mem(cond, cond_len, \"gitdir:\", &cond, &cond_len))\n-\t\treturn include_by_gitdir(cond, cond_len, 0);\n+\t\treturn include_by_gitdir(opts, cond, cond_len, 0);\n \telse if (skip_prefix_mem(cond, cond_len, \"gitdir/i:\", &cond, &cond_len))\n-\t\treturn include_by_gitdir(cond, cond_len, 1);\n+\t\treturn include_by_gitdir(opts, cond, cond_len, 1);\n \n \t/* unknown conditionals are always false */\n \treturn 0;\n@@ -273,7 +283,7 @@ int git_config_include(const char *var, const char *value, void *data)\n \t\tret = handle_path_include(value, inc);\n \n \tif (!parse_config_key(var, \"includeif\", &cond, &cond_len, &key) &&\n-\t    (cond && include_condition_is_true(cond, cond_len)) &&\n+\t    (cond && include_condition_is_true(inc->opts, cond, cond_len)) &&\n \t    !strcmp(key, \"path\"))\n \t\tret = handle_path_include(value, inc);\n \n@@ -1603,10 +1613,12 @@ void read_early_config(config_fn_t cb, void *data)\n {\n \tstruct config_options opts = {0};\n \tstruct strbuf buf = STRBUF_INIT;\n+\tchar *to_free = NULL;\n \n \topts.respect_includes = 1;\n-\tgit_config_with_options(cb, data, NULL, &opts);\n \n+\tif (have_git_dir())\n+\t\topts.git_dir = get_git_dir();\n \t/*\n \t * When setup_git_directory() was not yet asked to discover the\n \t * GIT_DIR, we ask discover_git_directory() to figure out whether there\n@@ -1615,7 +1627,12 @@ void read_early_config(config_fn_t cb, void *data)\n \t * notably, the current working directory is still the same after the\n \t * call).\n \t */\n-\tif (!have_git_dir() && discover_git_directory(&buf)) {\n+\telse if (discover_git_directory(&buf))\n+\t\topts.git_dir = to_free = xstrdup(buf.buf);\n+\n+\tgit_config_with_options(cb, data, NULL, &opts);\n+\n+\tif (!have_git_dir() && opts.git_dir) {\n \t\tstruct git_config_source repo_config;\n \n \t\tmemset(&repo_config, 0, sizeof(repo_config));\n@@ -1624,6 +1641,7 @@ void read_early_config(config_fn_t cb, void *data)\n \t\tgit_config_with_options(cb, data, &repo_config, &opts);\n \t}\n \tstrbuf_release(&buf);\n+\tfree(to_free);\n }\n \n static void git_config_check_init(void);\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex e833939320..fddb47bafa 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -208,6 +208,17 @@ test_expect_success 'conditional include, both unanchored, icase' '\n \t)\n '\n \n+test_expect_success 'conditional include, early config reading' '\n+\t(\n+\t\tcd foo &&\n+\t\techo \"[includeIf \\\"gitdir:foo/\\\"]path=bar6\" >>.git/config &&\n+\t\techo \"[test]six=6\" >.git/bar6 &&\n+\t\techo 6 >expect &&\n+\t\ttest-config read_early_config test.six >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'include cycles are detected' '\n \tcat >.gitconfig <<-\\EOF &&\n \t[test]value = gitconfig\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317001","messageId":"20170417101002.18272-3-pclouds@gmail.com","threadId":"45690","inReplyTo":"20170417101002.18272-1-pclouds@gmail.com","subject":"[PATCH v2 3/3] config: correct file reading order in read_early_config()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-17T10:10:02Z","receivedAt":"2017-04-17T10:10:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Config file reading order is important because each file can override\nvalues in the previous files and this is expected behavior. Normally\nwe read in this order, all in do_git_config_sequence():\n\n1. $HOME/.gitconfig\n2. $GIT_DIR/config\n3. config from command line\n\nHowever in read_early_config() the order may be swapped a bit if\nsetup_git_directory() has not been called:\n\n1. $HOME/.gitconfig\n2. $GIT_DIR/config is NOT read because .git dir is not found _yet_\n3. config from command line\n4. $GIT_DIR/config is now READ (after discover_git_directory() call)\n\nThe reading at step 4 could override config at step 3, which is not\nthe expectation.\n\nNow that we could pass the .git dir around, we could feed\ndiscover_git_directory() back to step 2, so that it works again, and\nremove step 4.\n\nNoticed-by: Jeff King <peff@peff.net>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n config.c                | 26 ++++++++++++--------------\n t/helper/test-config.c  |  4 ++++\n t/t1309-early-config.sh | 17 +++++++++++++++++\n 3 files changed, 33 insertions(+), 14 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 14f0417460..fffda653e3 100644\n--- a/config.c\n+++ b/config.c\n@@ -1504,12 +1504,20 @@ int git_config_system(void)\n \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\n }\n \n-static int do_git_config_sequence(config_fn_t fn, void *data)\n+static int do_git_config_sequence(const struct config_options *opts,\n+\t\t\t\t  config_fn_t fn, void *data)\n {\n \tint ret = 0;\n \tchar *xdg_config = xdg_config_home(\"config\");\n \tchar *user_config = expand_user_path(\"~/.gitconfig\");\n-\tchar *repo_config = have_git_dir() ? git_pathdup(\"config\") : NULL;\n+\tchar *repo_config;\n+\n+\tif (opts->git_dir)\n+\t\trepo_config = mkpathdup(\"%s/config\", opts->git_dir);\n+\telse if (have_git_dir())\n+\t\trepo_config = git_pathdup(\"config\");\n+\telse\n+\t\trepo_config = NULL;\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_SYSTEM;\n \tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))\n@@ -1563,7 +1571,7 @@ int git_config_with_options(config_fn_t fn, void *data,\n \telse if (config_source && config_source->blob)\n \t\treturn git_config_from_blob_ref(fn, config_source->blob, data);\n \n-\treturn do_git_config_sequence(fn, data);\n+\treturn do_git_config_sequence(opts, fn, data);\n }\n \n static void git_config_raw(config_fn_t fn, void *data)\n@@ -1613,7 +1621,6 @@ void read_early_config(config_fn_t cb, void *data)\n {\n \tstruct config_options opts = {0};\n \tstruct strbuf buf = STRBUF_INIT;\n-\tchar *to_free = NULL;\n \n \topts.respect_includes = 1;\n \n@@ -1628,20 +1635,11 @@ void read_early_config(config_fn_t cb, void *data)\n \t * call).\n \t */\n \telse if (discover_git_directory(&buf))\n-\t\topts.git_dir = to_free = xstrdup(buf.buf);\n+\t\topts.git_dir = buf.buf;\n \n \tgit_config_with_options(cb, data, NULL, &opts);\n \n-\tif (!have_git_dir() && opts.git_dir) {\n-\t\tstruct git_config_source repo_config;\n-\n-\t\tmemset(&repo_config, 0, sizeof(repo_config));\n-\t\tstrbuf_addstr(&buf, \"/config\");\n-\t\trepo_config.file = buf.buf;\n-\t\tgit_config_with_options(cb, data, &repo_config, &opts);\n-\t}\n \tstrbuf_release(&buf);\n-\tfree(to_free);\n }\n \n static void git_config_check_init(void);\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 8e3ed6a76c..696d0a52fd 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -84,6 +84,10 @@ int cmd_main(int argc, const char **argv)\n \tstruct config_set cs;\n \n \tif (argc == 3 && !strcmp(argv[1], \"read_early_config\")) {\n+\t\tconst char *cmdline_config = getenv(\"CMDL_CFG\");\n+\n+\t\tif (cmdline_config)\n+\t\t\tgit_config_push_parameter(cmdline_config);\n \t\tread_early_config(early_config_cb, (void *)argv[2]);\n \t\treturn 0;\n \t}\ndiff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh\nindex b97357b8ab..c15f18dd96 100755\n--- a/t/t1309-early-config.sh\n+++ b/t/t1309-early-config.sh\n@@ -47,6 +47,23 @@ test_expect_success 'ceiling #2' '\n \ttest xdg = \"$(cat output)\"\n '\n \n+test_expect_success 'read config file in right order' '\n+\techo \"[test]source = home\" >>.gitconfig &&\n+\tgit init foo &&\n+\t(\n+\t\tcd foo &&\n+\t\techo \"[test]source = repo\" >>.git/config &&\n+\t\tCMDL_CFG=test.source=cmdline test-config \\\n+\t\t\tread_early_config test.source >actual &&\n+\t\tcat >expected <<-\\EOF &&\n+\t\thome\n+\t\trepo\n+\t\tcmdline\n+\t\tEOF\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n test_with_config () {\n \trm -rf throwaway &&\n \tgit init throwaway &&\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317063","messageId":"xmqqd1cah2m3.fsf@gitster.mtv.corp.google.com","threadId":"45690","inReplyTo":"20170417101002.18272-1-pclouds@gmail.com","subject":"Re: [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-18T02:27:16Z","receivedAt":"2017-04-18T02:27:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> So far we can only pass one flag, respect_includes, to thie function. We\n\ns/thie/this/\n\n> need to pass some more (non-flag even), so let's make it accept a struct\n> instead of an integer.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  builtin/config.c | 21 ++++++++++++---------\n>  cache.h          |  7 ++++++-\n>  config.c         | 16 +++++++++++-----\n>  3 files changed, 29 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 4f49a0edb9..b937d175a9 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -26,7 +26,8 @@ static int use_global_config, use_system_config, use_local_config;\n>  static struct git_config_source given_config_source;\n>  static int actions, types;\n>  static int end_null;\n> -static int respect_includes = -1;\n> +static int respect_includes_opt = -1;\n> +static struct config_options config_options;\n>  static int show_origin;\n>  \n>  #define ACTION_GET (1<<0)\n> @@ -81,7 +82,7 @@ static struct option builtin_config_options[] = {\n>  \tOPT_GROUP(N_(\"Other\")),\n>  \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n>  \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n> -\tOPT_BOOL(0, \"includes\", &respect_includes, N_(\"respect include directives on lookup\")),\n> +\tOPT_BOOL(0, \"includes\", &respect_includes_opt, N_(\"respect include directives on lookup\")),\n\nIt would be more in line with what the log message advertised if you\ndid\n\n\tstatic struct config_options config_options = {\n\t\t-1, /* .respect_includes: unspecified */\n\t};\n\n\tOPT_BOOL(0, \"includes\", &config_options.respect_includes, N_(\"...\")),\n\nno?\n"},{"id":"317064","messageId":"xmqq7f2ih1l1.fsf@gitster.mtv.corp.google.com","threadId":"45690","inReplyTo":"20170417101002.18272-2-pclouds@gmail.com","subject":"Re: [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-18T02:49:30Z","receivedAt":"2017-04-18T02:49:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> @@ -1603,10 +1613,12 @@ void read_early_config(config_fn_t cb, void *data)\n>  {\n>  \tstruct config_options opts = {0};\n>  \tstruct strbuf buf = STRBUF_INIT;\n> +\tchar *to_free = NULL;\n>  \n>  \topts.respect_includes = 1;\n> -\tgit_config_with_options(cb, data, NULL, &opts);\n>  \n> +\tif (have_git_dir())\n> +\t\topts.git_dir = get_git_dir();\n>  \t/*\n>  \t * When setup_git_directory() was not yet asked to discover the\n>  \t * GIT_DIR, we ask discover_git_directory() to figure out whether there\n> @@ -1615,7 +1627,12 @@ void read_early_config(config_fn_t cb, void *data)\n>  \t * notably, the current working directory is still the same after the\n>  \t * call).\n>  \t */\n> -\tif (!have_git_dir() && discover_git_directory(&buf)) {\n> +\telse if (discover_git_directory(&buf))\n> +\t\topts.git_dir = to_free = xstrdup(buf.buf);\n> +\n> +\tgit_config_with_options(cb, data, NULL, &opts);\n\nThis one I can understand.  By having NULL for config_source, this\ndoes the usual do_git_config_sequence() dance, which knows to treat\nopts.git_dir is the \"repository config\" without necessarily being\nable to do git_pathdup(\"config\").\n\n> +\tif (!have_git_dir() && opts.git_dir) {\n>  \t\tstruct git_config_source repo_config;\n>  \n>  \t\tmemset(&repo_config, 0, sizeof(repo_config));\n\nBut this one I do not quite understand.  When have_git_dir() was\nfalse and asked discover_git_directory() to set opts.git_dir, we\nenter the body of this block and then end up doing\n\n\tgit_config_with_options(cb, data &repo_config, &opts);\n\nwith repo_config set to the discovered git directory plus \"/config\";\nwe'd read the repository configuration twice, in other words.\n\n"},{"id":"317065","messageId":"xmqqzifefmog.fsf@gitster.mtv.corp.google.com","threadId":"45690","inReplyTo":"xmqq7f2ih1l1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-18T02:56:47Z","receivedAt":"2017-04-18T02:56:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> +\tif (!have_git_dir() && opts.git_dir) {\n>>  \t\tstruct git_config_source repo_config;\n>>  \n>>  \t\tmemset(&repo_config, 0, sizeof(repo_config));\n>\n> But this one I do not quite understand.  When have_git_dir() was\n> false and asked discover_git_directory() to set opts.git_dir, we\n> enter the body of this block and then end up doing\n>\n> \tgit_config_with_options(cb, data &repo_config, &opts);\n>\n> with repo_config set to the discovered git directory plus \"/config\";\n> we'd read the repository configuration twice, in other words.\n\nAhh, nevermind.  The fix to make the \"usual sequence\" pay attention\nto the opts->git_dir comes in 3/3 and in that step this redundant\nreading is also removed, so all is well at the end.\n\nThanks.\n"},{"id":"317066","messageId":"xmqqvaq2flps.fsf@gitster.mtv.corp.google.com","threadId":"45690","inReplyTo":"CACsJy8D-=3qPbEO-dq2+Mr-WZ+5RrivkSw9NX7_FhEJFy+59zA@mail.gmail.com","subject":"Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-18T03:17:35Z","receivedAt":"2017-04-18T03:17:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> (To Junio, this series conflicts slightly with\n> nd/conditional-config-include, let me know if you want me to rebase\n> this on top of that)\n\nI think I can manage---having to resolve inter-topic conflict every\nonce in a while is a good sanity check for me anyway ;-).  Please\nholler if my resolution is faulty when I push things out tonight my\ntime (note: I'll be operating in GMT+9 for coming weeks).\n\nThanks.\n"},{"id":"317070","messageId":"20170418034610.oiex6x3d4pcyzbzy@sigill.intra.peff.net","threadId":"45690","inReplyTo":"xmqqzifefmog.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/3] config: handle conditional include when $GIT_DIR is not set up","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-18T03:46:10Z","receivedAt":"2017-04-18T03:46:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2017 at 07:56:47PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> >> +\tif (!have_git_dir() && opts.git_dir) {\n> >>  \t\tstruct git_config_source repo_config;\n> >>  \n> >>  \t\tmemset(&repo_config, 0, sizeof(repo_config));\n> >\n> > But this one I do not quite understand.  When have_git_dir() was\n> > false and asked discover_git_directory() to set opts.git_dir, we\n> > enter the body of this block and then end up doing\n> >\n> > \tgit_config_with_options(cb, data &repo_config, &opts);\n> >\n> > with repo_config set to the discovered git directory plus \"/config\";\n> > we'd read the repository configuration twice, in other words.\n> \n> Ahh, nevermind.  The fix to make the \"usual sequence\" pay attention\n> to the opts->git_dir comes in 3/3 and in that step this redundant\n> reading is also removed, so all is well at the end.\n\nI think 2/3 is actually OK as-is. We know we didn't read repo config in\nthe first call to git_config_with_options() because have_git_dir() is\nunset.\n\nSo 2/3 doesn't make anything worse; the bug already existed before Duy's\nseries (it goes back to my original read_early_config() hack).\n\n-Peff\n"},{"id":"317071","messageId":"20170418035344.feu27cwogvdbdwyj@sigill.intra.peff.net","threadId":"45690","inReplyTo":"20170417101002.18272-3-pclouds@gmail.com","subject":"Re: [PATCH v2 3/3] config: correct file reading order in read_early_config()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-18T03:53:44Z","receivedAt":"2017-04-18T03:53:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2017 at 05:10:02PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> diff --git a/t/helper/test-config.c b/t/helper/test-config.c\n> index 8e3ed6a76c..696d0a52fd 100644\n> --- a/t/helper/test-config.c\n> +++ b/t/helper/test-config.c\n> @@ -84,6 +84,10 @@ int cmd_main(int argc, const char **argv)\n>  \tstruct config_set cs;\n>  \n>  \tif (argc == 3 && !strcmp(argv[1], \"read_early_config\")) {\n> +\t\tconst char *cmdline_config = getenv(\"CMDL_CFG\");\n> +\n> +\t\tif (cmdline_config)\n> +\t\t\tgit_config_push_parameter(cmdline_config);\n\nI think you can do without this hunk by just setting:\n\n  GIT_CONFIG_PARAMETERS=\"'foo.bar=from-cmdline'\"\n\n(note the single-quotes which must be there). See how t1308 does it.\n\n> +test_expect_success 'read config file in right order' '\n> +\techo \"[test]source = home\" >>.gitconfig &&\n> +\tgit init foo &&\n> +\t(\n> +\t\tcd foo &&\n> +\t\techo \"[test]source = repo\" >>.git/config &&\n> +\t\tCMDL_CFG=test.source=cmdline test-config \\\n> +\t\t\tread_early_config test.source >actual &&\n> +\t\tcat >expected <<-\\EOF &&\n> +\t\thome\n> +\t\trepo\n> +\t\tcmdline\n> +\t\tEOF\n> +\t\ttest_cmp expected actual\n> +\t)\n> +'\n\nThis looks good (modulo the CMDL_CFG above).\n\nIf we wanted to trigger it in a real-world test, we'd have to use pager\nconfig (since it's the only thing that uses early-config; alias lookup\nprobably should do, but that's for another time). But I think this\nsynthetic test is fine; it makes the output easy to verify.\n\n-Peff\n"},{"id":"317072","messageId":"20170418035509.6zd4kxtfggei5yaj@sigill.intra.peff.net","threadId":"45690","inReplyTo":"xmqqd1cah2m3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-18T03:55:10Z","receivedAt":"2017-04-18T03:55:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2017 at 07:27:16PM -0700, Junio C Hamano wrote:\n\n> > @@ -81,7 +82,7 @@ static struct option builtin_config_options[] = {\n> >  \tOPT_GROUP(N_(\"Other\")),\n> >  \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n> >  \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n> > -\tOPT_BOOL(0, \"includes\", &respect_includes, N_(\"respect include directives on lookup\")),\n> > +\tOPT_BOOL(0, \"includes\", &respect_includes_opt, N_(\"respect include directives on lookup\")),\n> \n> It would be more in line with what the log message advertised if you\n> did\n> \n> \tstatic struct config_options config_options = {\n> \t\t-1, /* .respect_includes: unspecified */\n> \t};\n> \n> \tOPT_BOOL(0, \"includes\", &config_options.respect_includes, N_(\"...\")),\n> \n> no?\n\nI think I like the split between the option-value here and the \"final\"\nvalue that goes into config_options.respect_includes. Because we\nactually munge it later based on the given-config value anyway.\n\nSo I agree this makes the diff larger than it might need to be, but I\nthink the end result is a bit nicer.\n\n-Peff\n"},{"id":"317073","messageId":"20170418035632.y7rg3bu5oo6hqavk@sigill.intra.peff.net","threadId":"45690","inReplyTo":"20170417021335.GB8987@duynguyen.vn.dektech.internal","subject":"Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-18T03:56:32Z","receivedAt":"2017-04-18T03:56:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2017 at 09:13:35AM +0700, Duy Nguyen wrote:\n\n> > So we call get_git_dir() always when we're not in early config. Even if\n> > we don't have a git dir! Doesn't this mean that programs operating\n> > outside of a repo will still hit the BUG? I.e.:\n> > \n> >   git config --global includeif.gitdir:/whatever.path foo\n> >   cd /not/a/git/dir\n> >   git diff --no-index foo bar\n> > \n> > ?\n> > \n> > I think instead the logic should be:\n> > \n> >   if (opts->git_dir)\n> > \tstrbuf_add_absolute_path(&text, opts->git_dir);\n> >   else if (have_git_dir())\n> > \tstrbuf_add_absolute_path(&text, get_git_dir());\n> >   else\n> > \tgoto done;\n> \n> Do we care about the case when the override instruction is \"we don't\n> have $GIT_DIR, act as if it does not exist, even though have_git_dir()\n> returns true\"?\n> \n> I'm guessing no, we won't run into that situation (and am inclined to\n> restructure the code as you suggested). Just throwing it out there if\n> I'm mistaken.\n\nI can't think of a case where that would matter. We're always prodding\nthe function to look at something it might not, and I don't think there\nwould be any need to convince it _not_ to look at something.\n\n-Peff\n"},{"id":"317074","messageId":"20170418040305.t4soqhpllhd4f7u7@sigill.intra.peff.net","threadId":"45690","inReplyTo":"CACsJy8D-=3qPbEO-dq2+Mr-WZ+5RrivkSw9NX7_FhEJFy+59zA@mail.gmail.com","subject":"Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-18T04:03:05Z","receivedAt":"2017-04-18T04:03:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2017 at 05:07:52PM +0700, Duy Nguyen wrote:\n\n> > You can see the second problem with:\n> >\n> >   # random external\n> >   cat >git-foo <<-\\EOF\n> >   #!/bin/sh\n> >   echo foo\n> >   EOF\n> >   chmod +x git-foo\n> >   PATH=$PWD:$PATH\n> >\n> >   git init\n> >   git config pager.foo 'sed s/^/repo:/'\n> >   git -c pager.foo='sed s/^/cmdline:/' foo\n> >\n> > That command should prefer the cmdline config, but it doesn't.\n> \n> I actually had problems seeing the problem, for some reason it didn't\n> work for me. I guess I made a mistake somewhere.\n\nI just double-checked it, and the above works for me. <shrug>\n\n> > The fix is something like what's below, which is easy on top of your new\n> > options struct. I can wrap it up with a config message and test on top\n> > of your series.\n> \n> ... anyway I read this last sentence too late and spent too much time\n> wrapping your changes in a patch (well most of the time was spent on\n> writing a new test actually), so I'm  sending it out too. My test uses\n> test-config though (I have given up on dealing with pager and tty).\n\nThanks for doing that. I had planned to do it myself to avoid making\nwork for you, but what you posted looks good.\n\nI hadn't thought about tty issues. You'd have to run it under\ntest_terminal (which isn't even available everywhere). So your\ntest-config version is probably the best bet.\n\n> Off topic. Back to the pager.foo thing (because I had to fight it a\n> bit before giving up). I find it a bit unintuitive that \"--paginate\"\n> forces the pager back to less (or $PAGER) when I say \"pager.foo =\n> my-favorite-pager\". Back when pager.foo is a boolean thing, it makes\n> sense for --paginate to override the \"to page or not to page\"\n> decision. But then you added a new meaning too pager.foo (what command\n> to run). \"--paginate\" should respect the command pager.foo specifies\n> when its value is a command, I think.\n\nI never noticed that before, and I agree that it's nonsense. Using \"-p\"\nshould be unnecessary if you have pager.foo defined (because it already\nimplies \"yes, use the pager\"). But if you were to do it anyway, I agree\nit should not ignore the configured pager. One might use an extra \"-p\"\nhoping that it would override the tty check, but it doesn't. It's too\nlate to change that now, I think, but I have been tempted to add a\n--paginate-me-harder option. It would be handy for tests.\n\n-Peff\n"},{"id":"317081","messageId":"xmqqr30qe2sh.fsf@gitster.mtv.corp.google.com","threadId":"45690","inReplyTo":"20170418035509.6zd4kxtfggei5yaj@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/3] config: prepare to pass more info in git_config_with_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-18T04:51:42Z","receivedAt":"2017-04-18T04:51:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Apr 17, 2017 at 07:27:16PM -0700, Junio C Hamano wrote:\n>\n>> > @@ -81,7 +82,7 @@ static struct option builtin_config_options[] = {\n>> >  \tOPT_GROUP(N_(\"Other\")),\n>> >  \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n>> >  \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n>> > -\tOPT_BOOL(0, \"includes\", &respect_includes, N_(\"respect include directives on lookup\")),\n>> > +\tOPT_BOOL(0, \"includes\", &respect_includes_opt, N_(\"respect include directives on lookup\")),\n>> \n>> It would be more in line with what the log message advertised if you\n>> did\n>> \n>> \tstatic struct config_options config_options = {\n>> \t\t-1, /* .respect_includes: unspecified */\n>> \t};\n>> \n>> \tOPT_BOOL(0, \"includes\", &config_options.respect_includes, N_(\"...\")),\n>> \n>> no?\n>\n> I think I like the split between the option-value here and the \"final\"\n> value that goes into config_options.respect_includes. Because we\n> actually munge it later based on the given-config value anyway.\n>\n> So I agree this makes the diff larger than it might need to be, but I\n> think the end result is a bit nicer.\n\nYeah, I didn't see the end result was a single bit (unsigned :1).\nThis separation is OK.\n"}]}