> This converts the 'show_submodule_header' function to use
> the repository API properly, such that the submodule objects
> are not added to the main object store.
There is also a side effect in that the submodule now needs to pass all
the checks done by repo_init() instead of merely having the "objects/"
directory. Can you add information about this to the commit message?
Also, which tests exercise this functionality? Mention them in the
commit message.
> +/*
> + * Initialize 'out' based on the provided submodule path.
> + *
> + * Unlike repo_submodule_init, this tolerates submodules not present
> + * in .gitmodules. This function exists only to preserve historical behavior,
> + *
> + * Returns 0 on success, -1 when the submodule is not present.
> + */
> +static int open_submodule(struct repository *out, const char *path)
> +{
> + struct strbuf sb = STRBUF_INIT;
> +
> + if (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {
> + strbuf_release(&sb);
> + return -1;
> + }
> +
> + out->submodule_prefix = xstrdup(path);
> + out->submodule_prefix = xstrfmt("%s%s/",
> + the_repository->submodule_prefix ?
> + the_repository->submodule_prefix :
> + "", path);
You seem to say here [1] that we don't need submodule_prefix, but you're
instead setting it twice? :-)
[1]
https://public-inbox.org/git/cagz79kztxonmuyx+ehg0q3ss2m-etkf0yiw3e40u3vct4qm...@mail.gmail.com/
> +/*
> + * Helper function to display the submodule header line prior to the full
> + * summary output.
> + *
> + * If it can locate the submodule git directory it will create a repository
> + * handle for the submodule and lookup both the left and right commits and
> + * put them into the left and right pointers.
> */
> -static void show_submodule_header(struct diff_options *o, const char *path,
> +static void show_submodule_header(struct diff_options *o,
> + const char *path,
> struct object_id *one, struct object_id *two,
> unsigned dirty_submodule,
> + struct repository *sub,
> struct commit **left, struct commit **right,
> struct commit_list **merge_bases)
> {
Location of the submodule git directory is done by the caller of this
function, not by this function. Also this function doesn't create any
repository handles. The documentation probably needs to be updated. Also
mention what happens if sub is NULL.
> @@ -563,16 +596,20 @@ void show_submodule_summary(struct diff_options *o,
> const char *path,
> struct rev_info rev;
> struct commit *left = NULL, *right = NULL;
> struct commit_list *merge_bases = NULL;
> + struct repository subrepo, *sub = &subrepo;
> +
> + if (open_submodule(&subrepo, path) < 0)
> + sub = NULL;
Handling of the subrepo and *sub seems clumsy - it might be better to
just let open_submodule() return a struct repository pointer.
Previous 17 patches look good - most are the same as the previous
version, which I already was happy with, and I see that the patch I
requested in [2] was added.
[2]
https://public-inbox.org/git/20181011221114.186377-1-jonathanta...@google.com/
