Re: [PATCH] clone: support unusual remote ref configurations

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

Jonathan Tan <jonathantanmy@google.com> writes:

> When cloning a branchless and tagless but not refless remote using
> protocol v0 or v1, Git calls transport_fetch_refs() with an empty ref
> list. This makes the clone fail with the message "remote transport
> reported error".
>
> Git should have refrained from calling transport_fetch_refs(), just like
> it does in the case that the remote is refless. Therefore, teach Git to
> do this.

Makes sense.

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 727e16e0ae..3df441eb71 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -862,7 +862,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	const struct ref *refs, *remote_head;
>  	struct ref *remote_head_points_at = NULL;
>  	const struct ref *our_head_points_at;
> -	struct ref *mapped_refs;
> +	struct ref *mapped_refs = NULL;
>  	const struct ref *ref;
>  	struct strbuf key = STRBUF_INIT;
>  	struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;
> @@ -1184,7 +1184,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  
>  	refs = transport_get_remote_refs(transport, &transport_ls_refs_options);
>  
> -	if (refs) {
> +	if (refs)
> +		mapped_refs = wanted_peer_refs(refs, &remote->fetch);
> +
> +	if (mapped_refs) {
>  		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));

OK, we used to decide on what they advertised and then filtered that
to what we are interested in inside the "true" side of the if/else.
If the result of filtering became empty, we declared a trouble.

Now we do the filtering first and decide on that.  No matter how
many uninteresting refs they advertise, if they show no refs of
interest to us, it is like they have an empty repository.

> @@ -1193,8 +1196,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		 */
>  		initialize_repository_version(hash_algo, 1);
>  		repo_set_hash_algo(the_repository, hash_algo);
> -
> -		mapped_refs = wanted_peer_refs(refs, &remote->fetch);
>  		/*
>  		 * transport_get_remote_refs() may return refs with null sha-1
>  		 * in mapped_refs (see struct transport->get_refs_list

[start #leftoverbits]

I noticed these while reading outside the context of this patch.
None of them should be part of this patch, which deals only with the
case where mapped_refs becomes empty.

Here in the post-context of this hunk, there is a loop that iterates
over the original refs list, not the filtered mapped_refs list, to
compute "complete_refs_before_fetch".  Should we need to update the
loop to work on mapped_refs?

And after that, we compute remote_head using the original refs
list, not the mapped_refs list, when calling find_ref_by_name(),
but use mapped_refs when calling guess_remote_head().  The
inconsistency smells fishy.

[end #leftoverbits]


> @@ -1240,7 +1241,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  					option_branch, remote_name);
>  
>  		warning(_("You appear to have cloned an empty repository."));
> -		mapped_refs = NULL;

And we come here instead with the new code.  We claim "you appear to
have cloned an empty repository", which is much closer than
"reported an error".

> @@ -1271,7 +1271,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  
>  	if (is_local)
>  		clone_local(path, git_dir);
> -	else if (refs && complete_refs_before_fetch) {
> +	else if (mapped_refs && complete_refs_before_fetch) {
>  		if (transport_fetch_refs(transport, mapped_refs))
>  			die(_("remote transport reported error"));
>  	}

And this is the other crux of the fix.  As the root cause of the
problems is to decide with NULL-ness of refs if we do something that
uses mapped_refs, this fixes the inconsistency.

Looking good.

Will queue.  Thanks.

> diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
> index 468bd3e13e..6c8d4c6cf1 100755
> --- a/t/t5700-protocol-v1.sh
> +++ b/t/t5700-protocol-v1.sh
> @@ -149,6 +149,21 @@ test_expect_success 'push with file:// using protocol v1' '
>  	grep "push< version 1" log
>  '
>  
> +test_expect_success 'cloning branchless tagless but not refless remote' '
> +	rm -rf server client &&
> +
> +	git -c init.defaultbranch=main init server &&
> +	echo foo >server/foo.txt &&
> +	git -C server add foo.txt &&
> +	git -C server commit -m "message" &&
> +	git -C server update-ref refs/notbranch/alsonottag HEAD &&
> +	git -C server checkout --detach &&
> +	git -C server branch -D main &&
> +	git -C server symbolic-ref HEAD refs/heads/nonexistentbranch &&
> +
> +	git -c protocol.version=1 clone "file://$(pwd)/server" client
> +'
> +
>  # Test protocol v1 with 'ssh://' transport
>  #
>  test_expect_success 'setup ssh wrapper' '