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

Re: [PATCH v3] submodule: fetch missing objects from default remote

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 22, 2026, 18:49 UTC
Message-ID
<xmqq5x8to53y.fsf@gitster.g>
In-Reply-To
<20260122152722.866341-1-nasser.grainawi@oss.qualcomm.com>
Nasser Grainawi <nasser.grainawi@oss.qualcomm.com> writes:
Show 17 quoted lines
> When be76c21282 (fetch: ensure submodule objects fetched, 2018-12-06)
> added support for fetching a missing submodule object by id, it
> hardcoded the remote name as "origin" and deferred anything more
> complicated for a later patch. Implement the NEEDSWORK item to remove
> the hardcoded assumption by adding and using a submodule helper subcmd
> 'get-default-remote'. Fixing this lets 'git fetch --recurse-submodules'
> succeed when the fetched commit(s) in the superproject trigger a
> submodule fetch, and that submodule's default remote name is not
> "origin".
>
> Add non-"origin" remote tests to t5526-fetch-submodules.sh and
> t5572-pull-submodule.sh demonstrating this works as expected and add
> dedicated tests for get-default-remote.
>
> Signed-off-by: Nasser Grainawi <nasser.grainawi@oss.qualcomm.com>
> Reviewed-by: Jacob Keller <jacob.keller@gmail.com>
> ---

Thanks. Jacob, this v3 is not exactly the same as v1 that you reviewed (and range-diff relative to v2 does not capture what got changed between the version you saw and this version), but I just checked that they are "essentially identical" except for the proposed log message. Are you happy with having your Reviewed-by on this version?

