[BUG] Suspected with double asterisk in conditional include pattern

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

Hi Jason,

On Thu, Mar 14, 2019 at 01:30:43PM -0400, Jason Karns wrote:

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

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(-)

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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?

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(-)

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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.

Hi Duy,

On Sun, 24 Mar 2019, Nguyễn Thái Ngọc Duy wrote:

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...

... just drop the `mkdir foo/bar`, as `git init foo/bar/repo` has not the slightest problem creating the intermediate directories.

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

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(-)

On Tue, Mar 26, 2019 at 5:42 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Thank you all for looking into this! (and your work on git!) It is very much appreciated.

Jason