Weird behavior with git grep --recurse-submodules

Hi guys,

I work with submodules and use git grep a lot.

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.

Expected behavior:
git grep --recurse-submodules string:
- git grep string // search into main repo
- for each submodule, git grep string // search into submodule

Actual behavior:
git grep --recurse-submodules string:
- git grep string // search into main repo
- for each submodule, git grep --cached string // search into submodule

Do you get the same behavior? Am I doing something wrong? Was I understandable :-)? Is it a bug?

git --version: git version 2.22.0
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

Thanks
Daniel

Quoted 2 lines · Daniel Zaoui

Hi guys,

Hi, Daniel

Quoted 9 lines

I work with submodules and use git grep a lot.

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.

Expected behavior:
git grep --recurse-submodules string:
- git grep string // search into main repo
- for each submodule, git grep string // search into submodule

Quoted 6 lines

Actual behavior:
git grep --recurse-submodules string:
- git grep string // search into main repo
- for each submodule, git grep --cached string // search into submodule

Do you get the same behavior? Am I doing something wrong? Was I understandable :-)? Is it a bug?

It seems git-grep was taking into account the worktree modifications in submodules before f9ee2fc ("grep: recurse in-process using 'struct repository'", 02-08-2017). I'm not sure, thought, if this behavior change was a bug during the conversion or a project decision.

CC-ing Brandon, in case he has other inputs

Quoted 5 lines

git --version: git version 2.22.0
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

Thanks
Daniel

Best,
Matheus

[cC:ing Brandon via his current email address, as per .mailmap]

On Wed, 10 Jul 2019, Matheus Tavares Bernardino wrote:

[...]
[...]
[...]
[...]
[...]
[...]
[...]

Hi Matheus,

Thank you for your response.

I really hope the change Brandon made is not a project decision. At least, it does seem to me like a bug.

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.

builtin/grep.c +2 -1

@@ -523,7 +523,8 @@ static int grep_cache(struct grep_opt *opt,
 			}
 		} else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
 			   submodule_path_match(repo->index, pathspec, name.buf, NULL)) {
-			hit |= grep_submodule(opt, pathspec, NULL, ce->name, ce->name);
+			hit |= grep_submodule(opt, pathspec, NULL, ce->name,
+					      ce->name, cached);
 		} else {
 			continue;
 		}

builtin/grep.c static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, · line 603

Quoted 7 lines · Matheus Tavares

builtin/grep.c +2 -1

@@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
                      free(data);
              } else if (recurse_submodules && S_ISGITLINK(entry.mode)) {
                      hit |= grep_submodule(opt, pathspec, &entry.oid,
-                                           base->buf, base->buf + tn_len);
+                                           base->buf, base->buf + tn_len,
+                                           1); /* ignored */

The trailing comment is misleading. In the context of reviewing this patch, we can probably tell it applies only to that "1", but if you read only the postimage, the "ignored" comment looks as if the call itself is somehow ignored by somebody unspecified. It is not clear at all that it is only about the final parameter.

If you must...

                hit |= grep_submodule(opt, pathspec, &entry.oid,
                                      base->buf, base->buf + tn_len,
                                      1 /* ignored */);

... is a reasonable way to write it.

Quoted 17 lines · Junio C Hamano

builtin/grep.c static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, · line 603

Quoted 7 lines · Matheus Tavares

builtin/grep.c +2 -1

@@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
                      free(data);
              } else if (recurse_submodules && S_ISGITLINK(entry.mode)) {
                      hit |= grep_submodule(opt, pathspec, &entry.oid,
-                                           base->buf, base->buf + tn_len);
+                                           base->buf, base->buf + tn_len,
+                                           1); /* ignored */

The trailing comment is misleading. In the context of reviewing this patch, we can probably tell it applies only to that "1", but if you read only the postimage, the "ignored" comment looks as if the call itself is somehow ignored by somebody unspecified. It is not clear at all that it is only about the final parameter.

Quoted 5 lines

If you must...

                hit |= grep_submodule(opt, pathspec, &entry.oid,
                                      base->buf, base->buf + tn_len,
                                      1 /* ignored */);

Yeah, I suggested adding an "/* ignored */" comment, but I was indeed thinking about something like this.

Quoted 1 line

... is a reasonable way to write it.

Quoted 27 lines · Christian Couder

Quoted 17 lines · Junio C Hamano

builtin/grep.c static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, · line 603

Quoted 7 lines · Matheus Tavares

builtin/grep.c +2 -1

