{"thread":{"id":"45508","subject":"t1503 broken ?","startedAt":"2017-03-25T10:46:34Z","lastAt":"2017-04-05T10:26:48Z","messageCount":11,"participants":["Torsten Bögershausen","Duy Nguyen","Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"315379","messageId":"fad6ea92-0a44-de30-48e8-09e7c567cd4d@web.de","threadId":"45508","inReplyTo":null,"subject":"t1503 broken ?","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-03-25T10:46:23Z","receivedAt":"2017-03-25T10:46:34Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"./t1305-config-include.sh\nseems to be broken:\nnot ok 19 - conditional include, $HOME expansion\nnot ok 21 - conditional include, relative path\n\nBoth Mac and Linux.\n\nThe problem seems to be the line\n\ngit config test.two >actual &&\nand\ngit config test.four >actual &&\n\nIn both cases the config \"test.two or test.four\" is not found,\nat least that is what my debugging shows.\nAfter adding an\nexit 0\nbefore the test_done gives this:\n\n find trash\\ directory.t1305-config-include/ -name \"bar*\"\ntrash directory.t1305-config-include/foo/.git/bar\ntrash directory.t1305-config-include/foo/.git/bar3\ntrash directory.t1305-config-include/foo/.git/bar5\ntrash directory.t1305-config-include/foo/.git/bar2\ntrash directory.t1305-config-include/bar4\n\nWhere should bar2 and bar4 be ?\n\n"},{"id":"315383","messageId":"CACsJy8B1DWL9uHPZoQEO+ytkt1M+qV-x0F5CWFLFSG1utxO9EQ@mail.gmail.com","threadId":"45508","inReplyTo":"fad6ea92-0a44-de30-48e8-09e7c567cd4d@web.de","subject":"Re: t1503 broken ?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-25T11:46:52Z","receivedAt":"2017-03-25T11:55:04Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n> ./t1305-config-include.sh\n> seems to be broken:\n> not ok 19 - conditional include, $HOME expansion\n> not ok 21 - conditional include, relative path\n\nlet me guess, your \"git\" directory is in a symlink path?\n-- \nDuy\n"},{"id":"315389","messageId":"CACsJy8CZdZuxpwbfB+TgHdH9YneeUxUhkF7XuLhWw6Lp9ZLPDQ@mail.gmail.com","threadId":"45508","inReplyTo":"CACsJy8B1DWL9uHPZoQEO+ytkt1M+qV-x0F5CWFLFSG1utxO9EQ@mail.gmail.com","subject":"Re: t1503 broken ?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-25T12:26:14Z","receivedAt":"2017-03-25T12:26:49Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 25, 2017 at 6:46 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n>> ./t1305-config-include.sh\n>> seems to be broken:\n>> not ok 19 - conditional include, $HOME expansion\n>> not ok 21 - conditional include, relative path\n>\n> let me guess, your \"git\" directory is in a symlink path?\n\nYes I could reproduce it when I put my \"git\" in a symlink. There's a\nnote in document about \"Symlinks in `$GIT_DIR` are not resolved before\nmatching\" but failing tests is not acceptable. I'll fix it.\n-- \nDuy\n"},{"id":"315391","messageId":"20170325130549.GA20618@ash","threadId":"45508","inReplyTo":"CACsJy8CZdZuxpwbfB+TgHdH9YneeUxUhkF7XuLhWw6Lp9ZLPDQ@mail.gmail.com","subject":"Re: t1503 broken ?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-25T13:05:50Z","receivedAt":"2017-03-25T13:06:11Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 25, 2017 at 07:26:14PM +0700, Duy Nguyen wrote:\n> On Sat, Mar 25, 2017 at 6:46 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> > On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n> >> ./t1305-config-include.sh\n> >> seems to be broken:\n> >> not ok 19 - conditional include, $HOME expansion\n> >> not ok 21 - conditional include, relative path\n> >\n> > let me guess, your \"git\" directory is in a symlink path?\n> \n> Yes I could reproduce it when I put my \"git\" in a symlink. There's a\n> note in document about \"Symlinks in `$GIT_DIR` are not resolved before\n> matching\" but failing tests is not acceptable. I'll fix it.\n\nThe fix may be something like this. The problem is $GIT_DIR has symlinks\nresolved, but we don't do the same for other paths in this code. As a\nresult, matching paths fails.\n\nI'm a bit concerned about the change in expand_user_path() because I'm\nnot quite sure if it's a completely safe change. But at least could\nyou try the patch and see if it passe the tests on your machine too?\n\n-- 8< --\ndiff --git a/config.c b/config.c\nindex 1a4d855..fc4eae9 100644\n--- a/config.c\n+++ b/config.c\n@@ -191,7 +191,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n \t\t\treturn error(_(\"relative config include \"\n \t\t\t\t       \"conditionals must come from files\"));\n \n-\t\tstrbuf_add_absolute_path(&path, cf->path);\n+\t\tstrbuf_realpath(&path, cf->path, 1);\n \t\tslash = find_last_dir_sep(path.buf);\n \t\tif (!slash)\n \t\t\tdie(\"BUG: how is this possible?\");\n@@ -213,7 +213,7 @@ 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-\tstrbuf_add_absolute_path(&text, get_git_dir());\n+\tstrbuf_realpath(&text, get_git_dir(), 1);\n \tstrbuf_add(&pattern, cond, cond_len);\n \tprefix = prepare_include_condition_pattern(&pattern);\n \ndiff --git a/path.c b/path.c\nindex 2224843..18eaac3 100644\n--- a/path.c\n+++ b/path.c\n@@ -654,7 +654,7 @@ char *expand_user_path(const char *path)\n \t\t\tconst char *home = getenv(\"HOME\");\n \t\t\tif (!home)\n \t\t\t\tgoto return_null;\n-\t\t\tstrbuf_addstr(&user_path, home);\n+\t\t\tstrbuf_addstr(&user_path, real_path(home));\n #ifdef GIT_WINDOWS_NATIVE\n \t\t\tconvert_slashes(user_path.buf);\n #endif\n-- 8< --\n-- \nDuy\n"},{"id":"315404","messageId":"d2c587c4-18ca-a685-a53f-02106b1527ee@web.de","threadId":"45508","inReplyTo":"20170325130549.GA20618@ash","subject":"Re: t1503 broken ?","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-03-25T19:41:41Z","receivedAt":"2017-03-25T19:42:01Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n\nOn 03/25/2017 02:05 PM, Duy Nguyen wrote:\n> On Sat, Mar 25, 2017 at 07:26:14PM +0700, Duy Nguyen wrote:\n>> On Sat, Mar 25, 2017 at 6:46 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n>>> On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n>>>> ./t1305-config-include.sh\n>>>> seems to be broken:\n>>>> not ok 19 - conditional include, $HOME expansion\n>>>> not ok 21 - conditional include, relative path\n>>> let me guess, your \"git\" directory is in a symlink path?\n>> Yes I could reproduce it when I put my \"git\" in a symlink. There's a\n>> note in document about \"Symlinks in `$GIT_DIR` are not resolved before\n>> matching\" but failing tests is not acceptable. I'll fix it.\n> The fix may be something like this. The problem is $GIT_DIR has symlinks\n> resolved, but we don't do the same for other paths in this code. As a\n> result, matching paths fails.\n>\n> I'm a bit concerned about the change in expand_user_path() because I'm\n> not quite sure if it's a completely safe change. But at least could\n> you try the patch and see if it passe the tests on your machine too?\n>\n> -- 8< --\n> diff --git a/config.c b/config.c\n> index 1a4d855..fc4eae9 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -191,7 +191,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n>   \t\t\treturn error(_(\"relative config include \"\n>   \t\t\t\t       \"conditionals must come from files\"));\n>   \n> -\t\tstrbuf_add_absolute_path(&path, cf->path);\n> +\t\tstrbuf_realpath(&path, cf->path, 1);\n>   \t\tslash = find_last_dir_sep(path.buf);\n>   \t\tif (!slash)\n>   \t\t\tdie(\"BUG: how is this possible?\");\n> @@ -213,7 +213,7 @@ 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> -\tstrbuf_add_absolute_path(&text, get_git_dir());\n> +\tstrbuf_realpath(&text, get_git_dir(), 1);\n>   \tstrbuf_add(&pattern, cond, cond_len);\n>   \tprefix = prepare_include_condition_pattern(&pattern);\n>   \n> diff --git a/path.c b/path.c\n> index 2224843..18eaac3 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -654,7 +654,7 @@ char *expand_user_path(const char *path)\n>   \t\t\tconst char *home = getenv(\"HOME\");\n>   \t\t\tif (!home)\n>   \t\t\t\tgoto return_null;\n> -\t\t\tstrbuf_addstr(&user_path, home);\n> +\t\t\tstrbuf_addstr(&user_path, real_path(home));\n>   #ifdef GIT_WINDOWS_NATIVE\n>   \t\t\tconvert_slashes(user_path.buf);\n>   #endif\n> -- 8< --\n\nThanks for the fast reply.\nYes, my path is a softlink - into a directory under NoBackup/ - to make \nthe backup shorter.\nAnd I had forgotten about this :-(\nAnd yes, your patch fixes it- tested under Linux.\n\n\n\n"},{"id":"315768","messageId":"20170330113723.20474-1-pclouds@gmail.com","threadId":"45508","inReplyTo":"20170325130549.GA20618@ash","subject":"[PATCH 1/2] path.c: and an option to call real_path() in expand_user_path()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-30T11:37:22Z","receivedAt":"2017-03-30T11:37:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In the next patch we need the ability to expand '~' to\nreal_path($HOME). But we can't do that from outside because '~' is part\nof a pattern, not a true path. Add an option to expand_user_path() to do\nso.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/commit.c   |  2 +-\n builtin/config.c   |  2 +-\n cache.h            |  2 +-\n config.c           |  8 ++++----\n credential-cache.c |  2 +-\n credential-store.c |  2 +-\n path.c             | 11 ++++++++---\n 7 files changed, 17 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 4e288bc513..ad188fea9e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1404,7 +1404,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \n static const char *implicit_ident_advice(void)\n {\n-\tchar *user_config = expand_user_path(\"~/.gitconfig\");\n+\tchar *user_config = expand_user_path(\"~/.gitconfig\", 0);\n \tchar *xdg_config = xdg_config_home(\"config\");\n \tint config_exists = file_exists(user_config) || file_exists(xdg_config);\n \ndiff --git a/builtin/config.c b/builtin/config.c\nindex 05843a0f96..70bfaaaa1d 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -502,7 +502,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (use_global_config) {\n-\t\tchar *user_config = expand_user_path(\"~/.gitconfig\");\n+\t\tchar *user_config = expand_user_path(\"~/.gitconfig\", 0);\n \t\tchar *xdg_config = xdg_config_home(\"config\");\n \n \t\tif (!user_config)\ndiff --git a/cache.h b/cache.h\nindex 2214d52f61..62e44bfa2f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1146,7 +1146,7 @@ typedef int create_file_fn(const char *path, void *cb);\n int raceproof_create_file(const char *path, create_file_fn fn, void *cb);\n \n int mkdir_in_gitdir(const char *path);\n-extern char *expand_user_path(const char *path);\n+extern char *expand_user_path(const char *path, int real_home);\n const char *enter_repo(const char *path, int strict);\n static inline int is_absolute_path(const char *path)\n {\ndiff --git a/config.c b/config.c\nindex 1a4d85537b..f036c721e6 100644\n--- a/config.c\n+++ b/config.c\n@@ -135,7 +135,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \tif (!path)\n \t\treturn config_error_nonbool(\"include.path\");\n \n-\texpanded = expand_user_path(path);\n+\texpanded = expand_user_path(path, 0);\n \tif (!expanded)\n \t\treturn error(\"could not expand include path '%s'\", path);\n \tpath = expanded;\n@@ -177,7 +177,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n \tchar *expanded;\n \tint prefix = 0;\n \n-\texpanded = expand_user_path(pat->buf);\n+\texpanded = expand_user_path(pat->buf, 0);\n \tif (expanded) {\n \t\tstrbuf_reset(pat);\n \t\tstrbuf_addstr(pat, expanded);\n@@ -948,7 +948,7 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n {\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n-\t*dest = expand_user_path(value);\n+\t*dest = expand_user_path(value, 0);\n \tif (!*dest)\n \t\tdie(_(\"failed to expand user dir in: '%s'\"), value);\n \treturn 0;\n@@ -1498,7 +1498,7 @@ static int do_git_config_sequence(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 *user_config = expand_user_path(\"~/.gitconfig\", 0);\n \tchar *repo_config = have_git_dir() ? git_pathdup(\"config\") : NULL;\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_SYSTEM;\ndiff --git a/credential-cache.c b/credential-cache.c\nindex 3cbd420019..91550bfb0b 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -87,7 +87,7 @@ static char *get_socket_path(void)\n {\n \tstruct stat sb;\n \tchar *old_dir, *socket;\n-\told_dir = expand_user_path(\"~/.git-credential-cache\");\n+\told_dir = expand_user_path(\"~/.git-credential-cache\", 0);\n \tif (old_dir && !stat(old_dir, &sb) && S_ISDIR(sb.st_mode))\n \t\tsocket = xstrfmt(\"%s/socket\", old_dir);\n \telse\ndiff --git a/credential-store.c b/credential-store.c\nindex 55ca1b1334..ac295420dd 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -168,7 +168,7 @@ int cmd_main(int argc, const char **argv)\n \tif (file) {\n \t\tstring_list_append(&fns, file);\n \t} else {\n-\t\tif ((file = expand_user_path(\"~/.git-credentials\")))\n+\t\tif ((file = expand_user_path(\"~/.git-credentials\", 0)))\n \t\t\tstring_list_append_nodup(&fns, file);\n \t\tfile = xdg_config_home(\"credentials\");\n \t\tif (file)\ndiff --git a/path.c b/path.c\nindex 22248436bf..010c565512 100644\n--- a/path.c\n+++ b/path.c\n@@ -638,8 +638,10 @@ static struct passwd *getpw_str(const char *username, size_t len)\n  * Return a string with ~ and ~user expanded via getpw*.  If buf != NULL,\n  * then it is a newly allocated string. Returns NULL on getpw failure or\n  * if path is NULL.\n+ *\n+ * If real_home is true, real_path($HOME) is used in the expansion.\n  */\n-char *expand_user_path(const char *path)\n+char *expand_user_path(const char *path, int real_home)\n {\n \tstruct strbuf user_path = STRBUF_INIT;\n \tconst char *to_copy = path;\n@@ -654,7 +656,10 @@ char *expand_user_path(const char *path)\n \t\t\tconst char *home = getenv(\"HOME\");\n \t\t\tif (!home)\n \t\t\t\tgoto return_null;\n-\t\t\tstrbuf_addstr(&user_path, home);\n+\t\t\tif (real_home)\n+\t\t\t\tstrbuf_addstr(&user_path, real_path(home));\n+\t\t\telse\n+\t\t\t\tstrbuf_addstr(&user_path, home);\n #ifdef GIT_WINDOWS_NATIVE\n \t\t\tconvert_slashes(user_path.buf);\n #endif\n@@ -723,7 +728,7 @@ const char *enter_repo(const char *path, int strict)\n \t\tstrbuf_add(&validated_path, path, len);\n \n \t\tif (used_path.buf[0] == '~') {\n-\t\t\tchar *newpath = expand_user_path(used_path.buf);\n+\t\t\tchar *newpath = expand_user_path(used_path.buf, 0);\n \t\t\tif (!newpath)\n \t\t\t\treturn NULL;\n \t\t\tstrbuf_attach(&used_path, newpath, strlen(newpath),\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"315769","messageId":"20170330113723.20474-2-pclouds@gmail.com","threadId":"45508","inReplyTo":"20170330113723.20474-1-pclouds@gmail.com","subject":"[PATCH 2/2] config: resolve symlinks in conditional include's patterns","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-30T11:37:23Z","receivedAt":"2017-03-30T11:37:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"$GIT_DIR returned by get_git_dir() is normalized, with all symlinks\nresolved (see setup_work_tree function). In order to match paths (or\npatterns) against $GIT_DIR char-by-char, they have to be normalized\ntoo. There is a note in config.txt about this, that the user need to\nresolve symlinks by themselves if needed.\n\nThe problem is, we allow certain path expansion, '~/' and './', for\nconvenience and can't ask the user to resolve symlinks in these\nexpansions. Make sure the expanded paths have all symlinks resolved.\n\nPS. The strbuf_realpath(&text, get_git_dir(), 1) is still needed because\nget_git_dir() may return relative path.\n\nNoticed-by: Torsten Bögershausen <tboegi@web.de>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n config.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f036c721e6..d5ba848b65 100644\n--- a/config.c\n+++ b/config.c\n@@ -177,7 +177,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n \tchar *expanded;\n \tint prefix = 0;\n \n-\texpanded = expand_user_path(pat->buf, 0);\n+\texpanded = expand_user_path(pat->buf, 1);\n \tif (expanded) {\n \t\tstrbuf_reset(pat);\n \t\tstrbuf_addstr(pat, expanded);\n@@ -191,7 +191,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n \t\t\treturn error(_(\"relative config include \"\n \t\t\t\t       \"conditionals must come from files\"));\n \n-\t\tstrbuf_add_absolute_path(&path, cf->path);\n+\t\tstrbuf_realpath(&path, cf->path, 1);\n \t\tslash = find_last_dir_sep(path.buf);\n \t\tif (!slash)\n \t\t\tdie(\"BUG: how is this possible?\");\n@@ -213,7 +213,7 @@ 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-\tstrbuf_add_absolute_path(&text, get_git_dir());\n+\tstrbuf_realpath(&text, get_git_dir(), 1);\n \tstrbuf_add(&pattern, cond, cond_len);\n \tprefix = prepare_include_condition_pattern(&pattern);\n \n-- \n2.11.0.157.gd943d85\n\n"},{"id":"315808","messageId":"xmqqinmq4nkb.fsf@gitster.mtv.corp.google.com","threadId":"45508","inReplyTo":"20170330113723.20474-2-pclouds@gmail.com","subject":"Re: [PATCH 2/2] config: resolve symlinks in conditional include's patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-30T18:38:44Z","receivedAt":"2017-03-30T18:38:51Z","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> $GIT_DIR returned by get_git_dir() is normalized, with all symlinks\n> resolved (see setup_work_tree function). In order to match paths (or\n> patterns) against $GIT_DIR char-by-char, they have to be normalized\n> too. There is a note in config.txt about this, that the user need to\n> resolve symlinks by themselves if needed.\n>\n> The problem is, we allow certain path expansion, '~/' and './', for\n> convenience and can't ask the user to resolve symlinks in these\n> expansions. Make sure the expanded paths have all symlinks resolved.\n\nThat sounds sensible but I fail to see why 1/2 is the right approach\nto do this, and I must be missing something.  Wouldn't you get the\nsame result if you run realpath() yourself on expanded, after\nreceiving the return value of expand_user_path() in it?\n\nCan you add a test to demonstrate the issue (which would need to be\nprotected with SYMLINKS prereq)?\n\nThanks.\n\n> PS. The strbuf_realpath(&text, get_git_dir(), 1) is still needed because\n> get_git_dir() may return relative path.\n>\n> Noticed-by: Torsten Bögershausen <tboegi@web.de>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  config.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index f036c721e6..d5ba848b65 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -177,7 +177,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n>  \tchar *expanded;\n>  \tint prefix = 0;\n>  \n> -\texpanded = expand_user_path(pat->buf, 0);\n> +\texpanded = expand_user_path(pat->buf, 1);\n>  \tif (expanded) {\n>  \t\tstrbuf_reset(pat);\n>  \t\tstrbuf_addstr(pat, expanded);\n> @@ -191,7 +191,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n>  \t\t\treturn error(_(\"relative config include \"\n>  \t\t\t\t       \"conditionals must come from files\"));\n>  \n> -\t\tstrbuf_add_absolute_path(&path, cf->path);\n> +\t\tstrbuf_realpath(&path, cf->path, 1);\n>  \t\tslash = find_last_dir_sep(path.buf);\n>  \t\tif (!slash)\n>  \t\t\tdie(\"BUG: how is this possible?\");\n> @@ -213,7 +213,7 @@ 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> -\tstrbuf_add_absolute_path(&text, get_git_dir());\n> +\tstrbuf_realpath(&text, get_git_dir(), 1);\n>  \tstrbuf_add(&pattern, cond, cond_len);\n>  \tprefix = prepare_include_condition_pattern(&pattern);\n"},{"id":"316144","messageId":"CACsJy8CbOFAviPo9tCJ-4+ritd5WQboGHDyxaCxBK1UMZe0+kQ@mail.gmail.com","threadId":"45508","inReplyTo":"xmqqinmq4nkb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] config: resolve symlinks in conditional include's patterns","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-04T10:12:23Z","receivedAt":"2017-04-04T10:13:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 31, 2017 at 1:38 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> $GIT_DIR returned by get_git_dir() is normalized, with all symlinks\n>> resolved (see setup_work_tree function). In order to match paths (or\n>> patterns) against $GIT_DIR char-by-char, they have to be normalized\n>> too. There is a note in config.txt about this, that the user need to\n>> resolve symlinks by themselves if needed.\n>>\n>> The problem is, we allow certain path expansion, '~/' and './', for\n>> convenience and can't ask the user to resolve symlinks in these\n>> expansions. Make sure the expanded paths have all symlinks resolved.\n>\n> That sounds sensible but I fail to see why 1/2 is the right approach\n> to do this, and I must be missing something.  Wouldn't you get the\n> same result if you run realpath() yourself on expanded, after\n> receiving the return value of expand_user_path() in it?\n\nBecause at that point I don't know what part is $HOME (i.e. valid path\nthat real_path can walk through), what part is random wildcards from\nthe pattern. Note that in this case we pass a wildmatch pattern to\nexpand_user_path(), like ~/[ab]foo/*bar*/**. After expansion it\nbecomes /home/pclouds/[ab]foo/*bar*/**. It does not feel right to let\nreal_path() walk the \"[ab]foo...\" part. In the tests, I hit\ndie(\"Invalid path\") in strbuf_realpath(). Even if I set die_on_error()\nto avoid that, strbuf_realpath() will not return the resolved path\n\n> Can you add a test to demonstrate the issue (which would need to be\n> protected with SYMLINKS prereq)?\n\nWill do. It may look a bit ugly though because I have to force\nsetup_git_directory() to call real_path() because it doesn't always do\nthat.\n-- \nDuy\n"},{"id":"316186","messageId":"20170405102439.14797-1-pclouds@gmail.com","threadId":"45508","inReplyTo":"20170330113723.20474-1-pclouds@gmail.com","subject":"[PATCH v2 1/2] path.c: and an option to call real_path() in expand_user_path()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-05T10:24:38Z","receivedAt":"2017-04-05T10:26:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In the next patch we need the ability to expand '~' to\nreal_path($HOME). But we can't do that from outside because '~' is part\nof a pattern, not a true path. Add an option to expand_user_path() to do\nso.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n No changes in v2.\n\n builtin/commit.c   |  2 +-\n builtin/config.c   |  2 +-\n cache.h            |  2 +-\n config.c           |  8 ++++----\n credential-cache.c |  2 +-\n credential-store.c |  2 +-\n path.c             | 11 ++++++++---\n 7 files changed, 17 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 4e288bc513..ad188fea9e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1404,7 +1404,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \n static const char *implicit_ident_advice(void)\n {\n-\tchar *user_config = expand_user_path(\"~/.gitconfig\");\n+\tchar *user_config = expand_user_path(\"~/.gitconfig\", 0);\n \tchar *xdg_config = xdg_config_home(\"config\");\n \tint config_exists = file_exists(user_config) || file_exists(xdg_config);\n \ndiff --git a/builtin/config.c b/builtin/config.c\nindex 05843a0f96..70bfaaaa1d 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -502,7 +502,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (use_global_config) {\n-\t\tchar *user_config = expand_user_path(\"~/.gitconfig\");\n+\t\tchar *user_config = expand_user_path(\"~/.gitconfig\", 0);\n \t\tchar *xdg_config = xdg_config_home(\"config\");\n \n \t\tif (!user_config)\ndiff --git a/cache.h b/cache.h\nindex 2214d52f61..62e44bfa2f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1146,7 +1146,7 @@ typedef int create_file_fn(const char *path, void *cb);\n int raceproof_create_file(const char *path, create_file_fn fn, void *cb);\n \n int mkdir_in_gitdir(const char *path);\n-extern char *expand_user_path(const char *path);\n+extern char *expand_user_path(const char *path, int real_home);\n const char *enter_repo(const char *path, int strict);\n static inline int is_absolute_path(const char *path)\n {\ndiff --git a/config.c b/config.c\nindex 1a4d85537b..f036c721e6 100644\n--- a/config.c\n+++ b/config.c\n@@ -135,7 +135,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \tif (!path)\n \t\treturn config_error_nonbool(\"include.path\");\n \n-\texpanded = expand_user_path(path);\n+\texpanded = expand_user_path(path, 0);\n \tif (!expanded)\n \t\treturn error(\"could not expand include path '%s'\", path);\n \tpath = expanded;\n@@ -177,7 +177,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n \tchar *expanded;\n \tint prefix = 0;\n \n-\texpanded = expand_user_path(pat->buf);\n+\texpanded = expand_user_path(pat->buf, 0);\n \tif (expanded) {\n \t\tstrbuf_reset(pat);\n \t\tstrbuf_addstr(pat, expanded);\n@@ -948,7 +948,7 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n {\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n-\t*dest = expand_user_path(value);\n+\t*dest = expand_user_path(value, 0);\n \tif (!*dest)\n \t\tdie(_(\"failed to expand user dir in: '%s'\"), value);\n \treturn 0;\n@@ -1498,7 +1498,7 @@ static int do_git_config_sequence(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 *user_config = expand_user_path(\"~/.gitconfig\", 0);\n \tchar *repo_config = have_git_dir() ? git_pathdup(\"config\") : NULL;\n \n \tcurrent_parsing_scope = CONFIG_SCOPE_SYSTEM;\ndiff --git a/credential-cache.c b/credential-cache.c\nindex 3cbd420019..91550bfb0b 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -87,7 +87,7 @@ static char *get_socket_path(void)\n {\n \tstruct stat sb;\n \tchar *old_dir, *socket;\n-\told_dir = expand_user_path(\"~/.git-credential-cache\");\n+\told_dir = expand_user_path(\"~/.git-credential-cache\", 0);\n \tif (old_dir && !stat(old_dir, &sb) && S_ISDIR(sb.st_mode))\n \t\tsocket = xstrfmt(\"%s/socket\", old_dir);\n \telse\ndiff --git a/credential-store.c b/credential-store.c\nindex 55ca1b1334..ac295420dd 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -168,7 +168,7 @@ int cmd_main(int argc, const char **argv)\n \tif (file) {\n \t\tstring_list_append(&fns, file);\n \t} else {\n-\t\tif ((file = expand_user_path(\"~/.git-credentials\")))\n+\t\tif ((file = expand_user_path(\"~/.git-credentials\", 0)))\n \t\t\tstring_list_append_nodup(&fns, file);\n \t\tfile = xdg_config_home(\"credentials\");\n \t\tif (file)\ndiff --git a/path.c b/path.c\nindex 22248436bf..010c565512 100644\n--- a/path.c\n+++ b/path.c\n@@ -638,8 +638,10 @@ static struct passwd *getpw_str(const char *username, size_t len)\n  * Return a string with ~ and ~user expanded via getpw*.  If buf != NULL,\n  * then it is a newly allocated string. Returns NULL on getpw failure or\n  * if path is NULL.\n+ *\n+ * If real_home is true, real_path($HOME) is used in the expansion.\n  */\n-char *expand_user_path(const char *path)\n+char *expand_user_path(const char *path, int real_home)\n {\n \tstruct strbuf user_path = STRBUF_INIT;\n \tconst char *to_copy = path;\n@@ -654,7 +656,10 @@ char *expand_user_path(const char *path)\n \t\t\tconst char *home = getenv(\"HOME\");\n \t\t\tif (!home)\n \t\t\t\tgoto return_null;\n-\t\t\tstrbuf_addstr(&user_path, home);\n+\t\t\tif (real_home)\n+\t\t\t\tstrbuf_addstr(&user_path, real_path(home));\n+\t\t\telse\n+\t\t\t\tstrbuf_addstr(&user_path, home);\n #ifdef GIT_WINDOWS_NATIVE\n \t\t\tconvert_slashes(user_path.buf);\n #endif\n@@ -723,7 +728,7 @@ const char *enter_repo(const char *path, int strict)\n \t\tstrbuf_add(&validated_path, path, len);\n \n \t\tif (used_path.buf[0] == '~') {\n-\t\t\tchar *newpath = expand_user_path(used_path.buf);\n+\t\t\tchar *newpath = expand_user_path(used_path.buf, 0);\n \t\t\tif (!newpath)\n \t\t\t\treturn NULL;\n \t\t\tstrbuf_attach(&used_path, newpath, strlen(newpath),\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"316187","messageId":"20170405102439.14797-2-pclouds@gmail.com","threadId":"45508","inReplyTo":"20170405102439.14797-1-pclouds@gmail.com","subject":"[PATCH v2 2/2] config: resolve symlinks in conditional include's patterns","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-05T10:24:39Z","receivedAt":"2017-04-05T10:26:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"$GIT_DIR returned by get_git_dir() is normalized, with all symlinks\nresolved (see setup_work_tree function). In order to match paths (or\npatterns) against $GIT_DIR char-by-char, they have to be normalized\ntoo. There is a note in config.txt about this, that the user need to\nresolve symlinks by themselves if needed.\n\nThe problem is, we allow certain path expansion, '~/' and './', for\nconvenience and can't ask the user to resolve symlinks in these\nexpansions. Make sure the expanded paths have all symlinks resolved.\n\nPS. The strbuf_realpath(&text, get_git_dir(), 1) is still needed because\nget_git_dir() may return relative path.\n\nNoticed-by: Torsten Bögershausen <tboegi@web.de>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Tests are added in v2.\n\n config.c                  |  6 +++---\n t/t1305-config-include.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+), 3 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f036c721e6..d5ba848b65 100644\n--- a/config.c\n+++ b/config.c\n@@ -177,7 +177,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n \tchar *expanded;\n \tint prefix = 0;\n \n-\texpanded = expand_user_path(pat->buf, 0);\n+\texpanded = expand_user_path(pat->buf, 1);\n \tif (expanded) {\n \t\tstrbuf_reset(pat);\n \t\tstrbuf_addstr(pat, expanded);\n@@ -191,7 +191,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n \t\t\treturn error(_(\"relative config include \"\n \t\t\t\t       \"conditionals must come from files\"));\n \n-\t\tstrbuf_add_absolute_path(&path, cf->path);\n+\t\tstrbuf_realpath(&path, cf->path, 1);\n \t\tslash = find_last_dir_sep(path.buf);\n \t\tif (!slash)\n \t\t\tdie(\"BUG: how is this possible?\");\n@@ -213,7 +213,7 @@ 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-\tstrbuf_add_absolute_path(&text, get_git_dir());\n+\tstrbuf_realpath(&text, get_git_dir(), 1);\n \tstrbuf_add(&pattern, cond, cond_len);\n \tprefix = prepare_include_condition_pattern(&pattern);\n \ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex e833939320..8fbc7a029f 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -3,6 +3,16 @@\n test_description='test config file include directives'\n . ./test-lib.sh\n \n+# Force setup_explicit_git_dir() to run until the end. This is needed\n+# by some tests to make sure real_path() is called on $GIT_DIR. The\n+# caller needs to make sure git commands are run from a subdirectory\n+# though or real_path() will not be called.\n+force_setup_explicit_git_dir() {\n+    GIT_DIR=\"$(pwd)/.git\"\n+    GIT_WORK_TREE=\"$(pwd)\"\n+    export GIT_DIR GIT_WORK_TREE\n+}\n+\n test_expect_success 'include file by absolute path' '\n \techo \"[test]one = 1\" >one &&\n \techo \"[include]path = \\\"$(pwd)/one\\\"\" >.gitconfig &&\n@@ -208,6 +218,50 @@ test_expect_success 'conditional include, both unanchored, icase' '\n \t)\n '\n \n+test_expect_success SYMLINKS 'conditional include, set up symlinked $HOME' '\n+\tmkdir real-home &&\n+\tln -s real-home home &&\n+\t(\n+\t\tHOME=\"$TRASH_DIRECTORY/home\" &&\n+\t\texport HOME &&\n+\t\tcd \"$HOME\" &&\n+\n+\t\tgit init foo &&\n+\t\tcd foo &&\n+\t\tmkdir sub\n+\t)\n+'\n+\n+test_expect_success SYMLINKS 'conditional include, $HOME expansion with symlinks' '\n+\t(\n+\t\tHOME=\"$TRASH_DIRECTORY/home\" &&\n+\t\texport HOME &&\n+\t\tcd \"$HOME\"/foo &&\n+\n+\t\techo \"[includeIf \\\"gitdir:~/foo/\\\"]path=bar2\" >>.git/config &&\n+\t\techo \"[test]two=2\" >.git/bar2 &&\n+\t\techo 2 >expect &&\n+\t\tforce_setup_explicit_git_dir &&\n+\t\tgit -C sub config test.two >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success SYMLINKS 'conditional include, relative path with symlinks' '\n+\techo \"[includeIf \\\"gitdir:./foo/.git\\\"]path=bar4\" >home/.gitconfig &&\n+\techo \"[test]four=4\" >home/bar4 &&\n+\t(\n+\t\tHOME=\"$TRASH_DIRECTORY/home\" &&\n+\t\texport HOME &&\n+\t\tcd \"$HOME\"/foo &&\n+\n+\t\techo 4 >expect &&\n+\t\tforce_setup_explicit_git_dir &&\n+\t\tgit -C sub config test.four >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"}]}