builtin/grep: fix reading objects from wrong repository
cover builtin/grep: fix reading objects from wrong repository
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
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().
--- builtin/grep.c | 2 +- t/t7814-grep-recurse-submodules.sh | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -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)"),
t/t7814-grep-recurse-submodules.sh +23 -0
@@ -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
Signature · 1 line
2.56.0.341.g393f52fd96.dirty
Re: [PATCH 1/3] builtin/grep: read submodule trees via their repository
Quoted 19 lines · 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 callsgrep_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:
Quoted 5 lines
$ 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?
Quoted 15 lines
Fix this by reading nested trees via
opt->repo, which is set to the submodule's repository viagrep_submodule().Reported-by Root <[email protected]>Signed-off-by Patrick Steinhardt <[email protected]>--- builtin/grep.c | 2 +- t/t7814-grep-recurse-submodules.sh | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-)builtin/grep.c +0
-0
builtin/grep.c static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, · line 656
Quoted 5 lines
builtin/grep.c +1
-1@@ -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.
t/t7814-grep-recurse-submodules.sh test_expect_success 'check scope of core.useReplaceRefs' ' · line 635
Quoted 13 lines
builtin/grep.c +0
-0&entry.oid, &type, &size);if (!data) die(_("unable to read tree (%s)"),t/t7814-grep-recurse-submodules.sh +2
-0@@ -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?
Quoted 11 lines
t/t7814-grep-recurse-submodules.sh +11
-0+ 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" &&
t/t7814-grep-recurse-submodules.sh test_expect_success 'check scope of core.useReplaceRefs' ' · line 657
Quoted 10 lines
t/t7814-grep-recurse-submodules.sh +9
-0+ + 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.
Quoted 2 lines
Signature · 1 line
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.
Signature · 2 lines
Laters, Toon
Re: [PATCH 1/3] builtin/grep: read submodule trees via their repository
Quoted 21 lines · Toon Claes
Quoted 19 lines · 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 callsgrep_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:
Quoted 7 lines
Quoted 5 lines
$ 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?
Fair, can do.
Quoted 8 lines
Quoted 8 lines
t/t7814-grep-recurse-submodules.sh +0
-0@@ -631,4 +631,27 @@ test_expect_success 'check scope of core.useReplaceRefs' ' git -C base grep --cached --recurse-submodules C '
Quoted 4 lines
t/t7814-grep-recurse-submodules.sh test_expect_success 'check scope of core.useReplaceRefs' ' · line 635
Quoted 2 lines
t/t7814-grep-recurse-submodules.sh +2
-0+test_expect_success 'grep --recurse-submodules with tree-ish and nested directory' ' + rm -rf sub super &&Small nit, shall we use
test_when_finishedhere?
Normally I would, but in this case it would have felt weird to have "rm -rf sub super && test_when_finished "rm -rf sub super". We have to delete the old repos because they exist already, so arguably the correct location to add the test_when_finished would have been that preceding test. But I'd propose that this can be done at a later time?
Patrick
Re: [PATCH 1/3] builtin/grep: read submodule trees via their repository
Quoted 7 lines · Patrick Steinhardt
Quoted 1 line
Small nit, shall we use
test_when_finishedhere?Normally I would, but in this case it would have felt weird to have "rm -rf sub super &&
test_when_finished"rm -rf sub super". We have to delete the old repos because they exist already, so arguably the correct location to add thetest_when_finishedwould have been that preceding test. But I'd propose that this can be done at a later time?
Sure.
Signature · 2 lines
Laters, Toon
PATCH 2/3 builtin/grep: read sparse directory trees via passed-in repository
In reply to Patrick SteinhardtWhen 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.
--- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -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));
Signature · 1 line
2.56.0.341.g393f52fd96.dirty
PATCH 3/3 builtin/grep: read ".gitmodules" via passed-in repository
In reply to Patrick SteinhardtWhen 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.
--- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -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,
Signature · 1 line
2.56.0.341.g393f52fd96.dirty
v2 cover builtin/grep: fix reading objects from wrong repository
In reply to Patrick SteinhardtHi,
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.
Changes in v2:
- Mention that this was released as part of Git 2.56.0.
- Link to v1: https://patch.msgid.link/[email protected]
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(-)
Range-diff versus v1:
1: e9ffa0cdee5 ! 1: 46d165815b5 builtin/grep: read submodule trees via their repository
@@ Commit message
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.
+ (builtin/grep: stop registering submodule ODB as source, 2026-09-11),
+ which was released as part of Git 2.56, 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
2: dfe45977b15 = 2: f7516e29f68 builtin/grep: read sparse directory trees via passed-in repository 3: 4b914d9d07d = 3: dc19e6589d4 builtin/grep: read ".gitmodules" via passed-in repository
---
base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
change-id: 20261030-pks-grep-recurse-submodule-20177cc7794c
PATCH v2 1/3 builtin/grep: read submodule trees via their repository
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), which was released as part of Git 2.56, 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().
--- builtin/grep.c | 2 +- t/t7814-grep-recurse-submodules.sh | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -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)"),
t/t7814-grep-recurse-submodules.sh +23 -0
@@ -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
Signature · 1 line
2.56.0.500.g5d06dc02cd.dirty
PATCH v2 2/3 builtin/grep: read sparse directory trees via passed-in repository
In reply to Patrick SteinhardtWhen 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.
--- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -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));
Signature · 1 line
2.56.0.500.g5d06dc02cd.dirty
PATCH v2 3/3 builtin/grep: read ".gitmodules" via passed-in repository
In reply to Patrick SteinhardtWhen 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.
--- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -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,
Signature · 1 line
2.56.0.500.g5d06dc02cd.dirty