> Try fetching a submodule by object id if the object id that the
> superproject points to, cannot be found.
Mention here the consequences of what happens when this attempt to fetch
fails. Also, this seems to be a case of "do or do not, there is no try"
- maybe it's better to say "Fetch commits by ID from the submodule's
origin if the submodule doesn't already contain the commit that the
superproject references" (note that there is no "Try" word, since the
consequence is to fail the entire command).
Also mention that this fetch is always from the origin.
> builtin/fetch used to only inspect submodules when they were fetched
> "on-demand", as in either on/off case it was clear whether the submodule
> needs to be fetched. However to know whether we need to try fetching the
> object ids, we need to identify the object names, which is done in this
> function check_for_new_submodule_commits(), so we'll also run that code
> in case the submodule recursion is set to "on".
>
> The submodule checks were done only when a ref in the superproject
> changed, these checks were extended to also be performed when fetching
> into FETCH_HEAD for completeness, and add a test for that too.
"inspect submodules" and "submodule checks" are unnecessarily vague to
me - might be better to just say "A list of new submodule commits are
already generated in certain conditions (by
check_for_new_submodule_commits()); this new feature invokes that
function in more situations".
> - if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> - (recurse_submodules != RECURSE_SUBMODULES_ON))
> - check_for_new_submodule_commits(&ref->new_oid);
> r = s_update_ref(msg, ref, 0);
> format_display(display, r ? '!' : '*', what,
> r ? _("unable to update local ref") : NULL,
> @@ -779,9 +776,6 @@ static int update_local_ref(struct ref *ref,
> strbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);
> strbuf_addstr(&quickref, "..");
> strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
> - if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> - (recurse_submodules != RECURSE_SUBMODULES_ON))
> - check_for_new_submodule_commits(&ref->new_oid);
> r = s_update_ref("fast-forward", ref, 1);
> format_display(display, r ? '!' : ' ', quickref.buf,
> r ? _("unable to update local ref") : NULL,
> @@ -794,9 +788,6 @@ static int update_local_ref(struct ref *ref,
> strbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);
> strbuf_addstr(&quickref, "...");
> strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
> - if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> - (recurse_submodules != RECURSE_SUBMODULES_ON))
> - check_for_new_submodule_commits(&ref->new_oid);
> r = s_update_ref("forced-update", ref, 1);
> format_display(display, r ? '!' : '+', quickref.buf,
> r ? _("unable to update local ref") : _("forced update"),
> @@ -892,6 +883,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
> ref->force = rm->peer_ref->force;
> }
>
> + if (recurse_submodules != RECURSE_SUBMODULES_OFF)
> + check_for_new_submodule_commits(&rm->old_oid);
As discussed above, indeed, check_for_new_submodule_commits() is now
invoked in more situations (not just when there is a local ref, and also
when recurse_submodules is _ON).
> @@ -1231,8 +1231,14 @@ struct submodule_parallel_fetch {
> int result;
>
> struct string_list changed_submodule_names;
> +
> + /* The submodules to fetch in */
> + struct fetch_task **oid_fetch_tasks;
> + int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
> };
Better to document as "Pending fetches by OIDs", I think. (These are not
fetches by default refspec, and are not already in progress.)
> +struct fetch_task {
> + struct repository *repo;
> + const struct submodule *sub;
> + unsigned free_sub : 1; /* Do we need to free the submodule? */
> +
> + /* fetch specific oids if set, otherwise fetch default refspec */
> + struct oid_array *commits;
> +};
I would document this as "Fetch in progress (if callback data) or
pending (if in oid_fetch_tasks in struct submodule_parallel_fetch)".
This potential confusion is why I wanted 2 separate types, as I wrote in
[1].
[1] https://public-inbox.org/git/20181026204106.132296-1-jonathantanmy@xxxxxxxxxx/
> +/**
> + * When a submodule is not defined in .gitmodules, we cannot access it
> + * via the regular submodule-config. Create a fake submodule, which we can
> + * work on.
> + */
> +static const struct submodule *get_non_gitmodules_submodule(const char *path)
Thanks, this is a good explanation.
> static int get_next_submodule(struct child_process *cp,
> struct strbuf *err, void *data, void **task_cb)
> {
> - int ret = 0;
> struct submodule_parallel_fetch *spf = data;
>
> for (; spf->count < spf->r->index->cache_nr; spf->count++) {
> - struct strbuf submodule_prefix = STRBUF_INIT;
> + int recurse_config;
Unnecessary local variable recurse_config, but that's not a big deal.
[snip the part where we use a heap-allocated task instead of a few
variables on the stack]
> - repo_clear(repo);
> - free(repo);
> - ret = 1;
> + spf->count++;
> + *task_cb = task;
And here we see why we need the heap-allocated task - it needs to go
into the callback data.
> + if (spf->oid_fetch_tasks_nr) {
> + struct fetch_task *task =
> + spf->oid_fetch_tasks[spf->oid_fetch_tasks_nr - 1];
> + struct strbuf submodule_prefix = STRBUF_INIT;
> + spf->oid_fetch_tasks_nr--;
> +
> + strbuf_addf(&submodule_prefix, "%s%s/",
> + spf->prefix, task->sub->path);
> +
> + child_process_init(cp);
> + prepare_submodule_repo_env_in_gitdir(&cp->env_array);
> + cp->git_cmd = 1;
> + cp->dir = task->repo->gitdir;
> +
> + argv_array_init(&cp->args);
> + argv_array_pushv(&cp->args, spf->args.argv);
> + argv_array_push(&cp->args, "on-demand");
> + argv_array_push(&cp->args, "--submodule-prefix");
> + argv_array_push(&cp->args, submodule_prefix.buf);
> +
> + /* NEEDSWORK: have get_default_remote from submodule--helper */
> + argv_array_push(&cp->args, "origin");
> + oid_array_for_each_unique(task->commits,
> + append_oid_to_argv, &cp->args);
> +
> + *task_cb = task;
> strbuf_release(&submodule_prefix);
> - if (ret) {
> - spf->count++;
> - return 1;
> - }
> + return 1;
> }
And if we ran out of submodules but have pending fetch-by-OID tasks, we
execute them.
> +static int commit_exists_in_sub(const struct object_id *oid, void *data)
> +{
> + struct repository *subrepo = data;
> +
> + enum object_type type = oid_object_info(subrepo, oid, NULL);
> +
> + return type != OBJ_COMMIT;
> +}
Should be commit_missing_in_sub.
> + /* Is this the second time we process this submodule? */
> + if (task->commits)
> + return 0;
Should be goto out, to clean up properly?
> +test_expect_success "fetch new submodule commits on-demand outside standard refspec" '
> + # add a second submodule and ensure it is around in downstream first
> + git clone submodule sub1 &&
> + git submodule add ./sub1 &&
> + git commit -m "adding a second submodule" &&
> + git -C downstream pull &&
> + git -C downstream submodule update --init --recursive &&
> +
> + git checkout --detach &&
> +
> + C=$(git -C submodule commit-tree -m "new change outside refs/heads" HEAD^{tree}) &&
> + git -C submodule update-ref refs/changes/1 $C &&
> + git update-index --cacheinfo 160000 $C submodule &&
> + test_tick &&
> +
> + D=$(git -C sub1 commit-tree -m "new change outside refs/heads" HEAD^{tree}) &&
> + git -C sub1 update-ref refs/changes/2 $D &&
> + git update-index --cacheinfo 160000 $D sub1 &&
> +
> + git commit -m "updated submodules outside of refs/heads" &&
> + E=$(git rev-parse HEAD) &&
> + git update-ref refs/changes/3 $E &&
> + (
> + cd downstream &&
> + git fetch --recurse-submodules origin refs/changes/3:refs/heads/my_branch &&
> + git -C submodule cat-file -t $C &&
> + git -C sub1 cat-file -t $D &&
> + git checkout --recurse-submodules FETCH_HEAD
> + )
> +'
It would be nicer if all these tests started from scratch (that is, an
empty repository), but they are understandable - thanks.
In addition to the tests here, I would like a test that checks that
submodule commits pointed to by interior superproject commits also work.
For example:
submodule:
B C
\ /
A
superproject:
current HEAD -> a commit -> the ref we're fetching
gitlink=A gitlink=B gitlink=C
When fetching recursively in the superproject, we should make sure that
both B and C are fetched in the submodule.
