Message ID | 20200108071009.GA1675456@coredump.intra.peff.net (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | transport: don't flush when disconnecting stateless-rpc helper | expand |
On 2020-01-08 at 07:10:09, Jeff King wrote: > diff --git a/transport.c b/transport.c > index 83379a037d..1fdc7dac1a 100644 > --- a/transport.c > +++ b/transport.c > @@ -737,7 +737,7 @@ static int disconnect_git(struct transport *transport) > { > struct git_transport_data *data = transport->data; > if (data->conn) { > - if (data->got_remote_heads) > + if (data->got_remote_heads && !transport->stateless_rpc) > packet_flush(data->fd[1]); > close(data->fd[0]); > close(data->fd[1]); This is as simple and elegant as I'd hoped for, and as usual, it's well explained. Looks great.
On 1/8/2020 2:10 AM, Jeff King wrote: > On Wed, Jan 08, 2020 at 03:34:51AM +0000, brian m. carlson wrote: > >> A colleague (Jon Simons) today pointed out an interesting behavior of >> git ls-remote with protocol v2: it makes a second POST request and sends >> only a flush packet. This can be demonstrated with the following: >> >> GIT_CURL_VERBOSE=1 git -c protocol.version=2 ls-remote origin >> >> The Content-Length header on the second request will be exactly 4 bytes. Good find! > But when we've initiated a v2 stateless-connect session over a transport > helper, there's no point in sending this flush packet. Each operation > we've performed is self-contained, and the other side is fine with us > hanging up between operations. Makes sense to me. > But much worse, by sending the flush packet we may cause the helper to > issue an entirely new request _just_ to send the flush packet. So we can > incur an extra network request just to say "by the way, we have nothing > more to send". > > Let's drop this extra flush packet. As the test shows, this reduces the > number of POSTs required for a v2 ls-remote over http from 2 to 1. > +test_expect_success 'ls-remote with v2 http sends only one POST' ' > + test_when_finished "rm -f log" && > + > + git ls-remote "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" >expect && > + GIT_TRACE_CURL="$(pwd)/log" git -c protocol.version=2 \ > + ls-remote "$HTTPD_URL/smart/http_parent" >actual && > + test_cmp expect actual && > + > + grep "Send header: POST" log >posts && > + test_line_count = 1 posts > +' > + Nice test! > diff --git a/transport.c b/transport.c > index 83379a037d..1fdc7dac1a 100644 > --- a/transport.c > +++ b/transport.c > @@ -737,7 +737,7 @@ static int disconnect_git(struct transport *transport) > { > struct git_transport_data *data = transport->data; > if (data->conn) { > - if (data->got_remote_heads) > + if (data->got_remote_heads && !transport->stateless_rpc) > packet_flush(data->fd[1]); > close(data->fd[0]); > close(data->fd[1]); Looks good to me. Thanks! -Stolee
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index e73067d23f..7fd7102c87 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -665,6 +665,18 @@ test_expect_success 'fetch from namespaced repo respects namespaces' ' test_cmp expect actual ' +test_expect_success 'ls-remote with v2 http sends only one POST' ' + test_when_finished "rm -f log" && + + git ls-remote "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" >expect && + GIT_TRACE_CURL="$(pwd)/log" git -c protocol.version=2 \ + ls-remote "$HTTPD_URL/smart/http_parent" >actual && + test_cmp expect actual && + + grep "Send header: POST" log >posts && + test_line_count = 1 posts +' + test_expect_success 'push with http:// and a config of v2 does not request v2' ' test_when_finished "rm -f log" && # Till v2 for push is designed, make sure that if a client has diff --git a/transport.c b/transport.c index 83379a037d..1fdc7dac1a 100644 --- a/transport.c +++ b/transport.c @@ -737,7 +737,7 @@ static int disconnect_git(struct transport *transport) { struct git_transport_data *data = transport->data; if (data->conn) { - if (data->got_remote_heads) + if (data->got_remote_heads && !transport->stateless_rpc) packet_flush(data->fd[1]); close(data->fd[0]); close(data->fd[1]);