Re: [PATCH v3 4/9] tree: handle submodule case for read_tree_at properly
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Oct 17, 2022, 13:48 UTC
- Message-ID
- <bf890f94-10b7-e344-fe20-8ce129923c4f@dunelm.org.uk>
- In-Reply-To
- <d3d1738e670d5dbf1378fc5c3209b2e98234a771.1665973401.git.gitgitgadget@gmail.com>
Hi Heather
On 17/10/2022 03:23, Heather Lapointe via GitGitGadget wrote:
> 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,
I'm struggling to understand what this is saying.
Show 44 quoted lines
> 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);
> - else if (S_ISGITLINK(entry.mode)) {
> - struct commit *commit;
>
> - commit = lookup_commit(r, &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;
> + } else if (pathspec->recurse_submodules && S_ISGITLINK(entry.mode)) {
> + struct commit *commit;
> + struct repository subrepo;
> + struct repository* subrepo_p = &subrepo;Normally we'd just use &subrepo wherever we want a pointer rather than defining an alias like this. For example it is common to see
struct strbuf buf = STRBUF_INIT;
strbuf_add(&buf, "hello world");
we don't define a buf_p variable
Show 8 quoted lines
> + struct tree* submodule_tree; > + char *submodule_rel_path; > + 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
I found the comments in this section code code helpful, but single line comments should be formatted as /* single line comment */
> + 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));Rather than using assert() we tend to use BUG() as that then provides a grep-able message. It also means that we wont have an out of bounds access if the invariant is violated when compiling with NDEBUG. So we could drop the comment and write
if (strncmp(submodule_rel_path, r->submodule_prefix, name_base_len)
BUG("missing submodule path prefix");> + // 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
I think there is a typo here - if the prefix does end with a / then we're dropping it.
Show 23 quoted lines
> + 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");
> +
> + 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))Style comment for the patch that added this code. Multi-line comments should be formatted as /* * Multi-line * comment */
Best Wishes
Phillip
Show 39 quoted lines
> - 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);
> + }
> + // else, this is a file (or a submodule, but no pathspec->recurse_submodules)
> }
> return 0;
> }