* [BUG] Suspected with double asterisk in conditional include pattern @ 2019-03-14 17:30 Jason Karns 2019-03-22 19:04 ` Taylor Blau 2019-03-23 3:45 ` [PATCH] config: correct '**' matching in includeIf patterns Nguyễn Thái Ngọc Duy 0 siblings, 2 replies; 9+ messages in thread From: Jason Karns @ 2019-03-14 17:30 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [BUG] Suspected with double asterisk in conditional include pattern 2019-03-14 17:30 [BUG] Suspected with double asterisk in conditional include pattern Jason Karns @ 2019-03-22 19:04 ` Taylor Blau 2019-03-23 3:45 ` [PATCH] config: correct '**' matching in includeIf patterns Nguyễn Thái Ngọc Duy 1 sibling, 0 replies; 9+ messages in thread From: Taylor Blau @ 2019-03-22 19:04 UTC (permalink / raw) To: Jason Karns; +Cc: git 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] config: correct '**' matching in includeIf patterns 2019-03-14 17:30 [BUG] Suspected with double asterisk in conditional include pattern Jason Karns 2019-03-22 19:04 ` Taylor Blau @ 2019-03-23 3:45 ` Nguyễn Thái Ngọc Duy 2019-03-24 12:56 ` Junio C Hamano 2019-03-24 13:17 ` [PATCH v2] " Nguyễn Thái Ngọc Duy 1 sibling, 2 replies; 9+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-03-23 3:45 UTC (permalink / raw) To: jason.karns Cc: git, Taylor Blau, Junio C Hamano, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] config: correct '**' matching in includeIf patterns 2019-03-23 3:45 ` [PATCH] config: correct '**' matching in includeIf patterns Nguyễn Thái Ngọc Duy @ 2019-03-24 12:56 ` Junio C Hamano 2019-03-24 13:17 ` [PATCH v2] " Nguyễn Thái Ngọc Duy 1 sibling, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2019-03-24 12:56 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: jason.karns, git, Taylor Blau 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) { > /* ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] config: correct '**' matching in includeIf patterns 2019-03-23 3:45 ` [PATCH] config: correct '**' matching in includeIf patterns Nguyễn Thái Ngọc Duy 2019-03-24 12:56 ` Junio C Hamano @ 2019-03-24 13:17 ` Nguyễn Thái Ngọc Duy 2019-03-25 2:30 ` Junio C Hamano ` (2 more replies) 1 sibling, 3 replies; 9+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-03-24 13:17 UTC (permalink / raw) To: pclouds; +Cc: git, gitster, jason.karns, me 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] config: correct '**' matching in includeIf patterns 2019-03-24 13:17 ` [PATCH v2] " Nguyễn Thái Ngọc Duy @ 2019-03-25 2:30 ` Junio C Hamano 2019-03-25 21:40 ` Johannes Schindelin 2019-03-26 9:41 ` [PATCH v3] " Nguyễn Thái Ngọc Duy 2 siblings, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2019-03-25 2:30 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, jason.karns, me 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] config: correct '**' matching in includeIf patterns 2019-03-24 13:17 ` [PATCH v2] " Nguyễn Thái Ngọc Duy 2019-03-25 2:30 ` Junio C Hamano @ 2019-03-25 21:40 ` Johannes Schindelin 2019-03-26 9:41 ` [PATCH v3] " Nguyễn Thái Ngọc Duy 2 siblings, 0 replies; 9+ messages in thread From: Johannes Schindelin @ 2019-03-25 21:40 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, gitster, jason.karns, me [-- 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] config: correct '**' matching in includeIf patterns 2019-03-24 13:17 ` [PATCH v2] " Nguyễn Thái Ngọc Duy 2019-03-25 2:30 ` Junio C Hamano 2019-03-25 21:40 ` Johannes Schindelin @ 2019-03-26 9:41 ` Nguyễn Thái Ngọc Duy 2019-04-30 22:07 ` Jason Karns 2 siblings, 1 reply; 9+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-03-26 9:41 UTC (permalink / raw) To: pclouds; +Cc: git, gitster, jason.karns, me, johannes.schindelin 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] config: correct '**' matching in includeIf patterns 2019-03-26 9:41 ` [PATCH v3] " Nguyễn Thái Ngọc Duy @ 2019-04-30 22:07 ` Jason Karns 0 siblings, 0 replies; 9+ messages in thread From: Jason Karns @ 2019-04-30 22:07 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: git, gitster, me, johannes.schindelin 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-04-30 22:07 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-14 17:30 [BUG] Suspected with double asterisk in conditional include pattern Jason Karns 2019-03-22 19:04 ` Taylor Blau 2019-03-23 3:45 ` [PATCH] config: correct '**' matching in includeIf patterns Nguyễn Thái Ngọc Duy 2019-03-24 12:56 ` Junio C Hamano 2019-03-24 13:17 ` [PATCH v2] " Nguyễn Thái Ngọc Duy 2019-03-25 2:30 ` Junio C Hamano 2019-03-25 21:40 ` Johannes Schindelin 2019-03-26 9:41 ` [PATCH v3] " Nguyễn Thái Ngọc Duy 2019-04-30 22:07 ` Jason Karns
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).