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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 18, 2018, 00:47 UTC
Message-ID: <20181018004732.141825-1-jonathantanmy@google.com>
In-Reply-To: <20181016181327.107186-10-sbeller@google.com>

Show 7 quoted lines

> @@ -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);

Does this need to be guarded with a recurse_submodules check, just like in update_local_ref()?

Also, this warrants a comment - this is here because there is some code later that requires the new submodule commits to be registered, and the other branch does not require it only because update_local_ref() calls it.

Show 9 quoted lines

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

I think there should be a new test - we can tell from the code that just because fetching to FETCH_HEAD works doesn't mean that fetching to a ref works, and vice versa.

Also, can you make the test fetch 2 refs? So that we know that it works with more than one.