On Thu, Oct 11, 2018 at 3:41 PM Jonathan Tan <[email protected]> wrote:
>
> > +/*
> > + * Initialize 'out' based on the provided submodule path.
> > + *
> > + * Unlike repo_submodule_init, this tolerates submodules not present
> > + * in .gitmodules. NEEDSWORK: The repo_submodule_init behavior is
> > + * preferrable. This function exists only to preserve historical behavior.
>
> What do you mean by "The repo_submodule_init behavior is preferable"?
Preferable in the sense that it follows the definition of a submodule, but this
here works for "any repo" that happens to be at the gitlink.
> If
> we need to preserve historical behavior, then it seems that the most
> preferable one is something that meets our needs (like open_submodule()
> in this patch).
Yes, I'll reword to drop the preferrable, but still state the difference.
I wonder if instead we'd want to introduce a
repo_submodule_init(struct repository *submodule \
struct repository *superproject \
const char *path, \
int tolerate_lookalikes)
Another patch proposes to replace the path
by a struct submodule, but for lookalikes, we do not have
a struct submodule to begin with (though in the other
patches we cook up a fake entry in the submodule config)
> > +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);
>
> Do we need to set submodule_prefix?
Good point! Thanks for catching this.
>
> > @@ -507,7 +533,7 @@ static void show_submodule_header(struct diff_options
> > *o, const char *path,
> > else if (is_null_oid(two))
> > message = "(submodule deleted)";
> >
> > - if (add_submodule_odb(path)) {
> > + if (open_submodule(sub, path) < 0) {
>
> This function, as a side effect, writes the open repository to "sub" for
> its caller to use. I think it's better if its callers open "sub" and
> then pass it to show_submodule_header() if successful. If opening the
> submodule is expected to fail sometimes (like it seems here), then we
> can allow callers to also pass NULL, and document what happens when NULL
> is passed.
That looks intriguing, I'll take a look. Note that we also pass
in **left and **right to have it assigned in there.
>
> Also, repositories open in this way should also be freed.
Yes, thanks!