@@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
                      free(data);
              } else if (recurse_submodules && S_ISGITLINK(entry.mode)) {
                      hit |= grep_submodule(opt, pathspec, &entry.oid,
-                                           base->buf, base->buf + tn_len);
+                                           base->buf, base->buf + tn_len,
+                                           1); /* ignored */

The trailing comment is misleading. In the context of reviewing this patch, we can probably tell it applies only to that "1", but if you read only the postimage, the "ignored" comment looks as if the call itself is somehow ignored by somebody unspecified. It is not clear at all that it is only about the final parameter.

Quoted 5 lines

If you must...

                hit |= grep_submodule(opt, pathspec, &entry.oid,
                                      base->buf, base->buf + tn_len,
                                      1 /* ignored */);

Yeah, I suggested adding an "/* ignored */" comment, but I was indeed thinking about something like this.

Quoted 1 line

Quoted 1 line

... is a reasonable way to write it.

Thanks. In this case, I am not sure if the comment here really helps. If anything, shouldn't there be a comment near the top of grep_submodule() that says 'cached bit is meaningful only when you feed an empty oid, aka "not grepping inside a tree object"'?

builtin/grep.c static int grep_submodule(struct grep_opt *opt, · line 479

Quoted 7 lines · Matheus Tavares

builtin/grep.c +1 -1

@@ -475,7 +475,7 @@ static int grep_submodule(struct grep_opt *opt,
              strbuf_release(&base);
              free(data);
      } else {
-             hit = grep_cache(&subopt, pathspec, 1);
+             hit = grep_cache(&subopt, pathspec, cached);
      }

Interesting. It appears to me that this has always searched in submodule index and never working tree. I am not sure if there was any specific reason to avoid looking into the working tree files.

Passing the 'cached' bit down from grep_cache() does look like a sensible way to go.

Quoted 11 lines

builtin/grep.c +2 -1

@@ -523,7 +523,8 @@ static int grep_cache(struct grep_opt *opt,
                      }
              } else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
                         submodule_path_match(repo->index, pathspec, name.buf, NULL)) {
-                     hit |= grep_submodule(opt, pathspec, NULL, ce->name, ce->name);
+                     hit |= grep_submodule(opt, pathspec, NULL, ce->name,
+                                           ce->name, cached);
              } else {
                      continue;
              }

builtin/grep.c static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, · line 603

Quoted 6 lines

builtin/grep.c +2 -1

@@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
                      free(data);
              } else if (recurse_submodules && S_ISGITLINK(entry.mode)) {
                      hit |= grep_submodule(opt, pathspec, &entry.oid,
-                                           base->buf, base->buf + tn_len);
+                                           base->buf, base->buf + tn_len,
+                                           1); /* ignored */

The trailing comment is misleading. In the context of reviewing this patch, we can probably tell it applies only to that "1", but if you read only the postimage, the "ignored" comment looks as if the call itself is somehow ignored by somebody unspecified. It is not clear at all that it is only about the final parameter.

If you must...

                hit |= grep_submodule(opt, pathspec, &entry.oid,
                                      base->buf, base->buf + tn_len,
                                      1 /* ignored */);

... is a reasonable way to write it.

Quoted 8 lines

t/t7814-grep-recurse-submodules.sh +0 -0

index a11366b4ce..946f91fa57 100755
@@ -408,4 +408,25 @@ test_expect_success 'grep --recurse-submodules with submodules without .gitmodul
      test_cmp expect actual
 '
 

t/t7814-grep-recurse-submodules.sh test_expect_success 'grep --recurse-submodules with submodules without .gitmodul · line 415

Quoted 5 lines

t/t7814-grep-recurse-submodules.sh +5 -0

+reset_and_clean () {
+     git reset --hard &&
+     git clean -fd &&
+     git submodule foreach --recursive 'git reset --hard' &&
+     git submodule foreach --recursive 'git clean -fd'

Do we need two separate foreach, instread of a single one that does both reset and clean (i.e. "git reset --hard && git clean -f -d")?

Quoted 2 lines · Daniel Zaoui

Hi Matheus,

Hi, Daniel

I'm sorry, your last message went to my spam folder for some reason :(

Quoted 4 lines

Thank you for your response.

I really hope the change Brandon made is not a project decision. At least, it does seem to me like a bug.

Quoted 1 line

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.

I think I manage to solve the bug with this[1] patch. I'm going to ask my GSoC mentors to take a look and then I'll send it to the mailing list :) Thanks for reporting it.

Best,
Matheus

[1]: https://github.com/matheustavares/git/commit/37aeeb3ab86b5dfebfaafeb98d34e379341a529d