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

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

From
JTJonathan Tan <jonathantanmy@google.com>
Date
Oct 26, 2018, 20:41 UTC
Message-ID
<20181026204106.132296-1-jonathantanmy@google.com>
In-Reply-To
<20181025233231.102245-10-sbeller@google.com>
Show 9 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.

[1] https://public-inbox.org/git/20181018003954.139498-1-jonathantanmy@google.com/
> The try does not happen when the "git fetch" done at the
> superproject is not storing the fetched results in remote
> tracking branches (i.e. instead just recording them to
> FETCH_HEAD) in this step. A later patch will fix this.
E.g. here, I said that there was no remote tracking branch involved.
> -		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.

Show 7 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".

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.)

Show 15 quoted lines
> +static const struct submodule *get_default_submodule(const char *path)
> +{
> +	struct submodule *ret = NULL;
> +	const char *name = default_name_or_path(path);
> +
> +	if (!name)
> +		return NULL;
> +
> +	ret = xmalloc(sizeof(*ret));
> +	memset(ret, 0, sizeof(*ret));
> +	ret->path = name;
> +	ret->name = name;
> +
> +	return (const struct submodule *) ret;
> +}

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().

Show 15 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.

> +	if (spf->fetch_specific_oids_nr) {
> +		struct get_next_submodule_task *task = spf->fetch_specific_oids[spf->fetch_specific_oids_nr - 1];
Break lines to 80.
> +		argv_array_pushv(&cp->args, spf->args.argv);
> +		argv_array_push(&cp->args, "on-demand");
Same comment about "on-demand" as in my previous e-mail.
> +		argv_array_push(&cp->args, "--submodule-prefix");
> +		argv_array_push(&cp->args, submodule_prefix.buf);
> +
> +		/* NEEDSWORK: have get_default_remote from s--h */
Same comment about "s--h" as in my previous e-mail.
Show 10 quoted lines
> +	commits = it->util;
> +	oid_array_filter(commits,
> +			 commit_exists_in_sub,
> +			 task->repo);
> +
> +	/* 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.
> 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.

Previous: Stefan BellerNext: Stefan Beller
Message 15 of 19 in “Resending sb/submodule-recursive-fetch-gets-the-tip”
  1. 00/10 Resending sb/submodule-recursive-fetch-gets-the-tipStefan Beller, Oct 25, 2018
  2. 01/10 sha1-array: provide oid_array_filterStefan Beller, Oct 25, 2018
  3. 02/10 submodule.c: fix indentationStefan Beller, Oct 25, 2018
  4. 03/10 submodule.c: sort changed_submodule_names before searching itStefan Beller, Oct 25, 2018
  5. 04/10 submodule.c: tighten scope of changed_submodule_names structStefan Beller, Oct 25, 2018
  6. 05/10 submodule: store OIDs in changed_submodule_namesStefan Beller, Oct 25, 2018
  7. Jonathan TanOct 26, 2018
  8. 06/10 repository: repo_submodule_init to take a submodule structStefan Beller, Oct 25, 2018
  9. Jonathan TanOct 26, 2018
  10. Stefan BellerOct 26, 2018
  11. 07/10 submodule: migrate get_next_submodule to use repository structsStefan Beller, Oct 25, 2018
  12. Jonathan TanOct 26, 2018
  13. 08/10 submodule.c: fetch in submodules git directory instead of in worktreeStefan Beller, Oct 25, 2018
  14. 09/10 fetch: try fetching submodules if needed objects were not fetchedStefan Beller, Oct 25, 2018
  15. Jonathan TanOct 26, 2018
  16. Stefan BellerNov 29, 2018
  17. 10/10 builtin/fetch: check for submodule updates in any ref updateStefan Beller, Oct 25, 2018
  18. Jonathan TanOct 26, 2018
  19. Junio C HamanoOct 29, 2018

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.