{"thread":{"id":"52591","subject":"Extra request with protocol v2 and git ls-remote","startedAt":"2020-01-08T03:35:00Z","lastAt":"2020-01-08T13:43:27Z","messageCount":4,"participants":["brian m. carlson","Jeff King","Derrick Stolee"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"389446","messageId":"20200108033451.GN6570@camp.crustytoothpaste.net","threadId":"52591","inReplyTo":null,"subject":"Extra request with protocol v2 and git ls-remote","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-01-08T03:34:51Z","receivedAt":"2020-01-08T03:35:00Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"A colleague (Jon Simons) today pointed out an interesting behavior of\ngit ls-remote with protocol v2: it makes a second POST request and sends\nonly a flush packet.  This can be demonstrated with the following:\n\n  GIT_CURL_VERBOSE=1 git -c protocol.version=2 ls-remote origin\n\nThe Content-Length header on the second request will be exactly 4 bytes.\n\nI'm not sure exactly how to fix this by looking at remote-curl.c, but I\nsuspect that we can avoid sending a request if all we're going to do is\nsend a flush packet.  If I were a bit more familiar with the code, I'd\nsend a patch, but I'm not.\n\nI just thought I'd point this out since it was surprising to me.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"389452","messageId":"20200108071009.GA1675456@coredump.intra.peff.net","threadId":"52591","inReplyTo":"20200108033451.GN6570@camp.crustytoothpaste.net","subject":"[PATCH] transport: don't flush when disconnecting stateless-rpc helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-08T07:10:09Z","receivedAt":"2020-01-08T07:10:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 08, 2020 at 03:34:51AM +0000, brian m. carlson wrote:\n\n> A colleague (Jon Simons) today pointed out an interesting behavior of\n> git ls-remote with protocol v2: it makes a second POST request and sends\n> only a flush packet.  This can be demonstrated with the following:\n> \n>   GIT_CURL_VERBOSE=1 git -c protocol.version=2 ls-remote origin\n> \n> The Content-Length header on the second request will be exactly 4 bytes.\n> \n> I'm not sure exactly how to fix this by looking at remote-curl.c, but I\n> suspect that we can avoid sending a request if all we're going to do is\n> send a flush packet.  If I were a bit more familiar with the code, I'd\n> send a patch, but I'm not.\n\nYeah, in theory remote-curl could be smarter about turning such a\npointless request into a noop. But I think the root cause is actually on\nthe ls-remote side. How about this?\n\n-- >8 --\nSubject: transport: don't flush when disconnecting stateless-rpc helper\n\nSince ba227857d2 (Reduce the number of connects when fetching,\n2008-02-04), when we disconnect a git transport, we send a final flush\npacket. This cleanly tells the other side that we're done, and avoids\nthe other side complaining \"the remote end hung up unexpectedly\" (though\nwe'd only see that for transports that pass along the server stderr,\nlike ssh or local-host).\n\nBut when we've initiated a v2 stateless-connect session over a transport\nhelper, there's no point in sending this flush packet. Each operation\nwe've performed is self-contained, and the other side is fine with us\nhanging up between operations.\n\nBut much worse, by sending the flush packet we may cause the helper to\nissue an entirely new request _just_ to send the flush packet. So we can\nincur an extra network request just to say \"by the way, we have nothing\nmore to send\".\n\nLet's drop this extra flush packet. As the test shows, this reduces the\nnumber of POSTs required for a v2 ls-remote over http from 2 to 1.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5702-protocol-v2.sh | 12 ++++++++++++\n transport.c            |  2 +-\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex e73067d23f..7fd7102c87 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -665,6 +665,18 @@ test_expect_success 'fetch from namespaced repo respects namespaces' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ls-remote with v2 http sends only one POST' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\tgit ls-remote \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" >expect &&\n+\tGIT_TRACE_CURL=\"$(pwd)/log\" git -c protocol.version=2 \\\n+\t\tls-remote \"$HTTPD_URL/smart/http_parent\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgrep \"Send header: POST\" log >posts &&\n+\ttest_line_count = 1 posts\n+'\n+\n test_expect_success 'push with http:// and a config of v2 does not request v2' '\n \ttest_when_finished \"rm -f log\" &&\n \t# Till v2 for push is designed, make sure that if a client has\ndiff --git a/transport.c b/transport.c\nindex 83379a037d..1fdc7dac1a 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -737,7 +737,7 @@ static int disconnect_git(struct transport *transport)\n {\n \tstruct git_transport_data *data = transport->data;\n \tif (data->conn) {\n-\t\tif (data->got_remote_heads)\n+\t\tif (data->got_remote_heads && !transport->stateless_rpc)\n \t\t\tpacket_flush(data->fd[1]);\n \t\tclose(data->fd[0]);\n \t\tclose(data->fd[1]);\n-- \n2.25.0.rc1.618.gb996734ace\n\n"},{"id":"389470","messageId":"20200108124450.GO6570@camp.crustytoothpaste.net","threadId":"52591","inReplyTo":"20200108071009.GA1675456@coredump.intra.peff.net","subject":"Re: [PATCH] transport: don't flush when disconnecting stateless-rpc helper","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-01-08T12:44:50Z","receivedAt":"2020-01-08T12:44:58Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-01-08 at 07:10:09, Jeff King wrote:\n> diff --git a/transport.c b/transport.c\n> index 83379a037d..1fdc7dac1a 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -737,7 +737,7 @@ static int disconnect_git(struct transport *transport)\n>  {\n>  \tstruct git_transport_data *data = transport->data;\n>  \tif (data->conn) {\n> -\t\tif (data->got_remote_heads)\n> +\t\tif (data->got_remote_heads && !transport->stateless_rpc)\n>  \t\t\tpacket_flush(data->fd[1]);\n>  \t\tclose(data->fd[0]);\n>  \t\tclose(data->fd[1]);\n\nThis is as simple and elegant as I'd hoped for, and as usual, it's well\nexplained.  Looks great.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"389475","messageId":"bc8f8c12-75d0-c79c-2778-0e7afe6b796a@gmail.com","threadId":"52591","inReplyTo":"20200108071009.GA1675456@coredump.intra.peff.net","subject":"Re: [PATCH] transport: don't flush when disconnecting stateless-rpc helper","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-08T13:43:21Z","receivedAt":"2020-01-08T13:43:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/8/2020 2:10 AM, Jeff King wrote:\n> On Wed, Jan 08, 2020 at 03:34:51AM +0000, brian m. carlson wrote:\n> \n>> A colleague (Jon Simons) today pointed out an interesting behavior of\n>> git ls-remote with protocol v2: it makes a second POST request and sends\n>> only a flush packet.  This can be demonstrated with the following:\n>>\n>>   GIT_CURL_VERBOSE=1 git -c protocol.version=2 ls-remote origin\n>>\n>> The Content-Length header on the second request will be exactly 4 bytes.\n\nGood find!\n \n> But when we've initiated a v2 stateless-connect session over a transport\n> helper, there's no point in sending this flush packet. Each operation\n> we've performed is self-contained, and the other side is fine with us\n> hanging up between operations.\n\nMakes sense to me.\n\n> But much worse, by sending the flush packet we may cause the helper to\n> issue an entirely new request _just_ to send the flush packet. So we can\n> incur an extra network request just to say \"by the way, we have nothing\n> more to send\".\n> \n> Let's drop this extra flush packet. As the test shows, this reduces the\n> number of POSTs required for a v2 ls-remote over http from 2 to 1.\n\n> +test_expect_success 'ls-remote with v2 http sends only one POST' '\n> +\ttest_when_finished \"rm -f log\" &&\n> +\n> +\tgit ls-remote \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" >expect &&\n> +\tGIT_TRACE_CURL=\"$(pwd)/log\" git -c protocol.version=2 \\\n> +\t\tls-remote \"$HTTPD_URL/smart/http_parent\" >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tgrep \"Send header: POST\" log >posts &&\n> +\ttest_line_count = 1 posts\n> +'\n> +\n\nNice test!\n\n> diff --git a/transport.c b/transport.c\n> index 83379a037d..1fdc7dac1a 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -737,7 +737,7 @@ static int disconnect_git(struct transport *transport)\n>  {\n>  \tstruct git_transport_data *data = transport->data;\n>  \tif (data->conn) {\n> -\t\tif (data->got_remote_heads)\n> +\t\tif (data->got_remote_heads && !transport->stateless_rpc)\n>  \t\t\tpacket_flush(data->fd[1]);\n>  \t\tclose(data->fd[0]);\n>  \t\tclose(data->fd[1]);\n\nLooks good to me. Thanks!\n\n-Stolee\n\n"}]}