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

[Toon Claes <toon@iotcl.com>]

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