{"thread":{"id":"45967","subject":"[PATCH/RFC] The new IncludeIf facility doesn't DWIM when the repo is symlinked","startedAt":"2017-05-15T15:20:44Z","lastAt":"2017-05-16T08:29:07Z","messageCount":5,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"319790","messageId":"CACBZZX5d25UyGbb6=kPmzk_5-X0GMptN0kX_--aBADsDy-9kWw@mail.gmail.com","threadId":"45967","inReplyTo":null,"subject":"[PATCH/RFC] The new IncludeIf facility doesn't DWIM when the repo is symlinked","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-15T15:20:01Z","receivedAt":"2017-05-15T15:20:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I have a ~/git_tree in my homedir that's symlinked to an external\ndrive, and doing \"gitdir:~/git_tree/\" doesn't work, because instead of\nmatching against ~/git_tree it's matched against\n/mnt/some-other-storage/.\n\nHere's a WIP patch that makes this work for me, any reason I shouldn't\nfinish this up & that we shouldn't be doing this? The doc don't say\n\"we'll only match gitdir against the absolute resolved path\" or\nanything like that, so until I checked out the implementation I didn't\nrealize what was going on:\n\ndiff --git a/config.c b/config.c\nindex b4a3205da3..606acaa3f1 100644\n--- a/config.c\n+++ b/config.c\n@@ -214,6 +214,7 @@ static int include_by_gitdir(const struct\nconfig_options *opts,\n        struct strbuf pattern = STRBUF_INIT;\n        int ret = 0, prefix;\n        const char *git_dir;\n+       int tried_absolute = 0;\n\n        if (opts->git_dir)\n                git_dir = opts->git_dir;\n@@ -226,6 +227,7 @@ static int include_by_gitdir(const struct\nconfig_options *opts,\n        strbuf_add(&pattern, cond, cond_len);\n        prefix = prepare_include_condition_pattern(&pattern);\n\n+again:\n        if (prefix < 0)\n                goto done;\n\n@@ -245,6 +247,12 @@ static int include_by_gitdir(const struct\nconfig_options *opts,\n        ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,\n                         icase ? WM_CASEFOLD : 0, NULL);\n\n+       if (!ret && !tried_absolute) {\n+               tried_absolute = 1;\n+               strbuf_reset(&text);\n+               strbuf_add_absolute_path(&text, git_dir);\n+               goto again;\n+       }\n done:\n        strbuf_release(&pattern);\n        strbuf_release(&text);\n"},{"id":"319811","messageId":"CACBZZX5nchNUb-V3U8FL9fuhk=3t42Qhz4=Wh9Qk_0C=x85UdQ@mail.gmail.com","threadId":"45967","inReplyTo":"CACBZZX5d25UyGbb6=kPmzk_5-X0GMptN0kX_--aBADsDy-9kWw@mail.gmail.com","subject":"Re: [PATCH/RFC] The new IncludeIf facility doesn't DWIM when the repo is symlinked","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-15T18:30:31Z","receivedAt":"2017-05-15T18:30:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, May 15, 2017 at 5:20 PM, Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> I have a ~/git_tree in my homedir that's symlinked to an external\n> drive, and doing \"gitdir:~/git_tree/\" doesn't work, because instead of\n> matching against ~/git_tree it's matched against\n> /mnt/some-other-storage/.\n>\n> Here's a WIP patch that makes this work for me, any reason I shouldn't\n> finish this up & that we shouldn't be doing this? The doc don't say\n> \"we'll only match gitdir against the absolute resolved path\" or\n> anything like that, so until I checked out the implementation I didn't\n> realize what was going on:\n\nOn closer inspection I can see that this was actually broken in\n86f9515708 (\"config: resolve symlinks in conditional include's\npatterns\", 2017-04-05), but works in the initial addition of\nIncludeIf.\n\n> diff --git a/config.c b/config.c\n> index b4a3205da3..606acaa3f1 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -214,6 +214,7 @@ static int include_by_gitdir(const struct\n> config_options *opts,\n>         struct strbuf pattern = STRBUF_INIT;\n>         int ret = 0, prefix;\n>         const char *git_dir;\n> +       int tried_absolute = 0;\n>\n>         if (opts->git_dir)\n>                 git_dir = opts->git_dir;\n> @@ -226,6 +227,7 @@ static int include_by_gitdir(const struct\n> config_options *opts,\n>         strbuf_add(&pattern, cond, cond_len);\n>         prefix = prepare_include_condition_pattern(&pattern);\n>\n> +again:\n>         if (prefix < 0)\n>                 goto done;\n>\n> @@ -245,6 +247,12 @@ static int include_by_gitdir(const struct\n> config_options *opts,\n>         ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,\n>                          icase ? WM_CASEFOLD : 0, NULL);\n>\n> +       if (!ret && !tried_absolute) {\n> +               tried_absolute = 1;\n> +               strbuf_reset(&text);\n> +               strbuf_add_absolute_path(&text, git_dir);\n> +               goto again;\n> +       }\n>  done:\n>         strbuf_release(&pattern);\n>         strbuf_release(&text);\n"},{"id":"319824","messageId":"20170515191552.5193-1-avarab@gmail.com","threadId":"45967","inReplyTo":"CACBZZX5nchNUb-V3U8FL9fuhk=3t42Qhz4=Wh9Qk_0C=x85UdQ@mail.gmail.com","subject":"[PATCH] config: match both symlink & realpath versions in IncludeIf.gitdir:*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-15T19:15:52Z","receivedAt":"2017-05-15T19:16:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the conditional inclusion mechanism to support\ne.g. gitdir:~/git_tree/repo where ~/git_tree is a symlink to to\n/mnt/stuff/repo.\n\nThis worked in the initial version of this facility[1], but regressed\nlater in the series while solving a related bug[2].\n\nNow gitdir: will match against the symlinked\npath (e.g. gitdir:~/git_tree/repo) in addition to the current\n/mnt/stuff/repo path.\n\nSince this is already in a release version note in the documentation\nthat this behavior changed, so users who expect their configuration to\nwork on both v2.13.0 and some future version of git with this fix\naren't utterly confused.\n\n1. commit 3efd0bedc6 (\"config: add conditional include\", 2017-03-01)\n2. commit 86f9515708 (\"config: resolve symlinks in conditional\n   include's patterns\", 2017-04-05)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nHere's a non-RFC patch to fix this bug.\n\n Documentation/config.txt  | 11 +++++++++++\n config.c                  | 16 ++++++++++++++++\n t/t1305-config-include.sh | 23 +++++++++++++++++++++++\n 3 files changed, 50 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 475e874d51..137502a289 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -140,6 +140,17 @@ A few more notes on matching via `gitdir` and `gitdir/i`:\n \n  * Symlinks in `$GIT_DIR` are not resolved before matching.\n \n+ * Both the symlink & realpath versions of paths will be matched\n+   outside of `$GIT_DIR`. E.g. if ~/git is a symlink to\n+   /mnt/storage/git, both `gitdir:~/git` and `gitdir:/mnt/storage/git`\n+   will match.\n+\n+   This was not the case in the initial release of this feature in\n+   v2.13.0, which only matched the realpath version. Configuration\n+   that wants to be compatible with the initial release of this\n+   feature needs to either specify only the realpath version, or both\n+   versions.\n+\n  * Note that \"../\" is not special and will match literally, which is\n    unlikely what you want.\n \ndiff --git a/config.c b/config.c\nindex b4a3205da3..0498746112 100644\n--- a/config.c\n+++ b/config.c\n@@ -214,6 +214,7 @@ static int include_by_gitdir(const struct config_options *opts,\n \tstruct strbuf pattern = STRBUF_INIT;\n \tint ret = 0, prefix;\n \tconst char *git_dir;\n+\tint retry = 1;\n \n \tif (opts->git_dir)\n \t\tgit_dir = opts->git_dir;\n@@ -226,6 +227,7 @@ static int include_by_gitdir(const struct config_options *opts,\n \tstrbuf_add(&pattern, cond, cond_len);\n \tprefix = prepare_include_condition_pattern(&pattern);\n \n+again:\n \tif (prefix < 0)\n \t\tgoto done;\n \n@@ -245,6 +247,20 @@ static int include_by_gitdir(const struct config_options *opts,\n \tret = !wildmatch(pattern.buf + prefix, text.buf + prefix,\n \t\t\t icase ? WM_CASEFOLD : 0, NULL);\n \n+\tif (!ret && retry) {\n+\t\t/*\n+\t\t * We've tried e.g. matching gitdir:~/work, but if\n+\t\t * ~/work is a symlink to /mnt/storage/work\n+\t\t * strbuf_realpath() will expand it, so the rule won't\n+\t\t * match. Let's match against a\n+\t\t * strbuf_add_absolute_path() version of the path,\n+\t\t * which'll do the right thing\n+\t\t */\n+\t\tstrbuf_reset(&text);\n+\t\tstrbuf_add_absolute_path(&text, git_dir);\n+\t\tretry = 0;\n+\t\tgoto again;\n+\t}\n done:\n \tstrbuf_release(&pattern);\n \tstrbuf_release(&text);\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex 933915ec06..d9d2f545a4 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -273,6 +273,29 @@ test_expect_success SYMLINKS 'conditional include, relative path with symlinks'\n \t)\n '\n \n+test_expect_success SYMLINKS 'conditional include, gitdir matching symlink' '\n+\tln -s foo bar &&\n+\t(\n+\t\tcd bar &&\n+\t\techo \"[includeIf \\\"gitdir:bar/\\\"]path=bar7\" >>.git/config &&\n+\t\techo \"[test]seven=7\" >.git/bar7 &&\n+\t\techo 7 >expect &&\n+\t\tgit config test.seven >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success SYMLINKS 'conditional include, gitdir matching symlink, icase' '\n+\t(\n+\t\tcd bar &&\n+\t\techo \"[includeIf \\\"gitdir/i:BAR/\\\"]path=bar8\" >>.git/config &&\n+\t\techo \"[test]eight=8\" >.git/bar8 &&\n+\t\techo 8 >expect &&\n+\t\tgit config test.eight >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.13.0.rc2.291.g57267f2277\n\n"},{"id":"319862","messageId":"xmqqlgpx4n5v.fsf@gitster.mtv.corp.google.com","threadId":"45967","inReplyTo":"20170515191552.5193-1-avarab@gmail.com","subject":"Re: [PATCH] config: match both symlink & realpath versions in IncludeIf.gitdir:*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-16T01:15:40Z","receivedAt":"2017-05-16T01:15:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Change the conditional inclusion mechanism to support\n> e.g. gitdir:~/git_tree/repo where ~/git_tree is a symlink to to\n\ns/to to/to/;\n\n> /mnt/stuff/repo.\n>\n> This worked in the initial version of this facility[1], but regressed\n> later in the series while solving a related bug[2].\n>\n> Now gitdir: will match against the symlinked\n> path (e.g. gitdir:~/git_tree/repo) in addition to the current\n> /mnt/stuff/repo path.\n>\n> Since this is already in a release version note in the documentation\n> that this behavior changed, so users who expect their configuration to\n> work on both v2.13.0 and some future version of git with this fix\n> aren't utterly confused.\n>\n> 1. commit 3efd0bedc6 (\"config: add conditional include\", 2017-03-01)\n> 2. commit 86f9515708 (\"config: resolve symlinks in conditional\n>    include's patterns\", 2017-04-05)\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>\n> Here's a non-RFC patch to fix this bug.\n>\n>  Documentation/config.txt  | 11 +++++++++++\n>  config.c                  | 16 ++++++++++++++++\n>  t/t1305-config-include.sh | 23 +++++++++++++++++++++++\n>  3 files changed, 50 insertions(+)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 475e874d51..137502a289 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -140,6 +140,17 @@ A few more notes on matching via `gitdir` and `gitdir/i`:\n>  \n>   * Symlinks in `$GIT_DIR` are not resolved before matching.\n>  \n> + * Both the symlink & realpath versions of paths will be matched\n> +   outside of `$GIT_DIR`. E.g. if ~/git is a symlink to\n> +   /mnt/storage/git, both `gitdir:~/git` and `gitdir:/mnt/storage/git`\n> +   will match.\n> +\n> +   This was not the case in the initial release of this feature in\n> +   v2.13.0, which only matched the realpath version. Configuration\n> +   that wants to be compatible with the initial release of this\n> +   feature needs to either specify only the realpath version, or both\n> +   versions.\n> +\n\nDoes the second paragraph format correctly, or do we need the usual\n(and ugly) \"+ by itself on a line and then dedent the next\nparagraph\" trick?\n\n> diff --git a/config.c b/config.c\n> index b4a3205da3..0498746112 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -214,6 +214,7 @@ static int include_by_gitdir(const struct config_options *opts,\n>  \tstruct strbuf pattern = STRBUF_INIT;\n>  \tint ret = 0, prefix;\n>  \tconst char *git_dir;\n> +\tint retry = 1;\n\nI think your earlier RFC used a variable with a better name than\nthis; it is not like we are preparing ourselves for adding a\nmechanism here to retry for many random reasons.\n\nThe logic would be clearer With an \"already_tried_absolute\" variable\nthat is initially 0, retrying unless it is already set, and set it\nwhen we retry, like you did in your RFC.\n\nThanks.\n"},{"id":"319909","messageId":"20170516082846.26191-1-avarab@gmail.com","threadId":"45967","inReplyTo":"xmqqlgpx4n5v.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] config: match both symlink & realpath versions in IncludeIf.gitdir:*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-16T08:28:46Z","receivedAt":"2017-05-16T08:29:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the conditional inclusion mechanism to support\ne.g. gitdir:~/git_tree/repo where ~/git_tree is a symlink to\n/mnt/stuff/repo.\n\nThis worked in the initial version of this facility[1], but regressed\nlater in the series while solving a related bug[2].\n\nNow gitdir: will match against the symlinked\npath (e.g. gitdir:~/git_tree/repo) in addition to the current\n/mnt/stuff/repo path.\n\nSince this is already in a release version note in the documentation\nthat this behavior changed, so users who expect their configuration to\nwork on both v2.13.0 and some future version of git with this fix\naren't utterly confused.\n\n1. commit 3efd0bedc6 (\"config: add conditional include\", 2017-03-01)\n2. commit 86f9515708 (\"config: resolve symlinks in conditional\n   include's patterns\", 2017-04-05)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nThanks. All fixed in the v2 below.\n\n Documentation/config.txt  | 10 ++++++++++\n config.c                  | 16 ++++++++++++++++\n t/t1305-config-include.sh | 23 +++++++++++++++++++++++\n 3 files changed, 49 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 475e874d51..302b8af192 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -140,6 +140,16 @@ A few more notes on matching via `gitdir` and `gitdir/i`:\n \n  * Symlinks in `$GIT_DIR` are not resolved before matching.\n \n+ * Both the symlink & realpath versions of paths will be matched\n+   outside of `$GIT_DIR`. E.g. if ~/git is a symlink to\n+   /mnt/storage/git, both `gitdir:~/git` and `gitdir:/mnt/storage/git`\n+   will match.\n++\n+This was not the case in the initial release of this feature in\n+v2.13.0, which only matched the realpath version. Configuration that\n+wants to be compatible with the initial release of this feature needs\n+to either specify only the realpath version, or both versions.\n+\n  * Note that \"../\" is not special and will match literally, which is\n    unlikely what you want.\n \ndiff --git a/config.c b/config.c\nindex b4a3205da3..f978428695 100644\n--- a/config.c\n+++ b/config.c\n@@ -214,6 +214,7 @@ static int include_by_gitdir(const struct config_options *opts,\n \tstruct strbuf pattern = STRBUF_INIT;\n \tint ret = 0, prefix;\n \tconst char *git_dir;\n+\tint already_tried_absolute = 0;\n \n \tif (opts->git_dir)\n \t\tgit_dir = opts->git_dir;\n@@ -226,6 +227,7 @@ static int include_by_gitdir(const struct config_options *opts,\n \tstrbuf_add(&pattern, cond, cond_len);\n \tprefix = prepare_include_condition_pattern(&pattern);\n \n+again:\n \tif (prefix < 0)\n \t\tgoto done;\n \n@@ -245,6 +247,20 @@ static int include_by_gitdir(const struct config_options *opts,\n \tret = !wildmatch(pattern.buf + prefix, text.buf + prefix,\n \t\t\t icase ? WM_CASEFOLD : 0, NULL);\n \n+\tif (!ret && !already_tried_absolute) {\n+\t\t/*\n+\t\t * We've tried e.g. matching gitdir:~/work, but if\n+\t\t * ~/work is a symlink to /mnt/storage/work\n+\t\t * strbuf_realpath() will expand it, so the rule won't\n+\t\t * match. Let's match against a\n+\t\t * strbuf_add_absolute_path() version of the path,\n+\t\t * which'll do the right thing\n+\t\t */\n+\t\tstrbuf_reset(&text);\n+\t\tstrbuf_add_absolute_path(&text, git_dir);\n+\t\talready_tried_absolute = 1;\n+\t\tgoto again;\n+\t}\n done:\n \tstrbuf_release(&pattern);\n \tstrbuf_release(&text);\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex 933915ec06..d9d2f545a4 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -273,6 +273,29 @@ test_expect_success SYMLINKS 'conditional include, relative path with symlinks'\n \t)\n '\n \n+test_expect_success SYMLINKS 'conditional include, gitdir matching symlink' '\n+\tln -s foo bar &&\n+\t(\n+\t\tcd bar &&\n+\t\techo \"[includeIf \\\"gitdir:bar/\\\"]path=bar7\" >>.git/config &&\n+\t\techo \"[test]seven=7\" >.git/bar7 &&\n+\t\techo 7 >expect &&\n+\t\tgit config test.seven >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success SYMLINKS 'conditional include, gitdir matching symlink, icase' '\n+\t(\n+\t\tcd bar &&\n+\t\techo \"[includeIf \\\"gitdir/i:BAR/\\\"]path=bar8\" >>.git/config &&\n+\t\techo \"[test]eight=8\" >.git/bar8 &&\n+\t\techo 8 >expect &&\n+\t\tgit config test.eight >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.13.0.303.g4ebf302169\n\n"}]}