{"thread":{"id":"44050","subject":"[PATCH v5 2/3] connect: tighten check for unexpected early hang up","startedAt":"2016-09-09T20:17:56Z","lastAt":"2016-09-09T21:08:05Z","messageCount":5,"participants":["Jonathan Tan","Jonathan Nieder"],"isPatch":true,"patchVersion":5,"patchTotal":3},"messages":[{"id":"301552","messageId":"691658f3d805e3d8515e7b05f7f46d969b9bb301.1473452153.git.jonathantanmy@google.com","threadId":"44050","inReplyTo":"cover.1473452153.git.jonathantanmy@google.com","subject":"[PATCH v5 2/3] connect: tighten check for unexpected early hang up","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-09T20:17:36Z","receivedAt":"2016-09-09T20:17:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"From: Jonathan Nieder \n\nA server hanging up immediately to mark access being denied does not\nsend any .have refs, shallow lines, or anything else before hanging\nup. If the server has sent anything, then the hangup is unexpected.\n\nThat is, if the server hangs up after a shallow line but before sending\nany refs, then git should tell me so:\n\n\tfatal: The remote end hung up upon initial contact\n\ninstead of suggesting an access control problem:\n\n\tfatal: Could not read from remote repository.\n\tPlease make sure you have the correct access rights\n\tand the repository exists.\n\nNoticed while examining this code. This case isn't likely to come up\nin practice but tightening the check makes the code easier to read and\nmanipulate.\n\nSigned-off-by: Jonathan Nieder \nSigned-off-by: Junio C Hamano \n---\n connect.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 722dc3f..0c01a49 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)\n \treturn check_ref(ref->name, flags);\n }\n \n-static void die_initial_contact(int got_at_least_one_head)\n+static void die_initial_contact(int unexpected)\n {\n-\tif (got_at_least_one_head)\n+\tif (unexpected)\n \t\tdie(\"The remote end hung up upon initial contact\");\n \telse\n \t\tdie(\"Could not read from remote repository.\\n\\n\"\n@@ -115,10 +115,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t struct sha1_array *shallow_points)\n {\n \tstruct ref **orig_list = list;\n-\tint got_at_least_one_head = 0;\n+\n+\t/*\n+\t * A hang-up after seeing some response from the other end\n+\t * means that it is unexpected, as we know the other end is\n+\t * willing to talk to us. A hang-up before seeing any\n+\t * response does not necessarily mean an ACL problem, though.\n+\t */\n+\tint saw_response;\n \n \t*list = NULL;\n-\tfor (;;) {\n+\tfor (saw_response = 0; ; saw_response = 1) {\n \t\tstruct ref *ref;\n \t\tstruct object_id old_oid;\n \t\tchar *name;\n@@ -131,7 +138,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t\t PACKET_READ_GENTLE_ON_EOF |\n \t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n \t\tif (len < 0)\n-\t\t\tdie_initial_contact(got_at_least_one_head);\n+\t\t\tdie_initial_contact(saw_response);\n \n \t\tif (!len)\n \t\t\tbreak;\n@@ -171,7 +178,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\toidcpy(&ref->old_oid, &old_oid);\n \t\t*list = ref;\n \t\tlist = &ref->next;\n-\t\tgot_at_least_one_head = 1;\n \t}\n \n \tannotate_refs_with_symref_info(*orig_list);\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"301553","messageId":"cover.1473452153.git.jonathantanmy@google.com","threadId":"44050","inReplyTo":"cover.1472836026.git.jonathantanmy@google.com","subject":"[PATCH v5 0/3] handle empty spec-compliant remote repos correctly","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-09T20:17:34Z","receivedAt":"2016-09-09T20:17:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Updates from PATCH v4:\no use fatal errors (\"die\") instead of warnings for protocol errors\no reworded commit message - removed last sentence and wrote \"(following the\n specification in pack-protocol.txt)\" in one of the earlier paragraphs\n\nJonathan Nieder (1):\n connect: tighten check for unexpected early hang up\n\nJonathan Tan (2):\n tests: move test_lazy_prereq JGIT to test-lib.sh\n connect: advertized capability is not a ref\n\n connect.c | 32 ++++++++++++++++++++++++++------\n t/t5310-pack-bitmaps.sh | 4 ----\n t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh | 4 ++++\n 4 files changed, 70 insertions(+), 10 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"301554","messageId":"37c4e775948b88b2d5af1caee3a70e36f00bc88a.1473452153.git.jonathantanmy@google.com","threadId":"44050","inReplyTo":"cover.1473452153.git.jonathantanmy@google.com","subject":"[PATCH v5 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-09T20:17:35Z","receivedAt":"2016-09-09T20:18:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This enables JGIT to be used as a prereq in invocations of\ntest_expect_success (and other functions) in other test scripts.\n\nSigned-off-by: Jonathan Tan \n---\n t/t5310-pack-bitmaps.sh | 4 ----\n t/test-lib.sh | 4 ++++\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 3893afd..1e376ea 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '\n \tgit pack-objects --stdout --revs /dev/null\n '\n \n-test_lazy_prereq JGIT '\n-\ttype jgit\n-'\n-\n test_expect_success JGIT 'we can read jgit bitmaps' '\n \tgit clone . compat-jgit &&\n \t(\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex d731d66..c9c1037 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '\n \ttest \"$uid\" != 0\n '\n \n+test_lazy_prereq JGIT '\n+\ttype jgit\n+'\n+\n # SANITY is about \"can you correctly predict what the filesystem would\n # do by only looking at the permission bits of the files and\n # directories?\" A typical example of !SANITY is running the test\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"301555","messageId":"f2ccb239ba1f85b434c3dfa60e86b89c414dfffa.1473452153.git.jonathantanmy@google.com","threadId":"44050","inReplyTo":"cover.1473452153.git.jonathantanmy@google.com","subject":"[PATCH v5 3/3] connect: advertized capability is not a ref","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-09T20:17:37Z","receivedAt":"2016-09-09T20:18:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When cloning an empty repository served by standard git, \"git clone\" produces\nthe following reassuring message:\n\n\t$ git clone git://localhost/tmp/empty\n\tCloning into 'empty'...\n\twarning: You appear to have cloned an empty repository.\n\tChecking connectivity... done.\n\nMeanwhile when cloning an empty repository served by JGit, the output is more\nhaphazard:\n\n\t$ git clone git://localhost/tmp/empty\n\tCloning into 'empty'...\n\tChecking connectivity... done.\n\twarning: remote HEAD refers to nonexistent ref, unable to checkout.\n\nThis is a common command to run immediately after creating a remote repository\nas preparation for adding content to populate it and pushing. The warning is\nconfusing and needlessly worrying.\n\nThe cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities\nwith no refs in upload service., 2013-08-08), JGit's ref advertisement includes\na ref named capabilities^{} to advertise its capabilities on (following the\nspecification in pack-protocol.txt), while git's ref advertisement is empty in\nthis case. This allows the client to learn about the server's capabilities and\nis needed, for example, for fetch-by-sha1 to work when no refs are advertised.\n\nThis also affects \"ls-remote\". For example, against an empty repository served\nby JGit:\n\n\t$ git ls-remote git://localhost/tmp/empty\n\t0000000000000000000000000000000000000000 capabilities^{}\n\nGit advertises the same capabilities^{} ref in its ref advertisement for push\nbut since it never did so for fetch, the client didn't need to handle this\ncase. Handle it.\n\nSigned-off-by: Jonathan Tan \n---\n connect.c | 14 ++++++++++++++\n t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 0c01a49..7224b5e 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -123,6 +123,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t * response does not necessarily mean an ACL problem, though.\n \t */\n \tint saw_response;\n+\tint got_dummy_ref_with_capabilities_declaration = 0;\n \n \t*list = NULL;\n \tfor (saw_response = 0; ; saw_response = 1) {\n@@ -172,8 +173,21 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (!strcmp(name, \"capabilities^{}\")) {\n+\t\t\tif (saw_response)\n+\t\t\t\tdie(\"protocol error: unexpected capabilities^{}\");\n+\t\t\tif (got_dummy_ref_with_capabilities_declaration)\n+\t\t\t\tdie(\"protocol error: multiple capabilities^{}\");\n+\t\t\tgot_dummy_ref_with_capabilities_declaration = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tif (!check_ref(name, flags))\n \t\t\tcontinue;\n+\n+\t\tif (got_dummy_ref_with_capabilities_declaration)\n+\t\t\tdie(\"protocol error: unexpected ref after capabilities^{}\");\n+\n \t\tref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);\n \t\toidcpy(&ref->old_oid, &old_oid);\n \t\t*list = ref;\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 819b9dd..befdfee 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -207,5 +207,45 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n \ttest_cmp expect actual\n '\n \n+test_lazy_prereq GIT_DAEMON '\n+\ttest_tristate GIT_TEST_GIT_DAEMON &&\n+\ttest \"$GIT_TEST_GIT_DAEMON\" != false\n+'\n+\n+# This test spawns a daemon, so run it only if the user would be OK with\n+# testing with git-daemon.\n+test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '\n+\tJGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&\n+\tJGIT_DAEMON_PID= &&\n+\tgit init --bare empty.git &&\n+\t>empty.git/git-daemon-export-ok &&\n+\tmkfifo jgit_daemon_output &&\n+\t{\n+\t\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . >jgit_daemon_output &\n+\t\tJGIT_DAEMON_PID=$!\n+\t} &&\n+\ttest_when_finished kill \"$JGIT_DAEMON_PID\" &&\n+\t{\n+\t\tread line &&\n+\t\tcase $line in\n+\t\tExporting*)\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"Expected: Exporting\" &&\n+\t\t\tfalse;;\n+\t\tesac &&\n+\t\tread line &&\n+\t\tcase $line in\n+\t\t\"Listening on\"*)\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"Expected: Listening on\" &&\n+\t\t\tfalse;;\n+\t\tesac\n+\t} connect.c | 32 ++++++++++++++++++++++++++------\n> t/t5310-pack-bitmaps.sh | 4 ----\n> t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n> t/test-lib.sh | 4 ++++\n> 4 files changed, 70 insertions(+), 10 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\nThank you,\nJonathan\n"}]}