Re: [PATCH 9/9] builtin/fetch: check for submodule updates for non branch fetches

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

Stefan Beller <sbeller@google.com> writes:

> For Gerrit users that use submodules the invocation of fetch without a
> branch is their main use case.

That's way under explains this commit.  It is totally unclear how
that statement of fact relates to the problem this patch is trying
to address; it does not even make it clear what problem is being
addressed by the patch.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/fetch.c             | 5 ++++-
>  t/t5526-fetch-submodules.sh | 2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 95c44bf6ffa..ea6ecd123e7 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -887,11 +887,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  				rc |= update_local_ref(ref, what, rm, &note,
>  						       summary_width);
>  				free(ref);
> -			} else
> +			} else {
> +				check_for_new_submodule_commits(&rm->old_oid);

This happens when there is no "ref", which is set only when
rm->peer_ref exists, which is set only when we are using remote
tracking branch (or more generally storing the fetched rev somewhere
in our refs/ hierarchy), e.g. the rev is recorded only in FETCH_HEAD.

What does rm->old_oid have in such a case?  Is this the tip of the
superproject history we just fetched?

When we keep record of what we saw in the previous attempt to fetch,
we can tell "we have seen their history up to this old commit
before, and now we fetched their history up to this new commit" and
the question "during that time, which submodules have been modified
in the history of the superproject" becomes answerable.  When we are
not keeping the record of previous fetch, how would we answer that
question without going through the whole history?

	The answer is that check-for-new does not even do the "old
	branch tip was X and new branch tip is Y, so we can look
	only at X..Y"; it only cares about the new branch tip of the
	superproject, and excludes the existing tips of all branches
	in the superproject (i.e. computing something akin to "Y
	--not --all" instead of "X..Y").

So, I guess this is probably reasonable.  But does the call to
"check-for-new submodule" need to be unconditional?  In this
codepath, do we know when we are not doing a recursive fetch in a
superproject?  If so, perhaps we can omit the cost of going through
all the refs to populate ref_tips_before_fetch array in such a case.

>  				format_display(&note, '*',
>  					       *kind ? kind : "branch", NULL,
>  					       *what ? what : "HEAD",
>  					       "FETCH_HEAD", summary_width);
> +			}
> +
>  			if (note.len) {
>  				if (verbosity >= 0 && !shown_url) {
>  					fprintf(stderr, _("From %.*s\n"),
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index af12c50e7dd..a509eabb044 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -615,7 +615,7 @@ test_expect_success "fetch new commits on-demand when they are not reachable" '
>  	git update-ref refs/changes/2 $D &&
>  	(
>  		cd downstream &&
> -		git fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&
> +		git fetch --recurse-submodules origin refs/changes/2 &&
>  		git -C submodule cat-file -t $C &&
>  		git checkout --recurse-submodules FETCH_HEAD
>  	)