"Heather Lapointe via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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,
> 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.
> + } 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 ;-)
> + 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.
> + 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.
> + 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.
> + }
> + // else, this is a file (or a submodule, but no pathspec->recurse_submodules)
> }
> return 0;
> }
