Re: [PATCH v3 4/9] tree: handle submodule case for read_tree_at properly
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 17, 2022, 13:56 UTC
- Message-ID
- <xmqqsfjm1rl6.fsf@gitster.g>
- In-Reply-To
- <d3d1738e670d5dbf1378fc5c3209b2e98234a771.1665973401.git.gitgitgadget@gmail.com>
"Heather Lapointe via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 40 quoted lines
> 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,
> but previously hadn't been executed due to all fn() cases
> returning early for submodules.
>
> Signed-off-by: Heather Lapointe <alpha@alphaservcomputing.solutions>
> ---
> tree.c | 88 ++++++++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 61 insertions(+), 27 deletions(-)
>
> diff --git a/tree.c b/tree.c
> index 13f9173d45e..2a087c010f9 100644
> --- a/tree.c
> +++ b/tree.c
> @@ -8,6 +8,7 @@
> #include "alloc.h"
> #include "tree-walk.h"
> #include "repository.h"
> +#include "pathspec.h"
>
> const char *tree_type = "tree";
>
> @@ -47,40 +48,73 @@ int read_tree_at(struct repository *r,
> return -1;
> }
>
> - if (S_ISDIR(entry.mode))
> + if (S_ISDIR(entry.mode)) {
> oidcpy(&oid, &entry.oid);
> + len = tree_entry_len(&entry);
> + strbuf_add(base, entry.path, len);
> + strbuf_addch(base, '/');
> + retval = read_tree_at(r, lookup_tree(r, &oid),
> + base, pathspec,
> + fn, context);
> + strbuf_setlen(base, oldlen);
> + if (retval)
> + return -1;The diff output makes it appear as if we are now adding many extra processing to a normal directory case, but it actually folds the code that was originally outside the if/else if/ cascade here. So I think this is not breaking the normal directory case.
Show 5 quoted lines
> + } else if (pathspec->recurse_submodules && S_ISGITLINK(entry.mode)) {
> + struct commit *commit;
> + struct repository subrepo;
> + struct repository* subrepo_p = &subrepo;
> + struct tree* submodule_tree;In our codebase, star/asterisk for a pointer declaration sticks to the variable, not the type.
cf. Documentation/CodingGuidelines
> + char *submodule_rel_path;
Funny that the new code sometimes gets it right ;-)
Show 6 quoted lines
> + int name_base_len = 0; > + > + len = tree_entry_len(&entry); > + strbuf_add(base, entry.path, len); > + submodule_rel_path = base->buf; > + // repo_submodule_init expects a path relative to submodule_prefix
We avoid // comments.
Show 17 quoted lines
> + if (r->submodule_prefix) {
> + name_base_len = strlen(r->submodule_prefix);
> + // we should always expect to start with submodule_prefix
> + assert(!strncmp(submodule_rel_path, r->submodule_prefix, name_base_len));
> + // strip the prefix
> + submodule_rel_path += name_base_len;
> + // if submodule_prefix doesn't end with a /, we want to get rid of that too
> + if (is_dir_sep(submodule_rel_path[0])) {
> + submodule_rel_path++;
> + }
> + }
> +
> + if (repo_submodule_init(subrepo_p, r, submodule_rel_path, null_oid()))
> + die("couldn't init submodule %s", base->buf);
> +
> + if (repo_read_index(subrepo_p) < 0)
> + die("index file corrupt");Why? You are going to ask the object store of the submodule repository, and to do so you do not need to have its index read into the core.
Show 46 quoted lines
> + commit = lookup_commit(subrepo_p, &entry.oid);
> if (!commit)
> - die("Commit %s in submodule path %s%s not found",
> + die("Commit %s in submodule path %s not found",
> oid_to_hex(&entry.oid),
> - base->buf, entry.path);
> -
> - // FIXME: This is the wrong repo instance (it refers to the superproject)
> - // it will always fail as is (will fix in later patch)
> - // This current codepath isn't executed by any existing callbacks
> - // so it wouldn't show up as an issue at this time.
> - if (repo_parse_commit(r, commit))
> - die("Invalid commit %s in submodule path %s%s",
> + base->buf);
> +
> + if (repo_parse_commit(subrepo_p, commit))
> + die("Invalid commit %s in submodule path %s",
> oid_to_hex(&entry.oid),
> - base->buf, entry.path);
> + base->buf);
>
> - oidcpy(&oid, get_commit_tree_oid(commit));
> - }
> - else
> - continue;
> + submodule_tree = repo_get_commit_tree(subrepo_p, commit);
> + oidcpy(&oid, submodule_tree ? &submodule_tree->object.oid : NULL);
>
> - len = tree_entry_len(&entry);
> - strbuf_add(base, entry.path, len);
> - strbuf_addch(base, '/');
> - retval = read_tree_at(r, lookup_tree(r, &oid),
> - base, pathspec,
> - fn, context);
> - strbuf_setlen(base, oldlen);
> - if (retval)
> - return -1;
> + strbuf_addch(base, '/');
> +
> + retval = read_tree_at(subrepo_p, lookup_tree(subrepo_p, &oid),
> + base, pathspec,
> + fn, context);
> + if (retval)
> + die("failed to read tree for %s", base->buf);
> + strbuf_setlen(base, oldlen);
> + repo_clear(subrepo_p);This is a lot of new code, which must be done correctly. An easier way out to use the add_submodule_odb() trick that the original code assumed becomes somewhat tempting (I guess we would do that in fn() that would tell us to recurse into this codepath upon seeing a gitlink entry). Then we wouldn't have had to touch any tree() calls that were taught to take "struct repository *" in earlier steps in this series.
But at some point, we would need to bite the bullet and plumb the repository pointer through the callchain of more APIs, and this may be that point. I dunno.
Show 5 quoted lines
> + } > + // else, this is a file (or a submodule, but no pathspec->recurse_submodules) > } > return 0; > }