Re: [PATCH 09/10] fetch: try fetching submodules if needed objects were not fetched

From: Stefan Beller <sbeller@google.com>
Date: Nov 29, 2018, 00:30 UTC
Message-ID: <CAGZ79kYAyf9-9dNaaok-z-7WT9E-dC7RX4Sxd-9HBc2Qvr5mgw@mail.gmail.com>
In-Reply-To: <20181026204106.132296-1-jonathantanmy@google.com>

On Fri, Oct 26, 2018 at 1:41 PM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 14 quoted lines

>
> > But this default fetch is not sufficient, as a newly fetched commit in
> > the superproject could point to a commit in the submodule that is not
> > in the default refspec. This is common in workflows like Gerrit's.
> > When fetching a Gerrit change under review (from refs/changes/??), the
> > commits in that change likely point to submodule commits that have not
> > been merged to a branch yet.
> >
> > Try fetching a submodule by object id if the object id that the
> > superproject points to, cannot be found.
>
> I see that these suggestions of mine (from [1]) was implemented, but not
> others. If you disagree, that's fine, but I think they should be
> discussed.

ok.

Show 7 quoted lines

>
> > -             if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> > -                 (recurse_submodules != RECURSE_SUBMODULES_ON))
> > +             if (recurse_submodules != RECURSE_SUBMODULES_OFF)
>
> I think the next patch should be squashed into this patch. Then you can
> say that these are now redundant and can be removed.

ok.

Show 11 quoted lines

>
> > @@ -1218,8 +1218,12 @@ struct submodule_parallel_fetch {
> >       int result;
> >
> >       struct string_list changed_submodule_names;
> > +     struct get_next_submodule_task **fetch_specific_oids;
> > +     int fetch_specific_oids_nr, fetch_specific_oids_alloc;
> >  };
>
> Add documentation for fetch_specific_oids. Also, it might be better to
> call these oid_fetch_tasks and the struct, "struct fetch_task".

ok.

Show 11 quoted lines

> Here, struct get_next_submodule_task is used for 2 different things:
>  (1) After the first round fetch, fetch_finish() uses it to determine if
>      a second round is needed.
>  (2) In submodule_parallel_fetch.fetch_specific_oids, to tell the
>      parallel runner (through get_next_submodule_task()) to do this
>      fetch.
>
> I think that it's better to have 2 different structs for these 2
> different uses. (Note that task_cb can be NULL for the second round.
> Unless we plan to recheck the OIDs? Currently we recheck them, but we
> don't do anything either way.)

I think it is easier to only have one struct until we have substantially more to communicate. (1) is a boolean answer, for which (non-)NULL is sufficient.

> I think that this is best described as the submodule that has no entry
> in .gitmodules? Maybe call it "get_non_gitmodules_submodule" and
> document it with a similar comment as in get_submodule_repo_for().

done.

Show 19 quoted lines

>
> > +
> > +static struct get_next_submodule_task *get_next_submodule_task_create(
> > +     struct repository *r, const char *path)
> > +{
> > +     struct get_next_submodule_task *task = xmalloc(sizeof(*task));
> > +     memset(task, 0, sizeof(*task));
> > +
> > +     task->sub = submodule_from_path(r, &null_oid, path);
> > +     if (!task->sub) {
> > +             task->sub = get_default_submodule(path);
> > +             task->free_sub = 1;
> > +     }
> > +
> > +     return task;
> > +}
>
> Clearer to me would be to make get_next_submodule_task_create() return
> NULL if submodule_from_path() returns NULL.

I doubled down on this one and return NULL when get_default_submodule (now renamed to get_non_gitmodules_submodule) returns NULL, as then we can move the free() from get_next_submodule here and there we'll just have

    task = fetch_task_create(spf->r, ce->name);
    if (!task)
        continue;

which helps get_next_submodule to stay readable.

> Same comment about "on-demand" as in my previous e-mail.

I'd want to push back on refactoring and defer that to a later series.

> Break lines to 80.

[...]

> Same comment about "s--h" as in my previous e-mail.

done

Show 7 quoted lines

> > +     /* Are there commits that do not exist? */
> > +     if (commits->nr) {
> > +             /* We already tried fetching them, do not try again. */
> > +             if (task->commits)
> > +                     return 0;
>
> Same comment about "task->commits" as in my previous e-mail.

Good call. I reordered the function read easier and added a comment on any early return how it could happen.

Show 6 quoted lines

>
> > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> > index 6c2f9b2ba2..5a75b57852 100755
>
> One more thing to test is the case where a submodule doesn't have a
> .gitmodules entry.

added a test.

I just resent the series.

Stefan