Show 35 quoted lines
>  builtin/submodule--helper.c             |  38 +++++
>  submodule.c                             |  17 ++-
>  t/meson.build                           |   1 +
>  t/t5526-fetch-submodules.sh             |  52 +++++++
>  t/t5572-pull-submodule.sh               |  21 ++-
>  t/t7425-submodule-get-default-remote.sh | 186 ++++++++++++++++++++++++
>  6 files changed, 312 insertions(+), 3 deletions(-)
>  create mode 100755 t/t7425-submodule-get-default-remote.sh
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d537ab087a..b180a24091 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -112,6 +112,43 @@ static int get_default_remote_submodule(const char *module_path, char **default_
>  	return 0;
>  }
>  
> +static int module_get_default_remote(int argc, const char **argv, const char *prefix,
> +				     struct repository *repo UNUSED)
> +{
> +	const char *path;
> +	char *resolved_path = NULL;
> +	char *default_remote = NULL;
> +	int code;
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		N_("git submodule--helper get-default-remote <path>"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> +	if (argc != 1)
> +		usage_with_options(usage, options);

Hmph, I am not sure what is going on. What are we getting out of parse_options() here? Would it be the same to see if we got anything remaining on the command line by checking argc and call usage_with_options() without calling parse_options(), or am I missing something?

Show 13 quoted lines
> +	path = argv[0];
> +	if (prefix && *prefix && !is_absolute_path(path)) {
> +		resolved_path = xstrfmt("%s%s", prefix, path);
> +		path = resolved_path;
> +	}
> +
> +	code = get_default_remote_submodule(path, &default_remote);
> +	if (code) {
> +		free(resolved_path);
> +		return code;
> +	}
> +
> +	printf("%s\n", default_remote);

Do we know that the value of default_remote has no funny bytes in it, like newline? In the end the name has to become part of refs/remotes/<name>/HEAD that has to be a valid refname, so not giving any facility to quote funny bytes and allowing the caller of this helper to assume a LF terminated single line should be fine, so I am guessing that the answer is yes, but I offhand do not know how we know that we do not have to worry about such a situation in the code path that begins with get_default_remote_submodule().

Show 34 quoted lines
> diff --git a/submodule.c b/submodule.c
> index 40a5c6fb9d..6599657f34 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1706,6 +1706,8 @@ static int get_next_submodule(struct child_process *cp, struct strbuf *err,
>  	if (spf->oid_fetch_tasks_nr) {
>  		struct fetch_task *task =
>  			spf->oid_fetch_tasks[spf->oid_fetch_tasks_nr - 1];
> +		struct child_process cp_remote = CHILD_PROCESS_INIT;
> +		struct strbuf remote_name = STRBUF_INIT;
>  		spf->oid_fetch_tasks_nr--;
>  
>  		child_process_init(cp);
> @@ -1719,8 +1721,19 @@ static int get_next_submodule(struct child_process *cp, struct strbuf *err,
>  		strvec_pushf(&cp->args, "--submodule-prefix=%s%s/",
>  			     spf->prefix, task->sub->path);
>  
> -		/* NEEDSWORK: have get_default_remote from submodule--helper */
> -		strvec_push(&cp->args, "origin");
> +		cp_remote.git_cmd = 1;
> +		strvec_pushl(&cp_remote.args, "submodule--helper",
> +			     "get-default-remote", task->sub->path, NULL);
> +
> +		if (!capture_command(&cp_remote, &remote_name, 0)) {
> +			strbuf_trim_trailing_newline(&remote_name);
> +			strvec_push(&cp->args, remote_name.buf);
> +		} else {
> +			/* Fallback to "origin" if the helper fails */
> +			strvec_push(&cp->args, "origin");
> +		}
> +		strbuf_release(&remote_name);
> +
>  		oid_array_for_each_unique(task->commits,
>  					  append_oid_to_argv, &cp->args);
OK, this part is quite straight-forward.
Show 35 quoted lines
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 5e566205ba..a5a273b392 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -929,6 +929,58 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup
>  	)
>  '
>  
> +test_expect_success 'fetch --recurse-submodules works with custom remote names' '
> +	# depends on the previous test for setup
> +
> +	# Rename the remote in sub1 from "origin" to "custom_remote"
> +	git -C downstream/sub1 remote rename origin custom_remote &&
> +
> +	# Create new commits in the original submodules
> +	C=$(git -C submodule commit-tree -m "change outside refs/heads for custom remote" HEAD^{tree}) &&
> +	git -C submodule update-ref refs/changes/custom1 $C &&
> +	git update-index --cacheinfo 160000 $C submodule &&
> +	test_tick &&
> +
> +	D=$(git -C sub1 commit-tree -m "change outside refs/heads for custom remote" HEAD^{tree}) &&
> +	git -C sub1 update-ref refs/changes/custom2 $D &&
> +	git update-index --cacheinfo 160000 $D sub1 &&
> +
> +	git commit -m "updated submodules outside of refs/heads for custom remote" &&
> +	E=$(git rev-parse HEAD) &&
> +	git update-ref refs/changes/custom3 $E &&
> +	(
> +		cd downstream &&
> +		git fetch --recurse-submodules origin refs/changes/custom3:refs/heads/my_other_branch &&
> +		git -C submodule cat-file -t $C &&
> +		git -C sub1 cat-file -t $D &&
> +		git checkout --recurse-submodules FETCH_HEAD
> +	)
> +'

Hmph, these overly long lines are eyesore. I wonder if we can do something about them?

I also wonder if we want to make sure we are getting from the remote that is given the custom name in a more direct way (instead of "we see that our fetch succeeds, and because there is no other remote, it must have gotten what is needed from the renamed one"), or is it too much paranoia?

Show 23 quoted lines
> +test_expect_success 'fetch new submodule commit on-demand in FETCH_HEAD from custom remote' '
> +	# depends on the previous test for setup
> +
> +	C=$(git -C submodule commit-tree -m "another change outside refs/heads for custom remote" HEAD^{tree}) &&
> +	git -C submodule update-ref refs/changes/custom4 $C &&
> +	git update-index --cacheinfo 160000 $C submodule &&
> +	test_tick &&
> +
> +	D=$(git -C sub1 commit-tree -m "another change outside refs/heads for custom remote" HEAD^{tree}) &&
> +	git -C sub1 update-ref refs/changes/custom5 $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/custom6 $E &&
> +	(
> +		cd downstream &&
> +		git fetch --recurse-submodules origin refs/changes/custom6 &&
> +		git -C submodule cat-file -t $C &&
> +		git -C sub1 cat-file -t $D &&
> +		git checkout --recurse-submodules FETCH_HEAD
> +	)
> +'

Are we testing anything new in this test, compared to the previous one? Both update the submodule sub1 by adding a new ref under refs/changes/ hierachy and have "git fetch --recurse-submodules" follow the changes.

Show 15 quoted lines
> diff --git a/t/t7425-submodule-get-default-remote.sh b/t/t7425-submodule-get-default-remote.sh
> new file mode 100755
> index 0000000000..b842af9a2d
> --- /dev/null
> +++ b/t/t7425-submodule-get-default-remote.sh
> ...
> +test_expect_success 'get-default-remote returns origin for initialized submodule' '
> +	(
> +		cd super &&
> +		git submodule update --init &&
> +		echo "origin" >expect &&
> +		git submodule--helper get-default-remote subpath >actual &&
> +		test_cmp expect actual
> +	)
> +'

OK, we have dedicated tests like these to ensure that when we say "get default remote" we get what we expect, which is very good.

The "too much paranoia" question above is primarily about "are we really saying get-default-remote properly at the place where it matters?" IOW, we guarantee that the lower layer works as expected with t7425, but the way we validate that we call the lower layer correctly felt a bit indirect.

Other than these, looking very good.
Thanks.
Previous: Nasser GrainawiNext: Jacob Keller
Message 13 of 36 in “Fetch missing submodule objects from default remote”
  1. Fetch missing submodule objects from default remoteNasser Grainawi, Jan 12, 2026
  2. Jacob KellerJan 13, 2026
  3. Ben KnobleJan 13, 2026
  4. Nasser GrainawiJan 13, 2026
  5. D. Ben KnobleJan 14, 2026
  6. Junio C HamanoJan 14, 2026
  7. Ben KnobleJan 14, 2026
  8. Junio C HamanoJan 14, 2026
  9. Nasser GrainawiJan 14, 2026
  10. submodule: fetch missing objects from default remoteNasser Grainawi, Jan 14, 2026
  11. Nasser GrainawiJan 21, 2026
  12. submodule: fetch missing objects from default remoteNasser Grainawi, Jan 22, 2026
  13. Junio C HamanoJan 22, 2026
  14. Jacob KellerJan 22, 2026
  15. Junio C HamanoJan 22, 2026
  16. Junio C HamanoFeb 25, 2026
  17. Jacob KellerMar 2, 2026
  18. Nasser GrainawiFeb 27, 2026
  19. Junio C HamanoJan 22, 2026
  20. Junio C HamanoJan 23, 2026
  21. Junio C HamanoJan 24, 2026
  22. Junio C HamanoFeb 20, 2026
  23. Nasser GrainawiFeb 27, 2026
  24. submodule: fetch missing objects from default remoteNasser Grainawi, Mar 1, 2026
  25. Jacob KellerMar 2, 2026
  26. Junio C HamanoMar 2, 2026
  27. Junio C HamanoMar 3, 2026
  28. Nasser GrainawiMar 3, 2026
  29. Nasser GrainawiMar 3, 2026
  30. Junio C HamanoMar 3, 2026
  31. submodule: fetch missing objects from default remoteNasser Grainawi, Mar 3, 2026
  32. Ramsay JonesMar 3, 2026
  33. Junio C HamanoMar 3, 2026
  34. Nasser GrainawiMar 3, 2026
  35. submodule: fetch missing objects from default remoteNasser Grainawi, Mar 3, 2026
  36. Junio C HamanoMar 9, 2026

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.