# [BUG] Suspected with double asterisk in conditional include pattern 9 messages from 2019-03-14 to 2019-04-30. Participants: Jason Karns, Taylor Blau, Nguyễn Thái Ngọc Duy, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/50740 ## Jason Karns, 2019-03-14 17:30 Subject: [BUG] Suspected with double asterisk in conditional include pattern Message-ID: URL: https://gitlist.dev/e/CAKNmmv26G05GO7hG9bNvMsjpuUMHZRA%2B2f94TuG2wDNUwNhHkw%40mail.gmail.com ``` Hello, I believe I've encountered a bug regarding the use of double asterisk ( /**/ ) within includeIf patterns. git-config man pages state that **/ and /**, that can match multiple path components They then refer to the gitignore man pages which further define supported wildcard patterns: A slash followed by two consecutive asterisks then a slash matches zero or more directories. For example, "a/**/b" matches "a/b", "a/x/b", "a/x/y/b" and so on. My understanding of these docs are that the pattern `/usr/local/**/Homebrew/` ought to match both`/usr/local/cache/Homebrew/foo` and `/usr/local/Homebrew/foo`. However, given the following conditional include rule: [includeIf "gitdir:/usr/local/**/Homebrew/"] The external config file is successfully included while in repo `/usr/local/cache/Homebrew/foo` but NOT `/usr/local/Homebrew/foo`. If I change the pattern to `**/Homebrew/**`, then the pattern matches both desired repos. So it would seem that the `/**/` does not match 0 directories when the pattern begins with a slash. Thank you, Jason ``` ## Taylor Blau, 2019-03-22 19:04 Subject: Re: [BUG] Suspected with double asterisk in conditional include pattern Message-ID: <20190322190400.GD12155@Taylors-MacBook-Pro.local> URL: https://gitlist.dev/e/20190322190400.GD12155%40Taylors-MacBook-Pro.local In-Reply-To: ``` Hi Jason, On Thu, Mar 14, 2019 at 01:30:43PM -0400, Jason Karns wrote: > Hello, > > I believe I've encountered a bug regarding the use of double asterisk > ( /**/ ) within includeIf patterns. > > git-config man pages state that > > **/ and /**, that can match multiple path components > > They then refer to the gitignore man pages which further define > supported wildcard patterns: > > A slash followed by two consecutive asterisks then a slash matches > zero or more directories. > For example, "a/**/b" matches "a/b", "a/x/b", "a/x/y/b" and so on. > > My understanding of these docs are that the pattern > `/usr/local/**/Homebrew/` ought to match > both`/usr/local/cache/Homebrew/foo` and `/usr/local/Homebrew/foo`. > > However, given the following conditional include rule: > > [includeIf "gitdir:/usr/local/**/Homebrew/"] For what it's worth, Git LFS's implementation of the wildmatch algorithm [1] exhibits the same behavior (i.e., that it does _not_ match those double stars in the middle of the pathspec). Here are the tests I added to double check: { Pattern: `/usr/local/**/Homebrew/`, Subject: `/usr/local/cache/Homebrew/foo`, Match: false, }, { Pattern: `/usr/local/**/Homebrew/`, Subject: `/usr/local/Homebrew/foo`, Match: false, }, And they passed, indicating that neither of the above subjects are a match for the pattern '/usr/local/**/Homebrew'. I was suspicious that changing the pattern to '/usr/local/**/Homebrew/*' might have caused it to match, but it did not. But this isn't really telling us anything that we don't know. What's interesting is that '/usr/local/**/Homebrew' _does_ match '/usr/local/Homebrew' and '/usr/local/cache/Homebrew'. I think that this is consistent with [2]: A slash followed by two consecutive asterisks then a slash matches zero or more directories. For example, "a/**/b" matches "a/b", "a/x/b", "a/x/y/b" and so on. So I think that there _is_ a bug here, but not the one you suggested. Rather, I think that the bug is that with the _trailing_ wildcard, the pathspec doesn't accept '/usr/local/cache/Homebrew/a'. Thanks, Taylor [1]: https://github.com/git-lfs/wildmatch [2]: https://git-scm.com/docs/gitignore ``` ## Nguyễn Thái Ngọc Duy, 2019-03-23 03:45 Subject: [PATCH] config: correct '**' matching in includeIf patterns Message-ID: <20190323034535.23364-1-pclouds@gmail.com> URL: https://gitlist.dev/e/20190323034535.23364-1-pclouds%40gmail.com In-Reply-To: ``` The current wildmatch() call for includeIf's gitdir pattern does not pass the WM_PATHNAME flag. Without this flag, '*' is treated _almost_ the same as '**' (because '*' also matches slashes) with one exception: '/**/' can match a single slash. The pattern 'foo/**/bar' matches 'foo/bar'. But '/*/', which is essentially what wildmatch engine sees without WM_PATHNAME, has to match two slashes (and '*' matches nothing). Which means 'foo/*/bar' cannot match 'foo/bar'. It can only match 'foo//bar'. The result of this is the current wildmatch() call works most of the time until the user depends on '/**/' matching no path component. The fix is straightforward. Reported-by: Jason Karns Signed-off-by: Nguyễn Thái Ngọc Duy --- Sorry I didn't notice this until Taylor's reply. Not sure how to explain git-lfs behavior though. config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.c b/config.c index 0f0cdd8c0f..c2846df3f1 100644 --- a/config.c +++ b/config.c @@ -242,7 +242,7 @@ static int include_by_gitdir(const struct config_options *opts, } ret = !wildmatch(pattern.buf + prefix, text.buf + prefix, - icase ? WM_CASEFOLD : 0); + WM_PATHNAME | (icase ? WM_CASEFOLD : 0)); if (!ret && !already_tried_absolute) { /* -- 2.21.0.548.gd3c7d92dc2 ``` ## Junio C Hamano, 2019-03-24 12:56 Subject: Re: [PATCH] config: correct '**' matching in includeIf patterns Message-ID: URL: https://gitlist.dev/e/xmqq7ecoph14.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190323034535.23364-1-pclouds@gmail.com> ``` Nguyễn Thái Ngọc Duy writes: > The current wildmatch() call for includeIf's gitdir pattern does not > pass the WM_PATHNAME flag. Without this flag, '*' is treated _almost_ > the same as '**' (because '*' also matches slashes) with one exception: > > '/**/' can match a single slash. The pattern 'foo/**/bar' matches > 'foo/bar'. > > But '/*/', which is essentially what wildmatch engine sees without > WM_PATHNAME, has to match two slashes (and '*' matches nothing). Which > means 'foo/*/bar' cannot match 'foo/bar'. It can only match 'foo//bar'. > > The result of this is the current wildmatch() call works most of the > time until the user depends on '/**/' matching no path component. The > fix is straightforward. > > Reported-by: Jason Karns > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > Sorry I didn't notice this until Taylor's reply. Not sure how to > explain git-lfs behavior though. > > config.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) The analysis above is very good, and the updated code is a natural consequence of the analysis; very well done. Would it be easy to protect this fix from future breakages with a test or two? > diff --git a/config.c b/config.c > index 0f0cdd8c0f..c2846df3f1 100644 > --- a/config.c > +++ b/config.c > @@ -242,7 +242,7 @@ static int include_by_gitdir(const struct config_options *opts, > } > > ret = !wildmatch(pattern.buf + prefix, text.buf + prefix, > - icase ? WM_CASEFOLD : 0); > + WM_PATHNAME | (icase ? WM_CASEFOLD : 0)); > > if (!ret && !already_tried_absolute) { > /* ``` ## Nguyễn Thái Ngọc Duy, 2019-03-24 13:17 Subject: [PATCH v2] config: correct '**' matching in includeIf patterns Message-ID: <20190324131755.26821-1-pclouds@gmail.com> URL: https://gitlist.dev/e/20190324131755.26821-1-pclouds%40gmail.com In-Reply-To: <20190323034535.23364-1-pclouds@gmail.com> ``` The current wildmatch() call for includeIf's gitdir pattern does not pass the WM_PATHNAME flag. Without this flag, '*' is treated _almost_ the same as '**' (because '*' also matches slashes) with one exception: '/**/' can match a single slash. The pattern 'foo/**/bar' matches 'foo/bar'. But '/*/', which is essentially what wildmatch engine sees without WM_PATHNAME, has to match two slashes (and '*' matches nothing). Which means 'foo/*/bar' cannot match 'foo/bar'. It can only match 'foo//bar'. The result of this is the current wildmatch() call works most of the time until the user depends on '/**/' matching no path component. And also '*' matches slashes while it should not, but people probably haven't noticed this yet. The fix is straightforward. Reported-by: Jason Karns Signed-off-by: Nguyễn Thái Ngọc Duy --- v2 adds a test. My laziness can't get past Junio. config.c | 2 +- t/t1305-config-include.sh | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 0f0cdd8c0f..c2846df3f1 100644 --- a/config.c +++ b/config.c @@ -242,7 +242,7 @@ static int include_by_gitdir(const struct config_options *opts, } ret = !wildmatch(pattern.buf + prefix, text.buf + prefix, - icase ? WM_CASEFOLD : 0); + WM_PATHNAME | (icase ? WM_CASEFOLD : 0)); if (!ret && !already_tried_absolute) { /* diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh index 635918505d..4d6e70c11d 100755 --- a/t/t1305-config-include.sh +++ b/t/t1305-config-include.sh @@ -229,6 +229,19 @@ test_expect_success 'conditional include, early config reading' ' ) ' +test_expect_success 'conditional include with /**/' ' + mkdir foo/bar && + git init foo/bar/repo && + ( + cd foo/bar/repo && + echo "[includeIf \"gitdir:**/foo/**/bar/**\"]path=bar7" >>.git/config && + echo "[test]seven=7" >.git/bar7 && + echo 7 >expect && + git config test.seven >actual && + test_cmp expect actual + ) +' + test_expect_success SYMLINKS 'conditional include, set up symlinked $HOME' ' mkdir real-home && ln -s real-home home && -- 2.21.0.479.g47ac719cd3 ``` ## Junio C Hamano, 2019-03-25 02:30 Subject: Re: [PATCH v2] config: correct '**' matching in includeIf patterns Message-ID: URL: https://gitlist.dev/e/xmqqa7hjofc8.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190324131755.26821-1-pclouds@gmail.com> ``` Nguyễn Thái Ngọc Duy writes: > The current wildmatch() call for includeIf's gitdir pattern does not > pass the WM_PATHNAME flag. Without this flag, '*' is treated _almost_ > the same as '**' (because '*' also matches slashes) with one exception: > > '/**/' can match a single slash. The pattern 'foo/**/bar' matches > 'foo/bar'. > > But '/*/', which is essentially what wildmatch engine sees without > WM_PATHNAME, has to match two slashes (and '*' matches nothing). Which > means 'foo/*/bar' cannot match 'foo/bar'. It can only match 'foo//bar'. > > The result of this is the current wildmatch() call works most of the > time until the user depends on '/**/' matching no path component. And > also '*' matches slashes while it should not, but people probably > haven't noticed this yet. The fix is straightforward. > +test_expect_success 'conditional include with /**/' ' > + mkdir foo/bar && > + git init foo/bar/repo && > + ( > + cd foo/bar/repo && > + echo "[includeIf \"gitdir:**/foo/**/bar/**\"]path=bar7" >>.git/config && > + echo "[test]seven=7" >.git/bar7 && > + echo 7 >expect && > + git config test.seven >actual && > + test_cmp expect actual > + ) > +' That's cute ;-) Thanks for quickly working on the fix with an incredibly short turnaround time since the initial report. P.S. I expect to be offline for most of the week (packing, moving and unpacking. Even though the places packing and unpacking happens are within 1 kilometer radius, that does not make it less hassle X-<). See you guys next month. ``` ## Johannes Schindelin, 2019-03-25 21:40 Subject: Re: [PATCH v2] config: correct '**' matching in includeIf patterns Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1903252233580.41%40tvgsbejvaqbjf.bet In-Reply-To: <20190324131755.26821-1-pclouds@gmail.com> ``` Hi Duy, On Sun, 24 Mar 2019, Nguyễn Thái Ngọc Duy wrote: > diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh > index 635918505d..4d6e70c11d 100755 > --- a/t/t1305-config-include.sh > +++ b/t/t1305-config-include.sh > @@ -229,6 +229,19 @@ test_expect_success 'conditional include, early config reading' ' > ) > ' > > +test_expect_success 'conditional include with /**/' ' > + mkdir foo/bar && This does assume that no previous test case created `bar`, but that `foo` was created (which makes it harder to use `--run=` for quicker testing/debugging). It would be better to use mkdir -p foo/bar && here. Or *even* better... > + git init foo/bar/repo && ... just drop the `mkdir foo/bar`, as `git init foo/bar/repo` has not the slightest problem creating the intermediate directories. > + ( > + cd foo/bar/repo && > + echo "[includeIf \"gitdir:**/foo/**/bar/**\"]path=bar7" >>.git/config && This line is longer than the 80 columns asked for in SubmittingPatches, and while you have to wrap the line anyway, why not avoid the `cd`, too? echo "[includeIf \"gitdir:**/foo/**/bar/**\"]path=bar7" \ >>foo/bar/repo/.git/config && echo "[test]seven=7" >foo/bar/repo/.git/bar7 && echo 7 >expect && git -C foo/bar/repo config test.seven >actual && test_cmp expect actual Ciao, Johannes > + echo "[test]seven=7" >.git/bar7 && > + echo 7 >expect && > + git config test.seven >actual && > + test_cmp expect actual > + ) > +' > + > test_expect_success SYMLINKS 'conditional include, set up symlinked $HOME' ' > mkdir real-home && > ln -s real-home home && > -- > 2.21.0.479.g47ac719cd3 > > ``` ## Nguyễn Thái Ngọc Duy, 2019-03-26 09:41 Subject: [PATCH v3] config: correct '**' matching in includeIf patterns Message-ID: <20190326094101.26294-1-pclouds@gmail.com> URL: https://gitlist.dev/e/20190326094101.26294-1-pclouds%40gmail.com In-Reply-To: <20190324131755.26821-1-pclouds@gmail.com> ``` The current wildmatch() call for includeIf's gitdir pattern does not pass the WM_PATHNAME flag. Without this flag, '*' is treated _almost_ the same as '**' (because '*' also matches slashes) with one exception: '/**/' can match a single slash. The pattern 'foo/**/bar' matches 'foo/bar'. But '/*/', which is essentially what wildmatch engine sees without WM_PATHNAME, has to match two slashes (and '*' matches nothing). Which means 'foo/*/bar' cannot match 'foo/bar'. It can only match 'foo//bar'. The result of this is the current wildmatch() call works most of the time until the user depends on '/**/' matching no path component. And also '*' matches slashes while it should not, but people probably haven't noticed this yet. The fix is straightforward. Reported-by: Jason Karns Signed-off-by: Nguyễn Thái Ngọc Duy --- v3 updates the test to avoid mkdir and cd, and break long lines. config.c | 2 +- t/t1305-config-include.sh | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 0f0cdd8c0f..c2846df3f1 100644 --- a/config.c +++ b/config.c @@ -242,7 +242,7 @@ static int include_by_gitdir(const struct config_options *opts, } ret = !wildmatch(pattern.buf + prefix, text.buf + prefix, - icase ? WM_CASEFOLD : 0); + WM_PATHNAME | (icase ? WM_CASEFOLD : 0)); if (!ret && !already_tried_absolute) { /* diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh index 635918505d..579a86b7f8 100755 --- a/t/t1305-config-include.sh +++ b/t/t1305-config-include.sh @@ -229,6 +229,19 @@ test_expect_success 'conditional include, early config reading' ' ) ' +test_expect_success 'conditional include with /**/' ' + REPO=foo/bar/repo && + git init $REPO && + cat >>$REPO/.git/config <<-\EOF && + [includeIf "gitdir:**/foo/**/bar/**"] + path=bar7 + EOF + echo "[test]seven=7" >$REPO/.git/bar7 && + echo 7 >expect && + git -C $REPO config test.seven >actual && + test_cmp expect actual +' + test_expect_success SYMLINKS 'conditional include, set up symlinked $HOME' ' mkdir real-home && ln -s real-home home && -- 2.21.0.479.g47ac719cd3 ``` ## Jason Karns, 2019-04-30 22:07 Subject: Re: [PATCH v3] config: correct '**' matching in includeIf patterns Message-ID: URL: https://gitlist.dev/e/CAKNmmv1Y48zfUZ0TvjDVdVngrMyA1YyNacdNbw7O05_E4FHsbQ%40mail.gmail.com In-Reply-To: <20190326094101.26294-1-pclouds@gmail.com> ``` On Tue, Mar 26, 2019 at 5:42 AM Nguyễn Thái Ngọc Duy wrote: > > The current wildmatch() call for includeIf's gitdir pattern does not > pass the WM_PATHNAME flag. Without this flag, '*' is treated _almost_ > the same as '**' (because '*' also matches slashes) with one exception: > > '/**/' can match a single slash. The pattern 'foo/**/bar' matches > 'foo/bar'. > > But '/*/', which is essentially what wildmatch engine sees without > WM_PATHNAME, has to match two slashes (and '*' matches nothing). Which > means 'foo/*/bar' cannot match 'foo/bar'. It can only match 'foo//bar'. > > The result of this is the current wildmatch() call works most of the > time until the user depends on '/**/' matching no path component. And > also '*' matches slashes while it should not, but people probably > haven't noticed this yet. The fix is straightforward. > > Reported-by: Jason Karns > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > v3 updates the test to avoid mkdir and cd, and break long lines. > > config.c | 2 +- > t/t1305-config-include.sh | 13 +++++++++++++ > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/config.c b/config.c > index 0f0cdd8c0f..c2846df3f1 100644 > --- a/config.c > +++ b/config.c > @@ -242,7 +242,7 @@ static int include_by_gitdir(const struct config_options *opts, > } > > ret = !wildmatch(pattern.buf + prefix, text.buf + prefix, > - icase ? WM_CASEFOLD : 0); > + WM_PATHNAME | (icase ? WM_CASEFOLD : 0)); > > if (!ret && !already_tried_absolute) { > /* > diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh > index 635918505d..579a86b7f8 100755 > --- a/t/t1305-config-include.sh > +++ b/t/t1305-config-include.sh > @@ -229,6 +229,19 @@ test_expect_success 'conditional include, early config reading' ' > ) > ' > > +test_expect_success 'conditional include with /**/' ' > + REPO=foo/bar/repo && > + git init $REPO && > + cat >>$REPO/.git/config <<-\EOF && > + [includeIf "gitdir:**/foo/**/bar/**"] > + path=bar7 > + EOF > + echo "[test]seven=7" >$REPO/.git/bar7 && > + echo 7 >expect && > + git -C $REPO config test.seven >actual && > + test_cmp expect actual > +' > + > test_expect_success SYMLINKS 'conditional include, set up symlinked $HOME' ' > mkdir real-home && > ln -s real-home home && > -- > 2.21.0.479.g47ac719cd3 > Thank you all for looking into this! (and your work on git!) It is very much appreciated. Jason ```