[PATCH 0/3] builtin/grep: fix reading objects from wrong repository

[PATCH 0/3] builtin/grep: fix reading objects from wrong repository

[Patrick Steinhardt]

Hi,

this small patch series fixes the bug reported in [1], where doing `git
grep --recurse-submodules` with nested submodule trees fails to read the
trees. The only patch needed here is the first one, the other patches
are only belt-and-suspenders as I've been checking whether we use the
wrong repository anywhere else.

At the end of this patch series we only use `the_repository` in
`cmd_grep()` itself. Ideally we'd adapt "builtin/grep.c" to get rid of
`the_repository` altogether, but unfortunately it's a bit on the
involved side as we also have to worry about the case where git-grep(1)
is executed outside of a repository with "--no-index". So I skipped that
refactoring for now.

The patch is based on v2.56.0 so that it can be easily backported.

Thanks!

Patrick

[1]: <CAMtVYEjWd2XwJpXdk3PV2jbNdoOaqS3x+08EMu5byy55RxfRmA@mail.gmail.com>

---
Patrick Steinhardt (3):
      builtin/grep: read submodule trees via their repository
      builtin/grep: read sparse directory trees via passed-in repository
      builtin/grep: read ".gitmodules" via passed-in repository

 builtin/grep.c                     |  6 +++---
 t/t7814-grep-recurse-submodules.sh | 23 +++++++++++++++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)


---
base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
change-id: 20261030-pks-grep-recurse-submodule-20177cc7794c

[PATCH 1/3] builtin/grep: read submodule trees via their repository

[Patrick Steinhardt]

The function `grep_tree()` searches for a specific string recursively in
a tree. It can recurse in two different ways:

  - It may either see a subtree, in which case it recurses into itself.

  - Or it may see a submodule, and in that case it recurses via
    `grep_submodule()` which then again calls `grep_tree()`.

In the latter case we continue grepping in the submodule repository's
object graph. Historically this used to work by linking that submodule's
object directory into the main object database. But in 93cd344e34f
(builtin/grep: stop registering submodule ODB as source, 2026-09-11) we
stopped doing that and instead expect to read submodule objects via the
submodule repository.

This logic is broken though: while we indeed know to read blobs via the
submodule's object database, we didn't adapt trees in the same way. The
result is that Git fails to read the object:

    $ git grep --recurse-submodules -e hello HEAD
    fatal: unable to read tree (935cff11a11f8734cf4e0d40955d1e0e5c12038e)

This went unnoticed because our tests only cover submodules that contain
files at the top level.

Fix this by reading nested trees via `opt->repo`, which is set to the
submodule's repository via `grep_submodule()`.

Reported-by: Root <rootminus0x1@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/grep.c                     |  2 +-
 t/t7814-grep-recurse-submodules.sh | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index b045f8a488d..22fb365eeac 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -653,7 +653,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
 			void *data;
 			size_t size;
 
