{"thread":{"id":"37764","subject":"git fetch (http) hanging/failing on one specific repository, http only","startedAt":"2014-10-20T14:29:15Z","lastAt":"2014-12-06T00:48:10Z","messageCount":10,"participants":["Dennis Kaarsemaker","Junio C Hamano","Duy Nguyen","Shawn Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"250854","messageId":"1413815355.4175.26.camel@seahawk","threadId":"37764","inReplyTo":null,"subject":"git fetch (http) hanging/failing on one specific repository, http only","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2014-10-20T14:29:15Z","receivedAt":"2014-10-20T14:29:15Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"Since a few days, one of our repos is causing problems during git fetch,\nbasically git fetch over http hangs during find_common. When using ssh,\nthis does not happen. One thing to noe about this repo is that it has\n53000+ refs, though I'm not convinced that that is the cause of my\nptoblem.\n\nClient and server are both 2.1.2, the 'dirty' is from some debugging\nprintf's I threw in. I also tested with 1.8.4, which also exhibits this\nbehaviour:\n\nGIT_CURL_VERBOSE=1 /home/dkaarsemaker/git/git fetch -vvv\n\n* Couldn't find host xxx.booking.com in the .netrc file; using defaults\n* Expire cleared\n* About to connect() to xxx.booking.com port 80 (#0)\n*   Trying 10.196.70.243... >\nGET /git/main.git/info/refs?service=git-upload-pack HTTP/1.1\nUser-Agent: git/2.1.2.dirty\nHost: xxx.booking.com\nAccept: */*\nAccept-Encoding: gzip\nPragma: no-cache\n\n< HTTP/1.1 200 OK\n< Date: Mon, 20 Oct 2014 13:41:00 GMT\n< Server: Apache\n< Expires: Fri, 01 Jan 1980 00:00:00 GMT\n< Pragma: no-cache\n< Cache-Control: no-cache, max-age=0, must-revalidate\n< Transfer-Encoding: chunked\n< Content-Type: application/x-git-upload-pack-advertisement\n< \n* Expire cleared\n* Connection #0 to host xxx.booking.com left intact\nHEADServer supports multi_ack_detailed\nServer supports no-done\nServer supports side-band-64k\nServer supports ofs-delta\nServer version is git/2.1.2\nwant dfb112668697c44d3221ce1e603af1ee46fff41e (refs/heads/xxx)\nwant e884b8d79a935fdc9291c2424372cd372f0b7666 (refs/heads/xxx)\nwant d83ee364a2be10ea5d9fa9c19a64d8680f7f2327 (refs/heads/xxx)\nwant 3cf9d21630bb5e380be3537628cd026ddfee8a11 (refs/heads/xxx)\nwant 0ecb911a9062d2a94c5381efab8b10d76acfa487 (refs/heads/xxx)\nwant 53dca992d6162fb08a1c722317621866ade339ca (refs/heads/xxx)\nwant 49de5539e17ed53706159f4fdda2bdacac18f9d6 (refs/heads/xxx)\nwant 6fb579642139bff5e28f313012dbe7ffe4f3aae5 (refs/heads/xxx)\nwant 62bf71564cc70c3d9a5cd8046b09e136f9e27928 (refs/heads/xxx)\nwant c1f98f24174113f901950506ab6a08c54c88e867 (refs/heads/xxx)\nwant 830ee444f8e9f3127ee7c8ac054690581e6bea53 (refs/heads/xxx)\nwant dd7a12a3f7344e78136fe8ea1d8bd5b8a7592bda (refs/heads/xxx)\nwant acc5936de82f6f31a50a3c68dc1fe22bfe5818ef (refs/heads/xxx)\nwant 88c5f7c05e55a83826112c4cc19e2e9fc53bbdfb (refs/heads/xxx)\nwant 639339def49ba7f3d469413e7d26e0d205fa23b7 (refs/heads/xxx)\nwant 65a12becb29987726864709429052f8b63a9e468 (refs/heads/xxx)\nwant 69375797ac43b2427d6c88a07202cb986eb0e91e (refs/heads/xxx)\nwant c32e7a453643f43ad3a840d8972a591345eb4ce6 (refs/heads/xxx)\nwant 024a9c8904934522a8a625145997b118cb07ea85 (refs/heads/xxx)\nwant 7f3b10df5ddb7eb4d8b310812e7406515ff9c439 (refs/heads/xxx)\nwant 762cf79100279f14c1d6089daacd7a800d4574d9 (refs/heads/xxx)\nwant 8b74fe415472720cc72c761af8025e730f8378ce (refs/heads/xxx)\nwant 811b5bea55533bcc158da1a568b92a070a5aff46 (refs/heads/xxx)\nwant 8a46fb3dedb55e63fa580541c9f7633d425a8ea2 (refs/heads/xxx)\nwant d81598d11edcbda0e020b63c3ae6084029b0714c (refs/heads/xxx)\nwant 0641ea1ab05775a73f7e99342859ee638b534159 (refs/heads/xxx)\nwant 837466f5fbd51cf21c93e5d44124dce42deac469 (refs/heads/trunk)\nwant 8225bf4201c14518eef7c81c8964b5f11b12c0eb (refs/heads/xxx)\nwant d366df049d50d879cc3dfaced812ebac91a987ba (refs/heads/xxx)\nwant c47ff2f1b8155dd8a9e0113fd92c3a109dce72b7 (refs/heads/xxx)\nwant dd136cdae6ef772323b922c15b6166666ad987de (refs/tags/admin-20141016-163909)\n<skip 150-odd tags with the same format\n\n<skip lots of back anf forth have/ack, here are the apache logs for them>\n10.189.174.96 - - [20/Oct/2014:15:41:04 +0200] \"POST /git/main.git/git-upload-pack HTTP/1.1\" 200 904\n10.189.174.96 - - [20/Oct/2014:15:41:04 +0200] \"POST /git/main.git/git-upload-pack HTTP/1.1\" 200 1576\n10.189.174.96 - - [20/Oct/2014:15:41:04 +0200] \"POST /git/main.git/git-upload-pack HTTP/1.1\" 200 3312\n10.189.174.96 - - [20/Oct/2014:15:41:04 +0200] \"POST /git/main.git/git-upload-pack HTTP/1.1\" 200 6784\n10.189.174.96 - - [20/Oct/2014:15:41:04 +0200] \"POST /git/main.git/git-upload-pack HTTP/1.1\" 200 13728\n10.189.174.96 - - [20/Oct/2014:15:41:05 +0200] \"POST /git/main.git/git-upload-pack HTTP/1.1\" 200 27840\n10.189.174.96 - - [20/Oct/2014:15:41:05 +0200] \"POST /git/main.git/git-upload-pack HTTP/1.1\" 200 55392\n10.189.174.96 - - [20/Oct/2014:15:41:05 +0200] \"POST /git/main.git/git-upload-pack HTTP/1.1\" 200 112456\n10.189.174.96 - - [20/Oct/2014:15:41:06 +0200] \"POST /git/main.git/git-upload-pack HTTP/1.1\" 200 169800\n10.189.174.96 - - [20/Oct/2014:15:41:06 +0200] \"POST /git/main.git/git-upload-pack HTTP/1.1\" 200 227144\n\n<skip lots of acks>\ngot ack 3 87f1981596402d37d905dfb5356be7aa99020053\ngot ack 3 5ed75db2add2ae8abfb888fac1502ed3fac0221e\ngot ack 3 ee08fd65a137548c6769bf703f4da07307c3023d\n<skip lots of haves>\nhave d6d890f55dcf83d1f6a6328c07a57b0c60f28e30\nhave bd94ba6d06011aaaf620abcea37e756a590bebd5\nhave 3946073390c2548a3f065c88f41a4b8964b9a2dd\nhave 34f881dbb4af0e3b090a93b17fba9c31b7771f61\nhave a7c18a679ce9e6eefe7930556735b29ecfc33952\nPOST git-upload-pack (gzip 262103 to 130185 bytes)\n* Couldn't find host xxx.booking.com in the .netrc file; using defaults\n* Re-using existing connection! (#0) with host xxx.booking.com\n* Connected to xxx.booking.com (10.196.70.243) port 80 (#0)\n> POST /git/main.git/git-upload-pack HTTP/1.1\nUser-Agent: git/2.1.2.dirty\nHost: xxx.booking.com\nAccept-Encoding: gzip\nContent-Type: application/x-git-upload-pack-request\nAccept: application/x-git-upload-pack-result\nContent-Encoding: gzip\nContent-Length: 130185\n\nAnd here it hangs. The request does not show up in the apache\naccess/error log. The upload-pack process does get started though, but\ngit-upload-pack and git-http-backend seem deadlocked. strace shows:\n\nsudo strace -p 3094 -p 3095 -p 3096\nProcess 3094 attached - interrupt to quit\nProcess 3095 attached - interrupt to quit\nProcess 3096 attached - interrupt to quit\n[pid  3095] wait4(3096,  <unfinished ...>\n[pid  3096] write(1, \"0038ACK f3e8514d6e1dd402d27fecd9\"..., 56 <unfinished ...>\n[pid  3094] write(4, \"32have e099999f7a64a93bcf8dd7f59\"..., 8192 <unfinished ...>\n\n\n3094, 3095, 3096 are git-http-backend, git upload pack and\ngit-upload-pack respectively.\n\nBacktrace for the hanging git-fetch-pack (not suer if relevant):\n#0  0x00007feb7abbf530 in __read_nocancel () from /lib64/libpthread.so.0\n#1  0x000000000051531c in xread (fd=0, buf=0x7fff9fc04a60, len=4) at wrapper.c:152\n#2  0x000000000051539b in read_in_full (fd=0, buf=<value optimized out>, count=4) at wrapper.c:201\n#3  0x00000000004cea39 in get_packet_data (fd=<value optimized out>, src_buf=0x0, src_size=0x0, \n    dst=<value optimized out>, size=4, options=2) at pkt-line.c:122\n#4  0x00000000004ceced in packet_read (fd=<value optimized out>, src_buf=0x0, src_len=0x0, \n    buffer=0x7d7560 \"ACK c6597d5122bf0a5c831b4585cd2ad92d953cbdc9 common\", size=65520, options=2) at pkt-line.c:169\n#5  0x00000000004cee7e in packet_read_line_generic (fd=<value optimized out>, len_p=<value optimized out>)\n    at pkt-line.c:199\n#6  packet_read_line (fd=<value optimized out>, len_p=<value optimized out>) at pkt-line.c:209\n#7  0x00000000004a6504 in get_ack (fd=0, \n    result_sha1=0x7fff9fc04ba0 \"\\306Y}Q\\\"\\277\\n\\\\\\203\\033E\\205\\315*\\331-\\225<\\275\", <incomplete sequence \\311>)\n    at fetch-pack.c:194\n#8  0x00000000004a8062 in find_common (args=<value optimized out>, fd=0x7fff9fc04ce0, conn=<value optimized out>, \n    ref=<value optimized out>, dest=<value optimized out>, sought=0xe17610, nr_sought=160, shallow=0x7fff9fc04ca0, \n    pack_lockfile=0x7fff9fc04cd8) at fetch-pack.c:391\n#9  do_fetch_pack (args=<value optimized out>, fd=0x7fff9fc04ce0, conn=<value optimized out>, \n    ref=<value optimized out>, dest=<value optimized out>, sought=0xe17610, nr_sought=160, shallow=0x7fff9fc04ca0, \n    pack_lockfile=0x7fff9fc04cd8) at fetch-pack.c:856\n#10 fetch_pack (args=<value optimized out>, fd=0x7fff9fc04ce0, conn=<value optimized out>, ref=<value optimized out>, \n    dest=<value optimized out>, sought=0xe17610, nr_sought=160, shallow=0x7fff9fc04ca0, pack_lockfile=0x7fff9fc04cd8)\n    at fetch-pack.c:1058\n#11 0x000000000042baf3 in cmd_fetch_pack (argc=<value optimized out>, argv=<value optimized out>, \n    prefix=<value optimized out>) at builtin/fetch-pack.c:180\n#12 0x0000000000404dc5 in run_builtin (argc=9, argv=0x7fff9fc04f90) at git.c:351\n#13 handle_builtin (argc=9, argv=0x7fff9fc04f90) at git.c:529\n#14 0x0000000000405001 in run_argv (argc=9, av=<value optimized out>) at git.c:575\n#15 main (argc=9, av=<value optimized out>) at git.c:662\n\nAnd for the hanging git-upload-pack:\n#0  0x00007f7c8034b4d0 in __write_nocancel () from /lib64/libpthread.so.0\n#1  0x000000000043c9dc in xwrite (fd=1, buf=0x6c70e0, len=56) at wrapper.c:170\n#2  0x000000000043ca5b in write_in_full (fd=1, buf=<value optimized out>, count=56) at wrapper.c:220\n#3  0x000000000043d019 in write_or_die (fd=<value optimized out>, buf=<value optimized out>, \n    count=<value optimized out>) at write_or_die.c:61\n#4  0x00000000004131fa in packet_write (fd=1, fmt=<value optimized out>) at pkt-line.c:93\n#5  0x000000000040538c in get_common_commits (argc=<value optimized out>, argv=0x7fff00000001) at upload-pack.c:420\n#6  upload_pack (argc=<value optimized out>, argv=0x7fff00000001) at upload-pack.c:778\n#7  main (argc=<value optimized out>, argv=0x7fff00000001) at upload-pack.c:846\n\nAnd the hanging git-http-backend:\n#0  0x00007f4c9553d4d0 in __write_nocancel () from /lib64/libpthread.so.0\n#1  0x000000000042d31c in xwrite (fd=4, buf=0x7fff394ea570, len=8192) at wrapper.c:170\n#2  0x000000000042d39b in write_in_full (fd=4, buf=<value optimized out>, count=8192) at wrapper.c:220\n#3  0x0000000000403e5d in inflate_request (prog_name=0x490d98 \"upload-pack\", out=4) at http-backend.c:305\n#4  0x000000000040405d in run_service (argv=0x7fff394ee6d0) at http-backend.c:355\n#5  0x00000000004041d2 in service_rpc (service_name=<value optimized out>) at http-backend.c:508\n#6  0x0000000000404b35 in main (argc=<value optimized out>, argv=<value optimized out>) at http-backend.c:631\n\nBoth git-http-backend and git-upload-pack are trying to write at the\nsame time. I'm *guessing* I've hit some buffer limit here, given that\nthe have/ack exchanges are increasing in size and suddenly this one is\nmisbehaving. However I have no idea where to look next and would really\nappreciate some help.\n\n-- \nDennis Kaarsemaker\nhttp://www.kaarsemaker.net\n"},{"id":"250892","messageId":"1413884908.4175.49.camel@seahawk","threadId":"37764","inReplyTo":"1413815355.4175.26.camel@seahawk","subject":"Bug in fetch-pack causing ever-growing http requests. (Was: git fetch (http) hanging/failing on one specific repository, http only)","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2014-10-21T09:48:28Z","receivedAt":"2014-10-21T09:48:28Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On ma, 2014-10-20 at 16:29 +0200, Dennis Kaarsemaker wrote:\n> Since a few days, one of our repos is causing problems during git fetch,\n> basically git fetch over http hangs during find_common. When using ssh,\n> this does not happen. \n\n<snip things that may not be relevant>.\n\n> And for the hanging git-upload-pack:\n> #0  0x00007f7c8034b4d0 in __write_nocancel () from /lib64/libpthread.so.0\n> #1  0x000000000043c9dc in xwrite (fd=1, buf=0x6c70e0, len=56) at wrapper.c:170\n> #2  0x000000000043ca5b in write_in_full (fd=1, buf=<value optimized out>, count=56) at wrapper.c:220\n> #3  0x000000000043d019 in write_or_die (fd=<value optimized out>, buf=<value optimized out>, \n>     count=<value optimized out>) at write_or_die.c:61\n> #4  0x00000000004131fa in packet_write (fd=1, fmt=<value optimized out>) at pkt-line.c:93\n> #5  0x000000000040538c in get_common_commits (argc=<value optimized out>, argv=0x7fff00000001) at upload-pack.c:420\n> #6  upload_pack (argc=<value optimized out>, argv=0x7fff00000001) at upload-pack.c:778\n> #7  main (argc=<value optimized out>, argv=0x7fff00000001) at upload-pack.c:846\n> \n> And the hanging git-http-backend:\n> #0  0x00007f4c9553d4d0 in __write_nocancel () from /lib64/libpthread.so.0\n> #1  0x000000000042d31c in xwrite (fd=4, buf=0x7fff394ea570, len=8192) at wrapper.c:170\n> #2  0x000000000042d39b in write_in_full (fd=4, buf=<value optimized out>, count=8192) at wrapper.c:220\n> #3  0x0000000000403e5d in inflate_request (prog_name=0x490d98 \"upload-pack\", out=4) at http-backend.c:305\n> #4  0x000000000040405d in run_service (argv=0x7fff394ee6d0) at http-backend.c:355\n> #5  0x00000000004041d2 in service_rpc (service_name=<value optimized out>) at http-backend.c:508\n> #6  0x0000000000404b35 in main (argc=<value optimized out>, argv=<value optimized out>) at http-backend.c:631\n> \n> Both git-http-backend and git-upload-pack are trying to write at the\n> same time. I'm *guessing* I've hit some buffer limit here, given that\n> the have/ack exchanges are increasing in size and suddenly this one is\n> misbehaving. However I have no idea where to look next and would really\n> appreciate some help.\n\nI think the reasoning in 44d8dc54e73e8010c4bdf57a422fc8d5ce709029 is\nincomplete: there's still a pipe involved in the case of gzip'ed request\nbodies, and it's here that it hangs. However, I now do think that this\nis merely a symptom, because after inspecting the output a bit further I\nnoticed that all reponses start with the same lines:\n\ngot ack 3 a669f13aab3a2c192c15574ead70f92b303e8aee\ngot ack 3 360530ff695a4deb01575e85976060a083e17245\ngot ack 3 bab20d62a5a4c34885cf2acbf83aca91908f9af8\n\nIn fact, response N, is the same as response N-1 plus acks for the\ncommits in the 'have' lines of the debug output for the next request. So\nit looks like every request sends all common commits again, which seems\nwrong but does explain the ever-growing request size. After commenting\nout line 413 in fetch-pack.c (state_len = req_buf.len) the requests and\nresponses don't increase in size, and fetch completes, though the\nreceived pack seems too large (http response is 400MB), which makes me\nthink it's not actually ack'ing. Subsequent HTTP fetches don't get a big\npack in response though, so maybe the pack is the right size. THis is a\n*very* busy repo with thousands of commits between the last succesful\nfetch 5 days ago and the first succesfil fetch after my hack.\n\nIn any case, I think there's a bug here, but I don't know nearly enough\nabout the protocol to judge if my \"fix\" is even close to correct. I've\nalso not tested my \"fix\" with any other protocol yet.\n\n-- \nDennis Kaarsemaker\nhttp://www.kaarsemaker.net\n"},{"id":"250896","messageId":"20141021144838.GA11589@seahawk","threadId":"37764","inReplyTo":"1413884908.4175.49.camel@seahawk","subject":"[PATCH] fetch-pack: don't resend known-common refs in find_common","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2014-10-21T14:49:07Z","receivedAt":"2014-10-21T14:49:07Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"By not clearing the request buffer in stateless-rpc mode, fetch-pack\nwould keep sending already known-common commits, leading to ever bigger\nhttp requests, eventually getting too large for git-http-backend to\nhandle properly without filling up the pipe buffer in inflate_request.\n---\nI'm still not quite sure whether this is the right thing to do, but make\ntest still passes :) The new testcase demonstrates the problem, when\nrunning t5551 with EXPENSIVE, this test will hang without the patch to\nfetch-pack.c and succeed otherwise.\n\n fetch-pack.c                |  1 -\n t/t5551-http-fetch-smart.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 655ee64..258245c 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -410,7 +410,6 @@ static int find_common(struct fetch_pack_args *args,\n \t\t\t\t\t\t */\n \t\t\t\t\t\tconst char *hex = sha1_to_hex(result_sha1);\n \t\t\t\t\t\tpacket_buf_write(&req_buf, \"have %s\\n\", hex);\n-\t\t\t\t\t\tstate_len = req_buf.len;\n \t\t\t\t\t}\n \t\t\t\t\tmark_common(commit, 0, 1);\n \t\t\t\t\tretval = 0;\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 6cbc12d..2aac237 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -245,5 +245,37 @@ test_expect_success EXPENSIVE 'clone the 50,000 tag repo to check OS command lin\n \t)\n '\n \n+test_expect_success EXPENSIVE 'create 50,000 more tags' '\n+\t(\n+\tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tfor i in `test_seq 50001 100000`\n+\tdo\n+\t\techo \"commit refs/heads/too-many-refs-again\"\n+\t\techo \"mark :$i\"\n+\t\techo \"committer git <git@example.com> $i +0000\"\n+\t\techo \"data 0\"\n+\t\techo \"M 644 inline bla.txt\"\n+\t\techo \"data 4\"\n+\t\techo \"bla\"\n+\t\t# make every commit dangling by always\n+\t\t# rewinding the branch after each commit\n+\t\techo \"reset refs/heads/too-many-refs-again\"\n+\t\techo \"from :50001\"\n+\tdone | git fast-import --export-marks=marks &&\n+\n+\t# now assign tags to all the dangling commits we created above\n+\ttag=$(perl -e \"print \\\"bla\\\" x 30\") &&\n+\tsed -e \"s|^:\\([^ ]*\\) \\(.*\\)$|\\2 refs/tags/$tag-\\1|\" <marks >>packed-refs\n+\t)\n+'\n+\n+test_expect_success EXPENSIVE 'fetch the new tags' '\n+\t(\n+\t\tcd too-many-refs &&\n+\t\tgit fetch --tags &&\n+\t\ttest $(git for-each-ref refs/tags | wc -l) = 100000\n+\t)\n+'\n+\n stop_httpd\n test_done\n-- \n2.1.0-245-g26e60d4\n"},{"id":"250901","messageId":"xmqqd29l1f3p.fsf@gitster.dls.corp.google.com","threadId":"37764","inReplyTo":"20141021144838.GA11589@seahawk","subject":"Re: [PATCH] fetch-pack: don't resend known-common refs in find_common","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-21T17:56:26Z","receivedAt":"2014-10-21T17:56:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> By not clearing the request buffer in stateless-rpc mode, fetch-pack\n> would keep sending already known-common commits, leading to ever bigger\n> http requests, eventually getting too large for git-http-backend to\n> handle properly without filling up the pipe buffer in inflate_request.\n> ---\n> I'm still not quite sure whether this is the right thing to do, but make\n> test still passes :) The new testcase demonstrates the problem, when\n> running t5551 with EXPENSIVE, this test will hang without the patch to\n> fetch-pack.c and succeed otherwise.\n\nIIUC, because \"stateless\" is just that, i.e. the server-end does not\nkeep track of what is already known, not telling what is known to be\ncommon in each request would fundamentally break the protocol.  Am I\nmistaken?\n\n\n>  fetch-pack.c                |  1 -\n>  t/t5551-http-fetch-smart.sh | 32 ++++++++++++++++++++++++++++++++\n>  2 files changed, 32 insertions(+), 1 deletion(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 655ee64..258245c 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -410,7 +410,6 @@ static int find_common(struct fetch_pack_args *args,\n>  \t\t\t\t\t\t */\n>  \t\t\t\t\t\tconst char *hex = sha1_to_hex(result_sha1);\n>  \t\t\t\t\t\tpacket_buf_write(&req_buf, \"have %s\\n\", hex);\n> -\t\t\t\t\t\tstate_len = req_buf.len;\n>  \t\t\t\t\t}\n>  \t\t\t\t\tmark_common(commit, 0, 1);\n>  \t\t\t\t\tretval = 0;\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index 6cbc12d..2aac237 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -245,5 +245,37 @@ test_expect_success EXPENSIVE 'clone the 50,000 tag repo to check OS command lin\n>  \t)\n>  '\n>  \n> +test_expect_success EXPENSIVE 'create 50,000 more tags' '\n> +\t(\n> +\tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> +\tfor i in `test_seq 50001 100000`\n> +\tdo\n> +\t\techo \"commit refs/heads/too-many-refs-again\"\n> +\t\techo \"mark :$i\"\n> +\t\techo \"committer git <git@example.com> $i +0000\"\n> +\t\techo \"data 0\"\n> +\t\techo \"M 644 inline bla.txt\"\n> +\t\techo \"data 4\"\n> +\t\techo \"bla\"\n> +\t\t# make every commit dangling by always\n> +\t\t# rewinding the branch after each commit\n> +\t\techo \"reset refs/heads/too-many-refs-again\"\n> +\t\techo \"from :50001\"\n> +\tdone | git fast-import --export-marks=marks &&\n> +\n> +\t# now assign tags to all the dangling commits we created above\n> +\ttag=$(perl -e \"print \\\"bla\\\" x 30\") &&\n> +\tsed -e \"s|^:\\([^ ]*\\) \\(.*\\)$|\\2 refs/tags/$tag-\\1|\" <marks >>packed-refs\n> +\t)\n> +'\n> +\n> +test_expect_success EXPENSIVE 'fetch the new tags' '\n> +\t(\n> +\t\tcd too-many-refs &&\n> +\t\tgit fetch --tags &&\n> +\t\ttest $(git for-each-ref refs/tags | wc -l) = 100000\n> +\t)\n> +'\n> +\n>  stop_httpd\n>  test_done\n"},{"id":"250960","messageId":"1413963706.11656.5.camel@seahawk","threadId":"37764","inReplyTo":"xmqqd29l1f3p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] fetch-pack: don't resend known-common refs in find_common","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2014-10-22T07:41:46Z","receivedAt":"2014-10-22T07:41:46Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On di, 2014-10-21 at 10:56 -0700, Junio C Hamano wrote:\n> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n> \n> > By not clearing the request buffer in stateless-rpc mode, fetch-pack\n> > would keep sending already known-common commits, leading to ever bigger\n> > http requests, eventually getting too large for git-http-backend to\n> > handle properly without filling up the pipe buffer in inflate_request.\n> > ---\n> > I'm still not quite sure whether this is the right thing to do, but make\n> > test still passes :) The new testcase demonstrates the problem, when\n> > running t5551 with EXPENSIVE, this test will hang without the patch to\n> > fetch-pack.c and succeed otherwise.\n> \n> IIUC, because \"stateless\" is just that, i.e. the server-end does not\n> keep track of what is already known, not telling what is known to be\n> common in each request would fundamentally break the protocol.  Am I\n> mistaken?\n\nThat sounds plausible, but why then does the fetch complete with this\nline removed, and why does 'make test' still pass? I tried to understand\nthe protocol, but the documentation has TODO's in some critical\nplaces :)\n\nAnd if that's true, it means the inflate_request / upload-pack\ninteraction should be fixed, so more than 64k (current linux pipe buffer\nsize) of uncompressed data is supported. I see two options:\n\n* Turning that interaction into a more cooperative process, with a\n  select/poll loop\n* Make upload-pack buffer its entire response when run in stateless_rpc\n  mode until it has consumed all of the request\n\nThe latter sounds easier to do, but not being very familiar with the\nprotocol, I may have missed something obvious.\n\n-- \nDennis Kaarsemaker\nhttp://www.kaarsemaker.net\n"},{"id":"250962","messageId":"CACsJy8B7nOZCjdF_hki5amv-uCY17hz59oPDvx-ufbRrdvTweQ@mail.gmail.com","threadId":"37764","inReplyTo":"1413963706.11656.5.camel@seahawk","subject":"Re: [PATCH] fetch-pack: don't resend known-common refs in find_common","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-10-22T10:07:31Z","receivedAt":"2014-10-22T10:07:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 22, 2014 at 2:41 PM, Dennis Kaarsemaker\n<dennis@kaarsemaker.net> wrote:\n> I see two options:\n>\n> * Turning that interaction into a more cooperative process, with a\n>   select/poll loop\n> * Make upload-pack buffer its entire response when run in stateless_rpc\n>   mode until it has consumed all of the request\n\nOr add a helper daemon and support stateful smart http. Or maybe\nthat's what you meant in the first option.\n-- \nDuy\n"},{"id":"250970","messageId":"xmqqfveghvw3.fsf@gitster.dls.corp.google.com","threadId":"37764","inReplyTo":"1413963706.11656.5.camel@seahawk","subject":"Re: [PATCH] fetch-pack: don't resend known-common refs in find_common","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-22T17:11:40Z","receivedAt":"2014-10-22T17:11:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> On di, 2014-10-21 at 10:56 -0700, Junio C Hamano wrote:\n>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n>> \n>> > By not clearing the request buffer in stateless-rpc mode, fetch-pack\n>> > would keep sending already known-common commits, leading to ever bigger\n>> > http requests, eventually getting too large for git-http-backend to\n>> > handle properly without filling up the pipe buffer in inflate_request.\n>> > ---\n>> > I'm still not quite sure whether this is the right thing to do, but make\n>> > test still passes :) The new testcase demonstrates the problem, when\n>> > running t5551 with EXPENSIVE, this test will hang without the patch to\n>> > fetch-pack.c and succeed otherwise.\n>> \n>> IIUC, because \"stateless\" is just that, i.e. the server-end does not\n>> keep track of what is already known, not telling what is known to be\n>> common in each request would fundamentally break the protocol.  Am I\n>> mistaken?\n>\n> That sounds plausible, but why then does the fetch complete with this\n> line removed, and why does 'make test' still pass?\n\nThe fetch-pack program tries to help the upload-pack program(s)\nrunning on the other end find what nodes in the graph both\nrepositories have in common by sending what the repository on its\nend has.  Some commits may not be known by the other side (e.g. your\nnew commits that haven't been pushed there that are made on a branch\nforked from the common history), and some others may be known\n(i.e. you drilled down the history from the tips of your refs and\nreached a commit that you fetched from the common history\npreviously).  The latter are ACKed by the upload-pack process and\nare remembered to be re-sent to the _next_ incarnation of the\nupload-pack process when stateless RPC is in use.\n\nWith your patch, you stop telling the upload-pack process what these\ntwo programs already found to be common in their exchange.  After\nthe first exchange, fetch-pack and upload-pack may have noticed that\nboth ends have version 2.0, but because you do not convey that fact\nto the other side, the new incarnation of upload-pack may end up\ndeciding that the version 1.9 is the newest common commit between\nthe two, and sending commits between 1.9 and 2.0.\n\nIf you imagine an extreme case, it would be easy to see why \"the\nfetch completes\" and \"make test passes\" are not sufficient to say\nanything about this change.  Even if you break the protocol in in a\nway different from your patch, by not sending any \"have\", such a\nbutchered \"fetch-pack\" will become \"fetch everything from scratch\",\naka \"clone\".  The end result will still have correct history and\n\"fetch completes\" would be true.\n\nBut I'd prefer deferring a more detailed analysis/explanation to\nShawn, as stateless RPC is his creation.\n"},{"id":"251061","messageId":"20141026153931.GA28288@spirit","threadId":"37764","inReplyTo":"xmqqfveghvw3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] fetch-pack: don't resend known-common refs in find_common","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2014-10-26T15:39:33Z","receivedAt":"2014-10-26T15:39:33Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On Wed, Oct 22, 2014 at 10:11:40AM -0700, Junio C Hamano wrote:\n> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n> \n> > On di, 2014-10-21 at 10:56 -0700, Junio C Hamano wrote:\n> >> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n> >> \n> >> > By not clearing the request buffer in stateless-rpc mode, fetch-pack\n> >> > would keep sending already known-common commits, leading to ever bigger\n> >> > http requests, eventually getting too large for git-http-backend to\n> >> > handle properly without filling up the pipe buffer in inflate_request.\n> >> > ---\n> >> > I'm still not quite sure whether this is the right thing to do, but make\n> >> > test still passes :) The new testcase demonstrates the problem, when\n> >> > running t5551 with EXPENSIVE, this test will hang without the patch to\n> >> > fetch-pack.c and succeed otherwise.\n> >> \n> >> IIUC, because \"stateless\" is just that, i.e. the server-end does not\n> >> keep track of what is already known, not telling what is known to be\n> >> common in each request would fundamentally break the protocol.  Am I\n> >> mistaken?\n> >\n> > That sounds plausible, but why then does the fetch complete with this\n> > line removed, and why does 'make test' still pass?\n> \n> The fetch-pack program tries to help the upload-pack program(s)\n> running on the other end find what nodes in the graph both\n> repositories have in common by sending what the repository on its\n> end has.  Some commits may not be known by the other side (e.g. your\n> new commits that haven't been pushed there that are made on a branch\n> forked from the common history), and some others may be known\n> (i.e. you drilled down the history from the tips of your refs and\n> reached a commit that you fetched from the common history\n> previously).  The latter are ACKed by the upload-pack process and\n> are remembered to be re-sent to the _next_ incarnation of the\n> upload-pack process when stateless RPC is in use.\n> \n> With your patch, you stop telling the upload-pack process what these\n> two programs already found to be common in their exchange.  After\n> the first exchange, fetch-pack and upload-pack may have noticed that\n> both ends have version 2.0, but because you do not convey that fact\n> to the other side, the new incarnation of upload-pack may end up\n> deciding that the version 1.9 is the newest common commit between\n> the two, and sending commits between 1.9 and 2.0.\n> \n> If you imagine an extreme case, it would be easy to see why \"the\n> fetch completes\" and \"make test passes\" are not sufficient to say\n> anything about this change.  Even if you break the protocol in in a\n> way different from your patch, by not sending any \"have\", such a\n> butchered \"fetch-pack\" will become \"fetch everything from scratch\",\n> aka \"clone\".  The end result will still have correct history and\n> \"fetch completes\" would be true.\n> \n> But I'd prefer deferring a more detailed analysis/explanation to\n> Shawn, as stateless RPC is his creation.\n\nThanks for the explanation, that makes it quite clear that this approach\nis wrong. The patch below (apologies for the formatting, I'm not quite\nsure how to use format-patch in this situation) does it differently: by\nbuffering upload-pack's output until it has read all the input, the new\ntest still succeeds and again 'make test' passes. \n\n---\n t/t5551-http-fetch-smart.sh | 32 ++++++++++++++++++++++++++++++++\n upload-pack.c               | 29 +++++++++++++++++++----------\n 2 files changed, 51 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 6cbc12d..2aac237 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -245,5 +245,37 @@ test_expect_success EXPENSIVE 'clone the 50,000 tag repo to check OS command lin\n \t)\n '\n \n+test_expect_success EXPENSIVE 'create 50,000 more tags' '\n+\t(\n+\tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tfor i in `test_seq 50001 100000`\n+\tdo\n+\t\techo \"commit refs/heads/too-many-refs-again\"\n+\t\techo \"mark :$i\"\n+\t\techo \"committer git <git@example.com> $i +0000\"\n+\t\techo \"data 0\"\n+\t\techo \"M 644 inline bla.txt\"\n+\t\techo \"data 4\"\n+\t\techo \"bla\"\n+\t\t# make every commit dangling by always\n+\t\t# rewinding the branch after each commit\n+\t\techo \"reset refs/heads/too-many-refs-again\"\n+\t\techo \"from :50001\"\n+\tdone | git fast-import --export-marks=marks &&\n+\n+\t# now assign tags to all the dangling commits we created above\n+\ttag=$(perl -e \"print \\\"bla\\\" x 30\") &&\n+\tsed -e \"s|^:\\([^ ]*\\) \\(.*\\)$|\\2 refs/tags/$tag-\\1|\" <marks >>packed-refs\n+\t)\n+'\n+\n+test_expect_success EXPENSIVE 'fetch the new tags' '\n+\t(\n+\t\tcd too-many-refs &&\n+\t\tgit fetch --tags &&\n+\t\ttest $(git for-each-ref refs/tags | wc -l) = 100000\n+\t)\n+'\n+\n stop_httpd\n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex ac9ac15..3d76750 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -373,6 +373,7 @@ static int get_common_commits(void)\n \tint got_common = 0;\n \tint got_other = 0;\n \tint sent_ready = 0;\n+\tstruct strbuf resp_buf = STRBUF_INIT;\n \n \tsave_commit_buffer = 0;\n \n@@ -384,15 +385,19 @@ static int get_common_commits(void)\n \t\t\tif (multi_ack == 2 && got_common\n \t\t\t    && !got_other && ok_to_give_up()) {\n \t\t\t\tsent_ready = 1;\n-\t\t\t\tpacket_write(1, \"ACK %s ready\\n\", last_hex);\n+\t\t\t\tpacket_buf_write(&resp_buf, \"ACK %s ready\\n\", last_hex);\n \t\t\t}\n \t\t\tif (have_obj.nr == 0 || multi_ack)\n-\t\t\t\tpacket_write(1, \"NAK\\n\");\n+\t\t\t\tpacket_buf_write(&resp_buf, \"NAK\\n\");\n \n \t\t\tif (no_done && sent_ready) {\n-\t\t\t\tpacket_write(1, \"ACK %s\\n\", last_hex);\n+\t\t\t\tpacket_buf_write(&resp_buf, \"ACK %s\\n\", last_hex);\n+\t\t\t\twrite_or_die(1, resp_buf.buf, resp_buf.len);\n+\t\t\t\tstrbuf_release(&resp_buf);\n \t\t\t\treturn 0;\n \t\t\t}\n+\t\t\twrite_or_die(1, resp_buf.buf, resp_buf.len);\n+\t\t\tstrbuf_release(&resp_buf);\n \t\t\tif (stateless_rpc)\n \t\t\t\texit(0);\n \t\t\tgot_common = 0;\n@@ -407,20 +412,20 @@ static int get_common_commits(void)\n \t\t\t\t\tconst char *hex = sha1_to_hex(sha1);\n \t\t\t\t\tif (multi_ack == 2) {\n \t\t\t\t\t\tsent_ready = 1;\n-\t\t\t\t\t\tpacket_write(1, \"ACK %s ready\\n\", hex);\n+\t\t\t\t\t\tpacket_buf_write(&resp_buf, \"ACK %s ready\\n\", hex);\n \t\t\t\t\t} else\n-\t\t\t\t\t\tpacket_write(1, \"ACK %s continue\\n\", hex);\n+\t\t\t\t\t\tpacket_buf_write(&resp_buf, \"ACK %s continue\\n\", hex);\n \t\t\t\t}\n \t\t\t\tbreak;\n \t\t\tdefault:\n \t\t\t\tgot_common = 1;\n \t\t\t\tmemcpy(last_hex, sha1_to_hex(sha1), 41);\n \t\t\t\tif (multi_ack == 2)\n-\t\t\t\t\tpacket_write(1, \"ACK %s common\\n\", last_hex);\n+\t\t\t\t\tpacket_buf_write(&resp_buf, \"ACK %s common\\n\", last_hex);\n \t\t\t\telse if (multi_ack)\n-\t\t\t\t\tpacket_write(1, \"ACK %s continue\\n\", last_hex);\n+\t\t\t\t\tpacket_buf_write(&resp_buf, \"ACK %s continue\\n\", last_hex);\n \t\t\t\telse if (have_obj.nr == 1)\n-\t\t\t\t\tpacket_write(1, \"ACK %s\\n\", last_hex);\n+\t\t\t\t\tpacket_buf_write(&resp_buf, \"ACK %s\\n\", last_hex);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tcontinue;\n@@ -428,10 +433,14 @@ static int get_common_commits(void)\n \t\tif (!strcmp(line, \"done\")) {\n \t\t\tif (have_obj.nr > 0) {\n \t\t\t\tif (multi_ack)\n-\t\t\t\t\tpacket_write(1, \"ACK %s\\n\", last_hex);\n+\t\t\t\t\tpacket_buf_write(&resp_buf, \"ACK %s\\n\", last_hex);\n+\t\t\t\twrite_or_die(1, resp_buf.buf, resp_buf.len);\n+\t\t\t\tstrbuf_release(&resp_buf);\n \t\t\t\treturn 0;\n \t\t\t}\n-\t\t\tpacket_write(1, \"NAK\\n\");\n+\t\t\tpacket_buf_write(&resp_buf, \"NAK\\n\");\n+\t\t\twrite_or_die(1, resp_buf.buf, resp_buf.len);\n+\t\t\tstrbuf_release(&resp_buf);\n \t\t\treturn -1;\n \t\t}\n \t\tdie(\"git upload-pack: expected SHA1 list, got '%s'\", line);\n-- \n2.1.0-245-g26e60d4\n"},{"id":"251062","messageId":"20141026154241.GB28288@spirit","threadId":"37764","inReplyTo":"CACsJy8B7nOZCjdF_hki5amv-uCY17hz59oPDvx-ufbRrdvTweQ@mail.gmail.com","subject":"Re: [PATCH] fetch-pack: don't resend known-common refs in find_common","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2014-10-26T15:42:42Z","receivedAt":"2014-10-26T15:42:42Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On Wed, Oct 22, 2014 at 05:07:31PM +0700, Duy Nguyen wrote:\n> On Wed, Oct 22, 2014 at 2:41 PM, Dennis Kaarsemaker\n> <dennis@kaarsemaker.net> wrote:\n> > I see two options:\n> >\n> > * Turning that interaction into a more cooperative process, with a\n> >   select/poll loop\n> > * Make upload-pack buffer its entire response when run in stateless_rpc\n> >   mode until it has consumed all of the request\n> \n> Or add a helper daemon and support stateful smart http. Or maybe\n> that's what you meant in the first option.\n\nNo, I meant that get-http-backend should have a select/poll loop that\ncan read from and write to git-upload-pack at the same time, but option\ntwo was easier to implement :)\n\nStateful smart http seems to be against the design goals of smart http if I\ninterpret Documentation/technical/http-protocol.txt correctly though, so\nthat doesn't seem to be the right approach.\n-- \nDennis Kaarsemaker <dennis@kaarsemaker.net>\nhttp://twitter.com/seveas\n"},{"id":"253214","messageId":"CAJo=hJvt1fkgkMdQX=dkcdBcmv_PT4pnz5BC9d0DSLCi=Q7ZbQ@mail.gmail.com","threadId":"37764","inReplyTo":"20141026153931.GA28288@spirit","subject":"Re: [PATCH] fetch-pack: don't resend known-common refs in find_common","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2014-12-06T00:48:10Z","receivedAt":"2014-12-06T00:48:10Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Sun, Oct 26, 2014 at 8:39 AM, Dennis Kaarsemaker\n<dennis@kaarsemaker.net> wrote:\n> On Wed, Oct 22, 2014 at 10:11:40AM -0700, Junio C Hamano wrote:\n>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n>>\n>> > On di, 2014-10-21 at 10:56 -0700, Junio C Hamano wrote:\n>> >> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n>> >>\n>> >> > By not clearing the request buffer in stateless-rpc mode, fetch-pack\n>> >> > would keep sending already known-common commits, leading to ever bigger\n>> >> > http requests, eventually getting too large for git-http-backend to\n>> >> > handle properly without filling up the pipe buffer in inflate_request.\n>> >> > ---\n>> >> > I'm still not quite sure whether this is the right thing to do, but make\n>> >> > test still passes :) The new testcase demonstrates the problem, when\n>> >> > running t5551 with EXPENSIVE, this test will hang without the patch to\n>> >> > fetch-pack.c and succeed otherwise.\n>> >>\n>> >> IIUC, because \"stateless\" is just that, i.e. the server-end does not\n>> >> keep track of what is already known, not telling what is known to be\n>> >> common in each request would fundamentally break the protocol.  Am I\n>> >> mistaken?\n>> >\n>> > That sounds plausible, but why then does the fetch complete with this\n>> > line removed, and why does 'make test' still pass?\n>>\n>> The fetch-pack program tries to help the upload-pack program(s)\n>> running on the other end find what nodes in the graph both\n>> repositories have in common by sending what the repository on its\n>> end has.  Some commits may not be known by the other side (e.g. your\n>> new commits that haven't been pushed there that are made on a branch\n>> forked from the common history), and some others may be known\n>> (i.e. you drilled down the history from the tips of your refs and\n>> reached a commit that you fetched from the common history\n>> previously).  The latter are ACKed by the upload-pack process and\n>> are remembered to be re-sent to the _next_ incarnation of the\n>> upload-pack process when stateless RPC is in use.\n>>\n>> With your patch, you stop telling the upload-pack process what these\n>> two programs already found to be common in their exchange.  After\n>> the first exchange, fetch-pack and upload-pack may have noticed that\n>> both ends have version 2.0, but because you do not convey that fact\n>> to the other side, the new incarnation of upload-pack may end up\n>> deciding that the version 1.9 is the newest common commit between\n>> the two, and sending commits between 1.9 and 2.0.\n>>\n>> If you imagine an extreme case, it would be easy to see why \"the\n>> fetch completes\" and \"make test passes\" are not sufficient to say\n>> anything about this change.  Even if you break the protocol in in a\n>> way different from your patch, by not sending any \"have\", such a\n>> butchered \"fetch-pack\" will become \"fetch everything from scratch\",\n>> aka \"clone\".  The end result will still have correct history and\n>> \"fetch completes\" would be true.\n>>\n>> But I'd prefer deferring a more detailed analysis/explanation to\n>> Shawn, as stateless RPC is his creation.\n\nJunio's statement above about the world is correct.\n\n> Thanks for the explanation, that makes it quite clear that this approach\n> is wrong. The patch below (apologies for the formatting, I'm not quite\n> sure how to use format-patch in this situation) does it differently: by\n> buffering upload-pack's output until it has read all the input, the new\n> test still succeeds and again 'make test' passes.\n\nThis probably introduces latency into the traditional bidirectional\nmulti_ack protocol.\n\n> @@ -384,15 +385,19 @@ static int get_common_commits(void)\n>                         if (multi_ack == 2 && got_common\n>                             && !got_other && ok_to_give_up()) {\n>                                 sent_ready = 1;\n> -                               packet_write(1, \"ACK %s ready\\n\", last_hex);\n> +                               packet_buf_write(&resp_buf, \"ACK %s ready\\n\", last_hex);\n>                         }\n>                         if (have_obj.nr == 0 || multi_ack)\n> -                               packet_write(1, \"NAK\\n\");\n> +                               packet_buf_write(&resp_buf, \"NAK\\n\");\n\nBy buffering and delaying these when !stateless_rpc we defer telling\nthe peer in a multi_ack exchange. That delays the peer from cutting\noff its communication by about 1RTT.\n"}]}