{"thread":{"id":"47321","subject":"Bug in pathspec handling (in conjunction with submodules)","startedAt":"2017-11-26T02:16:07Z","lastAt":"2017-12-06T21:21:09Z","messageCount":8,"participants":["Johannes Schindelin","Brandon Williams","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"333508","messageId":"alpine.DEB.2.21.1.1711260210060.6482@virtualbox","threadId":"47321","inReplyTo":null,"subject":"Bug in pathspec handling (in conjunction with submodules)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-26T02:15:56Z","receivedAt":"2017-11-26T02:16:07Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy & Brandon,\n\nin 74ed43711fd (grep: enable recurse-submodules to work on <tree> objects,\n2016-12-16), the do_match() function in tree-walk.c was changed so that it\ncan recurse across submodule boundaries.\n\nHowever, there is a bug, and I *think* there may be two bugs actually. Or\neven three.\n\nFirst of all, here is an MCVE that I distilled from\nhttps://github.com/git-for-windows/git/issues/1371:\n\n\tgit init repo\n\tcd repo\n\n\tgit init submodule\n\tgit -C submodule commit -m initial --allow-empty\n\n\ttouch \"[bracket]\"\n\tgit add \"[bracket]\"\n\tgit commit -m bracket\n\tgit add submodule\n\tgit commit -m submodule\n\n\tgit rev-list HEAD -- \"[bracket]\"\n\nNothing fancy, just adding a file with brackets in the name, then a\nsubmodule, then showing the commit history filtered by the funny file\nname.\n\nHowever, the log prints *both* commits. Clearly the submodule commit\nshould *not* be shown.\n\nNow, how does this all happen?\n\nSince the pathspec contains brackets, parse_pathspec() marks it as\ncontaining wildcards and sets nowildcard_len to 0.\n\nNow, note that [bracket] *is* a wildcard expression: it should only match\na single character that is one of  a, b, c, e, k, r or t.\n\nI think this is the first bug: `git rev-list` should not even match the\ncommit that adds the file [bracket] because its file name does not match\nthat expression. From where I sit, it would appear that f1a2ddbbc2d\n(tree_entry_interesting(): optimize wildcard matching when base is\nmatched, 2010-12-15) simply added the fnmatch() code without disabling the\nliteral match_entry() code when the pathspec contains a pattern.\n\nBut it does not stop there: there is *another* bug which causes the\npattern to somehow match the submodule. I *guess* the idea of\nhttps://github.com/git/git/commit/74ed43711#diff-7a08243175f2cae66aedf53f7dce3bdfR1015\nwas to allow a pattern like *.c to match files in a submodule, but the\npattern [bracket] should not match any file in submodule/. I think that\nthat code needs to be a little bit more careful to try to match the\nsubmodule's name against the pattern (it seems to interpret nowildcard_len\n== 0 to mean that the wildcard is `*`).\n\nHowever, the commit introducing that code wanted to teach *grep* (not\n*rev-list*) a new trick, and it relies on the `recursive` flag of the\npathspec to be set.\n\nAnd now it gets really interesting. Or confusing, depending on your mental\ncondition. This recursive flag of the pathspec is set in\nll_diff_tree_paths() (yep, changing the flag in the passed-in opt\nstructure... which I found a bit... unexpected, given the function name, I\nwould have been less surprised if that function only diff'ed the trees and\nused the options without changing the options). That flag-change was\nintroduced in\nhttps://github.com/git/git/commit/bc96cc87dbb2#diff-15203e8cd8ee9191113894de9d97a8a6R149\nwhich is another patch that changed the tree diff machinery to accommodate\n`git grep` (but maybe not really paying a lot of attention to the fact\nthat the same machinery is called repeatedly by the revision machinery,\ntoo).\n\nI am really confused by this code mainly due to the fact that the term\n\"recursive\" is pretty ambiguous in that context: does it refer to\ndirectories/tree objects, or to submodules? I guess it is used for both\nwhen there should be two flags so that rev-list can recurse over tree\nobjects but not submodules (unless told to do so).\n\nThe problem, of course, is that `git rev-list HEAD -- '[bracket]'` never\nrecurses into the submodule. And therefore, the promised \"more accurate\nmatching [...] in the submodule\" is never performed. And the commit adding\nthe submodule is never pruned.\n\nSince I am not really familiar with all that tree diff code (and as a\ngeneral rule to protect my mental health, I try my best to stay away from\nsubmodules, too), but you two are, may I ask you gentle people to have a\ncloser look to fix those bugs?\n\nThanks,\nDscho\n"},{"id":"333710","messageId":"20171128230624.GA194092@google.com","threadId":"47321","inReplyTo":"alpine.DEB.2.21.1.1711260210060.6482@virtualbox","subject":"Re: Bug in pathspec handling (in conjunction with submodules)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-11-28T23:06:24Z","receivedAt":"2017-11-28T23:06:33Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/26, Johannes Schindelin wrote:\n> Hi Duy & Brandon,\n> \n> in 74ed43711fd (grep: enable recurse-submodules to work on <tree> objects,\n> 2016-12-16), the do_match() function in tree-walk.c was changed so that it\n> can recurse across submodule boundaries.\n> \n> However, there is a bug, and I *think* there may be two bugs actually. Or\n> even three.\n> \n> First of all, here is an MCVE that I distilled from\n> https://github.com/git-for-windows/git/issues/1371:\n> \n> \tgit init repo\n> \tcd repo\n> \n> \tgit init submodule\n> \tgit -C submodule commit -m initial --allow-empty\n> \n> \ttouch \"[bracket]\"\n> \tgit add \"[bracket]\"\n> \tgit commit -m bracket\n> \tgit add submodule\n> \tgit commit -m submodule\n> \n> \tgit rev-list HEAD -- \"[bracket]\"\n> \n> Nothing fancy, just adding a file with brackets in the name, then a\n> submodule, then showing the commit history filtered by the funny file\n> name.\n> \n> However, the log prints *both* commits. Clearly the submodule commit\n> should *not* be shown.\n> \n> Now, how does this all happen?\n> \n> Since the pathspec contains brackets, parse_pathspec() marks it as\n> containing wildcards and sets nowildcard_len to 0.\n> \n> Now, note that [bracket] *is* a wildcard expression: it should only match\n> a single character that is one of  a, b, c, e, k, r or t.\n> \n> I think this is the first bug: `git rev-list` should not even match the\n> commit that adds the file [bracket] because its file name does not match\n> that expression. From where I sit, it would appear that f1a2ddbbc2d\n> (tree_entry_interesting(): optimize wildcard matching when base is\n> matched, 2010-12-15) simply added the fnmatch() code without disabling the\n> literal match_entry() code when the pathspec contains a pattern.\n\nI can see both sides to this, wanting to try matching literally first\nand then trying the wildcards, so I don't really have an opinion on\nhow/if we should fix that.\n\n> \n> But it does not stop there: there is *another* bug which causes the\n> pattern to somehow match the submodule. I *guess* the idea of\n> https://github.com/git/git/commit/74ed43711#diff-7a08243175f2cae66aedf53f7dce3bdfR1015\n> was to allow a pattern like *.c to match files in a submodule, but the\n> pattern [bracket] should not match any file in submodule/. I think that\n> that code needs to be a little bit more careful to try to match the\n> submodule's name against the pattern (it seems to interpret nowildcard_len\n> == 0 to mean that the wildcard is `*`).\n\nThis is a much bigger issue and I'm surprised it took this long to find\nthis bug.  And of course its due to one of my earlier contributions to\nthe project :)\n\n> \n> However, the commit introducing that code wanted to teach *grep* (not\n> *rev-list*) a new trick, and it relies on the `recursive` flag of the\n> pathspec to be set.\n\nThis is the root cause of the bug.  The added code to match against\nsubmodules was intended to allow for matching past submodule boundaries\nfor those commands (like grep) which are recursing submodules.  So\nreally there should be an additional flag which is passed in to trigger\nthis logic instead of relying on the recursive flag of the pathspec.  Or\nwe can add a recurse_submodules flag to the pathspec struct and respect\nthat flag instead of the 'recursive' flag.\n\nI have a quick patch to do just that which I'll send shortly.\n\n> \n> And now it gets really interesting. Or confusing, depending on your mental\n> condition. This recursive flag of the pathspec is set in\n> ll_diff_tree_paths() (yep, changing the flag in the passed-in opt\n> structure... which I found a bit... unexpected, given the function name, I\n> would have been less surprised if that function only diff'ed the trees and\n> used the options without changing the options). That flag-change was\n> introduced in\n> https://github.com/git/git/commit/bc96cc87dbb2#diff-15203e8cd8ee9191113894de9d97a8a6R149\n> which is another patch that changed the tree diff machinery to accommodate\n> `git grep` (but maybe not really paying a lot of attention to the fact\n> that the same machinery is called repeatedly by the revision machinery,\n> too).\n> \n> I am really confused by this code mainly due to the fact that the term\n> \"recursive\" is pretty ambiguous in that context: does it refer to\n> directories/tree objects, or to submodules? I guess it is used for both\n> when there should be two flags so that rev-list can recurse over tree\n> objects but not submodules (unless told to do so).\n> \n> The problem, of course, is that `git rev-list HEAD -- '[bracket]'` never\n> recurses into the submodule. And therefore, the promised \"more accurate\n> matching [...] in the submodule\" is never performed. And the commit adding\n> the submodule is never pruned.\n> \n> Since I am not really familiar with all that tree diff code (and as a\n> general rule to protect my mental health, I try my best to stay away from\n> submodules, too), but you two are, may I ask you gentle people to have a\n> closer look to fix those bugs?\n> \n> Thanks,\n> Dscho\n\n-- \nBrandon Williams\n"},{"id":"333711","messageId":"20171128232237.54453-1-bmwill@google.com","threadId":"47321","inReplyTo":"alpine.DEB.2.21.1.1711260210060.6482@virtualbox","subject":"[PATCH] pathspec: only match across submodule boundaries when requested","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-11-28T23:22:37Z","receivedAt":"2017-11-28T23:22:54Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Commit 74ed43711fd (grep: enable recurse-submodules to work on <tree>\nobjects, 2016-12-16) taught 'tree_entry_interesting()' to be able to\nmatch across submodule boundaries in the presence of wildcards.  This is\ndone by performing literal matching up to the first wildcard and then\npunting to the submodule itself to perform more accurate pattern\nmatching.  Instead of introducing a new flag to request this behavior,\ncommit 74ed43711fd overloaded the already existing 'recursive' flag in\n'struct pathspec' to request this behavior.\n\nThis leads to a bug where whenever any other caller has the 'recursive'\nflag set as well as a pathspec with wildcards that all submodules will\nbe indicated as matches.  One simple example of this is:\n\n\tgit init repo\n\tcd repo\n\n\tgit init submodule\n\tgit -C submodule commit -m initial --allow-empty\n\n\ttouch \"[bracket]\"\n\tgit add \"[bracket]\"\n\tgit commit -m bracket\n\tgit add submodule\n\tgit commit -m submodule\n\n\tgit rev-list HEAD -- \"[bracket]\"\n\nFix this by introducing the new flag 'recurse_submodules' in 'struct\npathspec' and using this flag to determine if matches should be allowed\nto cross submodule boundaries.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n builtin/grep.c                |  1 +\n pathspec.h                    |  1 +\n t/t4208-log-magic-pathspec.sh | 17 +++++++++++++++++\n tree-walk.c                   |  5 +++--\n 4 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 5a6cfe6b4..3ca4ac80d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1015,6 +1015,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t       prefix, argv + i);\n \tpathspec.max_depth = opt.max_depth;\n \tpathspec.recursive = 1;\n+\tpathspec.recurse_submodules = !!recurse_submodules;\n \n #ifndef NO_PTHREADS\n \tif (list.nr || cached || show_in_pager)\ndiff --git a/pathspec.h b/pathspec.h\nindex 6420d1080..099a170c2 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -24,6 +24,7 @@ struct pathspec {\n \tint nr;\n \tunsigned int has_wildcard:1;\n \tunsigned int recursive:1;\n+\tunsigned int recurse_submodules:1;\n \tunsigned magic;\n \tint max_depth;\n \tstruct pathspec_item {\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex 935df6a65..bd583af0e 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -93,4 +93,21 @@ test_expect_success 'command line pathspec parsing for \"git log\"' '\n \tgit log --merge -- a\n '\n \n+test_expect_success 'tree_entry_interesting does not match past submodule boundaries' '\n+\ttest_when_finished \"rm -rf repo submodule\" &&\n+\tgit init submodule &&\n+\ttest_commit -C submodule initial &&\n+\tgit init repo &&\n+\t>\"repo/[bracket]\" &&\n+\tgit -C repo add \"[bracket]\" &&\n+\tgit -C repo commit -m bracket &&\n+\tgit -C repo rev-list HEAD -- \"[bracket]\" >expect &&\n+\n+\tgit -C repo submodule add ../submodule &&\n+\tgit -C repo commit -m submodule &&\n+\n+\tgit -C repo rev-list HEAD -- \"[bracket]\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 684f0e337..63a87ed66 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -1011,7 +1011,8 @@ static enum interesting do_match(const struct name_entry *entry,\n \t\t\t\t * character.  More accurate matching can then\n \t\t\t\t * be performed in the submodule itself.\n \t\t\t\t */\n-\t\t\t\tif (ps->recursive && S_ISGITLINK(entry->mode) &&\n+\t\t\t\tif (ps->recurse_submodules &&\n+\t\t\t\t    S_ISGITLINK(entry->mode) &&\n \t\t\t\t    !ps_strncmp(item, match + baselen,\n \t\t\t\t\t\tentry->path,\n \t\t\t\t\t\titem->nowildcard_len - baselen))\n@@ -1060,7 +1061,7 @@ static enum interesting do_match(const struct name_entry *entry,\n \t\t * character.  More accurate matching can then\n \t\t * be performed in the submodule itself.\n \t\t */\n-\t\tif (ps->recursive && S_ISGITLINK(entry->mode) &&\n+\t\tif (ps->recurse_submodules && S_ISGITLINK(entry->mode) &&\n \t\t    !ps_strncmp(item, match, base->buf + base_offset,\n \t\t\t\titem->nowildcard_len)) {\n \t\t\tstrbuf_setlen(base, base_offset + baselen);\n-- \n2.15.0.531.g2ccb3012c9-goog\n\n"},{"id":"333813","messageId":"alpine.DEB.2.21.1.1711292221060.6482@virtualbox","threadId":"47321","inReplyTo":"20171128232237.54453-1-bmwill@google.com","subject":"Re: [PATCH] pathspec: only match across submodule boundaries when requested","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-29T21:29:01Z","receivedAt":"2017-11-29T21:29:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Tue, 28 Nov 2017, Brandon Williams wrote:\n\n> Commit 74ed43711fd (grep: enable recurse-submodules to work on <tree>\n> objects, 2016-12-16) taught 'tree_entry_interesting()' to be able to\n> match across submodule boundaries in the presence of wildcards.  This is\n> done by performing literal matching up to the first wildcard and then\n> punting to the submodule itself to perform more accurate pattern\n> matching.  Instead of introducing a new flag to request this behavior,\n> commit 74ed43711fd overloaded the already existing 'recursive' flag in\n> 'struct pathspec' to request this behavior.\n> \n> This leads to a bug where whenever any other caller has the 'recursive'\n> flag set as well as a pathspec with wildcards that all submodules will\n> be indicated as matches.  One simple example of this is:\n> \n> \tgit init repo\n> \tcd repo\n> \n> \tgit init submodule\n> \tgit -C submodule commit -m initial --allow-empty\n> \n> \ttouch \"[bracket]\"\n> \tgit add \"[bracket]\"\n> \tgit commit -m bracket\n> \tgit add submodule\n> \tgit commit -m submodule\n> \n> \tgit rev-list HEAD -- \"[bracket]\"\n> \n> Fix this by introducing the new flag 'recurse_submodules' in 'struct\n> pathspec' and using this flag to determine if matches should be allowed\n> to cross submodule boundaries.\n> \n> Signed-off-by: Brandon Williams <bmwill@google.com>\n\nCould you also add something like\n\n\tThis fixes https://github.com/git-for-windows/git/issues/1371\n\nat the end of the commit message, to keep a reference to the original bug\nreport?\n\n>  4 files changed, 22 insertions(+), 2 deletions(-)\n\nPhew. That was much smaller than I expected.\n\n> +test_expect_success 'tree_entry_interesting does not match past submodule boundaries' '\n> +\ttest_when_finished \"rm -rf repo submodule\" &&\n> +\tgit init submodule &&\n> +\ttest_commit -C submodule initial &&\n> +\tgit init repo &&\n> +\t>\"repo/[bracket]\" &&\n> +\tgit -C repo add \"[bracket]\" &&\n> +\tgit -C repo commit -m bracket &&\n> +\tgit -C repo rev-list HEAD -- \"[bracket]\" >expect &&\n> +\n> +\tgit -C repo submodule add ../submodule &&\n> +\tgit -C repo commit -m submodule &&\n> +\n> +\tgit -C repo rev-list HEAD -- \"[bracket]\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nNicely prepared for a new hash function, too (no explicit SHA-1).\n\nI wonder, however, why we can't `git checkout -b bracket` and\n`test_when_finished \"git checkout master\"` and void those many `-C repo`\noptions. But then, it is actually one of the shorter test cases, and\npretty easy to understand.\n\nHowever, I would still like to see `test_tick`s before those `git commit`\ncalls, to make the commit names reproducible.\n\nThanks,\nDscho\n"},{"id":"334127","messageId":"20171205000734.69530-1-bmwill@google.com","threadId":"47321","inReplyTo":"20171128232237.54453-1-bmwill@google.com","subject":"[PATCH v2] pathspec: only match across submodule boundaries when requested","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-05T00:07:34Z","receivedAt":"2017-12-05T00:07:50Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Commit 74ed43711fd (grep: enable recurse-submodules to work on <tree>\nobjects, 2016-12-16) taught 'tree_entry_interesting()' to be able to\nmatch across submodule boundaries in the presence of wildcards.  This is\ndone by performing literal matching up to the first wildcard and then\npunting to the submodule itself to perform more accurate pattern\nmatching.  Instead of introducing a new flag to request this behavior,\ncommit 74ed43711fd overloaded the already existing 'recursive' flag in\n'struct pathspec' to request this behavior.\n\nThis leads to a bug where whenever any other caller has the 'recursive'\nflag set as well as a pathspec with wildcards that all submodules will\nbe indicated as matches.  One simple example of this is:\n\n\tgit init repo\n\tcd repo\n\n\tgit init submodule\n\tgit -C submodule commit -m initial --allow-empty\n\n\ttouch \"[bracket]\"\n\tgit add \"[bracket]\"\n\tgit commit -m bracket\n\tgit add submodule\n\tgit commit -m submodule\n\n\tgit rev-list HEAD -- \"[bracket]\"\n\nFix this by introducing the new flag 'recurse_submodules' in 'struct\npathspec' and using this flag to determine if matches should be allowed\nto cross submodule boundaries.\n\nThis fixes https://github.com/git-for-windows/git/issues/1371.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n builtin/grep.c                |  1 +\n pathspec.h                    |  1 +\n t/t4208-log-magic-pathspec.sh | 19 +++++++++++++++++++\n tree-walk.c                   |  5 +++--\n 4 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 5a6cfe6b4..3ca4ac80d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1015,6 +1015,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t       prefix, argv + i);\n \tpathspec.max_depth = opt.max_depth;\n \tpathspec.recursive = 1;\n+\tpathspec.recurse_submodules = !!recurse_submodules;\n \n #ifndef NO_PTHREADS\n \tif (list.nr || cached || show_in_pager)\ndiff --git a/pathspec.h b/pathspec.h\nindex 6420d1080..099a170c2 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -24,6 +24,7 @@ struct pathspec {\n \tint nr;\n \tunsigned int has_wildcard:1;\n \tunsigned int recursive:1;\n+\tunsigned int recurse_submodules:1;\n \tunsigned magic;\n \tint max_depth;\n \tstruct pathspec_item {\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex 935df6a65..a1705f70c 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -93,4 +93,23 @@ test_expect_success 'command line pathspec parsing for \"git log\"' '\n \tgit log --merge -- a\n '\n \n+test_expect_success 'tree_entry_interesting does not match past submodule boundaries' '\n+\ttest_when_finished \"rm -rf repo submodule\" &&\n+\tgit init submodule &&\n+\ttest_commit -C submodule initial &&\n+\tgit init repo &&\n+\t>\"repo/[bracket]\" &&\n+\tgit -C repo add \"[bracket]\" &&\n+\ttest_tick &&\n+\tgit -C repo commit -m bracket &&\n+\tgit -C repo rev-list HEAD -- \"[bracket]\" >expect &&\n+\n+\tgit -C repo submodule add ../submodule &&\n+\ttest_tick &&\n+\tgit -C repo commit -m submodule &&\n+\n+\tgit -C repo rev-list HEAD -- \"[bracket]\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 684f0e337..63a87ed66 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -1011,7 +1011,8 @@ static enum interesting do_match(const struct name_entry *entry,\n \t\t\t\t * character.  More accurate matching can then\n \t\t\t\t * be performed in the submodule itself.\n \t\t\t\t */\n-\t\t\t\tif (ps->recursive && S_ISGITLINK(entry->mode) &&\n+\t\t\t\tif (ps->recurse_submodules &&\n+\t\t\t\t    S_ISGITLINK(entry->mode) &&\n \t\t\t\t    !ps_strncmp(item, match + baselen,\n \t\t\t\t\t\tentry->path,\n \t\t\t\t\t\titem->nowildcard_len - baselen))\n@@ -1060,7 +1061,7 @@ static enum interesting do_match(const struct name_entry *entry,\n \t\t * character.  More accurate matching can then\n \t\t * be performed in the submodule itself.\n \t\t */\n-\t\tif (ps->recursive && S_ISGITLINK(entry->mode) &&\n+\t\tif (ps->recurse_submodules && S_ISGITLINK(entry->mode) &&\n \t\t    !ps_strncmp(item, match, base->buf + base_offset,\n \t\t\t\titem->nowildcard_len)) {\n \t\t\tstrbuf_setlen(base, base_offset + baselen);\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334128","messageId":"20171205000901.GB52452@google.com","threadId":"47321","inReplyTo":"alpine.DEB.2.21.1.1711292221060.6482@virtualbox","subject":"Re: [PATCH] pathspec: only match across submodule boundaries when requested","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-05T00:09:01Z","receivedAt":"2017-12-05T00:09:09Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/29, Johannes Schindelin wrote:\n> Hi Brandon,\n> \n> On Tue, 28 Nov 2017, Brandon Williams wrote:\n> \n> > Commit 74ed43711fd (grep: enable recurse-submodules to work on <tree>\n> > objects, 2016-12-16) taught 'tree_entry_interesting()' to be able to\n> > match across submodule boundaries in the presence of wildcards.  This is\n> > done by performing literal matching up to the first wildcard and then\n> > punting to the submodule itself to perform more accurate pattern\n> > matching.  Instead of introducing a new flag to request this behavior,\n> > commit 74ed43711fd overloaded the already existing 'recursive' flag in\n> > 'struct pathspec' to request this behavior.\n> > \n> > This leads to a bug where whenever any other caller has the 'recursive'\n> > flag set as well as a pathspec with wildcards that all submodules will\n> > be indicated as matches.  One simple example of this is:\n> > \n> > \tgit init repo\n> > \tcd repo\n> > \n> > \tgit init submodule\n> > \tgit -C submodule commit -m initial --allow-empty\n> > \n> > \ttouch \"[bracket]\"\n> > \tgit add \"[bracket]\"\n> > \tgit commit -m bracket\n> > \tgit add submodule\n> > \tgit commit -m submodule\n> > \n> > \tgit rev-list HEAD -- \"[bracket]\"\n> > \n> > Fix this by introducing the new flag 'recurse_submodules' in 'struct\n> > pathspec' and using this flag to determine if matches should be allowed\n> > to cross submodule boundaries.\n> > \n> > Signed-off-by: Brandon Williams <bmwill@google.com>\n> \n> Could you also add something like\n> \n> \tThis fixes https://github.com/git-for-windows/git/issues/1371\n> \n> at the end of the commit message, to keep a reference to the original bug\n> report?\n\nYep! I can do that.\n\n> \n> >  4 files changed, 22 insertions(+), 2 deletions(-)\n> \n> Phew. That was much smaller than I expected.\n> \n> > +test_expect_success 'tree_entry_interesting does not match past submodule boundaries' '\n> > +\ttest_when_finished \"rm -rf repo submodule\" &&\n> > +\tgit init submodule &&\n> > +\ttest_commit -C submodule initial &&\n> > +\tgit init repo &&\n> > +\t>\"repo/[bracket]\" &&\n> > +\tgit -C repo add \"[bracket]\" &&\n> > +\tgit -C repo commit -m bracket &&\n> > +\tgit -C repo rev-list HEAD -- \"[bracket]\" >expect &&\n> > +\n> > +\tgit -C repo submodule add ../submodule &&\n> > +\tgit -C repo commit -m submodule &&\n> > +\n> > +\tgit -C repo rev-list HEAD -- \"[bracket]\" >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> Nicely prepared for a new hash function, too (no explicit SHA-1).\n> \n> I wonder, however, why we can't `git checkout -b bracket` and\n> `test_when_finished \"git checkout master\"` and void those many `-C repo`\n> options. But then, it is actually one of the shorter test cases, and\n> pretty easy to understand.\n> \n> However, I would still like to see `test_tick`s before those `git commit`\n> calls, to make the commit names reproducible.\n\nIn v2 I added the calls to test_tick.  I've never used the function\nmyself so hopefully I used it correctly! :)\n\n> \n> Thanks,\n> Dscho\n\n-- \nBrandon Williams\n"},{"id":"334210","messageId":"xmqqo9ndhtjr.fsf@gitster.mtv.corp.google.com","threadId":"47321","inReplyTo":"20171205000734.69530-1-bmwill@google.com","subject":"Re: [PATCH v2] pathspec: only match across submodule boundaries when requested","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-05T19:19:04Z","receivedAt":"2017-12-05T19:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> Commit 74ed43711fd (grep: enable recurse-submodules to work on <tree>\n> objects, 2016-12-16) taught 'tree_entry_interesting()' to be able to\n> match across submodule boundaries in the presence of wildcards.  This is\n> done by performing literal matching up to the first wildcard and then\n> punting to the submodule itself to perform more accurate pattern\n> matching.  Instead of introducing a new flag to request this behavior,\n> commit 74ed43711fd overloaded the already existing 'recursive' flag in\n> 'struct pathspec' to request this behavior.\n>\n> This leads to a bug where whenever any other caller has the 'recursive'\n> flag set as well as a pathspec with wildcards that all submodules will\n> be indicated as matches.  One simple example of this is:\n>\n> \tgit init repo\n> \tcd repo\n>\n> \tgit init submodule\n> \tgit -C submodule commit -m initial --allow-empty\n>\n> \ttouch \"[bracket]\"\n> \tgit add \"[bracket]\"\n> \tgit commit -m bracket\n> \tgit add submodule\n> \tgit commit -m submodule\n>\n> \tgit rev-list HEAD -- \"[bracket]\"\n>\n> Fix this by introducing the new flag 'recurse_submodules' in 'struct\n> pathspec' and using this flag to determine if matches should be allowed\n> to cross submodule boundaries.\n\nMakes sense.\n\nI initially misread the title\n\n    \"pathspec: only match across submodule boundaries when requested\"\n\nto be saying that the match happens only at the boundary, but that\n\"only\" is not about where the match happens.\n\n    \"pathspec: match across submodule boundaries only when requested\"\n\nwould have avoided such a confusion.\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 5a6cfe6b4..3ca4ac80d 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1015,6 +1015,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t\t       prefix, argv + i);\n>  \tpathspec.max_depth = opt.max_depth;\n>  \tpathspec.recursive = 1;\n> +\tpathspec.recurse_submodules = !!recurse_submodules;\n\nWith the current code, recurse_submodules can only be 0 or 1 (the\nonly assignment is from the return value of git_config_bool()), so\nthe force-boolean !! is not strictly needed, but it may be a good\nfuture-proofing measure.\n\nWill queue; thanks.\n"},{"id":"334272","messageId":"alpine.DEB.2.21.1.1712062220380.98586@virtualbox","threadId":"47321","inReplyTo":"20171205000734.69530-1-bmwill@google.com","subject":"Re: [PATCH v2] pathspec: only match across submodule boundaries when requested","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-06T21:20:51Z","receivedAt":"2017-12-06T21:21:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Mon, 4 Dec 2017, Brandon Williams wrote:\n\n> Commit 74ed43711fd (grep: enable recurse-submodules to work on <tree>\n> objects, 2016-12-16) taught 'tree_entry_interesting()' to be able to\n> match across submodule boundaries in the presence of wildcards.  This is\n> done by performing literal matching up to the first wildcard and then\n> punting to the submodule itself to perform more accurate pattern\n> matching.  Instead of introducing a new flag to request this behavior,\n> commit 74ed43711fd overloaded the already existing 'recursive' flag in\n> 'struct pathspec' to request this behavior.\n> \n> This leads to a bug where whenever any other caller has the 'recursive'\n> flag set as well as a pathspec with wildcards that all submodules will\n> be indicated as matches.  One simple example of this is:\n> \n> \tgit init repo\n> \tcd repo\n> \n> \tgit init submodule\n> \tgit -C submodule commit -m initial --allow-empty\n> \n> \ttouch \"[bracket]\"\n> \tgit add \"[bracket]\"\n> \tgit commit -m bracket\n> \tgit add submodule\n> \tgit commit -m submodule\n> \n> \tgit rev-list HEAD -- \"[bracket]\"\n> \n> Fix this by introducing the new flag 'recurse_submodules' in 'struct\n> pathspec' and using this flag to determine if matches should be allowed\n> to cross submodule boundaries.\n> \n> This fixes https://github.com/git-for-windows/git/issues/1371.\n\nThanks,\nDscho\n"}]}