[PATCH] clone: support unusual remote ref configurations

[PATCH] clone: support unusual remote ref configurations

[Jonathan Tan]

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.

In protocol v2, this does not happen because the client passes
ref-prefix arguments that filter out non-branches and non-tags in the
ref advertisement, making the remote appear empty.

Note that this bug concerns logic in builtin/clone.c and only affects
cloning, not fetching.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Here's a bug we noticed at $DAYJOB regarding protocol v0/v1, and a
bugfix.

It may be confusing that the message currently printed is "remote
transport reported error". This is due to transport_fetch_refs() not
distinguishing between an empty input ref list and a transport error -
perhaps in the future, an empty input ref list could be signaled by a
BUG().
---
 builtin/clone.c        | 12 ++++++------
 t/t5700-protocol-v1.sh | 15 +++++++++++++++
 2 files changed, 21 insertions(+), 6 deletions(-)

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));
 
 		/*
@@ -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
@@ -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;
 		our_head_points_at = NULL;
 		remote_head_points_at = NULL;
 		remote_head = NULL;
@@ -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"));
 	}
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' '
-- 
2.35.0.rc0.227.g00780c9af4-goog

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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

Despite what the title says, I do not think this would be so
"unusual".  To a freshly created repository, if you added a remote
or two, fetched from them, but haven't created a commit of your own
(or forked from these remotes) yet, you'd have no refs in heads/ and
tags/ but refs in refs/remotes/.

2.35 would fail as expected with "remote transport reported error"
in such a situation.  With your fix, it creates an empty repository
with expected fetch refspec.

Which looks good.