{"thread":{"id":"57296","subject":"[PATCH] clone: support unusual remote ref configurations","startedAt":"2022-01-24T18:09:14Z","lastAt":"2022-01-26T21:51:09Z","messageCount":3,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"446766","messageId":"20220124180909.2437002-1-jonathantanmy@google.com","threadId":"57296","inReplyTo":null,"subject":"[PATCH] clone: support unusual remote ref configurations","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-01-24T18:09:09Z","receivedAt":"2022-01-24T18:09:14Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When cloning a branchless and tagless but not refless remote using\nprotocol v0 or v1, Git calls transport_fetch_refs() with an empty ref\nlist. This makes the clone fail with the message \"remote transport\nreported error\".\n\nGit should have refrained from calling transport_fetch_refs(), just like\nit does in the case that the remote is refless. Therefore, teach Git to\ndo this.\n\nIn protocol v2, this does not happen because the client passes\nref-prefix arguments that filter out non-branches and non-tags in the\nref advertisement, making the remote appear empty.\n\nNote that this bug concerns logic in builtin/clone.c and only affects\ncloning, not fetching.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nHere's a bug we noticed at $DAYJOB regarding protocol v0/v1, and a\nbugfix.\n\nIt may be confusing that the message currently printed is \"remote\ntransport reported error\". This is due to transport_fetch_refs() not\ndistinguishing between an empty input ref list and a transport error -\nperhaps in the future, an empty input ref list could be signaled by a\nBUG().\n---\n builtin/clone.c        | 12 ++++++------\n t/t5700-protocol-v1.sh | 15 +++++++++++++++\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 727e16e0ae..3df441eb71 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -862,7 +862,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tconst struct ref *refs, *remote_head;\n \tstruct ref *remote_head_points_at = NULL;\n \tconst struct ref *our_head_points_at;\n-\tstruct ref *mapped_refs;\n+\tstruct ref *mapped_refs = NULL;\n \tconst struct ref *ref;\n \tstruct strbuf key = STRBUF_INIT;\n \tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n@@ -1184,7 +1184,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \trefs = transport_get_remote_refs(transport, &transport_ls_refs_options);\n \n-\tif (refs) {\n+\tif (refs)\n+\t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\n+\n+\tif (mapped_refs) {\n \t\tint hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n \n \t\t/*\n@@ -1193,8 +1196,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t */\n \t\tinitialize_repository_version(hash_algo, 1);\n \t\trepo_set_hash_algo(the_repository, hash_algo);\n-\n-\t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\n \t\t/*\n \t\t * transport_get_remote_refs() may return refs with null sha-1\n \t\t * in mapped_refs (see struct transport->get_refs_list\n@@ -1240,7 +1241,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t\t\toption_branch, remote_name);\n \n \t\twarning(_(\"You appear to have cloned an empty repository.\"));\n-\t\tmapped_refs = NULL;\n \t\tour_head_points_at = NULL;\n \t\tremote_head_points_at = NULL;\n \t\tremote_head = NULL;\n@@ -1271,7 +1271,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tif (is_local)\n \t\tclone_local(path, git_dir);\n-\telse if (refs && complete_refs_before_fetch) {\n+\telse if (mapped_refs && complete_refs_before_fetch) {\n \t\tif (transport_fetch_refs(transport, mapped_refs))\n \t\t\tdie(_(\"remote transport reported error\"));\n \t}\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex 468bd3e13e..6c8d4c6cf1 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -149,6 +149,21 @@ test_expect_success 'push with file:// using protocol v1' '\n \tgrep \"push< version 1\" log\n '\n \n+test_expect_success 'cloning branchless tagless but not refless remote' '\n+\trm -rf server client &&\n+\n+\tgit -c init.defaultbranch=main init server &&\n+\techo foo >server/foo.txt &&\n+\tgit -C server add foo.txt &&\n+\tgit -C server commit -m \"message\" &&\n+\tgit -C server update-ref refs/notbranch/alsonottag HEAD &&\n+\tgit -C server checkout --detach &&\n+\tgit -C server branch -D main &&\n+\tgit -C server symbolic-ref HEAD refs/heads/nonexistentbranch &&\n+\n+\tgit -c protocol.version=1 clone \"file://$(pwd)/server\" client\n+'\n+\n # Test protocol v1 with 'ssh://' transport\n #\n test_expect_success 'setup ssh wrapper' '\n-- \n2.35.0.rc0.227.g00780c9af4-goog\n\n"},{"id":"446986","messageId":"xmqqfspas45g.fsf@gitster.g","threadId":"57296","inReplyTo":"20220124180909.2437002-1-jonathantanmy@google.com","subject":"Re: [PATCH] clone: support unusual remote ref configurations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T19:11:55Z","receivedAt":"2022-01-26T19:12:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When cloning a branchless and tagless but not refless remote using\n> protocol v0 or v1, Git calls transport_fetch_refs() with an empty ref\n> list. This makes the clone fail with the message \"remote transport\n> reported error\".\n>\n> Git should have refrained from calling transport_fetch_refs(), just like\n> it does in the case that the remote is refless. Therefore, teach Git to\n> do this.\n\nMakes sense.\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 727e16e0ae..3df441eb71 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -862,7 +862,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tconst struct ref *refs, *remote_head;\n>  \tstruct ref *remote_head_points_at = NULL;\n>  \tconst struct ref *our_head_points_at;\n> -\tstruct ref *mapped_refs;\n> +\tstruct ref *mapped_refs = NULL;\n>  \tconst struct ref *ref;\n>  \tstruct strbuf key = STRBUF_INIT;\n>  \tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n> @@ -1184,7 +1184,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \n>  \trefs = transport_get_remote_refs(transport, &transport_ls_refs_options);\n>  \n> -\tif (refs) {\n> +\tif (refs)\n> +\t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\n> +\n> +\tif (mapped_refs) {\n>  \t\tint hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n\nOK, we used to decide on what they advertised and then filtered that\nto what we are interested in inside the \"true\" side of the if/else.\nIf the result of filtering became empty, we declared a trouble.\n\nNow we do the filtering first and decide on that.  No matter how\nmany uninteresting refs they advertise, if they show no refs of\ninterest to us, it is like they have an empty repository.\n\n> @@ -1193,8 +1196,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\t */\n>  \t\tinitialize_repository_version(hash_algo, 1);\n>  \t\trepo_set_hash_algo(the_repository, hash_algo);\n> -\n> -\t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\n>  \t\t/*\n>  \t\t * transport_get_remote_refs() may return refs with null sha-1\n>  \t\t * in mapped_refs (see struct transport->get_refs_list\n\n[start #leftoverbits]\n\nI noticed these while reading outside the context of this patch.\nNone of them should be part of this patch, which deals only with the\ncase where mapped_refs becomes empty.\n\nHere in the post-context of this hunk, there is a loop that iterates\nover the original refs list, not the filtered mapped_refs list, to\ncompute \"complete_refs_before_fetch\".  Should we need to update the\nloop to work on mapped_refs?\n\nAnd after that, we compute remote_head using the original refs\nlist, not the mapped_refs list, when calling find_ref_by_name(),\nbut use mapped_refs when calling guess_remote_head().  The\ninconsistency smells fishy.\n\n[end #leftoverbits]\n\n\n> @@ -1240,7 +1241,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t\toption_branch, remote_name);\n>  \n>  \t\twarning(_(\"You appear to have cloned an empty repository.\"));\n> -\t\tmapped_refs = NULL;\n\nAnd we come here instead with the new code.  We claim \"you appear to\nhave cloned an empty repository\", which is much closer than\n\"reported an error\".\n\n> @@ -1271,7 +1271,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \n>  \tif (is_local)\n>  \t\tclone_local(path, git_dir);\n> -\telse if (refs && complete_refs_before_fetch) {\n> +\telse if (mapped_refs && complete_refs_before_fetch) {\n>  \t\tif (transport_fetch_refs(transport, mapped_refs))\n>  \t\t\tdie(_(\"remote transport reported error\"));\n>  \t}\n\nAnd this is the other crux of the fix.  As the root cause of the\nproblems is to decide with NULL-ness of refs if we do something that\nuses mapped_refs, this fixes the inconsistency.\n\nLooking good.\n\nWill queue.  Thanks.\n\n> diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\n> index 468bd3e13e..6c8d4c6cf1 100755\n> --- a/t/t5700-protocol-v1.sh\n> +++ b/t/t5700-protocol-v1.sh\n> @@ -149,6 +149,21 @@ test_expect_success 'push with file:// using protocol v1' '\n>  \tgrep \"push< version 1\" log\n>  '\n>  \n> +test_expect_success 'cloning branchless tagless but not refless remote' '\n> +\trm -rf server client &&\n> +\n> +\tgit -c init.defaultbranch=main init server &&\n> +\techo foo >server/foo.txt &&\n> +\tgit -C server add foo.txt &&\n> +\tgit -C server commit -m \"message\" &&\n> +\tgit -C server update-ref refs/notbranch/alsonottag HEAD &&\n> +\tgit -C server checkout --detach &&\n> +\tgit -C server branch -D main &&\n> +\tgit -C server symbolic-ref HEAD refs/heads/nonexistentbranch &&\n> +\n> +\tgit -c protocol.version=1 clone \"file://$(pwd)/server\" client\n> +'\n> +\n>  # Test protocol v1 with 'ssh://' transport\n>  #\n>  test_expect_success 'setup ssh wrapper' '\n"},{"id":"447003","messageId":"xmqqwnimp3nb.fsf@gitster.g","threadId":"57296","inReplyTo":"20220124180909.2437002-1-jonathantanmy@google.com","subject":"Re: [PATCH] clone: support unusual remote ref configurations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T21:51:04Z","receivedAt":"2022-01-26T21:51:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When cloning a branchless and tagless but not refless remote using\n> protocol v0 or v1, Git calls transport_fetch_refs() with an empty ref\n> list. This makes the clone fail with the message \"remote transport\n> reported error\".\n\nDespite what the title says, I do not think this would be so\n\"unusual\".  To a freshly created repository, if you added a remote\nor two, fetched from them, but haven't created a commit of your own\n(or forked from these remotes) yet, you'd have no refs in heads/ and\ntags/ but refs in refs/remotes/.\n\n2.35 would fail as expected with \"remote transport reported error\"\nin such a situation.  With your fix, it creates an empty repository\nwith expected fetch refspec.\n\nWhich looks good.\n\n"}]}