{"thread":{"id":"51444","subject":"Weird behavior with git grep --recurse-submodules","startedAt":"2019-07-08T08:22:21Z","lastAt":"2019-08-03T23:39:44Z","messageCount":14,"participants":["Daniel Zaoui","Matheus Tavares Bernardino","Johannes Schindelin","Matheus Tavares","Junio C Hamano","Christian Couder","Brandon Williams"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"378672","messageId":"20190708111459.135abe50@zen","threadId":"51444","inReplyTo":null,"subject":"Weird behavior with git grep --recurse-submodules","fromName":"Daniel Zaoui","fromEmail":"jackdanielz@eyomi.org","sentAt":"2019-07-08T08:14:59Z","receivedAt":"2019-07-08T08:22:21Z","isPatch":false,"sender":{"key":"jackdanielz@eyomi.org","avatar":null},"body":"Hi guys,\n\nI work with submodules and use git grep a lot.\n\nI noted that when it is invoked used with --recurse-submodules, the result is not as expected for the submodules. I get submodules results as if no files were modified (like --cached option) although I would expect results taking into account the modifications.\n\nExpected behavior:\ngit grep --recurse-submodules string:\n- git grep string // search into main repo\n- for each submodule, git grep string // search into submodule\n\nActual behavior:\ngit grep --recurse-submodules string:\n- git grep string // search into main repo\n- for each submodule, git grep --cached string // search into submodule\n\nDo you get the same behavior? Am I doing something wrong? Was I understandable :-)? Is it a bug?\n\ngit --version: git version 2.22.0\nuname -a: Linux daniel 5.1.15-arch1-1-ARCH #1 SMP PREEMPT Tue Jun 25 04:49:39 UTC 2019 x86_64 GNU/Linux\n\nThanks\nDaniel\n"},{"id":"378754","messageId":"CAHd-oW5Q1PWDzWKpsmdv4ecUMrQTj4Vyh_SoVu6vaFeSFmxE5g@mail.gmail.com","threadId":"51444","inReplyTo":"20190708111459.135abe50@zen","subject":"Re: Weird behavior with git grep --recurse-submodules","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-07-10T06:43:02Z","receivedAt":"2019-07-10T06:43:16Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Jul 8, 2019 at 5:22 AM Daniel Zaoui wrote:\n>\n> Hi guys,\n\nHi, Daniel\n\n> I work with submodules and use git grep a lot.\n>\n> I noted that when it is invoked used with --recurse-submodules, the result is not as expected for the submodules. I get submodules results as if no files were modified (like --cached option) although I would expect results taking into account the modifications.\n>\n> Expected behavior:\n> git grep --recurse-submodules string:\n> - git grep string // search into main repo\n> - for each submodule, git grep string // search into submodule\n>\n> Actual behavior:\n> git grep --recurse-submodules string:\n> - git grep string // search into main repo\n> - for each submodule, git grep --cached string // search into submodule\n>\n> Do you get the same behavior? Am I doing something wrong? Was I understandable :-)? Is it a bug?\n\nIt seems git-grep was taking into account the worktree modifications\nin submodules before f9ee2fc (\"grep: recurse in-process using 'struct\nrepository'\", 02-08-2017). I'm not sure, thought, if this behavior\nchange was a bug during the conversion or a project decision.\n\nCC-ing Brandon, in case he has other inputs\n\n> git --version: git version 2.22.0\n> uname -a: Linux daniel 5.1.15-arch1-1-ARCH #1 SMP PREEMPT Tue Jun 25 04:49:39 UTC 2019 x86_64 GNU/Linux\n>\n> Thanks\n> Daniel\n\nBest,\nMatheus\n"},{"id":"378760","messageId":"nycvar.QRO.7.76.6.1907101313450.46@tvgsbejvaqbjf.bet","threadId":"51444","inReplyTo":"CAHd-oW5Q1PWDzWKpsmdv4ecUMrQTj4Vyh_SoVu6vaFeSFmxE5g@mail.gmail.com","subject":"Re: Weird behavior with git grep --recurse-submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-10T11:14:18Z","receivedAt":"2019-07-10T11:14:41Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"[cC:ing Brandon via his current email address, as per .mailmap]\n\n\nOn Wed, 10 Jul 2019, Matheus Tavares Bernardino wrote:\n\n> On Mon, Jul 8, 2019 at 5:22 AM Daniel Zaoui wrote:\n> >\n> > Hi guys,\n>\n> Hi, Daniel\n>\n> > I work with submodules and use git grep a lot.\n> >\n> > I noted that when it is invoked used with --recurse-submodules, the result is not as expected for the submodules. I get submodules results as if no files were modified (like --cached option) although I would expect results taking into account the modifications.\n> >\n> > Expected behavior:\n> > git grep --recurse-submodules string:\n> > - git grep string // search into main repo\n> > - for each submodule, git grep string // search into submodule\n> >\n> > Actual behavior:\n> > git grep --recurse-submodules string:\n> > - git grep string // search into main repo\n> > - for each submodule, git grep --cached string // search into submodule\n> >\n> > Do you get the same behavior? Am I doing something wrong? Was I understandable :-)? Is it a bug?\n>\n> It seems git-grep was taking into account the worktree modifications\n> in submodules before f9ee2fc (\"grep: recurse in-process using 'struct\n> repository'\", 02-08-2017). I'm not sure, thought, if this behavior\n> change was a bug during the conversion or a project decision.\n>\n> CC-ing Brandon, in case he has other inputs\n>\n> > git --version: git version 2.22.0\n> > uname -a: Linux daniel 5.1.15-arch1-1-ARCH #1 SMP PREEMPT Tue Jun 25 04:49:39 UTC 2019 x86_64 GNU/Linux\n> >\n> > Thanks\n> > Daniel\n>\n> Best,\n> Matheus\n>\n"},{"id":"378983","messageId":"20190716211012.226b7059@zen","threadId":"51444","inReplyTo":"nycvar.QRO.7.76.6.1907101313450.46@tvgsbejvaqbjf.bet","subject":"Re: Weird behavior with git grep --recurse-submodules","fromName":"Daniel Zaoui","fromEmail":"jackdanielz@eyomi.org","sentAt":"2019-07-16T18:10:12Z","receivedAt":"2019-07-16T18:10:01Z","isPatch":false,"sender":{"key":"jackdanielz@eyomi.org","avatar":null},"body":"Hi Matheus,\n\nThank you for your response.\n\nI really hope the change Brandon made is not a project decision. At least, it does seem to me like a bug.\n\nHow do you recommend me to solve this issue? Is there some place where I can check if some bug ticket has been created on this matter? I didn't find anything in the mailing list archives about this.\n\nBR\nDaniel\n\nOn Wed, 10 Jul 2019 13:14:18 +0200 (CEST)\nJohannes Schindelin wrote:\n\n> [cC:ing Brandon via his current email address, as per .mailmap]\n> \n> \n> On Wed, 10 Jul 2019, Matheus Tavares Bernardino wrote:\n> \n> [...] \n> [...] \n> [...] \n> [...] \n> [...] \n> [...] \n> [...] \n\n"},{"id":"379548","messageId":"CAHd-oW61RFCySF+gUj8iYuV6afEoD0RD9oYE+N6rYd7rv3J2nA@mail.gmail.com","threadId":"51444","inReplyTo":"20190716211012.226b7059@zen","subject":"Re: Weird behavior with git grep --recurse-submodules","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-07-29T20:27:23Z","receivedAt":"2019-07-29T20:27:37Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Jul 16, 2019 at 3:09 PM Daniel Zaoui wrote:\n>\n> Hi Matheus,\n\nHi, Daniel\n\nI'm sorry, your last message went to my spam folder for some reason :(\n\n> Thank you for your response.\n>\n> I really hope the change Brandon made is not a project decision. At least, it does seem to me like a bug.\n>\n> How do you recommend me to solve this issue? Is there some place where I can check if some bug ticket has been created on this matter? I didn't find anything in the mailing list archives about this.\n\nI think I manage to solve the bug with this[1] patch. I'm going to ask\nmy GSoC mentors to take a look and then I'll send it to the mailing\nlist :) Thanks for reporting it.\n\nBest,\nMatheus\n\n[1]: https://github.com/matheustavares/git/commit/37aeeb3ab86b5dfebfaafeb98d34e379341a529d\n"},{"id":"379583","messageId":"ba3d8a953a2cc5b4ff03fefa434ffd7bd6a78f15.1564505605.git.matheus.bernardino@usp.br","threadId":"51444","inReplyTo":"CAHd-oW61RFCySF+gUj8iYuV6afEoD0RD9oYE+N6rYd7rv3J2nA@mail.gmail.com","subject":"[GSoC][PATCH] grep: fix worktree case in submodules","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-07-30T16:53:27Z","receivedAt":"2019-07-30T16:53:41Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Running git-grep with --recurse-submodules results in a cached grep for\nthe submodules even when --cached is not used. This makes all\nmodifications in submodules' tracked files be always ignored when\ngrepping. Solve that making git-grep respect the cached option when\ninvoking grep_cache() inside grep_submodule(). Also, add tests to\nensure that the desired behavior is performed.\n\nReported-by: Daniel Zaoui \nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 10 ++++++----\n t/t7814-grep-recurse-submodules.sh | 21 +++++++++++++++++++++\n 2 files changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 560051784e..2699001fbd 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -403,7 +403,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n static int grep_submodule(struct grep_opt *opt,\n \t\t\t const struct pathspec *pathspec,\n \t\t\t const struct object_id *oid,\n-\t\t\t const char *filename, const char *path)\n+\t\t\t const char *filename, const char *path, int cached)\n {\n \tstruct repository subrepo;\n \tstruct repository *superproject = opt->repo;\n@@ -475,7 +475,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tstrbuf_release(&base);\n \t\tfree(data);\n \t} else {\n-\t\thit = grep_cache(&subopt, pathspec, 1);\n+\t\thit = grep_cache(&subopt, pathspec, cached);\n \t}\n \n \trepo_clear(&subrepo);\n@@ -523,7 +523,8 @@ static int grep_cache(struct grep_opt *opt,\n \t\t\t}\n \t\t} else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&\n \t\t\t submodule_path_match(repo->index, pathspec, name.buf, NULL)) {\n-\t\t\thit |= grep_submodule(opt, pathspec, NULL, ce->name, ce->name);\n+\t\t\thit |= grep_submodule(opt, pathspec, NULL, ce->name,\n+\t\t\t\t\t ce->name, cached);\n \t\t} else {\n \t\t\tcontinue;\n \t\t}\n@@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t\tfree(data);\n \t\t} else if (recurse_submodules && S_ISGITLINK(entry.mode)) {\n \t\t\thit |= grep_submodule(opt, pathspec, &entry.oid,\n-\t\t\t\t\t base->buf, base->buf + tn_len);\n+\t\t\t\t\t base->buf, base->buf + tn_len,\n+\t\t\t\t\t 1); /* ignored */\n \t\t}\n \n \t\tstrbuf_setlen(base, old_baselen);\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex a11366b4ce..946f91fa57 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -408,4 +408,25 @@ test_expect_success 'grep --recurse-submodules with submodules without .gitmodul\n \ttest_cmp expect actual\n '\n \n+reset_and_clean () {\n+\tgit reset --hard &&\n+\tgit clean -fd &&\n+\tgit submodule foreach --recursive 'git reset --hard' &&\n+\tgit submodule foreach --recursive 'git clean -fd'\n+}\n+\n+test_expect_success 'grep --recurse-submodules without --cached considers worktree modifications' '\n+\treset_and_clean &&\n+\techo \"A modified line in submodule\" >>submodule/a &&\n+\techo \"submodule/a:A modified line in submodule\" >expect &&\n+\tgit grep --recurse-submodules \"A modified line in submodule\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --recurse-submodules with --cached ignores worktree modifications' '\n+\treset_and_clean &&\n+\techo \"A modified line in submodule\" >>submodule/a &&\n+\ttest_must_fail git grep --recurse-submodules --cached \"A modified line in submodule\" >actual 2>&1 &&\n+\ttest_must_be_empty actual\n+'\n test_done\n-- \n2.22.0\n\n"},{"id":"379605","messageId":"xmqqtvb3s2zi.fsf@gitster-ct.c.googlers.com","threadId":"51444","inReplyTo":"ba3d8a953a2cc5b4ff03fefa434ffd7bd6a78f15.1564505605.git.matheus.bernardino@usp.br","subject":"Re: [GSoC][PATCH] grep: fix worktree case in submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-30T20:04:49Z","receivedAt":"2019-07-30T20:04:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> @@ -475,7 +475,7 @@ static int grep_submodule(struct grep_opt *opt,\n> \t\tstrbuf_release(&base);\n> \t\tfree(data);\n> \t} else {\n> -\t\thit = grep_cache(&subopt, pathspec, 1);\n> +\t\thit = grep_cache(&subopt, pathspec, cached);\n> \t}\n\nInteresting. It appears to me that this has always searched in\nsubmodule index and never working tree. I am not sure if there was\nany specific reason to avoid looking into the working tree files.\n\nPassing the 'cached' bit down from grep_cache() does look like a\nsensible way to go.\n\n> @@ -523,7 +523,8 @@ static int grep_cache(struct grep_opt *opt,\n> \t\t\t}\n> \t\t} else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&\n> \t\t\t submodule_path_match(repo->index, pathspec, name.buf, NULL)) {\n> -\t\t\thit |= grep_submodule(opt, pathspec, NULL, ce->name, ce->name);\n> +\t\t\thit |= grep_submodule(opt, pathspec, NULL, ce->name,\n> +\t\t\t\t\t ce->name, cached);\n> \t\t} else {\n> \t\t\tcontinue;\n> \t\t}\n> @@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n> \t\t\tfree(data);\n> \t\t} else if (recurse_submodules && S_ISGITLINK(entry.mode)) {\n> \t\t\thit |= grep_submodule(opt, pathspec, &entry.oid,\n> -\t\t\t\t\t base->buf, base->buf + tn_len);\n> +\t\t\t\t\t base->buf, base->buf + tn_len,\n> +\t\t\t\t\t 1); /* ignored */\n\nThe trailing comment is misleading. In the context of reviewing\nthis patch, we can probably tell it applies only to that \"1\", but\nif you read only the postimage, the \"ignored\" comment looks as if\nthe call itself is somehow ignored by somebody unspecified. It is\nnot clear at all that it is only about the final parameter.\n\nIf you must...\n\n\t\thit |= grep_submodule(opt, pathspec, &entry.oid,\n\t\t\t\t base->buf, base->buf + tn_len,\n\t\t\t\t 1 /* ignored */);\n\n... is a reasonable way to write it.\n\n> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> index a11366b4ce..946f91fa57 100755\n> --- a/t/t7814-grep-recurse-submodules.sh\n> +++ b/t/t7814-grep-recurse-submodules.sh\n> @@ -408,4 +408,25 @@ test_expect_success 'grep --recurse-submodules with submodules without .gitmodul\n> \ttest_cmp expect actual\n> '\n> \n> +reset_and_clean () {\n> +\tgit reset --hard &&\n> +\tgit clean -fd &&\n> +\tgit submodule foreach --recursive 'git reset --hard' &&\n> +\tgit submodule foreach --recursive 'git clean -fd'\n\nDo we need two separate foreach, instread of a single one that does\nboth reset and clean (i.e. \"git reset --hard && git clean -f -d\")?\n\n"},{"id":"379621","messageId":"CAP8UFD0bH7ZNWFt3MfkAQf2tkF6CAgFj5FsZyc9zechg7MkCUw@mail.gmail.com","threadId":"51444","inReplyTo":"xmqqtvb3s2zi.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH] grep: fix worktree case in submodules","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-07-30T22:02:44Z","receivedAt":"2019-07-30T22:02:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jul 30, 2019 at 10:04 PM Junio C Hamano wrote:\n>\n> Matheus Tavares writes:\n\n> > @@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n> > free(data);\n> > } else if (recurse_submodules && S_ISGITLINK(entry.mode)) {\n> > hit |= grep_submodule(opt, pathspec, &entry.oid,\n> > - base->buf, base->buf + tn_len);\n> > + base->buf, base->buf + tn_len,\n> > + 1); /* ignored */\n>\n> The trailing comment is misleading. In the context of reviewing\n> this patch, we can probably tell it applies only to that \"1\", but\n> if you read only the postimage, the \"ignored\" comment looks as if\n> the call itself is somehow ignored by somebody unspecified. It is\n> not clear at all that it is only about the final parameter.\n>\n> If you must...\n>\n> hit |= grep_submodule(opt, pathspec, &entry.oid,\n> base->buf, base->buf + tn_len,\n> 1 /* ignored */);\n\nYeah, I suggested adding an \"/* ignored */\" comment, but I was indeed\nthinking about something like this.\n\n> ... is a reasonable way to write it.\n"},{"id":"379627","messageId":"CAHd-oW48sa1Pc7AutxhKRQLry74wSYu_0xZmaz4XJnCjJdRH2A@mail.gmail.com","threadId":"51444","inReplyTo":"xmqqtvb3s2zi.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH] grep: fix worktree case in submodules","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-07-30T23:40:03Z","receivedAt":"2019-07-30T23:40:17Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Jul 30, 2019 at 5:04 PM Junio C Hamano wrote:\n>\n> Matheus Tavares writes:\n>\n> > @@ -475,7 +475,7 @@ static int grep_submodule(struct grep_opt *opt,\n> > strbuf_release(&base);\n> > free(data);\n> > } else {\n> > - hit = grep_cache(&subopt, pathspec, 1);\n> > + hit = grep_cache(&subopt, pathspec, cached);\n> > }\n>\n> Interesting. It appears to me that this has always searched in\n> submodule index and never working tree. I am not sure if there was\n> any specific reason to avoid looking into the working tree files.\n\nIt seems that git-grep was taking the worktree into account before\nf9ee2fc (\"grep: recurse in-process using 'struct repository'\",\n02-08-2017). So maybe it was just a mistake during the in-process\nconversion.\n\n> Passing the 'cached' bit down from grep_cache() does look like a\n> sensible way to go.\n>\n> > @@ -523,7 +523,8 @@ static int grep_cache(struct grep_opt *opt,\n> > }\n> > } else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&\n> > submodule_path_match(repo->index, pathspec, name.buf, NULL)) {\n> > - hit |= grep_submodule(opt, pathspec, NULL, ce->name, ce->name);\n> > + hit |= grep_submodule(opt, pathspec, NULL, ce->name,\n> > + ce->name, cached);\n> > } else {\n> > continue;\n> > }\n> > @@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n> > free(data);\n> > } else if (recurse_submodules && S_ISGITLINK(entry.mode)) {\n> > hit |= grep_submodule(opt, pathspec, &entry.oid,\n> > - base->buf, base->buf + tn_len);\n> > + base->buf, base->buf + tn_len,\n> > + 1); /* ignored */\n>\n> The trailing comment is misleading. In the context of reviewing\n> this patch, we can probably tell it applies only to that \"1\", but\n> if you read only the postimage, the \"ignored\" comment looks as if\n> the call itself is somehow ignored by somebody unspecified. It is\n> not clear at all that it is only about the final parameter.\n>\n> If you must...\n>\n> hit |= grep_submodule(opt, pathspec, &entry.oid,\n> base->buf, base->buf + tn_len,\n> 1 /* ignored */);\n>\n> ... is a reasonable way to write it.\n\nRight, thanks.\n\n> > diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> > index a11366b4ce..946f91fa57 100755\n> > --- a/t/t7814-grep-recurse-submodules.sh\n> > +++ b/t/t7814-grep-recurse-submodules.sh\n> > @@ -408,4 +408,25 @@ test_expect_success 'grep --recurse-submodules with submodules without .gitmodul\n> > test_cmp expect actual\n> > '\n> >\n> > +reset_and_clean () {\n> > + git reset --hard &&\n> > + git clean -fd &&\n> > + git submodule foreach --recursive 'git reset --hard' &&\n> > + git submodule foreach --recursive 'git clean -fd'\n>\n> Do we need two separate foreach, instread of a single one that does\n> both reset and clean (i.e. \"git reset --hard && git clean -f -d\")?\n\nIndeed! Thanks. I'll send a v2 addressing both comments.\n"},{"id":"379686","messageId":"d1fdd091f289e836633b875d87341a66d1fc528a.1564587317.git.matheus.bernardino@usp.br","threadId":"51444","inReplyTo":"ba3d8a953a2cc5b4ff03fefa434ffd7bd6a78f15.1564505605.git.matheus.bernardino@usp.br","subject":"[GSoC][PATCH v2] grep: fix worktree case in submodules","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-07-31T15:35:32Z","receivedAt":"2019-07-31T15:36:10Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Running git-grep with --recurse-submodules results in a cached grep for\nthe submodules even when --cached is not used. This makes all\nmodifications in submodules' tracked files be always ignored when\ngrepping. Solve that making git-grep respect the cached option when\ninvoking grep_cache() inside grep_submodule(). Also, add tests to\nensure that the desired behavior is performed.\n\nReported-by: Daniel Zaoui \nSigned-off-by: Matheus Tavares \n---\nChanges in v2:\n- repositioned the '/* ignored */' comment to avoid ambiguity\n- joined `git clean` and `git reset` invokations in a single `git\n submodule foreach`. \n\ntravis build: https://travis-ci.org/matheustavares/git/builds/565749070\n\nbuiltin/grep.c | 10 ++++++----\n t/t7814-grep-recurse-submodules.sh | 20 ++++++++++++++++++++\n 2 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 560051784e..d9866dd936 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -403,7 +403,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n static int grep_submodule(struct grep_opt *opt,\n \t\t\t const struct pathspec *pathspec,\n \t\t\t const struct object_id *oid,\n-\t\t\t const char *filename, const char *path)\n+\t\t\t const char *filename, const char *path, int cached)\n {\n \tstruct repository subrepo;\n \tstruct repository *superproject = opt->repo;\n@@ -475,7 +475,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tstrbuf_release(&base);\n \t\tfree(data);\n \t} else {\n-\t\thit = grep_cache(&subopt, pathspec, 1);\n+\t\thit = grep_cache(&subopt, pathspec, cached);\n \t}\n \n \trepo_clear(&subrepo);\n@@ -523,7 +523,8 @@ static int grep_cache(struct grep_opt *opt,\n \t\t\t}\n \t\t} else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&\n \t\t\t submodule_path_match(repo->index, pathspec, name.buf, NULL)) {\n-\t\t\thit |= grep_submodule(opt, pathspec, NULL, ce->name, ce->name);\n+\t\t\thit |= grep_submodule(opt, pathspec, NULL, ce->name,\n+\t\t\t\t\t ce->name, cached);\n \t\t} else {\n \t\t\tcontinue;\n \t\t}\n@@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t\tfree(data);\n \t\t} else if (recurse_submodules && S_ISGITLINK(entry.mode)) {\n \t\t\thit |= grep_submodule(opt, pathspec, &entry.oid,\n-\t\t\t\t\t base->buf, base->buf + tn_len);\n+\t\t\t\t\t base->buf, base->buf + tn_len,\n+\t\t\t\t\t 1 /* ignored */);\n \t\t}\n \n \t\tstrbuf_setlen(base, old_baselen);\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex a11366b4ce..edb64690e6 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -408,4 +408,24 @@ test_expect_success 'grep --recurse-submodules with submodules without .gitmodul\n \ttest_cmp expect actual\n '\n \n+reset_and_clean () {\n+\tgit reset --hard &&\n+\tgit clean -fd &&\n+\tgit submodule foreach --recursive 'git reset --hard && git clean -fd'\n+}\n+\n+test_expect_success 'grep --recurse-submodules without --cached considers worktree modifications' '\n+\treset_and_clean &&\n+\techo \"A modified line in submodule\" >>submodule/a &&\n+\techo \"submodule/a:A modified line in submodule\" >expect &&\n+\tgit grep --recurse-submodules \"A modified line in submodule\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --recurse-submodules with --cached ignores worktree modifications' '\n+\treset_and_clean &&\n+\techo \"A modified line in submodule\" >>submodule/a &&\n+\ttest_must_fail git grep --recurse-submodules --cached \"A modified line in submodule\" >actual 2>&1 &&\n+\ttest_must_be_empty actual\n+'\n test_done\n-- \n2.22.0\n\n"},{"id":"379688","messageId":"xmqqwofyqjrz.fsf@gitster-ct.c.googlers.com","threadId":"51444","inReplyTo":"CAP8UFD0bH7ZNWFt3MfkAQf2tkF6CAgFj5FsZyc9zechg7MkCUw@mail.gmail.com","subject":"Re: [GSoC][PATCH] grep: fix worktree case in submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-31T15:57:20Z","receivedAt":"2019-07-31T15:57:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Tue, Jul 30, 2019 at 10:04 PM Junio C Hamano wrote:\n>>\n>> Matheus Tavares writes:\n>\n>> > @@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n>> > free(data);\n>> > } else if (recurse_submodules && S_ISGITLINK(entry.mode)) {\n>> > hit |= grep_submodule(opt, pathspec, &entry.oid,\n>> > - base->buf, base->buf + tn_len);\n>> > + base->buf, base->buf + tn_len,\n>> > + 1); /* ignored */\n>>\n>> The trailing comment is misleading. In the context of reviewing\n>> this patch, we can probably tell it applies only to that \"1\", but\n>> if you read only the postimage, the \"ignored\" comment looks as if\n>> the call itself is somehow ignored by somebody unspecified. It is\n>> not clear at all that it is only about the final parameter.\n>>\n>> If you must...\n>>\n>> hit |= grep_submodule(opt, pathspec, &entry.oid,\n>> base->buf, base->buf + tn_len,\n>> 1 /* ignored */);\n>\n> Yeah, I suggested adding an \"/* ignored */\" comment, but I was indeed\n> thinking about something like this.\n>\n>> ... is a reasonable way to write it.\n\nThanks. In this case, I am not sure if the comment here really\nhelps. If anything, shouldn't there be a comment near the top of\ngrep_submodule() that says 'cached bit is meaningful only when you\nfeed an empty oid, aka \"not grepping inside a tree object\"'?\n\n"},{"id":"379736","messageId":"CAHd-oW78PpLspY94F9JDfw3C6v3oRbt0tZY1Jth4R_WbYUSQCg@mail.gmail.com","threadId":"51444","inReplyTo":"xmqqwofyqjrz.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH] grep: fix worktree case in submodules","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-01T03:08:09Z","receivedAt":"2019-08-01T03:08:24Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Wed, Jul 31, 2019 at 12:57 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > On Tue, Jul 30, 2019 at 10:04 PM Junio C Hamano wrote:\n> >>\n> >> Matheus Tavares writes:\n> >\n> >> > @@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n> >> > free(data);\n> >> > } else if (recurse_submodules && S_ISGITLINK(entry.mode)) {\n> >> > hit |= grep_submodule(opt, pathspec, &entry.oid,\n> >> > - base->buf, base->buf + tn_len);\n> >> > + base->buf, base->buf + tn_len,\n> >> > + 1); /* ignored */\n> >>\n> >> The trailing comment is misleading. In the context of reviewing\n> >> this patch, we can probably tell it applies only to that \"1\", but\n> >> if you read only the postimage, the \"ignored\" comment looks as if\n> >> the call itself is somehow ignored by somebody unspecified. It is\n> >> not clear at all that it is only about the final parameter.\n> >>\n> >> If you must...\n> >>\n> >> hit |= grep_submodule(opt, pathspec, &entry.oid,\n> >> base->buf, base->buf + tn_len,\n> >> 1 /* ignored */);\n> >\n> > Yeah, I suggested adding an \"/* ignored */\" comment, but I was indeed\n> > thinking about something like this.\n> >\n> >> ... is a reasonable way to write it.\n>\n> Thanks. In this case, I am not sure if the comment here really\n> helps. If anything, shouldn't there be a comment near the top of\n> grep_submodule() that says 'cached bit is meaningful only when you\n> feed an empty oid, aka \"not grepping inside a tree object\"'?\n\nRight, it makes sense. I'll add that.\n"},{"id":"379737","messageId":"901ddbf6a1d9eeff51b1b2282ebefad51e61b12d.1564629070.git.matheus.bernardino@usp.br","threadId":"51444","inReplyTo":"d1fdd091f289e836633b875d87341a66d1fc528a.1564587317.git.matheus.bernardino@usp.br","subject":"[GSoC][PATCH v3] grep: fix worktree case in submodules","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-01T03:13:44Z","receivedAt":"2019-08-01T03:14:08Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Running git-grep with --recurse-submodules results in a cached grep for\nthe submodules even when --cached is not used. This makes all\nmodifications in submodules' tracked files be always ignored when\ngrepping. Solve that making git-grep respect the cached option when\ninvoking grep_cache() inside grep_submodule(). Also, add tests to\nensure that the desired behavior is performed.\n\nReported-by: Daniel Zaoui \nSigned-off-by: Matheus Tavares \n---\nChanges in v3:\n- Replaced the \"\\* ignored *\\\" comment by a more meaningful note at the\n top of grep_submodule()\n\nbuiltin/grep.c | 13 +++++++++----\n t/t7814-grep-recurse-submodules.sh | 20 ++++++++++++++++++++\n 2 files changed, 29 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 560051784e..df8cdecdae 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -400,10 +400,14 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t struct tree_desc *tree, struct strbuf *base, int tn_len,\n \t\t int check_attr);\n \n+/*\n+ * The cached bit is only meaningful when a NULL oid is given (i.e. when not\n+ * grepping inside a tree object).\n+ */\n static int grep_submodule(struct grep_opt *opt,\n \t\t\t const struct pathspec *pathspec,\n \t\t\t const struct object_id *oid,\n-\t\t\t const char *filename, const char *path)\n+\t\t\t const char *filename, const char *path, int cached)\n {\n \tstruct repository subrepo;\n \tstruct repository *superproject = opt->repo;\n@@ -475,7 +479,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tstrbuf_release(&base);\n \t\tfree(data);\n \t} else {\n-\t\thit = grep_cache(&subopt, pathspec, 1);\n+\t\thit = grep_cache(&subopt, pathspec, cached);\n \t}\n \n \trepo_clear(&subrepo);\n@@ -523,7 +527,8 @@ static int grep_cache(struct grep_opt *opt,\n \t\t\t}\n \t\t} else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&\n \t\t\t submodule_path_match(repo->index, pathspec, name.buf, NULL)) {\n-\t\t\thit |= grep_submodule(opt, pathspec, NULL, ce->name, ce->name);\n+\t\t\thit |= grep_submodule(opt, pathspec, NULL, ce->name,\n+\t\t\t\t\t ce->name, cached);\n \t\t} else {\n \t\t\tcontinue;\n \t\t}\n@@ -598,7 +603,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t\tfree(data);\n \t\t} else if (recurse_submodules && S_ISGITLINK(entry.mode)) {\n \t\t\thit |= grep_submodule(opt, pathspec, &entry.oid,\n-\t\t\t\t\t base->buf, base->buf + tn_len);\n+\t\t\t\t\t base->buf, base->buf + tn_len, 1);\n \t\t}\n \n \t\tstrbuf_setlen(base, old_baselen);\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex a11366b4ce..edb64690e6 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -408,4 +408,24 @@ test_expect_success 'grep --recurse-submodules with submodules without .gitmodul\n \ttest_cmp expect actual\n '\n \n+reset_and_clean () {\n+\tgit reset --hard &&\n+\tgit clean -fd &&\n+\tgit submodule foreach --recursive 'git reset --hard && git clean -fd'\n+}\n+\n+test_expect_success 'grep --recurse-submodules without --cached considers worktree modifications' '\n+\treset_and_clean &&\n+\techo \"A modified line in submodule\" >>submodule/a &&\n+\techo \"submodule/a:A modified line in submodule\" >expect &&\n+\tgit grep --recurse-submodules \"A modified line in submodule\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --recurse-submodules with --cached ignores worktree modifications' '\n+\treset_and_clean &&\n+\techo \"A modified line in submodule\" >>submodule/a &&\n+\ttest_must_fail git grep --recurse-submodules --cached \"A modified line in submodule\" >actual 2>&1 &&\n+\ttest_must_be_empty actual\n+'\n test_done\n-- \n2.22.0\n\n"},{"id":"379894","messageId":"CALN-EhTugXw6J7eA=JznGGuk7d5FfMu7s2p44OYKZgOqaN1oOA@mail.gmail.com","threadId":"51444","inReplyTo":"CAHd-oW61RFCySF+gUj8iYuV6afEoD0RD9oYE+N6rYd7rv3J2nA@mail.gmail.com","subject":"Re: Weird behavior with git grep --recurse-submodules","fromName":"Brandon Williams","fromEmail":"bwilliamseng@gmail.com","sentAt":"2019-08-03T23:39:30Z","receivedAt":"2019-08-03T23:39:44Z","isPatch":false,"sender":{"key":"bwilliamseng@gmail.com","avatar":null},"body":"On Mon, Jul 29, 2019 at 1:27 PM Matheus Tavares Bernardino\n wrote:\n>\n> On Tue, Jul 16, 2019 at 3:09 PM Daniel Zaoui wrote:\n> >\n> > Hi Matheus,\n>\n> Hi, Daniel\n>\n> I'm sorry, your last message went to my spam folder for some reason :(\n>\n> > Thank you for your response.\n> >\n> > I really hope the change Brandon made is not a project decision. At least, it does seem to me like a bug.\n> >\n\nSince I'm the one who originally implemented the --recurse-submodules\nfeature to git grep back in\n0281e487f (grep: optionally recurse into submodules, 2016-12-16) and\nthen subsequently changed\nthe implementation to do the recursion in process in f9ee2fc (\"grep:\nrecurse in-process using\n'struct repository'\", 02-08-2017) I can say it was definitely a bug I\nintroduced during the conversion\nas I don't believe I would have intended to introduce that change of\nbehavior (especially without\ndocumenting it).\n\n> > How do you recommend me to solve this issue? Is there some place where I can check if some bug ticket has been created on this matter? I didn't find anything in the mailing list archives about this.\n>\n> I think I manage to solve the bug with this[1] patch. I'm going to ask\n> my GSoC mentors to take a look and then I'll send it to the mailing\n> list :) Thanks for reporting it.\n>\n> Best,\n> Matheus\n>\n> [1]: https://github.com/matheustavares/git/commit/37aeeb3ab86b5dfebfaafeb98d34e379341a529d\n"}]}