Re: [PATCH 18/23] submodule: use submodule repos for object lookup

From: Stefan Beller <sbeller@google.com>
Date: Nov 15, 2018, 20:36 UTC
Message-ID: <CAGZ79kZ72eQPiQ_KW1QkiRqMFUAUpCQCHZdk3RSMVK7TGPE2aw@mail.gmail.com>
In-Reply-To: <20181115195416.21890-1-jonathantanmy@google.com>

On Thu, Nov 15, 2018 at 11:54 AM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 14 quoted lines

>
> > +/*
> > + * 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 void show_submodule_header(struct diff_options *o, const char *path,
> > +static struct repository *open_submodule(const char *path)
>
> The function documentation needs to be reworded - there's no "out", and
> the return value is now a possibly NULL pointer to struct repository.

Noted.

Show 15 quoted lines

>
> > +{
> > +     struct strbuf sb = STRBUF_INIT;
> > +     struct repository *out = xmalloc(sizeof(*out));
> > +
> > +     if (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {
> > +             strbuf_release(&sb);
> > +             free(out);
> > +             return NULL;
> > +     }
> > +
> > +     out->submodule_prefix = xstrdup(path);
>
> I've discussed this submodule_prefix line before [1] - do we really need
> this? Tests pass even if I remove this line.

We might not need it yet as the tests indicate, but it's the right thing to do:
/*
 * Path from the root of the top-level superproject down to this
 * repository.  This is only non-NULL if the repository is initialized
 * as a submodule of another repository.
 */

We're not (yet) using this string in our error reporting, but I anticipate that we'll do eventually.

> Other than that, this patch looks good.

Thanks, Stefan