Re: [PATCH 17/19] submodule: use submodule repos for object lookup

From: Stefan Beller <sbeller@google.com>
Date: Oct 13, 2018, 00:20 UTC
Message-ID: <CAGZ79kZTxoNMUyX+EHg0q3Ss2M-Etkf0yiw3E40U3VCt4QMwrQ@mail.gmail.com>
In-Reply-To: <20181011224052.191281-1-jonathantanmy@google.com>

On Thu, Oct 11, 2018 at 3:41 PM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 9 quoted lines

>
> > +/*
> > + * 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)

Show 12 quoted lines

> > +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.

Show 14 quoted lines

>
> > @@ -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!