git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
JTJonathan Tan <jonathantanmy@google.com>
Date
Dec 5, 2018, 01:07 UTC
Message-ID
<20181205010704.84790-1-jonathantanmy@google.com>
In-Reply-To
<20181129002756.167615-10-sbeller@google.com>
> 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.
Show 10 quoted lines
> 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".

Show 32 quoted lines
> -		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, &current->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, &current->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).

Show 9 quoted lines
> @@ -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.)

Show 8 quoted lines
> +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@google.com/
Show 6 quoted lines
> +/**
> + * 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.
Show 9 quoted lines
>  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]

Show 5 quoted lines
> -			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.

Show 33 quoted lines
> +	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.

Show 8 quoted lines
> +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?
Show 30 quoted lines
> +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.

Previous: Stefan BellerNext: Stefan Beller
Message 15 of 21 in “[PATCHv2 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip”
  1. Stefan BellerNov 29, 2018
  2. 1/9 sha1-array: provide oid_array_filterStefan Beller, Nov 29, 2018
  3. 2/9 submodule.c: fix indentationStefan Beller, Nov 29, 2018
  4. 3/9 submodule.c: sort changed_submodule_names before searching itStefan Beller, Nov 29, 2018
  5. Jonathan TanDec 5, 2018
  6. 4/9 submodule.c: tighten scope of changed_submodule_names structStefan Beller, Nov 29, 2018
  7. 5/9 submodule: store OIDs in changed_submodule_namesStefan Beller, Nov 29, 2018
  8. 6/9 repository: repo_submodule_init to take a submodule structStefan Beller, Nov 29, 2018
  9. 7/9 submodule: migrate get_next_submodule to use repository structsStefan Beller, Nov 29, 2018
  10. Jonathan TanDec 5, 2018
  11. Jonathan NiederFeb 2, 2019
  12. 8/9 submodule.c: fetch in submodules git directory instead of in worktreeStefan Beller, Nov 29, 2018
  13. Jonathan TanDec 5, 2018
  14. 9/9 fetch: try fetching submodules if needed objects were not fetchedStefan Beller, Nov 29, 2018
  15. Jonathan TanDec 5, 2018
  16. fetch: ensure submodule objects fetchedStefan Beller, Dec 6, 2018
  17. Junio C HamanoDec 9, 2018
  18. Junio C HamanoDec 5, 2018
  19. Stefan BellerDec 6, 2018
  20. Josh SteadmonDec 7, 2018
  21. Jonathan NiederJan 15, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.