{"thread":{"id":"49620","subject":"[PATCH] fetch-pack: be more precise in parsing v2 response","startedAt":"2018-10-19T22:54:10Z","lastAt":"2018-10-25T18:34:45Z","messageCount":4,"participants":["Jonathan Tan","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"360986","messageId":"20181019225404.34496-1-jonathantanmy@google.com","threadId":"49620","inReplyTo":null,"subject":"[PATCH] fetch-pack: be more precise in parsing v2 response","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-19T22:54:04Z","receivedAt":"2018-10-19T22:54:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Each section in a protocol v2 response is followed by either a DELIM\npacket (indicating more sections to follow) or a FLUSH packet\n(indicating none to follow). But when parsing the \"acknowledgments\"\nsection, do_fetch_pack_v2() is liberal in accepting both, but determines\nwhether to continue reading or not based solely on the contents of the\n\"acknowledgments\" section, not on whether DELIM or FLUSH was read.\n\nThere is no issue with a protocol-compliant server, but can result in\nconfusing error messages when communicating with a server that\nserves unexpected additional sections. Consider a server that sends\n\"new-section\" after \"acknowledgments\":\n\n - client writes request\n - client reads the \"acknowledgments\" section which contains no \"ready\",\n   then DELIM\n - since there was no \"ready\", client needs to continue negotiation, and\n   writes request\n - client reads \"new-section\", and reports to the end user \"expected\n   'acknowledgments', received 'new-section'\"\n\nFor the person debugging the involved Git implementation(s), the error\nmessage is confusing in that \"new-section\" was not received in response\nto the latest request, but to the first one.\n\nOne solution is to always continue reading after DELIM, but in this\ncase, we can do better. We know from the protocol that \"ready\" means at\nleast the packfile section is coming (hence, DELIM) and that no \"ready\"\nmeans that no sections are to follow (hence, FLUSH). So teach\nprocess_acks() to enforce this.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c           | 12 ++++++++++\n t/t5702-protocol-v2.sh | 50 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b3ed7121bc..9691046e64 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1248,6 +1248,18 @@ static int process_acks(struct fetch_negotiator *negotiator,\n \t    reader->status != PACKET_READ_DELIM)\n \t\tdie(_(\"error processing acks: %d\"), reader->status);\n \n+\t/*\n+\t * If an \"acknowledgments\" section is sent, a packfile is sent if and\n+\t * only if \"ready\" was sent in this section. The other sections\n+\t * (\"shallow-info\" and \"wanted-refs\") are sent only if a packfile is\n+\t * sent. Therefore, a DELIM is expected if \"ready\" is sent, and a FLUSH\n+\t * otherwise.\n+\t */\n+\tif (received_ready && reader->status != PACKET_READ_DELIM)\n+\t\tdie(_(\"expected packfile to be sent after 'ready'\"));\n+\tif (!received_ready && reader->status != PACKET_READ_FLUSH)\n+\t\tdie(_(\"expected no other sections to be sent after no 'ready'\"));\n+\n \t/* return 0 if no common, 1 if there are common, or 2 if ready */\n \treturn received_ready ? 2 : (received_ack ? 1 : 0);\n }\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 8360188c01..51009ca391 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -512,6 +512,56 @@ test_expect_success 'push with http:// and a config of v2 does not request v2' '\n \t! grep \"git< version 2\" log\n '\n \n+test_expect_success 'when server sends \"ready\", expect DELIM' '\n+\trm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child &&\n+\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" one &&\n+\n+\tgit clone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" two &&\n+\n+\t# After \"ready\" in the acknowledgments section, pretend that a FLUSH\n+\t# (0000) was sent instead of a DELIM (0001).\n+\tprintf \"/ready/,$ s/0001/0000/\" \\\n+\t\t>\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n+\n+\ttest_must_fail git -C http_child -c protocol.version=2 \\\n+\t\tfetch \"$HTTPD_URL/one_time_sed/http_parent\" 2> err &&\n+\ttest_i18ngrep \"expected packfile to be sent after .ready.\" err\n+'\n+\n+test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n+\trm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child log &&\n+\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" one &&\n+\n+\tgit clone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" two &&\n+\n+\t# Create many commits to extend the negotiation phase across multiple\n+\t# requests, so that the server does not send \"ready\" in the first\n+\t# request.\n+\tfor i in $(test_seq 1 32)\n+\tdo\n+\t\ttest_commit -C http_child c$i\n+\tdone &&\n+\n+\t# After the acknowledgments section, pretend that a DELIM\n+\t# (0001) was sent instead of a FLUSH (0000).\n+\tprintf \"/acknowledgments/,$ s/0000/0001/\" \\\n+\t\t>\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/log\" test_must_fail git -C http_child \\\n+\t\t-c protocol.version=2 \\\n+\t\tfetch \"$HTTPD_URL/one_time_sed/http_parent\" 2> err &&\n+\tgrep \"fetch< acknowledgments\" log &&\n+\t! grep \"fetch< ready\" log &&\n+\ttest_i18ngrep \"expected no other sections to be sent after no .ready.\" err\n+'\n \n stop_httpd\n \n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"361139","messageId":"xmqqwoqav8g6.fsf@gitster-ct.c.googlers.com","threadId":"49620","inReplyTo":"20181019225404.34496-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch-pack: be more precise in parsing v2 response","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-22T05:47:37Z","receivedAt":"2018-10-22T05:47:43Z","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> +\tGIT_TRACE_PACKET=\"$(pwd)/log\" test_must_fail git -C http_child \\\n> +\t\t-c protocol.version=2 \\\n> +\t\tfetch \"$HTTPD_URL/one_time_sed/http_parent\" 2> err &&\n\nBecause test_must_fail is a shell function, the above is not a\ncorrect way to say \"I want GIT_TRACE_PACKET exported only while this\nthing runs\".\n\nI'll squash the following in.\n\n t/t5702-protocol-v2.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 51009ca391..d58fbfa9e5 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -555,7 +555,7 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n \tprintf \"/acknowledgments/,$ s/0000/0001/\" \\\n \t\t>\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n \n-\tGIT_TRACE_PACKET=\"$(pwd)/log\" test_must_fail git -C http_child \\\n+\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" git -C http_child \\\n \t\t-c protocol.version=2 \\\n \t\tfetch \"$HTTPD_URL/one_time_sed/http_parent\" 2> err &&\n \tgrep \"fetch< acknowledgments\" log &&\n"},{"id":"361493","messageId":"xmqqpnvygzy6.fsf@gitster-ct.c.googlers.com","threadId":"49620","inReplyTo":"xmqqwoqav8g6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch-pack: be more precise in parsing v2 response","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-25T09:04:01Z","receivedAt":"2018-10-25T09:04:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n>> +\tGIT_TRACE_PACKET=\"$(pwd)/log\" test_must_fail git -C http_child \\\n>> +\t\t-c protocol.version=2 \\\n>> +\t\tfetch \"$HTTPD_URL/one_time_sed/http_parent\" 2> err &&\n>\n> Because test_must_fail is a shell function, the above is not a\n> correct way to say \"I want GIT_TRACE_PACKET exported only while this\n> thing runs\".\n>\n> I'll squash the following in.\n>\n>  t/t5702-protocol-v2.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 51009ca391..d58fbfa9e5 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -555,7 +555,7 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n>  \tprintf \"/acknowledgments/,$ s/0000/0001/\" \\\n>  \t\t>\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n>  \n> -\tGIT_TRACE_PACKET=\"$(pwd)/log\" test_must_fail git -C http_child \\\n> +\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" git -C http_child \\\n>  \t\t-c protocol.version=2 \\\n>  \t\tfetch \"$HTTPD_URL/one_time_sed/http_parent\" 2> err &&\n>  \tgrep \"fetch< acknowledgments\" log &&\n\nI know it only has been a few days, but is there any other issue\nin the patch, anybody?\n\nOtherwise, I am wondering if we can move this forwared after\nsquashing the above fix in.\n\nThanks.\n"},{"id":"361538","messageId":"CAGZ79kYoMOL-et63cky8LOxoT0yuuCo+7x=qjUcmBH_VZL+Y-w@mail.gmail.com","threadId":"49620","inReplyTo":"xmqqpnvygzy6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch-pack: be more precise in parsing v2 response","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T18:34:30Z","receivedAt":"2018-10-25T18:34:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 25, 2018 at 2:04 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > Jonathan Tan <jonathantanmy@google.com> writes:\n> >\n> >> +    GIT_TRACE_PACKET=\"$(pwd)/log\" test_must_fail git -C http_child \\\n> >> +            -c protocol.version=2 \\\n> >> +            fetch \"$HTTPD_URL/one_time_sed/http_parent\" 2> err &&\n> >\n> > Because test_must_fail is a shell function, the above is not a\n> > correct way to say \"I want GIT_TRACE_PACKET exported only while this\n> > thing runs\".\n> >\n> > I'll squash the following in.\n> >\n> >  t/t5702-protocol-v2.sh | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> > index 51009ca391..d58fbfa9e5 100755\n> > --- a/t/t5702-protocol-v2.sh\n> > +++ b/t/t5702-protocol-v2.sh\n> > @@ -555,7 +555,7 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n> >       printf \"/acknowledgments/,$ s/0000/0001/\" \\\n> >               >\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n> >\n> > -     GIT_TRACE_PACKET=\"$(pwd)/log\" test_must_fail git -C http_child \\\n> > +     test_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" git -C http_child \\\n> >               -c protocol.version=2 \\\n> >               fetch \"$HTTPD_URL/one_time_sed/http_parent\" 2> err &&\n> >       grep \"fetch< acknowledgments\" log &&\n>\n> I know it only has been a few days, but is there any other issue\n> in the patch, anybody?\n\nI have reviewed the patch and I think it is good with the squashed change above.\n\nThanks,\nStefan\n"}]}