[BUG] Suspected with double asterisk in conditional include pattern

[BUG] Suspected with double asterisk in conditional include pattern

[Jason Karns]

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

Re: [BUG] Suspected with double asterisk in conditional include pattern

[Taylor Blau]

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

[PATCH] config: correct '**' matching in includeIf patterns

[Nguyễn Thái Ngọc Duy]

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 <jason.karns@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH] config: correct '**' matching in includeIf patterns

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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 <jason.karns@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  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) {
>  		/*

[PATCH v2] config: correct '**' matching in includeIf patterns

[Nguyễn Thái Ngọc Duy]

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 <jason.karns@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH v2] config: correct '**' matching in includeIf patterns

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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.

Re: [PATCH v2] config: correct '**' matching in includeIf patterns

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

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=<N>` 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
>
>

[PATCH v3] config: correct '**' matching in includeIf patterns

[Nguyễn Thái Ngọc Duy]

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 <jason.karns@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH v3] config: correct '**' matching in includeIf patterns

[Jason Karns]

On Tue, Mar 26, 2019 at 5:42 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <jason.karns@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  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