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

[Junio C Hamano <gitster@pobox.com>]

Nasser Grainawi <nasser.grainawi@oss.qualcomm.com> writes:

> 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?

>  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?

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

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

> 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?

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

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