Re: [PATCH v3 4/9] tree: handle submodule case for read_tree_at properly

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 27, 2022, 18:43 UTC
Message-ID: <20221027184335.1840969-1-jonathantanmy@google.com>
In-Reply-To: <d3d1738e670d5dbf1378fc5c3209b2e98234a771.1665973401.git.gitgitgadget@gmail.com>

"Heather Lapointe via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Heather Lapointe <alpha@alphaservcomputing.solutions>
> 
> This supports traversal into an actual submodule for read_tree_at.
> The logic is blocked on pathspec->recurse_submodules now,

What do you mean by "blocked"? Do you mean only that pathspec->recurse_submodules needs to be specified, or do you also mean that no caller specifies pathspec->recurse_submodules now, so this code path is never executed?

> but previously hadn't been executed due to all fn() cases
> returning early for submodules.

What do you mean by "previously hadn't been executed due to..."? At a glance, I would think that this new logic is introduced in this patch, so of course it would never have been previously executed.

> +			if (repo_submodule_init(subrepo_p, r, submodule_rel_path, null_oid()))

I don't think this can be null_oid() here - it has to match the tree 
from which you constructed submodule_rel_path. (That tree may not be 
the tree at HEAD.)