On Fri, Oct 26, 2018 at 1:41 PM Jonathan Tan <[email protected]> wrote:
>
> > 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.
>
> > - 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.
>
> > @@ -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.
> 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.
>
> > +
> > +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
> > + /* 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.
>
> > 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
