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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Oct 30, 2026 at 05:25:11PM +0100, Toon Claes wrote:
> 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?

Fair, can do.

> > 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?

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