-			data = odb_read_object(the_repository->objects,
+			data = odb_read_object(repo->objects,
 					       &entry.oid, &type, &size);
 			if (!data)
 				die(_("unable to read tree (%s)"),
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 3d149d34c15..f51c5181805 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -631,4 +631,27 @@ test_expect_success 'check scope of core.useReplaceRefs' '
 	git -C base grep --cached --recurse-submodules C
 '
 
+test_expect_success 'grep --recurse-submodules with tree-ish and nested directory' '
+	rm -rf sub super &&
+
+	git init sub &&
+	mkdir sub/dir &&
+	echo hello >sub/top &&
+	echo hello >sub/dir/nested &&
+	git -C sub add top dir/nested &&
+	git -C sub commit --message "sub" &&
+
+	test_config_global protocol.file.allow always &&
+	git init super &&
+	git -C super submodule add "$(pwd)/sub" sub &&
+	git -C super commit --message "super" &&
+
+	cat >expect <<-\EOF &&
+	HEAD:sub/dir/nested:hello
+	HEAD:sub/top:hello
+	EOF
+	git -C super grep --recurse-submodules -e hello HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.56.0.341.g393f52fd96.dirty

Re: [PATCH 1/3] builtin/grep: read submodule trees via their repository

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> The function `grep_tree()` searches for a specific string recursively in
> a tree. It can recurse in two different ways:
>
>   - It may either see a subtree, in which case it recurses into itself.
>
>   - Or it may see a submodule, and in that case it recurses via
>     `grep_submodule()` which then again calls `grep_tree()`.
>
> In the latter case we continue grepping in the submodule repository's
> object graph. Historically this used to work by linking that submodule's
> object directory into the main object database. But in 93cd344e34f
> (builtin/grep: stop registering submodule ODB as source, 2026-09-11) we
> stopped doing that and instead expect to read submodule objects via the
> submodule repository.
>
> This logic is broken though: while we indeed know to read blobs via the
> submodule's object database, we didn't adapt trees in the same way. The
> result is that Git fails to read the object:
>
>     $ git grep --recurse-submodules -e hello HEAD
>     fatal: unable to read tree (935cff11a11f8734cf4e0d40955d1e0e5c12038e)
>
> This went unnoticed because our tests only cover submodules that contain
> files at the top level.

Should we mention this is a regression in 2.56?

> Fix this by reading nested trees via `opt->repo`, which is set to the
> submodule's repository via `grep_submodule()`.
>
> Reported-by: Root <rootminus0x1@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/grep.c                     |  2 +-
>  t/t7814-grep-recurse-submodules.sh | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index b045f8a488d..22fb365eeac 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -653,7 +653,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
>  			void *data;
>  			size_t size;
>  
> -			data = odb_read_object(the_repository->objects,
> +			data = odb_read_object(repo->objects,

Derp... What simple (in hindsight) fix.

>  					       &entry.oid, &type, &size);
>  			if (!data)
>  				die(_("unable to read tree (%s)"),
> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
> index 3d149d34c15..f51c5181805 100755
> --- a/t/t7814-grep-recurse-submodules.sh
> +++ b/t/t7814-grep-recurse-submodules.sh
> @@ -631,4 +631,27 @@ test_expect_success 'check scope of core.useReplaceRefs' '
>  	git -C base grep --cached --recurse-submodules C
>  '
>  
> +test_expect_success 'grep --recurse-submodules with tree-ish and nested directory' '
> +	rm -rf sub super &&

Small nit, shall we use test_when_finished here?

> +	git init sub &&
> +	mkdir sub/dir &&
> +	echo hello >sub/top &&
> +	echo hello >sub/dir/nested &&
> +	git -C sub add top dir/nested &&
> +	git -C sub commit --message "sub" &&
> +
> +	test_config_global protocol.file.allow always &&
> +	git init super &&
> +	git -C super submodule add "$(pwd)/sub" sub &&
> +	git -C super commit --message "super" &&
> +
> +	cat >expect <<-\EOF &&
> +	HEAD:sub/dir/nested:hello
> +	HEAD:sub/top:hello
> +	EOF
> +	git -C super grep --recurse-submodules -e hello HEAD >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

Verified the test fails without the fix above.

All changes look good.

> -- 
> 2.56.0.341.g393f52fd96.dirty

The other two patches make sense to me as well. Seems worthwhile to
include them even if they aren't directly covered with a test.

-- 
Laters,
Toon

[PATCH 2/3] builtin/grep: read sparse directory trees via passed-in repository

[Patrick Steinhardt]

When iterating through the index in `grep_cache()` we may encounter
sparse directory entries. In that case we read the tree object they
point to so that we can recurse into it via `grep_tree()`. We do so via
`the_repository` though, even though `grep_cache()` may be called for a
submodule via `grep_submodule()`.

This issue is similar to the one fixed in the preceding commit, where
`grep_tree()` didn't use the correct repository. But in this case here
the issue cannot be triggered as we call `repo_read_gitmodules()` on the
submodule repository, and that function will cause us to read its index.
But because we only ever set `command_requires_full_index` for the
top-level repository we will convert the submodule's index into a full
index, and consequently we will never see sparse directory entries. This
is also documented as such in a NEEDSWORK comment in `grep_submodule()`.

Regardless of that the code is still wrong, and it may very easily turn
into a bug once we start to handle sparse indices for submodules. Adapt
it to use the correct repository to preempt such a future bug.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 22fb365eeac..cb518d8155e 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -560,7 +560,7 @@ static int grep_cache(struct grep_opt *opt,
 			void *data;
 			size_t size;
 
-			data = odb_read_object(the_repository->objects, &ce->oid,
+			data = odb_read_object(repo->objects, &ce->oid,
 					       &type, &size);
 			if (!data)
 				die(_("unable to read tree %s"), oid_to_hex(&ce->oid));

-- 
2.56.0.341.g393f52fd96.dirty

[PATCH 3/3] builtin/grep: read ".gitmodules" via passed-in repository

[Patrick Steinhardt]

When grepping through a list of objects with "--recurse-submodules", we
call `gitmodules_config_oid()` to load gitmodules for the specific
revision. But instead of passing `opt->repo` we pass `the_repository` to
that function, which is somewhat fishy.

This is again not a bug because `grep_objects()` is only ever called by
`cmd_grep()`, and consequently we know that `opt->repo` is always going
to be set to `the_repository`. But it's a code smell regardless of that,
so adapt the code to use `opt->repo` instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index cb518d8155e..22f0a1b0d0e 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -883,7 +883,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,
 		if (recurse_submodules) {
 			submodule_free(opt->repo);
 			obj_read_lock();
-			gitmodules_config_oid(the_repository, &real_obj->oid);
+			gitmodules_config_oid(opt->repo, &real_obj->oid);
 			obj_read_unlock();
 		}
 		if (grep_object(opt, pathspec, real_obj, list->objects[i].name,

-- 
2.56.0.341.g393f52fd96.dirty