{"thread":{"id":"32917","subject":"[PATCH 0/3] make smart-http more robust against bogus server input","startedAt":"2013-02-16T06:44:55Z","lastAt":"2013-02-20T08:54:15Z","messageCount":50,"participants":["Jeff King","Jonathan Nieder","Junio C Hamano","Shawn Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"209582","messageId":"20130216064455.GA27063@sigill.intra.peff.net","threadId":"32917","inReplyTo":null,"subject":"[PATCH 0/3] make smart-http more robust against bogus server input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-16T06:44:55Z","receivedAt":"2013-02-16T06:44:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For the most part, smart-http just passes data to fetch-pack and\nsend-pack, which take care of the heavy lifting. However, I did find a\nfew corner cases around truncated data from the server, one of which can\nactually cause a deadlock.\n\nI found these because I was trying to figure out what was going on with\nsome hung git processes which were in a deadlock like the one described\nin patch 3. But having experimented and read the code, I don't think\nthat it is triggerable from a normal clone, but rather only when you\npoke git-remote-curl in the right way. So it may or may not be my\nculprit, but these patches do make remote-curl more robust, which is a\ngood thing.\n\n [1/3]: pkt-line: teach packet_get_line a no-op mode\n [2/3]: remote-curl: verify smart-http metadata lines\n [3/3]: remote-curl: sanity check ref advertisement from server\n\n-Peff\n"},{"id":"209583","messageId":"20130216064612.GA22626@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130216064455.GA27063@sigill.intra.peff.net","subject":"[PATCH 1/3] pkt-line: teach packet_get_line a no-op mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-16T06:46:12Z","receivedAt":"2013-02-16T06:46:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"You can use packet_get_line to parse a single packet out of\na stream and into a buffer. However, if you just want to\nthrow away a set of packets from the stream, there's no need\nto even bother copying the bytes. This patch treats a NULL\noutput buffer as a hint that the caller does not even want\nto see the output.\n\nWe have to tweak the packet_trace call, too, since it showed\nthe trace from the copied buffer, which now might not exist.\nThe new code is actually more correct, though, as it shows\njust what we parsed, not any cruft that may have been in the\noutput buffer before (it never mattered, though, because all\ncallers gave us a fresh buffer).\n\nSigned-off-by: Jeff King \n---\n pkt-line.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex eaba15f..7f28701 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -234,9 +234,10 @@ int packet_get_line(struct strbuf *out,\n \t*src_len -= 4;\n \tlen -= 4;\n \n-\tstrbuf_add(out, *src_buf, len);\n+\tif (out)\n+\t\tstrbuf_add(out, *src_buf, len);\n+\tpacket_trace(*src_buf, len, 0);\n \t*src_buf += len;\n \t*src_len -= len;\n-\tpacket_trace(out->buf, out->len, 0);\n \treturn len;\n }\n-- \n1.8.1.2.11.g1a2f572\n"},{"id":"209584","messageId":"20130216064707.GB22626@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130216064455.GA27063@sigill.intra.peff.net","subject":"[PATCH 2/3] remote-curl: verify smart-http metadata lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-16T06:47:07Z","receivedAt":"2013-02-16T06:47:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A smart http ref advertisement starts with a packet\ncontaining the service header, followed by an arbitrary\nnumber of packets containing other metadata headers,\nfollowed by a flush packet.\n\nWe don't currently recognize any other metadata headers, so\nwe just parse through any extra packets, throwing away their\ncontents. However, we don't do so very carefully, and just\nstop at the first error or flush packet.\n\nLet's flag any errors we see here, which might be a sign of\ntruncated or corrupted output. Since the rest of the data\nshould be the ref advertisement, and since we pass that\nalong to our helper programs (like fetch-pack), they will\nprobably notice the error, as whatever cruft is in the\nbuffer will not parse. However, it's nice to note problems\nas early as possible, which can help in debugging the root\ncause.\n\nSigned-off-by: Jeff King \n---\n remote-curl.c | 21 +++++++++++++++++----\n 1 file changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 933c69a..73134f5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -90,6 +90,17 @@ static void free_discovery(struct discovery *d)\n \t}\n }\n \n+static int read_packets_until_flush(char **buf, size_t *len)\n+{\n+\twhile (1) {\n+\t\tint r = packet_get_line(NULL, buf, len);\n+\t\tif (r < 0)\n+\t\t\treturn -1;\n+\t\tif (r == 0)\n+\t\t\treturn 0;\n+\t}\n+}\n+\n static struct discovery* discover_refs(const char *service)\n {\n \tstruct strbuf exp = STRBUF_INIT;\n@@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char *service)\n \n \t\t/* The header can include additional metadata lines, up\n \t\t * until a packet flush marker. Ignore these now, but\n-\t\t * in the future we might start to scan them.\n+\t\t * in the future we might start to scan them. However, we do\n+\t\t * still check to make sure we are getting valid packet lines,\n+\t\t * ending with a flush.\n \t\t */\n-\t\tstrbuf_reset(&buffer);\n-\t\twhile (packet_get_line(&buffer, &last->buf, &last->len) > 0)\n-\t\t\tstrbuf_reset(&buffer);\n+\t\tif (read_packets_until_flush(&last->buf, &last->len) < 0)\n+\t\t\tdie(\"smart-http metadata lines are invalid at %s\",\n+\t\t\t refs_url);\n \n \t\tlast->proto_git = 1;\n \t}\n-- \n1.8.1.2.11.g1a2f572\n"},{"id":"209585","messageId":"20130216064929.GC22626@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130216064455.GA27063@sigill.intra.peff.net","subject":"[PATCH 3/3] remote-curl: sanity check ref advertisement from server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-16T06:49:29Z","receivedAt":"2013-02-16T06:49:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the smart HTTP response from the server is truncated for\nany reason, we will get an incomplete ref advertisement. If\nwe then feed this incomplete list to \"fetch-pack\", one of a\nfew things may happen:\n\n 1. If the truncation is in a packet header, fetch-pack\n will notice the bogus line and complain.\n\n 2. If the truncation is inside a packet, fetch-pack will\n keep waiting for us to send the rest of the packet,\n which we never will.\n\n 3. If the truncation is at a packet boundary, fetch-pack\n will keep waiting for us to send the next packet, which\n we never will.\n\nAs a result, fetch-pack hangs, waiting for input. However,\nremote-curl believes it has sent all of the advertisement,\nand therefore waits for fetch-pack to speak. The two\nprocesses end up in a deadlock.\n\nThis fortunately doesn't happen in the normal fetching\nworkflow, because git-fetch first uses the \"list\" command,\nwhich feeds the refs to get_remote_heads, which does notice\nthe error. However, you can trigger it by sending a direct\n\"fetch\" to the remote-curl helper.\n\nWe can make this more robust by verifying that the packet\nstream we got from the server does indeed parse correctly\nand ends with a flush packet, which means that what\nfetch-pack receives will at least be syntactically correct.\n\nThe normal non-stateless-rpc case does not have to deal with\nthis problem; it detects a truncation by getting EOF on the\nfile descriptor before it has read all data. So it is\ntempting to think that we can solve this by closing the\ndescriptor after relaying the server's advertisement.\nUnfortunately, in the stateless rpc case, we need to keep\nthe descriptor to fetch-pack open in order to pass more data\nto it.\n\nWe could solve that by using two descriptors, but our\nrun-command interface does not support that (and modifying\nit to create more pipes would make life hard for the Windows\nport of git).\n\nSigned-off-by: Jeff King \n---\n remote-curl.c | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 73134f5..c7647c7 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -101,6 +101,15 @@ static int read_packets_until_flush(char **buf, size_t *len)\n \t}\n }\n \n+static int verify_ref_advertisement(char *buf, size_t len)\n+{\n+\t/*\n+\t * Our function parameters are copies, so we do not\n+\t * have to care that read_packets will increment our pointers.\n+\t */\n+\treturn read_packets_until_flush(&buf, &len);\n+}\n+\n static struct discovery* discover_refs(const char *service)\n {\n \tstruct strbuf exp = STRBUF_INIT;\n@@ -174,6 +183,9 @@ static struct discovery* discover_refs(const char *service)\n \t\t\tdie(\"smart-http metadata lines are invalid at %s\",\n \t\t\t refs_url);\n \n+\t\tif (verify_ref_advertisement(last->buf, last->len) < 0)\n+\t\t\tdie(\"ref advertisement is invalid at %s\", refs_url);\n+\n \t\tlast->proto_git = 1;\n \t}\n \n-- \n1.8.1.2.11.g1a2f572\n"},{"id":"209591","messageId":"20130217104111.GD6759@elie.Belkin","threadId":"32917","inReplyTo":"20130216064612.GA22626@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] pkt-line: teach packet_get_line a no-op mode","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-17T10:41:11Z","receivedAt":"2013-02-17T10:41:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -234,9 +234,10 @@ int packet_get_line(struct strbuf *out,\n> \t*src_len -= 4;\n> \tlen -= 4;\n> \n> -\tstrbuf_add(out, *src_buf, len);\n> +\tif (out)\n> +\t\tstrbuf_add(out, *src_buf, len);\n> +\tpacket_trace(*src_buf, len, 0);\n> \t*src_buf += len;\n> \t*src_len -= len;\n> -\tpacket_trace(out->buf, out->len, 0);\n> \treturn len;\n\nFor what it's worth,\nReviewed-by: Jonathan Nieder \n\nThe above code has a structure of\n\n\tprepare to return(buf, len);\n\ttrace(buf, len);\n\n\tdiscard used part of buf;\n\treturn;\n\nwhich is nice and readable.\nJonathan\n"},{"id":"209592","messageId":"20130217104939.GE6759@elie.Belkin","threadId":"32917","inReplyTo":"20130216064707.GB22626@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-17T10:49:39Z","receivedAt":"2013-02-17T10:49:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n[...]\n> @@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char *service)\n[...]\n> -\t\tstrbuf_reset(&buffer);\n> -\t\twhile (packet_get_line(&buffer, &last->buf, &last->len) > 0)\n> -\t\t\tstrbuf_reset(&buffer);\n> +\t\tif (read_packets_until_flush(&last->buf, &last->len) < 0)\n\nStyle nit: this made me wonder \"What would it mean if\nread_packets_until_flush() > 0?\" Since the convention for this\nfunction is \"0 for success\", I would personally find\n\n\t\tif (read_packets_until_flush(...))\n\t\t\thandle error;\n\neasier to read.\n\n> +\t\t\tdie(\"smart-http metadata lines are invalid at %s\",\n> +\t\t\t refs_url);\n\nEspecially given that other clients would be likely to run into\ntrouble in the same situation, as long as this cooks in \"next\" for a\nsuitable amount of time to catch bad servers, it looks like a good\nidea.\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"209593","messageId":"20130217110533.GF6759@elie.Belkin","threadId":"32917","inReplyTo":"20130216064929.GC22626@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] remote-curl: sanity check ref advertisement from server","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-17T11:05:34Z","receivedAt":"2013-02-17T11:05:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> If the smart HTTP response from the server is truncated for\n> any reason, we will get an incomplete ref advertisement. If\n> we then feed this incomplete list to \"fetch-pack\", one of a\n> few things may happen:\n>\n> 1. If the truncation is in a packet header, fetch-pack\n> will notice the bogus line and complain.\n>\n> 2. If the truncation is inside a packet, fetch-pack will\n> keep waiting for us to send the rest of the packet,\n> which we never will.\n\nMostly harmless since the operator could hit ^C, but still unpleasant.\n\n[...]\n> This fortunately doesn't happen in the normal fetching\n> workflow, because git-fetch first uses the \"list\" command,\n> which feeds the refs to get_remote_heads, which does notice\n> the error. However, you can trigger it by sending a direct\n> \"fetch\" to the remote-curl helper.\n\nAh. Would a test for this make sense?\n\n[...]\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n[...]\n> @@ -174,6 +183,9 @@ static struct discovery* discover_refs(const char *service)\n> \t\t\tdie(\"smart-http metadata lines are invalid at %s\",\n> \t\t\t refs_url);\n> \n> +\t\tif (verify_ref_advertisement(last->buf, last->len) < 0)\n> +\t\t\tdie(\"ref advertisement is invalid at %s\", refs_url);\n\nWon't this error out with\n\n\tprotocol error: bad line length character: ERR\n\ninstead of the current more helpful behavior for ERR lines?\n\nSame stylistic comment about \"what would it mean for the return value\nto be positive?\" as in patch 2/3.\n\nAside from those two details, the idea looks sane, though. Good\ncatch, and thanks for a pleasant read.\n\nGood night,\nJonathan\n"},{"id":"209608","messageId":"20130217191422.GA25096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130217104939.GE6759@elie.Belkin","subject":"Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-17T19:14:22Z","receivedAt":"2013-02-17T19:14:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 17, 2013 at 02:49:39AM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > --- a/remote-curl.c\n> > +++ b/remote-curl.c\n> [...]\n> > @@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char *service)\n> [...]\n> > -\t\tstrbuf_reset(&buffer);\n> > -\t\twhile (packet_get_line(&buffer, &last->buf, &last->len) > 0)\n> > -\t\t\tstrbuf_reset(&buffer);\n> > +\t\tif (read_packets_until_flush(&last->buf, &last->len) < 0)\n> \n> Style nit: this made me wonder \"What would it mean if\n> read_packets_until_flush() > 0?\" Since the convention for this\n> function is \"0 for success\", I would personally find\n> \n> \t\tif (read_packets_until_flush(...))\n> \t\t\thandle error;\n> \n> easier to read.\n\nMy intent was that it followed the error convention of \"negative is\nerror, 0 is success, and positive is not used, but reserved for\nfuture use\". And I tend to think the \"< 0\" makes it obvious that we are\ninterested in error. But I don't feel that strongly, so if people would\nrather see it the other way, I can live with it.\n\n> > +\t\t\tdie(\"smart-http metadata lines are invalid at %s\",\n> > +\t\t\t refs_url);\n> \n> Especially given that other clients would be likely to run into\n> trouble in the same situation, as long as this cooks in \"next\" for a\n> suitable amount of time to catch bad servers, it looks like a good\n> idea.\n\nYeah, I have a slight concern that this series would break something in\nanother implementation, so I would like to see this cook in \"next\" for a\nwhile (and would be slated for master probably not in this release, but\nin the next one). But I think this change is pretty straightforward. If\nan implementation is producing bogus packet lines and expecting us not\nto complain, it really needs to be fixed.\n\n-Peff\n"},{"id":"209609","messageId":"20130217192830.GB25096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130217110533.GF6759@elie.Belkin","subject":"Re: [PATCH 3/3] remote-curl: sanity check ref advertisement from server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-17T19:28:30Z","receivedAt":"2013-02-17T19:28:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 17, 2013 at 03:05:34AM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > If the smart HTTP response from the server is truncated for\n> > any reason, we will get an incomplete ref advertisement. If\n> > we then feed this incomplete list to \"fetch-pack\", one of a\n> > few things may happen:\n> >\n> > 1. If the truncation is in a packet header, fetch-pack\n> > will notice the bogus line and complain.\n> >\n> > 2. If the truncation is inside a packet, fetch-pack will\n> > keep waiting for us to send the rest of the packet,\n> > which we never will.\n> \n> Mostly harmless since the operator could hit ^C, but still unpleasant.\n\nFetching is not always interactive. The deadlock I ran into (and again,\nI am not sure if this fixes it or not, but it is _a_ deadlock) was on a\nserver farm doing a large number of \"fetch && checkout && deploy\"\noperations. Only some of them hung, but it took a while to figure out\nwhat was going on.\n\n> [...]\n> > This fortunately doesn't happen in the normal fetching\n> > workflow, because git-fetch first uses the \"list\" command,\n> > which feeds the refs to get_remote_heads, which does notice\n> > the error. However, you can trigger it by sending a direct\n> > \"fetch\" to the remote-curl helper.\n> \n> Ah. Would a test for this make sense?\n\nA test would be great, if you can devise a way to reliably produce\ntruncated git output (but still valid http output). In the real-world\nproblem I had, I believe the truncation was caused by an intermediate\nreverse proxy that hit a timeout. I simulated truncation by using netcat\nto replay munged http headers and git output.\n\nI suspect the simplest portable thing would be a static file of\ntruncated git output, served by apache, which would need custom\nconfiguration to serve it with the correct content-type header. It\nseemed like a lot of test infrastructure to check for a very specific\nthing, so I abandoned trying to make a test.\n\n> > +\t\tif (verify_ref_advertisement(last->buf, last->len) < 0)\n> > +\t\t\tdie(\"ref advertisement is invalid at %s\", refs_url);\n> \n> Won't this error out with\n> \n> \tprotocol error: bad line length character: ERR\n> \n> instead of the current more helpful behavior for ERR lines?\n\nI don't think so. Don't ERR lines appear inside their own packets? We\nare just verifying that our packets are syntactically correct here, and\nmy reading of get_remote_heads is that the ERR appears inside the\npacketized data.\n\nThe one thing we do also check, though, is that we end with a flush\npacket. So depending on what servers produce, it may mean we trigger\nthis complaint instead of passing the ERR along to fetch-pack.\n\nRather than doing this fake syntactic verification, I wonder if we\nshould simply call get_remote_heads, which does a more thorough check\n(and is what we _would_ call in the list case, and what fetch-pack will\ncall once we pass data to it). It's slightly less efficient, in that it\nstarts a new thread and actually builds the linked list of refs. But it\nprobably isn't that big a deal (and normal operation does a \"list\" first\nwhich does that _anyway_).\n\n> Same stylistic comment about \"what would it mean for the return value\n> to be positive?\" as in patch 2/3.\n\nSame response. :)\n\n-Peff\n"},{"id":"209629","messageId":"20130218005443.GB3221@elie.Belkin","threadId":"32917","inReplyTo":"20130217191422.GA25096@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T00:54:43Z","receivedAt":"2013-02-18T00:54:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Sun, Feb 17, 2013 at 02:49:39AM -0800, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> --- a/remote-curl.c\n[...]\n>>> +\t\tif (read_packets_until_flush(&last->buf, &last->len) < 0)\n>> \n>> Style nit: this made me wonder \"What would it mean if\n>> read_packets_until_flush() > 0?\"\n[...]\n> My intent was that it followed the error convention of \"negative is\n> error, 0 is success, and positive is not used, but reserved for\n> future use\".\n\n>From a maintainability perspective, that kind of contract would be\ndangerous, since some *other* caller could arrive and use the function\nwithout a \"< 0\" without knowing it is doing anything wrong. When new\nreturn values appear, the function should be renamed to help the patch\nauthor and reviewers remember to check all callers.\n\nThat is, from the point of view of maintainability, there is no\ndistinction between \"if (read_packets_until_... < 0)\" and\n\"if (read_packets_until_...)\" and either form is fine.\n\nMy comment was just to say the \"< 0\" forced me to pause a moment and\ncheck out the implementation. This is basically a stylistic thing and\nif you prefer to keep the \"< 0\", that's fine with me.\n\n> If\n> an implementation is producing bogus packet lines and expecting us not\n> to complain, it really needs to be fixed.\n\nAgreed completely. Thanks again for the patch.\n\nJonathan\n"},{"id":"209630","messageId":"20130218014113.GC3221@elie.Belkin","threadId":"32917","inReplyTo":"20130217192830.GB25096@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] remote-curl: sanity check ref advertisement from server","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T01:41:13Z","receivedAt":"2013-02-18T01:41:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Sun, Feb 17, 2013 at 03:05:34AM -0800, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> +\t\tif (verify_ref_advertisement(last->buf, last->len) < 0)\n>>> +\t\t\tdie(\"ref advertisement is invalid at %s\", refs_url);\n>>\n>> Won't this error out with\n>>\n>> \tprotocol error: bad line length character: ERR\n>>\n>> instead of the current more helpful behavior for ERR lines?\n>\n> I don't think so. Don't ERR lines appear inside their own packets?\n\nYes, I misread get_remote_heads for some reason. Thanks for checking.\n\n[...]\n> The one thing we do also check, though, is that we end with a flush\n> packet. So depending on what servers produce, it may mean we trigger\n> this complaint instead of passing the ERR along to fetch-pack.\n>\n> Rather than doing this fake syntactic verification, I wonder if we\n> should simply call get_remote_heads, which does a more thorough check\n\nI'm not sure whether servers are expected to send a flush after an\nERR packet. The only codepath I know of in git itself that sends\nsuch packets is git-daemon, which does not flush after the error (but\nis not used in the stateless-rpc case). http-backend uses HTTP error\ncodes for its errors.\n\nIf I am reading get_remote_heads correctly, calling it with the\nfollowing tweak should work ok. The extra thread is just to feed a\nstring into a fd-based interface and could be avoided for \"list\", too,\nif it costs too much.\n\ndiff --git i/connect.c w/connect.c\nindex 49e56ba3..55ee7de7 100644\n--- i/connect.c\n+++ w/connect.c\n@@ -68,7 +68,8 @@ struct ref **get_remote_heads(int in, struct ref **list,\n {\n \tint got_at_least_one_head = 0;\n \n-\t*list = NULL;\n+\tif (list)\n+\t\t*list = NULL;\n \tfor (;;) {\n \t\tstruct ref *ref;\n \t\tunsigned char old_sha1[20];\n@@ -92,6 +93,9 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \t\t\tdie(\"protocol error: expected sha/ref, got '%s'\", buffer);\n \t\tname = buffer + 41;\n \n+\t\tif (!list)\n+\t\t\tcontinue;\n+\n \t\tname_len = strlen(name);\n \t\tif (len != name_len + 41) {\n \t\t\tfree(server_capabilities);\n"},{"id":"209648","messageId":"20130218085936.GA17003@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218005443.GB3221@elie.Belkin","subject":"Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T08:59:36Z","receivedAt":"2013-02-18T08:59:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 17, 2013 at 04:54:43PM -0800, Jonathan Nieder wrote:\n\n> > My intent was that it followed the error convention of \"negative is\n> > error, 0 is success, and positive is not used, but reserved for\n> > future use\".\n> \n> From a maintainability perspective, that kind of contract would be\n> dangerous, since some *other* caller could arrive and use the function\n> without a \"< 0\" without knowing it is doing anything wrong. When new\n> return values appear, the function should be renamed to help the patch\n> author and reviewers remember to check all callers.\n\nTrue. That's why I always write \"< 0\". :)\n\n> That is, from the point of view of maintainability, there is no\n> distinction between \"if (read_packets_until_... < 0)\" and\n> \"if (read_packets_until_...)\" and either form is fine.\n> \n> My comment was just to say the \"< 0\" forced me to pause a moment and\n> check out the implementation. This is basically a stylistic thing and\n> if you prefer to keep the \"< 0\", that's fine with me.\n\nInteresting. To me, \"foo() < 0\" just reads idiomatically as \"error-check\nthe foo call\".\n\nAnyway, I've redone the patch series to just re-use get_remote_heads,\nwhich is more robust. So this function has gone away in the new version.\n\n-Peff\n"},{"id":"209649","messageId":"20130218091203.GB17003@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218014113.GC3221@elie.Belkin","subject":"[PATCHv2 0/10] pkt-line and remote-curl cleanups server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:12:03Z","receivedAt":"2013-02-18T09:12:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 17, 2013 at 05:41:13PM -0800, Jonathan Nieder wrote:\n\n> > I don't think so. Don't ERR lines appear inside their own packets?\n> \n> Yes, I misread get_remote_heads for some reason. Thanks for checking.\n\nThanks for bringing it up. I had not even thought about ERR at all. So\nit was luck rather than skill that I was right. :)\n\n> I'm not sure whether servers are expected to send a flush after an\n> ERR packet. The only codepath I know of in git itself that sends\n> such packets is git-daemon, which does not flush after the error (but\n> is not used in the stateless-rpc case). http-backend uses HTTP error\n> codes for its errors.\n\nI just checked, and GitHub also does not send flush packets after ERR.\nWhich makes sense; ERR is supposed to end the conversation. I can change\nGitHub, of course, but who knows what other implementations exist (e.g.,\nI do not know off-hand whether gitolite has custom ERR responses). So it\nseems pretty clear that just checking for a flush packet is not the\nright thing, and we need to actually parse the packet contents (at least\nto some degree).\n\n> If I am reading get_remote_heads correctly, calling it with the\n> following tweak should work ok. The extra thread is just to feed a\n> string into a fd-based interface and could be avoided for \"list\", too,\n> if it costs too much.\n\nYeah, your patch does work, though we miss out on some of the refname\nchecks. I think what I'd rather do is just teach get_remote_heads to\nread from a buffer (to avoid the extra thread and pipe), and then just\nrun (and cache) the ref parsing unconditionally once we've read from the\nserver. It shouldn't make a difference in the normal case, as we would\nusually do a \"list\" anyway (and by caching, \"list\" can just feed out the\ncached copy).\n\nWhile looking into this, I noticed a bunch of other possible cleanups.\nPatches to follow:\n\n [01/10]: pkt-line: move a misplaced comment\n [02/10]: pkt-line: drop safe_write function\n [03/10]: pkt-line: clean up \"gentle\" reading function\n [04/10]: pkt-line: change error message for oversized packet\n [05/10]: pkt-line: rename s/packet_read_line/packet_read/\n\nThese are all just cleanups I noticed while looking at pkt-line. Any of\nthem can be dropped, though there would be some textual conflicts for\nthe later patches.\n\n [06/10]: pkt-line: share buffer/descriptor reading implementation\n [07/10]: teach get_remote_heads to read from a memory buffer\n [08/10]: remote-curl: pass buffer straight to get_remote_heads\n\nThese all build on each other to get rid of the extra thread/pipe, which\nI think is worth doing even without the rest of the series.\n\n [09/10]: remote-curl: move ref-parsing code up in file\n [10/10]: remote-curl: always parse incoming refs\n\nAnd these ones actually fix the problem I noticed.\n\n-Peff\n"},{"id":"209650","messageId":"20130218091430.GA5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"[PATCHv2 01/10] pkt-line: move a misplaced comment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:14:30Z","receivedAt":"2013-02-18T09:14:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The comment describing the packet writing interface was\noriginally written above packet_write, but migrated to be\nabove safe_write in f3a3214, probably because it is meant to\ngenerally describe the packet writing interface and not a\nsingle function. Let's move it into the header file, where\nusers of the interface are more likely to see it.\n\nSigned-off-by: Jeff King \n---\nI just left the comment intact as I moved it. It kind of implies to me\nthat you hand a big buffer to these functions and they would packetize\nit for you, which is not true. I don't know if anybody else sees that;\nit might be worth tweaking the text.\n\n pkt-line.c | 15 ---------------\n pkt-line.h | 14 +++++++++++++-\n 2 files changed, 13 insertions(+), 16 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex eaba15f..5138f47 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -46,21 +46,6 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n \tstrbuf_release(&out);\n }\n \n-/*\n- * Write a packetized stream, where each line is preceded by\n- * its length (including the header) as a 4-byte hex number.\n- * A length of 'zero' means end of stream (and a length of 1-3\n- * would be an error).\n- *\n- * This is all pretty stupid, but we use this packetized line\n- * format to make a streaming format possible without ever\n- * over-running the read buffers. That way we'll never read\n- * into what might be the pack data (which should go to another\n- * process entirely).\n- *\n- * The writing side could use stdio, but since the reading\n- * side can't, we stay with pure read/write interfaces.\n- */\n ssize_t safe_write(int fd, const void *buf, ssize_t n)\n {\n \tssize_t nn = n;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 8cfeb0c..7a67e9c 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -5,7 +5,19 @@\n #include \"strbuf.h\"\n \n /*\n- * Silly packetized line writing interface\n+ * Write a packetized stream, where each line is preceded by\n+ * its length (including the header) as a 4-byte hex number.\n+ * A length of 'zero' means end of stream (and a length of 1-3\n+ * would be an error).\n+ *\n+ * This is all pretty stupid, but we use this packetized line\n+ * format to make a streaming format possible without ever\n+ * over-running the read buffers. That way we'll never read\n+ * into what might be the pack data (which should go to another\n+ * process entirely).\n+ *\n+ * The writing side could use stdio, but since the reading\n+ * side can't, we stay with pure read/write interfaces.\n */\n void packet_flush(int fd);\n void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n-- \n1.8.1.20.g7078b03\n"},{"id":"209651","messageId":"20130218091519.GB5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"[PATCHv2 02/10] pkt-line: drop safe_write function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:15:19Z","receivedAt":"2013-02-18T09:15:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is just write_or_die by another name.\n\nSigned-off-by: Jeff King \n---\nActually, they are not quite the same. write_or_die will exit(0) when it\nsees EPIPE. Which makes me a little nervous.\n\n builtin/receive-pack.c | 2 +-\n builtin/send-pack.c | 2 +-\n fetch-pack.c | 2 +-\n http-backend.c | 8 ++++----\n pkt-line.c | 21 ++-------------------\n remote-curl.c | 4 ++--\n send-pack.c | 2 +-\n sideband.c | 9 +++++----\n upload-pack.c | 3 ++-\n 9 files changed, 19 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e8878de..48cd5dc 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -908,7 +908,7 @@ static void report(struct command *commands, const char *unpack_status)\n \tif (use_sideband)\n \t\tsend_sideband(1, 1, buf.buf, buf.len, use_sideband);\n \telse\n-\t\tsafe_write(1, buf.buf, buf.len);\n+\t\twrite_or_die(1, buf.buf, buf.len);\n \tstrbuf_release(&buf);\n }\n \ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 57a46b2..8778519 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -79,7 +79,7 @@ static void print_helper_status(struct ref *ref)\n \t\t}\n \t\tstrbuf_addch(&buf, '\\n');\n \n-\t\tsafe_write(1, buf.buf, buf.len);\n+\t\twrite_or_die(1, buf.buf, buf.len);\n \t}\n \tstrbuf_release(&buf);\n }\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 6d8926a..b90dadf 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -245,7 +245,7 @@ static void send_request(struct fetch_pack_args *args,\n \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n \t\tpacket_flush(fd);\n \t} else\n-\t\tsafe_write(fd, buf->buf, buf->len);\n+\t\twrite_or_die(fd, buf->buf, buf->len);\n }\n \n static void insert_one_alternate_ref(const struct ref *ref, void *unused)\ndiff --git a/http-backend.c b/http-backend.c\nindex f50e77f..8144f3a 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -70,7 +70,7 @@ static void format_write(int fd, const char *fmt, ...)\n \tif (n >= sizeof(buffer))\n \t\tdie(\"protocol error: impossibly long line\");\n \n-\tsafe_write(fd, buffer, n);\n+\twrite_or_die(fd, buffer, n);\n }\n \n static void http_status(unsigned code, const char *msg)\n@@ -111,7 +111,7 @@ static void end_headers(void)\n \n static void end_headers(void)\n {\n-\tsafe_write(1, \"\\r\\n\", 2);\n+\twrite_or_die(1, \"\\r\\n\", 2);\n }\n \n __attribute__((format (printf, 1, 2)))\n@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)\n \thdr_int(content_length, buf->len);\n \thdr_str(content_type, type);\n \tend_headers();\n-\tsafe_write(1, buf->buf, buf->len);\n+\twrite_or_die(1, buf->buf, buf->len);\n }\n \n static void send_local_file(const char *the_type, const char *name)\n@@ -185,7 +185,7 @@ static void send_local_file(const char *the_type, const char *name)\n \t\t\tdie_errno(\"Cannot read '%s'\", p);\n \t\tif (!n)\n \t\t\tbreak;\n-\t\tsafe_write(1, buf, n);\n+\t\twrite_or_die(1, buf, n);\n \t}\n \tclose(fd);\n \tfree(buf);\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 5138f47..699c2dd 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -46,23 +46,6 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n \tstrbuf_release(&out);\n }\n \n-ssize_t safe_write(int fd, const void *buf, ssize_t n)\n-{\n-\tssize_t nn = n;\n-\twhile (n) {\n-\t\tint ret = xwrite(fd, buf, n);\n-\t\tif (ret > 0) {\n-\t\t\tbuf = (char *) buf + ret;\n-\t\t\tn -= ret;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!ret)\n-\t\t\tdie(\"write error (disk full?)\");\n-\t\tdie_errno(\"write error\");\n-\t}\n-\treturn nn;\n-}\n-\n /*\n * If we buffered things up above (we don't, but we should),\n * we'd flush it here\n@@ -70,7 +53,7 @@ void packet_flush(int fd)\n void packet_flush(int fd)\n {\n \tpacket_trace(\"0000\", 4, 1);\n-\tsafe_write(fd, \"0000\", 4);\n+\twrite_or_die(fd, \"0000\", 4);\n }\n \n void packet_buf_flush(struct strbuf *buf)\n@@ -106,7 +89,7 @@ void packet_write(int fd, const char *fmt, ...)\n \tva_start(args, fmt);\n \tn = format_packet(fmt, args);\n \tva_end(args);\n-\tsafe_write(fd, buffer, n);\n+\twrite_or_die(fd, buffer, n);\n }\n \n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 933c69a..7be4b53 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -685,7 +685,7 @@ static int fetch_git(struct discovery *heads,\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n-\t\tsafe_write(1, rpc.result.buf, rpc.result.len);\n+\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n \tstrbuf_release(&preamble);\n \tfree(depth_arg);\n@@ -805,7 +805,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n-\t\tsafe_write(1, rpc.result.buf, rpc.result.len);\n+\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n \tfree(argv);\n \treturn err;\ndiff --git a/send-pack.c b/send-pack.c\nindex 97ab336..a99a1e4 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -281,7 +281,7 @@ int send_pack(struct send_pack_args *args,\n \t\t\tsend_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);\n \t\t}\n \t} else {\n-\t\tsafe_write(out, req_buf.buf, req_buf.len);\n+\t\twrite_or_die(out, req_buf.buf, req_buf.len);\n \t\tpacket_flush(out);\n \t}\n \tstrbuf_release(&req_buf);\ndiff --git a/sideband.c b/sideband.c\nindex d5ffa1c..8f7b25b 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -1,3 +1,4 @@\n+#include \"cache.h\"\n #include \"pkt-line.h\"\n #include \"sideband.h\"\n \n@@ -108,7 +109,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\t\t} while (len);\n \t\t\tcontinue;\n \t\tcase 1:\n-\t\t\tsafe_write(out, buf + pf+1, len);\n+\t\t\twrite_or_die(out, buf + pf+1, len);\n \t\t\tcontinue;\n \t\tdefault:\n \t\t\tfprintf(stderr, \"%s: protocol error: bad band #%d\\n\",\n@@ -138,12 +139,12 @@ ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet\n \t\tif (0 <= band) {\n \t\t\tsprintf(hdr, \"%04x\", n + 5);\n \t\t\thdr[4] = band;\n-\t\t\tsafe_write(fd, hdr, 5);\n+\t\t\twrite_or_die(fd, hdr, 5);\n \t\t} else {\n \t\t\tsprintf(hdr, \"%04x\", n + 4);\n-\t\t\tsafe_write(fd, hdr, 4);\n+\t\t\twrite_or_die(fd, hdr, 4);\n \t\t}\n-\t\tsafe_write(fd, p, n);\n+\t\twrite_or_die(fd, p, n);\n \t\tp += n;\n \t\tsz -= n;\n \t}\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7c05b15..04d6bd7 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -69,7 +69,8 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)\n \t\txwrite(fd, data, sz);\n \t\treturn sz;\n \t}\n-\treturn safe_write(fd, data, sz);\n+\twrite_or_die(fd, data, sz);\n+\treturn sz;\n }\n \n static FILE *pack_pipe = NULL;\n-- \n1.8.1.20.g7078b03\n"},{"id":"209652","messageId":"20130218091619.GC5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"[PATCHv2 03/10] pkt-line: clean up \"gentle\" reading function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:16:19Z","receivedAt":"2013-02-18T09:16:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Originally we had a single function for reading packetized\ndata: packet_read_line. Commit 46284dd grew a more \"gentle\"\nform that would return an error instead of dying upon\nreading a truncated input stream. However:\n\n 1. The two functions were called \"packet_read\" and\n \"packet_read_line\", with no indication that the only\n difference is in the error handling.\n\n 2. There was no documentation about which error conditions\n were handled in the gentle form, and which still caused\n a death.\n\n 3. The internal variable to trigger the gentle mode was\n called \"return_line_fail\", which was not very\n expressive.\n\nThis patch converts packet_line to packet_read_line_gently\nto more clearly indicate its relationship to\npacket_read_line, and renames the internal variable to\n\"gently\". This is also not incredibly expressive, but it is\nat least a convention within the git code. And finally, we\ndocument the exact behavior for the gentle and non-gentle\nmodes.\n\nWhile we are cleaning up the names, we can drop the\n\"return_line_fail\" checks in packet_read_internal entirely.\nThey look like this:\n\n ret = safe_read(..., return_line_fail);\n if (return_line_fail && ret < 0)\n\t ...\n\nThe check for return_line_fail is a no-op; safe_read will\nonly ever return an error value if we passed it\nreturn_line_fail in the first place.\n\nSigned-off-by: Jeff King \n---\nObviously this one is a matter of taste, but I think the result is much\nbetter. Certainly the documentation bits are hard to argue with. :)\n\n connect.c | 2 +-\n pkt-line.c | 16 ++++++++--------\n pkt-line.h | 21 ++++++++++++++++++++-\n 3 files changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 49e56ba..7e0920d 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -76,7 +76,7 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \t\tchar *name;\n \t\tint len, name_len;\n \n-\t\tlen = packet_read(in, buffer, sizeof(buffer));\n+\t\tlen = packet_read_line_gently(in, buffer, sizeof(buffer));\n \t\tif (len < 0)\n \t\t\tdie_initial_contact(got_at_least_one_head);\n \ndiff --git a/pkt-line.c b/pkt-line.c\nindex 699c2dd..62479d3 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -103,13 +103,13 @@ static int safe_read(int fd, void *buffer, unsigned size, int return_line_fail)\n \tstrbuf_add(buf, buffer, n);\n }\n \n-static int safe_read(int fd, void *buffer, unsigned size, int return_line_fail)\n+static int safe_read(int fd, void *buffer, unsigned size, int gently)\n {\n \tssize_t ret = read_in_full(fd, buffer, size);\n \tif (ret < 0)\n \t\tdie_errno(\"read error\");\n \telse if (ret < size) {\n-\t\tif (return_line_fail)\n+\t\tif (gently)\n \t\t\treturn -1;\n \n \t\tdie(\"The remote end hung up unexpectedly\");\n@@ -143,13 +143,13 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int return_\n \treturn len;\n }\n \n-static int packet_read_internal(int fd, char *buffer, unsigned size, int return_line_fail)\n+static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)\n {\n \tint len, ret;\n \tchar linelen[4];\n \n-\tret = safe_read(fd, linelen, 4, return_line_fail);\n-\tif (return_line_fail && ret < 0)\n+\tret = safe_read(fd, linelen, 4, gently);\n+\tif (ret < 0)\n \t\treturn ret;\n \tlen = packet_length(linelen);\n \tif (len < 0)\n@@ -161,15 +161,15 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int return_\n \tlen -= 4;\n \tif (len >= size)\n \t\tdie(\"protocol error: bad line length %d\", len);\n-\tret = safe_read(fd, buffer, len, return_line_fail);\n-\tif (return_line_fail && ret < 0)\n+\tret = safe_read(fd, buffer, len, gently);\n+\tif (ret < 0)\n \t\treturn ret;\n \tbuffer[len] = 0;\n \tpacket_trace(buffer, len, 0);\n \treturn len;\n }\n \n-int packet_read(int fd, char *buffer, unsigned size)\n+int packet_read_line_gently(int fd, char *buffer, unsigned size)\n {\n \treturn packet_read_internal(fd, buffer, size, 1);\n }\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 7a67e9c..31bd069 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -24,8 +24,27 @@ int packet_read_line(int fd, char *buffer, unsigned size);\n void packet_buf_flush(struct strbuf *buf);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n \n+/*\n+ * Read a packetized line from the descriptor into the buffer. We will die\n+ * under any of the following conditions:\n+ *\n+ * 1. Read error from descriptor.\n+ *\n+ * 2. Protocol error from the remote (e.g., bogus length characters).\n+ *\n+ * 3. Receiving a packet larger than \"size\" bytes.\n+ *\n+ * 4. Truncated output from the remote (e.g., we expected a packet but got\n+ * EOF, or we got a partial packet followed by EOF).\n+ */\n int packet_read_line(int fd, char *buffer, unsigned size);\n-int packet_read(int fd, char *buffer, unsigned size);\n+\n+/*\n+ * Same as packet_read_line, but do not die on condition 4 (truncated input);\n+ * instead return -1. We still die for the other conditions.\n+ */\n+int packet_read_line_gently(int fd, char *buffer, unsigned size);\n+\n int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);\n ssize_t safe_write(int, const void *, ssize_t);\n \n-- \n1.8.1.20.g7078b03\n"},{"id":"209653","messageId":"20130218092221.GD5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"[PATCHv2 04/10] pkt-line: change error message for oversized packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:22:21Z","receivedAt":"2013-02-18T09:22:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we get a packet from the remote side that is too large to\nfit in our buffer, we currently complain \"protocol error:\nbad line length\". This is a bit vague. The line length the\nother side sent is not \"bad\" per se; the actual problem is\nthat it exceeded our expectation for buffer length.\n\nThis will generally not happen between two git-core\nimplementations, because the sender limits themselves to\neither 1000, or to LARGE_PACKET_MAX (depending on what is\nbeing sent, sideband-64k negotiation, etc), and the receiver\nuses a buffer of the appropriate size.\n\nThe protocol document indicates the LARGE_PACKET_MAX limit\n(of 65520), but does not actually specify the 1000-byte\nlimit for ref lines. It is likely that other implementations\njust create a packet as large as they need, and this doesn't\ncome up often because nobody has 1000-character ref names\n(or close to it, counting sha1 and other boilerplate).\n\nWe may want to increase the size of our receive buffers for\nref lines to prepare for such writers (whether they are\nother implementations, or if we eventually want to bump the\nwrite size in git-core). In the meantime, this patch tries\nto give a more clear message in case it does come up.\n\nSigned-off-by: Jeff King \n---\nI'm really tempted to bump all of our 1000-byte buffers to just use\nLARGE_PACKET_MAX. If we provided a packet_read variant that used a\nstatic buffer (which is fine for all but one or two callers), then it\nwould not take much memory (right now we stick some LARGE_PACKET_MAX\nbuffers on the stack, which is slightly questionable for\nstack-restricted systems). But I left that for a different topic (and\neven if we do, we would still want this message to catch anything over\nthe bizarre 65520 limit).\n\nOut of curiosity, I grepped the list archives, and found only one\ninstance of this message. And it was somebody whose data stream was tainted\nwith random crud that happened to be numbers (much more common is \"bad line\nlength character\", when the crud does not look like a packet length).\n\n pkt-line.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 62479d3..f2a7575 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -160,7 +160,8 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)\n \t}\n \tlen -= 4;\n \tif (len >= size)\n-\t\tdie(\"protocol error: bad line length %d\", len);\n+\t\tdie(\"protocol error: line too large: (expected %u, got %d)\",\n+\t\t size, len);\n \tret = safe_read(fd, buffer, len, gently);\n \tif (ret < 0)\n \t\treturn ret;\n-- \n1.8.1.20.g7078b03\n"},{"id":"209655","messageId":"20130218092252.GE5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"[PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:22:52Z","receivedAt":"2013-02-18T09:22:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Originally packets were used just for the line-oriented ref\nadvertisement and negotiation. These days, we also stuff\npackfiles and sidebands into them, and they do not\nnecessarily represent a line. Drop the \"_line\" suffix, as it\nis not informative and makes the function names quite long\n(especially as we add \"_gently\" and other variants).\n\nSigned-off-by: Jeff King \n---\nAgain, this is a taste issue. Can be optional.\n\n builtin/archive.c | 4 ++--\n builtin/fetch-pack.c | 2 +-\n builtin/receive-pack.c | 2 +-\n builtin/upload-archive.c | 2 +-\n connect.c | 2 +-\n daemon.c | 2 +-\n fetch-pack.c | 6 +++---\n pkt-line.c | 4 ++--\n pkt-line.h | 6 +++---\n remote-curl.c | 6 +++---\n send-pack.c | 4 ++--\n sideband.c | 2 +-\n upload-pack.c | 4 ++--\n 13 files changed, 23 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 9a1cfd3..bf600f5 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -53,7 +53,7 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\tpacket_write(fd[1], \"argument %s\\n\", argv[i]);\n \tpacket_flush(fd[1]);\n \n-\tlen = packet_read_line(fd[0], buf, sizeof(buf));\n+\tlen = packet_read(fd[0], buf, sizeof(buf));\n \tif (!len)\n \t\tdie(_(\"git archive: expected ACK/NAK, got EOF\"));\n \tif (buf[len-1] == '\\n')\n@@ -66,7 +66,7 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\tdie(_(\"git archive: protocol error\"));\n \t}\n \n-\tlen = packet_read_line(fd[0], buf, sizeof(buf));\n+\tlen = packet_read(fd[0], buf, sizeof(buf));\n \tif (len)\n \t\tdie(_(\"git archive: expected a flush\"));\n \ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 940ae35..b59e60e 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -102,7 +102,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t */\n \t\t\tstatic char line[1000];\n \t\t\tfor (;;) {\n-\t\t\t\tint n = packet_read_line(0, line, sizeof(line));\n+\t\t\t\tint n = packet_read(0, line, sizeof(line));\n \t\t\t\tif (!n)\n \t\t\t\t\tbreak;\n \t\t\t\tif (line[n-1] == '\\n')\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 48cd5dc..3f58ce8 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -736,7 +736,7 @@ static struct command *read_head_info(void)\n \t\tchar *refname;\n \t\tint len, reflen;\n \n-\t\tlen = packet_read_line(0, line, sizeof(line));\n+\t\tlen = packet_read(0, line, sizeof(line));\n \t\tif (!len)\n \t\t\tbreak;\n \t\tif (line[len-1] == '\\n')\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex b928beb..2ecf461 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -40,7 +40,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \tsent_argv[0] = \"git-upload-archive\";\n \tfor (p = buf;;) {\n \t\t/* This will die if not enough free space in buf */\n-\t\tlen = packet_read_line(0, p, (buf + sizeof buf) - p);\n+\t\tlen = packet_read(0, p, (buf + sizeof buf) - p);\n \t\tif (len == 0)\n \t\t\tbreak;\t/* got a flush */\n \t\tif (sent_argc > MAX_ARGS - 2)\ndiff --git a/connect.c b/connect.c\nindex 7e0920d..59266b1 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -76,7 +76,7 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \t\tchar *name;\n \t\tint len, name_len;\n \n-\t\tlen = packet_read_line_gently(in, buffer, sizeof(buffer));\n+\t\tlen = packet_read_gently(in, buffer, sizeof(buffer));\n \t\tif (len < 0)\n \t\t\tdie_initial_contact(got_at_least_one_head);\n \ndiff --git a/daemon.c b/daemon.c\nindex 4602b46..ac26b93 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -612,7 +612,7 @@ static int execute(void)\n \t\tloginfo(\"Connection from %s:%s\", addr, port);\n \n \talarm(init_timeout ? init_timeout : timeout);\n-\tpktlen = packet_read_line(0, line, sizeof(line));\n+\tpktlen = packet_read(0, line, sizeof(line));\n \talarm(0);\n \n \tlen = strlen(line);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b90dadf..d89c9d0 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -173,7 +173,7 @@ static void consume_shallow_list(struct fetch_pack_args *args, int fd)\n \t\t * is a block of have lines exchanged.\n \t\t */\n \t\tchar line[1000];\n-\t\twhile (packet_read_line(fd, line, sizeof(line))) {\n+\t\twhile (packet_read(fd, line, sizeof(line))) {\n \t\t\tif (!prefixcmp(line, \"shallow \"))\n \t\t\t\tcontinue;\n \t\t\tif (!prefixcmp(line, \"unshallow \"))\n@@ -216,7 +216,7 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n {\n \tstatic char line[1000];\n-\tint len = packet_read_line(fd, line, sizeof(line));\n+\tint len = packet_read(fd, line, sizeof(line));\n \n \tif (!len)\n \t\tdie(\"git fetch-pack: expected ACK/NAK, got EOF\");\n@@ -350,7 +350,7 @@ static int find_common(struct fetch_pack_args *args,\n \t\tunsigned char sha1[20];\n \n \t\tsend_request(args, fd[1], &req_buf);\n-\t\twhile (packet_read_line(fd[0], line, sizeof(line))) {\n+\t\twhile (packet_read(fd[0], line, sizeof(line))) {\n \t\t\tif (!prefixcmp(line, \"shallow \")) {\n \t\t\t\tif (get_sha1_hex(line + 8, sha1))\n \t\t\t\t\tdie(\"invalid shallow line: %s\", line);\ndiff --git a/pkt-line.c b/pkt-line.c\nindex f2a7575..85faf73 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -170,12 +170,12 @@ int packet_read_line_gently(int fd, char *buffer, unsigned size)\n \treturn len;\n }\n \n-int packet_read_line_gently(int fd, char *buffer, unsigned size)\n+int packet_read_gently(int fd, char *buffer, unsigned size)\n {\n \treturn packet_read_internal(fd, buffer, size, 1);\n }\n \n-int packet_read_line(int fd, char *buffer, unsigned size)\n+int packet_read(int fd, char *buffer, unsigned size)\n {\n \treturn packet_read_internal(fd, buffer, size, 0);\n }\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 31bd069..2dc4941 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -37,13 +37,13 @@ int packet_read_line(int fd, char *buffer, unsigned size);\n * 4. Truncated output from the remote (e.g., we expected a packet but got\n * EOF, or we got a partial packet followed by EOF).\n */\n-int packet_read_line(int fd, char *buffer, unsigned size);\n+int packet_read(int fd, char *buffer, unsigned size);\n \n /*\n- * Same as packet_read_line, but do not die on condition 4 (truncated input);\n+ * Same as packet_read, but do not die on condition 4 (truncated input);\n * instead return -1. We still die for the other conditions.\n */\n-int packet_read_line_gently(int fd, char *buffer, unsigned size);\n+int packet_read_gently(int fd, char *buffer, unsigned size);\n \n int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);\n ssize_t safe_write(int, const void *, ssize_t);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 7be4b53..99cc016 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -308,7 +308,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \n \tif (!avail) {\n \t\trpc->initial_buffer = 0;\n-\t\tavail = packet_read_line(rpc->out, rpc->buf, rpc->alloc);\n+\t\tavail = packet_read(rpc->out, rpc->buf, rpc->alloc);\n \t\tif (!avail)\n \t\t\treturn 0;\n \t\trpc->pos = 0;\n@@ -425,7 +425,7 @@ static int post_rpc(struct rpc_state *rpc)\n \t\t\tbreak;\n \t\t}\n \n-\t\tn = packet_read_line(rpc->out, buf, left);\n+\t\tn = packet_read(rpc->out, buf, left);\n \t\tif (!n)\n \t\t\tbreak;\n \t\trpc->len += n;\n@@ -579,7 +579,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \trpc->hdr_accept = strbuf_detach(&buf, NULL);\n \n \twhile (!err) {\n-\t\tint n = packet_read_line(rpc->out, rpc->buf, rpc->alloc);\n+\t\tint n = packet_read(rpc->out, rpc->buf, rpc->alloc);\n \t\tif (!n)\n \t\t\tbreak;\n \t\trpc->pos = 0;\ndiff --git a/send-pack.c b/send-pack.c\nindex a99a1e4..7b22235 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -108,7 +108,7 @@ static int receive_status(int in, struct ref *refs)\n \tstruct ref *hint;\n \tchar line[1000];\n \tint ret = 0;\n-\tint len = packet_read_line(in, line, sizeof(line));\n+\tint len = packet_read(in, line, sizeof(line));\n \tif (len < 10 || memcmp(line, \"unpack \", 7))\n \t\treturn error(\"did not receive remote status\");\n \tif (memcmp(line, \"unpack ok\\n\", 10)) {\n@@ -122,7 +122,7 @@ static int receive_status(int in, struct ref *refs)\n \twhile (1) {\n \t\tchar *refname;\n \t\tchar *msg;\n-\t\tlen = packet_read_line(in, line, sizeof(line));\n+\t\tlen = packet_read(in, line, sizeof(line));\n \t\tif (!len)\n \t\t\tbreak;\n \t\tif (len < 3 ||\ndiff --git a/sideband.c b/sideband.c\nindex 8f7b25b..4a6446d 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -38,7 +38,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \n \twhile (1) {\n \t\tint band, len;\n-\t\tlen = packet_read_line(in_stream, buf + pf, LARGE_PACKET_MAX);\n+\t\tlen = packet_read(in_stream, buf + pf, LARGE_PACKET_MAX);\n \t\tif (len == 0)\n \t\t\tbreak;\n \t\tif (len < 1) {\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 04d6bd7..04231d7 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -428,7 +428,7 @@ static int get_common_commits(void)\n \tsave_commit_buffer = 0;\n \n \tfor (;;) {\n-\t\tint len = packet_read_line(0, line, sizeof(line));\n+\t\tint len = packet_read(0, line, sizeof(line));\n \t\treset_timeout();\n \n \t\tif (!len) {\n@@ -589,7 +589,7 @@ static void receive_needs(void)\n \t\tstruct object *o;\n \t\tconst char *features;\n \t\tunsigned char sha1_buf[20];\n-\t\tlen = packet_read_line(0, line, sizeof(line));\n+\t\tlen = packet_read(0, line, sizeof(line));\n \t\treset_timeout();\n \t\tif (!len)\n \t\t\tbreak;\n-- \n1.8.1.20.g7078b03\n"},{"id":"209656","messageId":"20130218092612.GF5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"[PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:26:12Z","receivedAt":"2013-02-18T09:26:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The packet_read function reads from a descriptor. The\npacket_get_line function is similar, but reads from an\nin-memory buffer, and uses a completely separate\nimplementation. This patch teaches the internal function\nbacking packet_read to accept either source, and use the\nappropriate one. As a result:\n\n 1. The function has been renamed to packet_read_from_buf,\n which more clearly indicates its relationship to the\n packet_read function.\n\n 2. The original packet_get_line wrote to a strbuf; the new\n function, like its descriptor counterpart, reads into a\n buffer provided by the caller.\n\n 3. The original function did not die on any errors, but\n instead returned an error code. Now we have the usual\n \"normal\" and \"gently\" forms.\n\nThere are only two existing calls to packet_get_line which\nhave to be converted, and both are in remote-curl. The first\nreads and checks the \"# service=git-foo\" line from a smart\nhttp server. The second just reads past any additional\nsmart headers, without bothering to look at them.\n\nThis patch converts both to the new form, with a few\nimplications:\n\n 1. Because we use the non-gentle form, the first caller\n can drop its own error checking. As a result, we will get\n more accurate error reporting about protocol breakage,\n since the errors come from inside the protocol code. We\n will no longer print the URL as part of the error, but\n that's OK. Protocol breakages should be rare (and we\n are pretty sure at this point in the code that it is a\n real smart server, so we won't be confused by dumb\n servers), and the first debugging step would probably\n be GIT_CURL_VERBOSE, anyway.\n\n 2. The second caller did not error check at all, and now\n does. This can help us catch broken or truncated input\n close to the source.\n\n 3. Since we are no longer using a strbuf, we now have a\n 1000-byte limit on the smart-http headers. That should\n be fine, as the only header that has ever been sent\n here is the short \"service=git-foo\" header.\n\nSigned-off-by: Jeff King \n---\nThe diffstat shows more lines appearing, but it is mainly from comments\nand from the various parse_line{_from_buf,}{,_gently} variants; we\nreally do get rid of a duplicate parsing implementation, and we\nharmonize all of the error conditions and messages.\n\nWe can also make \"gently\" a parameter to avoid the proliferation of\nrelated functions, but would mean all but one callsite would have to\npass an extra \"0\". Choose your poison, I guess.\n\n pkt-line.c | 69 ++++++++++++++++++++++++++++-------------------------------\n pkt-line.h | 11 +++++++++-\n remote-curl.c | 19 ++++++++--------\n 3 files changed, 53 insertions(+), 46 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 85faf73..7ee91e0 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -103,12 +103,26 @@ static int safe_read(int fd, void *buffer, unsigned size, int gently)\n \tstrbuf_add(buf, buffer, n);\n }\n \n-static int safe_read(int fd, void *buffer, unsigned size, int gently)\n+static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n+\t\t\t void *dst, unsigned size, int gently)\n {\n-\tssize_t ret = read_in_full(fd, buffer, size);\n-\tif (ret < 0)\n-\t\tdie_errno(\"read error\");\n-\telse if (ret < size) {\n+\tssize_t ret;\n+\n+\t/* Read up to \"size\" bytes from our source, whatever it is. */\n+\tif (src_buf) {\n+\t\tret = size < *src_size ? size : *src_size;\n+\t\tmemcpy(dst, *src_buf, ret);\n+\t\t*src_buf += size;\n+\t\t*src_size -= size;\n+\t}\n+\telse {\n+\t\tret = read_in_full(fd, dst, size);\n+\t\tif (ret < 0)\n+\t\t\tdie_errno(\"read error\");\n+\t}\n+\n+\t/* And complain if we didn't get enough bytes to satisfy the read. */\n+\tif (ret < size) {\n \t\tif (gently)\n \t\t\treturn -1;\n \n@@ -143,12 +157,13 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)\n \treturn len;\n }\n \n-static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)\n+static int packet_read_internal(int fd, char **src_buf, size_t *src_len,\n+\t\t\t\tchar *buffer, unsigned size, int gently)\n {\n \tint len, ret;\n \tchar linelen[4];\n \n-\tret = safe_read(fd, linelen, 4, gently);\n+\tret = get_packet_data(fd, src_buf, src_len, linelen, 4, gently);\n \tif (ret < 0)\n \t\treturn ret;\n \tlen = packet_length(linelen);\n@@ -162,7 +177,7 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)\n \tif (len >= size)\n \t\tdie(\"protocol error: line too large: (expected %u, got %d)\",\n \t\t size, len);\n-\tret = safe_read(fd, buffer, len, gently);\n+\tret = get_packet_data(fd, src_buf, src_len, buffer, len, gently);\n \tif (ret < 0)\n \t\treturn ret;\n \tbuffer[len] = 0;\n@@ -172,40 +187,22 @@ int packet_get_line(struct strbuf *out,\n \n int packet_read_gently(int fd, char *buffer, unsigned size)\n {\n-\treturn packet_read_internal(fd, buffer, size, 1);\n+\treturn packet_read_internal(fd, NULL, 0, buffer, size, 1);\n }\n \n int packet_read(int fd, char *buffer, unsigned size)\n {\n-\treturn packet_read_internal(fd, buffer, size, 0);\n+\treturn packet_read_internal(fd, NULL, 0, buffer, size, 0);\n }\n \n-int packet_get_line(struct strbuf *out,\n-\tchar **src_buf, size_t *src_len)\n+int packet_read_from_buf(char *dst, unsigned dst_len,\n+\t\t\t char **src_buf, size_t *src_len)\n {\n-\tint len;\n-\n-\tif (*src_len < 4)\n-\t\treturn -1;\n-\tlen = packet_length(*src_buf);\n-\tif (len < 0)\n-\t\treturn -1;\n-\tif (!len) {\n-\t\t*src_buf += 4;\n-\t\t*src_len -= 4;\n-\t\tpacket_trace(\"0000\", 4, 0);\n-\t\treturn 0;\n-\t}\n-\tif (*src_len < len)\n-\t\treturn -2;\n-\n-\t*src_buf += 4;\n-\t*src_len -= 4;\n-\tlen -= 4;\n+\treturn packet_read_internal(-1, src_buf, src_len, dst, dst_len, 0);\n+}\n \n-\tstrbuf_add(out, *src_buf, len);\n-\t*src_buf += len;\n-\t*src_len -= len;\n-\tpacket_trace(out->buf, out->len, 0);\n-\treturn len;\n+int packet_read_from_buf_gently(char *dst, unsigned dst_len,\n+\t\t\t\tchar **src_buf, size_t *src_len)\n+{\n+\treturn packet_read_internal(-1, src_buf, src_len, dst, dst_len, 1);\n }\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 2dc4941..287a391 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -45,7 +45,16 @@ int packet_read_gently(int fd, char *buffer, unsigned size);\n */\n int packet_read_gently(int fd, char *buffer, unsigned size);\n \n-int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);\n+/*\n+ * Same as packet_read above, but read from an in-memory buffer\n+ * instead of a file descriptor. The src_buf and src_len are modified\n+ * to iterate past the consumed data.\n+ */\n+int packet_read_from_buf(char *dst, unsigned dst_len,\n+\t\t\t char **src_buf, size_t *src_len);\n+int packet_read_from_buf_gently(char *dst, unsigned dst_len,\n+\t\t\t\tchar **src_buf, size_t *src_len);\n+\n ssize_t safe_write(int, const void *, ssize_t);\n \n #endif\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 99cc016..2ec5854 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -138,28 +138,29 @@ static struct discovery* discover_refs(const char *service)\n \tif (maybe_smart &&\n \t (5 <= last->len && last->buf[4] == '#') &&\n \t !strbuf_cmp(&exp, &type)) {\n+\t\tchar line[1000];\n+\t\tint len;\n+\n \t\t/*\n \t\t * smart HTTP response; validate that the service\n \t\t * pkt-line matches our request.\n \t\t */\n-\t\tif (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n-\t\t\tdie(\"%s has invalid packet header\", refs_url);\n-\t\tif (buffer.len && buffer.buf[buffer.len - 1] == '\\n')\n-\t\t\tstrbuf_setlen(&buffer, buffer.len - 1);\n+\t\tlen = packet_read_from_buf(line, sizeof(line), &last->buf, &last->len);\n+\t\tif (len && line[len - 1] == '\\n')\n+\t\t\tlen--;\n \n \t\tstrbuf_reset(&exp);\n \t\tstrbuf_addf(&exp, \"# service=%s\", service);\n-\t\tif (strbuf_cmp(&exp, &buffer))\n-\t\t\tdie(\"invalid server response; got '%s'\", buffer.buf);\n+\t\tif (len != exp.len || memcmp(exp.buf, line, len))\n+\t\t\tdie(\"invalid server response; got '%s'\", line);\n \t\tstrbuf_release(&exp);\n \n \t\t/* The header can include additional metadata lines, up\n \t\t * until a packet flush marker. Ignore these now, but\n \t\t * in the future we might start to scan them.\n \t\t */\n-\t\tstrbuf_reset(&buffer);\n-\t\twhile (packet_get_line(&buffer, &last->buf, &last->len) > 0)\n-\t\t\tstrbuf_reset(&buffer);\n+\t\twhile (packet_read_from_buf(line, sizeof(line), &last->buf, &last->len) > 0)\n+\t\t\t;\n \n \t\tlast->proto_git = 1;\n \t}\n-- \n1.8.1.20.g7078b03\n"},{"id":"209657","messageId":"20130218092759.GG5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"[PATCHv2 07/10] teach get_remote_heads to read from a memory buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:27:59Z","receivedAt":"2013-02-18T09:27:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that we can read packet data from memory as easily as a\ndescriptor, get_remote_heads can take either one as a\nsource. This will allow further refactoring in remote-curl.\n\nSigned-off-by: Jeff King \n---\nThere aren't that many callers of get_remote_heads, so I just added the\noptional parameters and tweaked each callsite. We can do it as a\nget_remote_heads_from_buf wrapper function and leave them be, if we\nwant.\n\n builtin/fetch-pack.c | 2 +-\n builtin/send-pack.c | 2 +-\n cache.h | 4 +++-\n connect.c | 10 +++++++---\n remote-curl.c | 2 +-\n transport.c | 6 +++---\n 6 files changed, 16 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex b59e60e..bf850d0 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -128,7 +128,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\t args.verbose ? CONNECT_VERBOSE : 0);\n \t}\n \n-\tget_remote_heads(fd[0], &ref, 0, NULL);\n+\tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL);\n \n \tref = fetch_pack(&args, fd, conn, ref, dest,\n \t\t\t &sought, pack_lockfile_ptr);\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 8778519..152c4ea 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -207,7 +207,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \n \tmemset(&extra_have, 0, sizeof(extra_have));\n \n-\tget_remote_heads(fd[0], &remote_refs, REF_NORMAL, &extra_have);\n+\tget_remote_heads(fd[0], NULL, 0, &remote_refs, REF_NORMAL, &extra_have);\n \n \ttransport_verify_remote_names(nr_refspecs, refspecs);\n \ndiff --git a/cache.h b/cache.h\nindex e493563..db646a2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1049,7 +1049,9 @@ struct extra_have_objects {\n \tint nr, alloc;\n \tunsigned char (*array)[20];\n };\n-extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);\n+extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n+\t\t\t\t struct ref **list, unsigned int flags,\n+\t\t\t\t struct extra_have_objects *);\n extern int server_supports(const char *feature);\n extern int parse_feature_request(const char *features, const char *feature);\n extern const char *server_feature_value(const char *feature, int *len_ret);\ndiff --git a/connect.c b/connect.c\nindex 59266b1..28f21d0 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -62,8 +62,8 @@ static void die_initial_contact(int got_at_least_one_head)\n /*\n * Read all the refs from the other end\n */\n-struct ref **get_remote_heads(int in, struct ref **list,\n-\t\t\t unsigned int flags,\n+struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n+\t\t\t struct ref **list, unsigned int flags,\n \t\t\t struct extra_have_objects *extra_have)\n {\n \tint got_at_least_one_head = 0;\n@@ -76,7 +76,11 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \t\tchar *name;\n \t\tint len, name_len;\n \n-\t\tlen = packet_read_gently(in, buffer, sizeof(buffer));\n+\t\tif (src_buf)\n+\t\t\tlen = packet_read_from_buf_gently(buffer, sizeof(buffer),\n+\t\t\t\t\t\t\t &src_buf, &src_len);\n+\t\telse\n+\t\t\tlen = packet_read_gently(in, buffer, sizeof(buffer));\n \t\tif (len < 0)\n \t\t\tdie_initial_contact(got_at_least_one_head);\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex 2ec5854..6e43463 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -195,7 +195,7 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n \n \tif (start_async(&async))\n \t\tdie(\"cannot start thread to parse advertised refs\");\n-\tget_remote_heads(async.out, &list,\n+\tget_remote_heads(async.out, NULL, 0, &list,\n \t\t\tfor_push ? REF_NORMAL : 0, NULL);\n \tclose(async.out);\n \tif (finish_async(&async))\ndiff --git a/transport.c b/transport.c\nindex 886ffd8..62df466 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -507,7 +507,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus\n \tstruct ref *refs;\n \n \tconnect_setup(transport, for_push, 0);\n-\tget_remote_heads(data->fd[0], &refs,\n+\tget_remote_heads(data->fd[0], NULL, 0, &refs,\n \t\t\t for_push ? REF_NORMAL : 0, &data->extra_have);\n \tdata->got_remote_heads = 1;\n \n@@ -541,7 +541,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \n \tif (!data->got_remote_heads) {\n \t\tconnect_setup(transport, 0, 0);\n-\t\tget_remote_heads(data->fd[0], &refs_tmp, 0, NULL);\n+\t\tget_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0, NULL);\n \t\tdata->got_remote_heads = 1;\n \t}\n \n@@ -799,7 +799,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \t\tstruct ref *tmp_refs;\n \t\tconnect_setup(transport, 1, 0);\n \n-\t\tget_remote_heads(data->fd[0], &tmp_refs, REF_NORMAL, NULL);\n+\t\tget_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL, NULL);\n \t\tdata->got_remote_heads = 1;\n \t}\n \n-- \n1.8.1.20.g7078b03\n"},{"id":"209658","messageId":"20130218092905.GH5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"[PATCHv2 08/10] remote-curl: pass buffer straight to get_remote_heads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:29:05Z","receivedAt":"2013-02-18T09:29:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Until recently, get_remote_heads only knew how to read refs\nfrom a file descriptor. To hack around this, we spawned a\nthread (or forked a process) to write the buffer back to us.\n\nNow that we can just pass it our buffer directly, we don't\nhave to use this hack anymore.\n\nSigned-off-by: Jeff King \n---\nI don't know that this code was hurting anything, but it has always\nstruck me as ugly and a possible source of error. And now it's gone.\n\n remote-curl.c | 26 ++------------------------\n 1 file changed, 2 insertions(+), 24 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 6e43463..f049da2 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -173,33 +173,11 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n \treturn last;\n }\n \n-static int write_discovery(int in, int out, void *data)\n-{\n-\tstruct discovery *heads = data;\n-\tint err = 0;\n-\tif (write_in_full(out, heads->buf, heads->len) != heads->len)\n-\t\terr = 1;\n-\tclose(out);\n-\treturn err;\n-}\n-\n static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n {\n \tstruct ref *list = NULL;\n-\tstruct async async;\n-\n-\tmemset(&async, 0, sizeof(async));\n-\tasync.proc = write_discovery;\n-\tasync.data = heads;\n-\tasync.out = -1;\n-\n-\tif (start_async(&async))\n-\t\tdie(\"cannot start thread to parse advertised refs\");\n-\tget_remote_heads(async.out, NULL, 0, &list,\n-\t\t\tfor_push ? REF_NORMAL : 0, NULL);\n-\tclose(async.out);\n-\tif (finish_async(&async))\n-\t\tdie(\"ref parsing thread failed\");\n+\tget_remote_heads(-1, heads->buf, heads->len, &list,\n+\t\t\t for_push ? REF_NORMAL : 0, NULL);\n \treturn list;\n }\n \n-- \n1.8.1.20.g7078b03\n"},{"id":"209659","messageId":"7vhalaas2b.fsf@alter.siamese.dyndns.org","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/10] pkt-line and remote-curl cleanups server","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-18T09:29:16Z","receivedAt":"2013-02-18T09:29:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Feb 17, 2013 at 05:41:13PM -0800, Jonathan Nieder wrote:\n>\n>> > I don't think so. Don't ERR lines appear inside their own packets?\n>> \n>> Yes, I misread get_remote_heads for some reason. Thanks for checking.\n>\n> Thanks for bringing it up. I had not even thought about ERR at all. So\n> it was luck rather than skill that I was right. :)\n>\n>> I'm not sure whether servers are expected to send a flush after an\n>> ERR packet. The only codepath I know of in git itself that sends\n>> such packets is git-daemon, which does not flush after the error (but\n>> is not used in the stateless-rpc case). http-backend uses HTTP error\n>> codes for its errors.\n>\n> I just checked, and GitHub also does not send flush packets after ERR.\n> Which makes sense; ERR is supposed to end the conversation.\n\nHmph. A flush packet was supposed to be a mark to say \"all the\npackets before this one can be buffered and kept without getting\npassed to write(2), but this and all such buffered data _must_ go on\nthe wire _now_\". So in the sense, ERR not followed by a flush may\nnot even have a chance to be seen on the other end, no? That is\nwhat the comment before the implementation of packet_flush() is all\nabout.\n"},{"id":"209660","messageId":"20130218092936.GI5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"[PATCHv2 09/10] remote-curl: move ref-parsing code up in file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:29:36Z","receivedAt":"2013-02-18T09:29:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ref-parsing functions are static. Let's move them up in\nthe file to be available to more functions, which will help\nus with later refactoring.\n\nSigned-off-by: Jeff King \n---\nJust a cleanup for the next patch. We could also just do extra\ndeclarations at the top.\n\n remote-curl.c | 117 +++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 59 insertions(+), 58 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex f049da2..62f82d1 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -80,6 +80,65 @@ static struct discovery *last_discovery;\n };\n static struct discovery *last_discovery;\n \n+static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n+{\n+\tstruct ref *list = NULL;\n+\tget_remote_heads(-1, heads->buf, heads->len, &list,\n+\t\t\t for_push ? REF_NORMAL : 0, NULL);\n+\treturn list;\n+}\n+\n+static struct ref *parse_info_refs(struct discovery *heads)\n+{\n+\tchar *data, *start, *mid;\n+\tchar *ref_name;\n+\tint i = 0;\n+\n+\tstruct ref *refs = NULL;\n+\tstruct ref *ref = NULL;\n+\tstruct ref *last_ref = NULL;\n+\n+\tdata = heads->buf;\n+\tstart = NULL;\n+\tmid = data;\n+\twhile (i < heads->len) {\n+\t\tif (!start) {\n+\t\t\tstart = &data[i];\n+\t\t}\n+\t\tif (data[i] == '\\t')\n+\t\t\tmid = &data[i];\n+\t\tif (data[i] == '\\n') {\n+\t\t\tif (mid - start != 40)\n+\t\t\t\tdie(\"%sinfo/refs not valid: is this a git repository?\", url);\n+\t\t\tdata[i] = 0;\n+\t\t\tref_name = mid + 1;\n+\t\t\tref = xmalloc(sizeof(struct ref) +\n+\t\t\t\t strlen(ref_name) + 1);\n+\t\t\tmemset(ref, 0, sizeof(struct ref));\n+\t\t\tstrcpy(ref->name, ref_name);\n+\t\t\tget_sha1_hex(start, ref->old_sha1);\n+\t\t\tif (!refs)\n+\t\t\t\trefs = ref;\n+\t\t\tif (last_ref)\n+\t\t\t\tlast_ref->next = ref;\n+\t\t\tlast_ref = ref;\n+\t\t\tstart = NULL;\n+\t\t}\n+\t\ti++;\n+\t}\n+\n+\tref = alloc_ref(\"HEAD\");\n+\tif (!http_fetch_ref(url, ref) &&\n+\t !resolve_remote_symref(ref, refs)) {\n+\t\tref->next = refs;\n+\t\trefs = ref;\n+\t} else {\n+\t\tfree(ref);\n+\t}\n+\n+\treturn refs;\n+}\n+\n static void free_discovery(struct discovery *d)\n {\n \tif (d) {\n@@ -173,64 +232,6 @@ static struct discovery* discover_refs(const char *service)\n \treturn last;\n }\n \n-static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n-{\n-\tstruct ref *list = NULL;\n-\tget_remote_heads(-1, heads->buf, heads->len, &list,\n-\t\t\t for_push ? REF_NORMAL : 0, NULL);\n-\treturn list;\n-}\n-\n-static struct ref *parse_info_refs(struct discovery *heads)\n-{\n-\tchar *data, *start, *mid;\n-\tchar *ref_name;\n-\tint i = 0;\n-\n-\tstruct ref *refs = NULL;\n-\tstruct ref *ref = NULL;\n-\tstruct ref *last_ref = NULL;\n-\n-\tdata = heads->buf;\n-\tstart = NULL;\n-\tmid = data;\n-\twhile (i < heads->len) {\n-\t\tif (!start) {\n-\t\t\tstart = &data[i];\n-\t\t}\n-\t\tif (data[i] == '\\t')\n-\t\t\tmid = &data[i];\n-\t\tif (data[i] == '\\n') {\n-\t\t\tif (mid - start != 40)\n-\t\t\t\tdie(\"%sinfo/refs not valid: is this a git repository?\", url);\n-\t\t\tdata[i] = 0;\n-\t\t\tref_name = mid + 1;\n-\t\t\tref = xmalloc(sizeof(struct ref) +\n-\t\t\t\t strlen(ref_name) + 1);\n-\t\t\tmemset(ref, 0, sizeof(struct ref));\n-\t\t\tstrcpy(ref->name, ref_name);\n-\t\t\tget_sha1_hex(start, ref->old_sha1);\n-\t\t\tif (!refs)\n-\t\t\t\trefs = ref;\n-\t\t\tif (last_ref)\n-\t\t\t\tlast_ref->next = ref;\n-\t\t\tlast_ref = ref;\n-\t\t\tstart = NULL;\n-\t\t}\n-\t\ti++;\n-\t}\n-\n-\tref = alloc_ref(\"HEAD\");\n-\tif (!http_fetch_ref(url, ref) &&\n-\t !resolve_remote_symref(ref, refs)) {\n-\t\tref->next = refs;\n-\t\trefs = ref;\n-\t} else {\n-\t\tfree(ref);\n-\t}\n-\n-\treturn refs;\n-}\n \n static struct ref *get_refs(int for_push)\n {\n-- \n1.8.1.20.g7078b03\n"},{"id":"209661","messageId":"20130218093056.GJ5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"[PATCHv2 10/10] remote-curl: always parse incoming refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:30:56Z","receivedAt":"2013-02-18T09:30:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When remote-curl receives a list of refs from a server, it\nkeeps the whole buffer intact. When we get a \"list\" command,\nwe feed the result to get_remote_heads, and when we get a\n\"fetch\" or \"push\" command, we feed it to fetch-pack or\nsend-pack, respectively.\n\nIf the HTTP response from the server is truncated for any\nreason, we will get an incomplete ref advertisement. If we\nthen feed this incomplete list to fetch-pack, one of a few\nthings may happen:\n\n 1. If the truncation is in a packet header, fetch-pack\n will notice the bogus line and complain.\n\n 2. If the truncation is inside a packet, fetch-pack will\n keep waiting for us to send the rest of the packet,\n which we never will.\n\n 3. If the truncation is at a packet boundary, fetch-pack\n will keep waiting for us to send the next packet, which\n we never will.\n\nAs a result, fetch-pack hangs, waiting for input. However,\nremote-curl believes it has sent all of the advertisement,\nand therefore waits for fetch-pack to speak. The two\nprocesses end up in a deadlock.\n\nWe do notice the broken ref list if we feed it to\nget_remote_heads. So if git asks the helper to do a \"list\"\nfollowed by a \"fetch\", we are safe; we'll abort during the\nlist operation, which parses the refs.\n\nThis patch teaches remote-curl to always parse and save the\nincoming ref list when we read the ref advertisement from a\nserver. That means that we will always verify and abort\nbefore even running fetch-pack (or send-pack) when reading a\ncorrupted list, even if we do not run the \"list\" command\nexplicitly.\n\nSince we save the result, in the common case of running\n\"list\" then \"fetch\", we do not do any extra parsing at all.\nIn the case of just a \"fetch\", we do an extra round of\nparsing, but only once.\n\nNote also that the \"fetch\" case will now also initialize\nserver_capabilities from the remote (in remote-curl; we\nalready would do so inside fetch-pack). Doing \"list+fetch\"\nalready does this. It doesn't actually matter now, but the\nnew behavior is arguably more correct, should remote-curl\never start caring about the server's capability list.\n\nSigned-off-by: Jeff King \n---\nAnd this does the equivalent of patch 3/3 from the first series, but I\nthink this is much more robust (certainly it solves the ERR problem, but\nmore importantly, it uses the exact same function that other code paths\ndo, so we do not have to worry about it diverging).\n\n remote-curl.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 62f82d1..33af198 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -76,6 +76,7 @@ struct discovery {\n \tchar *buf_alloc;\n \tchar *buf;\n \tsize_t len;\n+\tstruct ref *refs;\n \tunsigned proto_git : 1;\n };\n static struct discovery *last_discovery;\n@@ -145,11 +146,12 @@ static void free_discovery(struct discovery *d)\n \t\tif (d == last_discovery)\n \t\t\tlast_discovery = NULL;\n \t\tfree(d->buf_alloc);\n+\t\tfree_refs(d->refs);\n \t\tfree(d);\n \t}\n }\n \n-static struct discovery* discover_refs(const char *service)\n+static struct discovery* discover_refs(const char *service, int for_push)\n {\n \tstruct strbuf exp = STRBUF_INIT;\n \tstruct strbuf type = STRBUF_INIT;\n@@ -224,6 +226,11 @@ static struct discovery* discover_refs(const char *service)\n \t\tlast->proto_git = 1;\n \t}\n \n+\tif (last->proto_git)\n+\t\tlast->refs = parse_git_refs(last, for_push);\n+\telse\n+\t\tlast->refs = parse_info_refs(last);\n+\n \tfree(refs_url);\n \tstrbuf_release(&exp);\n \tstrbuf_release(&type);\n@@ -232,19 +239,16 @@ static struct ref *get_refs(int for_push)\n \treturn last;\n }\n \n-\n static struct ref *get_refs(int for_push)\n {\n \tstruct discovery *heads;\n \n \tif (for_push)\n-\t\theads = discover_refs(\"git-receive-pack\");\n+\t\theads = discover_refs(\"git-receive-pack\", for_push);\n \telse\n-\t\theads = discover_refs(\"git-upload-pack\");\n+\t\theads = discover_refs(\"git-upload-pack\", for_push);\n \n-\tif (heads->proto_git)\n-\t\treturn parse_git_refs(heads, for_push);\n-\treturn parse_info_refs(heads);\n+\treturn heads->refs;\n }\n \n static void output_refs(struct ref *refs)\n@@ -258,7 +262,6 @@ static void output_refs(struct ref *refs)\n \t}\n \tprintf(\"\\n\");\n \tfflush(stdout);\n-\tfree_refs(refs);\n }\n \n struct rpc_state {\n@@ -674,7 +677,7 @@ static int fetch(int nr_heads, struct ref **to_fetch)\n \n static int fetch(int nr_heads, struct ref **to_fetch)\n {\n-\tstruct discovery *d = discover_refs(\"git-upload-pack\");\n+\tstruct discovery *d = discover_refs(\"git-upload-pack\", 0);\n \tif (d->proto_git)\n \t\treturn fetch_git(d, nr_heads, to_fetch);\n \telse\n@@ -793,7 +796,7 @@ static int push(int nr_spec, char **specs)\n \n static int push(int nr_spec, char **specs)\n {\n-\tstruct discovery *heads = discover_refs(\"git-receive-pack\");\n+\tstruct discovery *heads = discover_refs(\"git-receive-pack\", 1);\n \tint ret;\n \n \tif (heads->proto_git)\n-- \n1.8.1.20.g7078b03\n"},{"id":"209662","messageId":"20130218093335.GK5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"7vhalaas2b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 0/10] pkt-line and remote-curl cleanups server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:33:36Z","receivedAt":"2013-02-18T09:33:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 18, 2013 at 01:29:16AM -0800, Junio C Hamano wrote:\n\n> > I just checked, and GitHub also does not send flush packets after ERR.\n> > Which makes sense; ERR is supposed to end the conversation.\n> \n> Hmph. A flush packet was supposed to be a mark to say \"all the\n> packets before this one can be buffered and kept without getting\n> passed to write(2), but this and all such buffered data _must_ go on\n> the wire _now_\". So in the sense, ERR not followed by a flush may\n> not even have a chance to be seen on the other end, no? That is\n> what the comment before the implementation of packet_flush() is all\n> about.\n\nDespite the name, I always thought of packet_flush as more of a signal\nfor the _receiver_, who then knows that they have gotten all of a\nparticular list. In other words, we seem to use it as a sequence point\nas much as anything (mostly because we immediately write() out any other\npackets anyway, so there is no flushing to be done; but perhaps there\nwere originally thoughts that we could do more buffering on the writing\nside).\n\n-Peff\n"},{"id":"209663","messageId":"7vd2vyarjy.fsf@alter.siamese.dyndns.org","threadId":"32917","inReplyTo":"20130218092221.GD5096@sigill.intra.peff.net","subject":"Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-18T09:40:17Z","receivedAt":"2013-02-18T09:40:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm really tempted to bump all of our 1000-byte buffers to just use\n> LARGE_PACKET_MAX. If we provided a packet_read variant that used a\n> static buffer (which is fine for all but one or two callers), then it\n> would not take much memory...\n\nI thought that 1000-byte limit was kept when we introduced the 64k\ninterface to interoperate with other side that does not yet support\nthe 64k interface. Is your justification that such an old version of\nGit no longer matters in the real world (which is true, I think), or\nwe use 1000-byte limit in some codepaths even when we know that we\nare talking with a 64k-capable version of Git on the other side?\n"},{"id":"209664","messageId":"7v8v6mar4e.fsf@alter.siamese.dyndns.org","threadId":"32917","inReplyTo":"20130218093335.GK5096@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/10] pkt-line and remote-curl cleanups server","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-18T09:49:37Z","receivedAt":"2013-02-18T09:49:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Feb 18, 2013 at 01:29:16AM -0800, Junio C Hamano wrote:\n>\n>> > I just checked, and GitHub also does not send flush packets after ERR.\n>> > Which makes sense; ERR is supposed to end the conversation.\n>> \n>> Hmph. A flush packet was supposed to be a mark to say \"all the\n>> packets before this one can be buffered and kept without getting\n>> passed to write(2), but this and all such buffered data _must_ go on\n>> the wire _now_\". So in the sense, ERR not followed by a flush may\n>> not even have a chance to be seen on the other end, no? That is\n>> what the comment before the implementation of packet_flush() is all\n>> about.\n>\n> Despite the name, I always thought of packet_flush as more of a signal\n> for the _receiver_, who then knows that they have gotten all of a\n> particular list. In other words, we seem to use it as a sequence point\n> as much as anything (mostly because we immediately write() out any other\n> packets anyway, so there is no flushing to be done; but perhaps there\n> were originally thoughts that we could do more buffering on the writing\n> side).\n\nExactly.\n\nThe logic behind the comment \"we do not buffer, but we should\" is\nthat flush tells the receiver that the sending end is \"done\" feeding\nit a class of data, and the data before flush do not have to reach\nthe receiver immediately, hence we can afford to buffer on the\nsending end if we can measure that doing so would give us better\nperformance. We haven't measure and turned buffering on yet.\n\nBut when dying, we may of course have data before flushing. We may\ndisconnect (by dying) without showing flush (or preceding ERR) to\nthe other side, and for that reason, not relying on the flush packet\non the receiving end is a good change. Once we turn buffering on, we\nprobably need to be careful when sending an ERR indicator by making\nit always drain any buffered data and show the ERR indicator without\nbuffering, or something.\n"},{"id":"209665","messageId":"20130218094959.GA16408@sigill.intra.peff.net","threadId":"32917","inReplyTo":"7vd2vyarjy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:49:59Z","receivedAt":"2013-02-18T09:49:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 18, 2013 at 01:40:17AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'm really tempted to bump all of our 1000-byte buffers to just use\n> > LARGE_PACKET_MAX. If we provided a packet_read variant that used a\n> > static buffer (which is fine for all but one or two callers), then it\n> > would not take much memory...\n> \n> I thought that 1000-byte limit was kept when we introduced the 64k\n> interface to interoperate with other side that does not yet support\n> the 64k interface. Is your justification that such an old version of\n> Git no longer matters in the real world (which is true, I think), or\n> we use 1000-byte limit in some codepaths even when we know that we\n> are talking with a 64k-capable version of Git on the other side?\n\nI should have been more clear that I want to bump only the _reading_\nside, not the writing.\n\nThe sideband-64k capability bumps the sideband chunk size. But the size\nfor packetized ref advertisement lines has remained at 1000. Which it\nmust, since we start outputting them before doing capability\nnegotiation. The sender will die before writing a longer ref line (see\npkg-line.c:format_packet), and most of the reading callsites feed a\n1000-byte buffer to packet_read_line (which will die if we get a larger\npacket). So the upgrade path for that would be:\n\n 1. Git bumps up all reading buffers to LARGE_PACKET_MAX, just in case.\n This immediately covers us for any alternate implementations that\n send larger ref packets (I do not know if any exist, but the\n documentation does not mention this limitation at all, so I would\n not be surprised if other implementations just use LARGE_PACKET_MAX\n as a maximum).\n\n 2. Many years pass. We decide that Git v1.8.2 and older are ancient\n history and not worth caring about.\n\n 3. We bump the 1000-byte limit for format_packet to LARGE_PACKET_MAX.\n\nThis is not pressing at all; I wouldn't have even noticed it if I hadn't\nbeen wondering how large to make the new buffer I was adding in a later\npatch. I have not heard of anyone running up against this limit in\npractice. But it's easy to do (1), and it starts the clock ticking for\nthe 1000-byte readers to become obsolete.\n\n-Peff\n"},{"id":"209666","messageId":"20130218095537.GL5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"7v8v6mar4e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 0/10] pkt-line and remote-curl cleanups server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T09:55:37Z","receivedAt":"2013-02-18T09:55:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 18, 2013 at 01:49:37AM -0800, Junio C Hamano wrote:\n\n> The logic behind the comment \"we do not buffer, but we should\" is\n> that flush tells the receiver that the sending end is \"done\" feeding\n> it a class of data, and the data before flush do not have to reach\n> the receiver immediately, hence we can afford to buffer on the\n> sending end if we can measure that doing so would give us better\n> performance. We haven't measure and turned buffering on yet.\n> \n> But when dying, we may of course have data before flushing. We may\n> disconnect (by dying) without showing flush (or preceding ERR) to\n> the other side, and for that reason, not relying on the flush packet\n> on the receiving end is a good change. Once we turn buffering on, we\n> probably need to be careful when sending an ERR indicator by making\n> it always drain any buffered data and show the ERR indicator without\n> buffering, or something.\n\nYeah, I'd agree. An ERR packet should always be accompanied by a flush\n(whether an actual flush packet or not doesn't really matter, but\ndefinitely a buffer flush on the sender). But I think that is really the\nsender's problem, and we can deal with it there if and when we buffer.\n\n>From the receiver's end, they simply want to be liberal in interpreting\nan ERR as the end of the data stream. They do not have to be picky about\nwhether it is followed by more data, or by a flush packet, or whatever.\nBut that is not a change I am introducing; that is how get_remote_heads\nhas always worked. I am merely correcting the proposed change from v1 of\nthe series that did not correctly take that into account (and therefore\nregressed the error message in the ERR case).\n\n-Peff\n"},{"id":"209667","messageId":"20130218095644.GA7049@elie.Belkin","threadId":"32917","inReplyTo":"20130218091519.GB5096@sigill.intra.peff.net","subject":"Re: [PATCHv2 02/10] pkt-line: drop safe_write function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T09:56:45Z","receivedAt":"2013-02-18T09:56:45Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> This is just write_or_die by another name.\n>\n> Signed-off-by: Jeff King \n> ---\n> Actually, they are not quite the same. write_or_die will exit(0) when it\n> sees EPIPE.\n\nThat information definitely belongs in the commit message.\n\n> Which makes me a little nervous.\n\nMe, too. Let's see:\n\n[...]\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -908,7 +908,7 @@ static void report(struct command *commands, const char *unpack_status)\n> \tif (use_sideband)\n> \t\tsend_sideband(1, 1, buf.buf, buf.len, use_sideband);\n> \telse\n> -\t\tsafe_write(1, buf.buf, buf.len);\n> +\t\twrite_or_die(1, buf.buf, buf.len);\n\nIf the connection to send-pack is lost and stdout becomes a broken\npipe and I am updating enough refs to overflow the pipe buffer,\nreceive-pack will die with SIGPIPE. So unless the sadistic caller has\nset the inherited SIGPIPE action to SIG_IGN (for example by wrapping\ngit with an uncautious Python wrapper that uses subprocess.Popen), the\nchange to EPIPE handling is not a behavior change.\n\nSince the pipe is closed, presumably the calling send-pack has hung up\nand won't notice the exit status, so this should be safe.\n\nArguably it would be more friendly to stay alive to run the\npost-receive and post-update hooks, though, given that a ref update\nhas occurred. Maybe transport commands like this one should always\nset the disposition of SIGPIPE to SIG_IGN.\n\n[...]\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -79,7 +79,7 @@ static void print_helper_status(struct ref *ref)\n> \t\t}\n> \t\tstrbuf_addch(&buf, '\\n');\n> \n> -\t\tsafe_write(1, buf.buf, buf.len);\n> +\t\twrite_or_die(1, buf.buf, buf.len);\n\nA signal will kill send-pack before write_or_die has a chance to\nintervene so this change is a no-op unless the caller is sadistic\n(as in the [1] case). In the signal(SIGPIPE, SIG_IGN) case, it might\nbe a regression, since \"git push\" should not declare success when its\nconnection to receive-pack closes early.\n\n[1] http://www.chiark.greenend.org.uk/ucgi/~cjwatson/blosxom/2009-07-02-python-sigpipe.html\n\n[...]\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -245,7 +245,7 @@ static void send_request(struct fetch_pack_args *args,\n> \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n> \t\tpacket_flush(fd);\n> \t} else\n> -\t\tsafe_write(fd, buf->buf, buf->len);\n> +\t\twrite_or_die(fd, buf->buf, buf->len);\n\nAlso a no-op except when the parent process is insane enough to let us\ninherit signal(SIGPIPE, SIG_IGN).\n\nIn that case, if triggerable this looks like a bad change: if\nupload-pack has gone missing, the fetch should not be considered a\nsuccess.\n\n[...]\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -70,7 +70,7 @@ static void format_write(int fd, const char *fmt, ...)\n> \tif (n >= sizeof(buffer))\n> \t\tdie(\"protocol error: impossibly long line\");\n> \n> -\tsafe_write(fd, buffer, n);\n> +\twrite_or_die(fd, buffer, n);\n\nEtc. I'm stopping here.\n\nI'm thinking before a patch like this we should make the following\nchange:\n\n 1. at startup, set the signal action of SIGPIPE to SIG_DFL, to make\n the behavior a little more predictable.\n\nPerhaps the following as well:\n\n 2. in write_or_die(), when encountering EPIPE, set the signal action\n of SIGPIPE to SIG_DFL and raise(SIGPIPE), ensuring the exit status\n reflects the broken pipe. If the parent process is unnecessarily\n noisy about that, that's a bug in the parent process (hopefully\n uncommon).\n\nOr alternatively:\n\n 2b. never set SIGPIPE to SIG_IGN except in short blocks of code that\n do not call write_or_die()\n\nWhat do you think?\nJonathan\n"},{"id":"209669","messageId":"20130218101209.GC7049@elie.Belkin","threadId":"32917","inReplyTo":"20130218091619.GC5096@sigill.intra.peff.net","subject":"Re: [PATCHv2 03/10] pkt-line: clean up \"gentle\" reading function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T10:12:09Z","receivedAt":"2013-02-18T10:12:09Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Originally we had a single function for reading packetized\n> data: packet_read_line. Commit 46284dd grew a more \"gentle\"\n> form that would return an error instead of dying upon\n> reading a truncated input stream. However:\n\nIn other words:\n\n\tBased on the names of two functions \"packet_read\" and\n\t\"packet_read_line\", it is not obvious which to use and what the\n\tramifications of that choice are.\n\n\tRename packet_read to packet_read_line_gently and add a comment\n\texplaining that the latter is a \"gentler\" form that returns an\n\terror instead of dying upon reading a truncated input stream.\n\n\tWhile at it:\n\n\t * Rename the internal argument triggering the gentle mode to\n\t \"gentle\" instead of \"return_line_fail\".\n\n\t * Drop the redundant \"return_line_fail &&\" in checks like\n\t \"if (return_line_fail && ret < 0)\". safe_read() never\n\t returns an error when !gentle.\n\n\tNo functional change intended.\n\nFWIW, the patch itself is\nReviewed-by: Jonathan Nieder \n"},{"id":"209670","messageId":"20130218101523.GD7049@elie.Belkin","threadId":"32917","inReplyTo":"20130218092221.GD5096@sigill.intra.peff.net","subject":"Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T10:15:23Z","receivedAt":"2013-02-18T10:15:23Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -160,7 +160,8 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)\n> \t}\n> \tlen -= 4;\n> \tif (len >= size)\n> -\t\tdie(\"protocol error: bad line length %d\", len);\n> +\t\tdie(\"protocol error: line too large: (expected %u, got %d)\",\n> +\t\t size, len);\n\nMakes sense. I think this should say \"expected < %u, got %d\", since we\ndon't actually expect most lines to be 1004 bytes in practice.\n\nWith or without such a change,\nReviewed-by: Jonathan Nieder \n"},{"id":"209671","messageId":"20130218101915.GE7049@elie.Belkin","threadId":"32917","inReplyTo":"20130218092252.GE5096@sigill.intra.peff.net","subject":"Re: [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T10:19:15Z","receivedAt":"2013-02-18T10:19:15Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Originally packets were used just for the line-oriented ref\n> advertisement and negotiation. These days, we also stuff\n> packfiles and sidebands into them, and they do not\n> necessarily represent a line. Drop the \"_line\" suffix, as it\n> is not informative and makes the function names quite long\n> (especially as we add \"_gently\" and other variants).\n>\n> Signed-off-by: Jeff King \n> ---\n> Again, this is a taste issue. Can be optional.\n\nIn combination with patch 3, this changes the meaning of packet_read()\nwithout changing its signature, which could make other patches\ncherry-picked on top change behavior in unpredictable ways. :(\n\nSo I'd be all for this if the signature changes (for example to put\nthe fd at the end or something), but not so if not.\n\nThanks,\nJonathan\n"},{"id":"209672","messageId":"20130218102430.GM5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218095644.GA7049@elie.Belkin","subject":"Re: [PATCHv2 02/10] pkt-line: drop safe_write function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T10:24:30Z","receivedAt":"2013-02-18T10:24:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 18, 2013 at 01:56:45AM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > This is just write_or_die by another name.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> > Actually, they are not quite the same. write_or_die will exit(0) when it\n> > sees EPIPE.\n> \n> That information definitely belongs in the commit message.\n\nYeah, this one was more RFC; I was hoping for some input on the EPIPE\nthing, so I could know _what_ to put in the commit message. Is it safe,\nand if so, why? And if not, we should drop the patch.\n\n> If the connection to send-pack is lost and stdout becomes a broken\n> pipe and I am updating enough refs to overflow the pipe buffer,\n> receive-pack will die with SIGPIPE. So unless the sadistic caller has\n> set the inherited SIGPIPE action to SIG_IGN (for example by wrapping\n> git with an uncautious Python wrapper that uses subprocess.Popen), the\n> change to EPIPE handling is not a behavior change.\n\nYeah, but I don't want to count on always catching SIGPIPE. There's the\ninherited signal handler thing, but there's also the fact that we may\nend up ignoring SIGPIPE from backend programs like upload-pack and\nreceive-pack; they check their writes anyway, and we have already run\ninto issues with getting SIGPIPE when we don't necessarily expect or\ncare about it.\n\nThe nice thing about write_or_die is that it still _exits_ on EPIPE.\nIt's just that it doesn't print an error (which is really not a big\ndeal) and exit with a 0 return code. I really wonder if we should just\nchange the latter. For programs which are creating copious output (e.g.,\n\"git log\"), the return value is not important anyway. For backend\nprograms, an unexpected EPIPE from something like write_or_die should\nprobably involve a non-successful return code.\n\n> Arguably it would be more friendly to stay alive to run the\n> post-receive and post-update hooks, though, given that a ref update\n> has occurred. Maybe transport commands like this one should always\n> set the disposition of SIGPIPE to SIG_IGN.\n\nYeah, I've suggested that in the past. And I do think it's sane, because\nif you took a ref update, you almost certainly want to run the\npost-receive, even if the client is no longer around (e.g., if it is\ngoing to email out the changeset).\n\n> [...]\n> > --- a/builtin/send-pack.c\n> > +++ b/builtin/send-pack.c\n> > @@ -79,7 +79,7 @@ static void print_helper_status(struct ref *ref)\n> > \t\t}\n> > \t\tstrbuf_addch(&buf, '\\n');\n> > \n> > -\t\tsafe_write(1, buf.buf, buf.len);\n> > +\t\twrite_or_die(1, buf.buf, buf.len);\n> \n> A signal will kill send-pack before write_or_die has a chance to\n> intervene so this change is a no-op unless the caller is sadistic\n> (as in the [1] case). In the signal(SIGPIPE, SIG_IGN) case, it might\n> be a regression, since \"git push\" should not declare success when its\n> connection to receive-pack closes early.\n\nBut that isn't going to receive-pack, is it? Send-pack's stdout is\nreally just going to the user (or wherever). So it would have an effect\nmore for:\n\n (git push && echo >&2 OK) | grep -m1 foo\n\nwhich might print \"OK\" even if we failed. That's quite contrived, but it\nis at least a measurable change. And anyway...\n\n> In that case, if triggerable this looks like a bad change: if\n> upload-pack has gone missing, the fetch should not be considered a\n> success.\n> [...]\n> Etc. I'm stopping here.\n\nYeah, there are definitely some bad ones.\n\n> I'm thinking before a patch like this we should make the following\n> change:\n> \n> 1. at startup, set the signal action of SIGPIPE to SIG_DFL, to make\n> the behavior a little more predictable.\n\nI'm lukewarm on that, just because we may want to ignore SIGPIPE\nourselves at some point.\n\n> Perhaps the following as well:\n> \n> 2. in write_or_die(), when encountering EPIPE, set the signal action\n> of SIGPIPE to SIG_DFL and raise(SIGPIPE), ensuring the exit status\n> reflects the broken pipe. If the parent process is unnecessarily\n> noisy about that, that's a bug in the parent process (hopefully\n> uncommon).\n\nI like this. My suggestion would be to just exit(1) instead of exit(0).\nBut really, raising SIGPIPE makes the most sense, because it\ncommunicates to the parent what happened (and the shell will wisely not\nprint a message, but careful parents like \"git fetch\" and \"git push\"\nwill check it properly and notice that the child did not succeed).\n\n> Or alternatively:\n> \n> 2b. never set SIGPIPE to SIG_IGN except in short blocks of code that\n> do not call write_or_die()\n\nYuck. :)\n\n> What do you think?\n\nI really like option 2. That exit(0) when we see SIGPIPE bugs me.\nBecause we _are_ dying due to our write failing, and I think the only\nreason to exit(0) was to avoid unnecessary complaints from parents. But\nraising SIGPIPE seems like the best of both worlds to me.\n\n-Peff\n"},{"id":"209673","messageId":"20130218102551.GN5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218101209.GC7049@elie.Belkin","subject":"Re: [PATCHv2 03/10] pkt-line: clean up \"gentle\" reading function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T10:25:51Z","receivedAt":"2013-02-18T10:25:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 18, 2013 at 02:12:09AM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Originally we had a single function for reading packetized\n> > data: packet_read_line. Commit 46284dd grew a more \"gentle\"\n> > form that would return an error instead of dying upon\n> > reading a truncated input stream. However:\n> \n> In other words:\n\nHmph. I had originally written a commit message organized more like\nyours, then I changed it to try to be more clear. I guess that didn't\nwork.\n\nBut yes, you get the intent exactly.\n\n-Peff\n"},{"id":"209674","messageId":"20130218102628.GO5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218101523.GD7049@elie.Belkin","subject":"Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T10:26:28Z","receivedAt":"2013-02-18T10:26:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 18, 2013 at 02:15:23AM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > --- a/pkt-line.c\n> > +++ b/pkt-line.c\n> > @@ -160,7 +160,8 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)\n> > \t}\n> > \tlen -= 4;\n> > \tif (len >= size)\n> > -\t\tdie(\"protocol error: bad line length %d\", len);\n> > +\t\tdie(\"protocol error: line too large: (expected %u, got %d)\",\n> > +\t\t size, len);\n> \n> Makes sense. I think this should say \"expected < %u, got %d\", since we\n> don't actually expect most lines to be 1004 bytes in practice.\n\nYeah, I had toyed with writing \"expected max %u\" for the same reason.\nI'll tweak it in the re-roll.\n\n-Peff\n"},{"id":"209675","messageId":"20130218102931.GP5096@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218101915.GE7049@elie.Belkin","subject":"Re: [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T10:29:31Z","receivedAt":"2013-02-18T10:29:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 18, 2013 at 02:19:15AM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Originally packets were used just for the line-oriented ref\n> > advertisement and negotiation. These days, we also stuff\n> > packfiles and sidebands into them, and they do not\n> > necessarily represent a line. Drop the \"_line\" suffix, as it\n> > is not informative and makes the function names quite long\n> > (especially as we add \"_gently\" and other variants).\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> > Again, this is a taste issue. Can be optional.\n> \n> In combination with patch 3, this changes the meaning of packet_read()\n> without changing its signature, which could make other patches\n> cherry-picked on top change behavior in unpredictable ways. :(\n> \n> So I'd be all for this if the signature changes (for example to put\n> the fd at the end or something), but not so if not.\n\nTrue. Though packet_read has only existed since last June, only had one\ncallsite (which would now conflict, since I'm touching it in this\nseries), and has no new calls in origin..origin/pu. So it's relatively\nlow risk for such a problem. I don't know how careful we want to be.\n\n-Peff\n"},{"id":"209676","messageId":"20130218104350.GF7049@elie.Belkin","threadId":"32917","inReplyTo":"20130218092612.GF5096@sigill.intra.peff.net","subject":"Re: [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T10:43:50Z","receivedAt":"2013-02-18T10:43:50Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> The packet_read function reads from a descriptor.\n\nAh, so this introduces a new analagous helper that reads from\na strbuf, to avoid the copy-from-async-procedure hack?\n\n[...]\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -103,12 +103,26 @@ static int safe_read(int fd, void *buffer, unsigned size, int gently)\n> \tstrbuf_add(buf, buffer, n);\n> }\n> \n> -static int safe_read(int fd, void *buffer, unsigned size, int gently)\n> +static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n> +\t\t\t void *dst, unsigned size, int gently)\n> {\n> -\tssize_t ret = read_in_full(fd, buffer, size);\n> -\tif (ret < 0)\n> -\t\tdie_errno(\"read error\");\n> -\telse if (ret < size) {\n> +\tssize_t ret;\n> +\n> +\t/* Read up to \"size\" bytes from our source, whatever it is. */\n> +\tif (src_buf) {\n> +\t\tret = size < *src_size ? size : *src_size;\n> +\t\tmemcpy(dst, *src_buf, ret);\n> +\t\t*src_buf += size;\n> +\t\t*src_size -= size;\n> +\t}\n> +\telse {\n\nStyle: git cuddles its \"else\"s.\n\n\tassert(src_buf ? fd < 0 : fd >= 0);\n\n\tif (src_buf) {\n\t\t...\n\t} else {\n\t\t...\n\t}\n\n> +\t\tret = read_in_full(fd, dst, size);\n> +\t\tif (ret < 0)\n> +\t\t\tdie_errno(\"read error\");\n\nThis is noisy about upstream pipe gone missing, which makes sense\nsince this is transport-related. Maybe that deserves a comment.\n\n[...]\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -138,28 +138,29 @@ static struct discovery* discover_refs(const char *service)\n> \tif (maybe_smart &&\n> \t (5 <= last->len && last->buf[4] == '#') &&\n> \t !strbuf_cmp(&exp, &type)) {\n> +\t\tchar line[1000];\n> +\t\tint len;\n> +\n> \t\t/*\n> \t\t * smart HTTP response; validate that the service\n> \t\t * pkt-line matches our request.\n> \t\t */\n> -\t\tif (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n> -\t\t\tdie(\"%s has invalid packet header\", refs_url);\n> -\t\tif (buffer.len && buffer.buf[buffer.len - 1] == '\\n')\n> -\t\t\tstrbuf_setlen(&buffer, buffer.len - 1);\n> +\t\tlen = packet_read_from_buf(line, sizeof(line), &last->buf, &last->len);\n> +\t\tif (len && line[len - 1] == '\\n')\n> +\t\t\tlen--;\n\nWas anything guaranteeing that buffer.len < 1000 before this change?\n\nThe rest looks good from a quick glance.\n\nJonathan\n"},{"id":"209677","messageId":"20130218104718.GG7049@elie.Belkin","threadId":"32917","inReplyTo":"20130218092905.GH5096@sigill.intra.peff.net","subject":"Re: [PATCHv2 08/10] remote-curl: pass buffer straight to get_remote_heads","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T10:47:18Z","receivedAt":"2013-02-18T10:47:18Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I don't know that this code was hurting anything, but it has always\n> struck me as ugly and a possible source of error. And now it's gone.\n\nHeh. Belongs in the commit message, presumably.\n\nI don't think the async procedure was very harmful, but it's nice to\navoid the cost of a new thread and some copying.\n\n> remote-curl.c | 26 ++------------------------\n> 1 file changed, 2 insertions(+), 24 deletions(-)\n\nNice. The patch looks sane.\n"},{"id":"209678","messageId":"20130218104804.GB16408@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218104350.GF7049@elie.Belkin","subject":"Re: [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T10:48:04Z","receivedAt":"2013-02-18T10:48:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 18, 2013 at 02:43:50AM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > The packet_read function reads from a descriptor.\n> \n> Ah, so this introduces a new analagous helper that reads from\n> a strbuf, to avoid the copy-from-async-procedure hack?\n\nNot from a strbuf, but basically, yes.\n\n> > +\t\tret = read_in_full(fd, dst, size);\n> > +\t\tif (ret < 0)\n> > +\t\t\tdie_errno(\"read error\");\n> \n> This is noisy about upstream pipe gone missing, which makes sense\n> since this is transport-related. Maybe that deserves a comment.\n\nThat is not new code; it is just reindented from the original safe_read.\nBut is it noisy about a missing pipe? We do not get EPIPE for reading.\nWe should just get a short read or EOF, both of which is handled later.\n\n> > +\t\tlen = packet_read_from_buf(line, sizeof(line), &last->buf, &last->len);\n> > +\t\tif (len && line[len - 1] == '\\n')\n> > +\t\t\tlen--;\n> \n> Was anything guaranteeing that buffer.len < 1000 before this change?\n\nNo. That's discussed in point (3) of the \"implications\" in the commit\nmessage.\n\n-Peff\n"},{"id":"209679","messageId":"20130218105033.GH7049@elie.Belkin","threadId":"32917","inReplyTo":"20130218093056.GJ5096@sigill.intra.peff.net","subject":"Re: [PATCHv2 10/10] remote-curl: always parse incoming refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T10:50:33Z","receivedAt":"2013-02-18T10:50:33Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> remote-curl.c | 23 +++++++++++++----------\n> 1 file changed, 13 insertions(+), 10 deletions(-)\n\nI like.\n"},{"id":"209680","messageId":"20130218105440.GI7049@elie.Belkin","threadId":"32917","inReplyTo":"20130218104804.GB16408@sigill.intra.peff.net","subject":"Re: [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T10:54:41Z","receivedAt":"2013-02-18T10:54:41Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> But is it noisy about a missing pipe? We do not get EPIPE for reading.\n\nAh, that makes more sense.\n\n[...]\n>>> +\t\tlen = packet_read_from_buf(line, sizeof(line), &last->buf, &last->len);\n>>> +\t\tif (len && line[len - 1] == '\\n')\n>>> +\t\t\tlen--;\n>>\n>> Was anything guaranteeing that buffer.len < 1000 before this change?\n>\n> No. That's discussed in point (3) of the \"implications\" in the commit\n> message.\n\nThanks. Sorry I missed it the first time.\n"},{"id":"209681","messageId":"20130218110556.GA9294@elie.Belkin","threadId":"32917","inReplyTo":"20130218102931.GP5096@sigill.intra.peff.net","subject":"Re: [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T11:05:56Z","receivedAt":"2013-02-18T11:05:56Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Feb 18, 2013 at 02:19:15AM -0800, Jonathan Nieder wrote:\n\n>> In combination with patch 3, this changes the meaning of packet_read()\n>> without changing its signature, which could make other patches\n>> cherry-picked on top change behavior in unpredictable ways. :(\n>>\n>> So I'd be all for this if the signature changes (for example to put\n>> the fd at the end or something), but not so if not.\n>\n> True. Though packet_read has only existed since last June, only had one\n> callsite (which would now conflict, since I'm touching it in this\n> series), and has no new calls in origin..origin/pu. So it's relatively\n> low risk for such a problem. I don't know how careful we want to be.\n\nI was unclear. What I am worried about is that someone using a\nversion of git without this patch will try some yet-to-be-written\npatch using packet_read from the mailing list and not notice that they\nare using the wrong function. For example, if someone is using\n1.7.12.y or 1.8.1.y and wants to try a patch from after the above,\nthey would get subtly different and wrong results.\n\nThe rule \"change the name or signature when breaking the ABI of a\nglobal function\" is easy to remember and follow. I think we want not\nto have to be careful at all, and such rules can help with that. :)\n\nThanks,\nJonathan\n"},{"id":"209738","messageId":"7v1ucdb9gw.fsf@alter.siamese.dyndns.org","threadId":"32917","inReplyTo":"20130218094959.GA16408@sigill.intra.peff.net","subject":"Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-18T21:25:35Z","receivedAt":"2013-02-18T21:25:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But it's easy to do (1), and it starts the clock ticking for\n> the 1000-byte readers to become obsolete.\n\nYup, I agree with that goal.\n"},{"id":"209741","messageId":"20130218213331.GA29200@sigill.intra.peff.net","threadId":"32917","inReplyTo":"7v1ucdb9gw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-18T21:33:31Z","receivedAt":"2013-02-18T21:33:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 18, 2013 at 01:25:35PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But it's easy to do (1), and it starts the clock ticking for\n> > the 1000-byte readers to become obsolete.\n> \n> Yup, I agree with that goal.\n\nHaving just looked at the pkt-line callers a lot, I think most of them\ncould go for something like:\n\n char *packet_read(int fd, unsigned *len_p)\n {\n static char buffer[LARGE_PACKET_MAX];\n int len;\n\n len = packet_read_to_buf(fd, buffer, sizeof(buffer));\n if (len < 0)\n return NULL;\n\n *len_p = len;\n return buffer;\n }\n\nThat would actually simplify the callers a bit, and would harmonize the\nbuffer sizes at the same time. I'll look into doing a series tonight.\n\n-Peff\n"},{"id":"209890","messageId":"CAJo=hJvrc3EUfzr2BDZ3i68rwZVeHFGHmKwrvhhMMV45DLaWzw@mail.gmail.com","threadId":"32917","inReplyTo":"20130218091203.GB17003@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/10] pkt-line and remote-curl cleanups server","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2013-02-20T07:05:36Z","receivedAt":"2013-02-20T07:05:36Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Mon, Feb 18, 2013 at 1:12 AM, Jeff King wrote:\n> On Sun, Feb 17, 2013 at 05:41:13PM -0800, Jonathan Nieder wrote:\n>\n>> > I don't think so. Don't ERR lines appear inside their own packets?\n>>\n>> Yes, I misread get_remote_heads for some reason. Thanks for checking.\n>\n> Thanks for bringing it up. I had not even thought about ERR at all. So\n> it was luck rather than skill that I was right. :)\n>\n>> I'm not sure whether servers are expected to send a flush after an\n>> ERR packet. The only codepath I know of in git itself that sends\n>> such packets is git-daemon, which does not flush after the error (but\n>> is not used in the stateless-rpc case). http-backend uses HTTP error\n>> codes for its errors.\n>\n> I just checked, and GitHub also does not send flush packets after ERR.\n> Which makes sense; ERR is supposed to end the conversation. I can change\n> GitHub, of course, but who knows what other implementations exist (e.g.,\n> I do not know off-hand whether gitolite has custom ERR responses). So it\n> seems pretty clear that just checking for a flush packet is not the\n> right thing, and we need to actually parse the packet contents (at least\n> to some degree).\n\nJGit (and by extension Gerrit Code Review, android.googlesource.com)\nsends ERR with no flush-pkt. I would like to sort of keep the protocol\nthis way, given how many servers in the wild are running Gerrit and\ncurrently use ERR with no flush-pkt. IMHO its a little late to be\nclosing that door and stuffing a flush-pkt after the ERR that ends the\nconversation.\n"},{"id":"209891","messageId":"CAJo=hJsRJ4iEL0MmH9sdz2UXFm76vVE3OxcVBR6hE1PavzGfPQ@mail.gmail.com","threadId":"32917","inReplyTo":"7v8v6mar4e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 0/10] pkt-line and remote-curl cleanups server","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2013-02-20T07:14:45Z","receivedAt":"2013-02-20T07:14:45Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Mon, Feb 18, 2013 at 1:49 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Mon, Feb 18, 2013 at 01:29:16AM -0800, Junio C Hamano wrote:\n>>\n>>> > I just checked, and GitHub also does not send flush packets after ERR.\n>>> > Which makes sense; ERR is supposed to end the conversation.\n>>>\n>>> Hmph. A flush packet was supposed to be a mark to say \"all the\n>>> packets before this one can be buffered and kept without getting\n>>> passed to write(2), but this and all such buffered data _must_ go on\n>>> the wire _now_\". So in the sense, ERR not followed by a flush may\n>>> not even have a chance to be seen on the other end, no? That is\n>>> what the comment before the implementation of packet_flush() is all\n>>> about.\n>>\n>> Despite the name, I always thought of packet_flush as more of a signal\n>> for the _receiver_, who then knows that they have gotten all of a\n>> particular list. In other words, we seem to use it as a sequence point\n>> as much as anything (mostly because we immediately write() out any other\n>> packets anyway, so there is no flushing to be done; but perhaps there\n>> were originally thoughts that we could do more buffering on the writing\n>> side).\n>\n> Exactly.\n\nThis is also my understanding of the flush-pkt (\"0000\"). Its an\nend-of-list/end-of-section marker to let the peer know the protocol is\nmoving to the next state. Except where the protocol can move to the\nnext state without a flush-pkt of course (see below).\n\n> The logic behind the comment \"we do not buffer, but we should\" is\n> that flush tells the receiver that the sending end is \"done\" feeding\n> it a class of data, and the data before flush do not have to reach\n> the receiver immediately, hence we can afford to buffer on the\n> sending end if we can measure that doing so would give us better\n> performance. We haven't measure and turned buffering on yet.\n\nSo funny story. JGit actually buffers the pkt-line writes in memory\nand does flushes to the network socket when any of the following\nhappen:\n\n- fixed size in-memory buffer is full (8k or 32k by default)\n- flush-pkt is needed in the protocol\n- JGit forces a flush without a flush-pkt\n\nThere are places in the protocol where data needs to be shared with\nthe peer *despite* not having a flush-pkt present in the data stream\nto do that. ERR is one of these places. \"done\\n\" at the end of the\nnegotiation in a client is another. Sending ACK/NAK in a multi_ack\nfrom upload-pack is another. I may be missing more.\n\nJGit had to define three methods to make the pkt-line protocol work correctly:\n\n- writeString: format a string as a single pkt-line, insert into buffer.\n- end: write \"000\" into the buffer, flush the buffer.\n- flush: flush the buffer if it has any content.\n\nI always found that comment in front of that function funny. Its\ntotally not true. Fixing it is harder than just stuffing a buffer in\nthere and hoping for the best. The callers need work. At which point\nthat function isn't really what its author was trying to do.\n\n> But when dying, we may of course have data before flushing. We may\n> disconnect (by dying) without showing flush (or preceding ERR) to\n> the other side, and for that reason, not relying on the flush packet\n> on the receiving end is a good change. Once we turn buffering on, we\n> probably need to be careful when sending an ERR indicator by making\n> it always drain any buffered data and show the ERR indicator without\n> buffering, or something.\n\nYes.\n"},{"id":"209892","messageId":"CAJo=hJvUvEsBw0Tz_c4sk_j2HeK7cZikyiPvQZDL0JgumaKepg@mail.gmail.com","threadId":"32917","inReplyTo":"20130218105033.GH7049@elie.Belkin","subject":"Re: [PATCHv2 10/10] remote-curl: always parse incoming refs","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2013-02-20T07:41:23Z","receivedAt":"2013-02-20T07:41:23Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Mon, Feb 18, 2013 at 1:30 AM, Jeff King wrote:\n> When remote-curl receives a list of refs from a server, it\n> keeps the whole buffer intact. When we get a \"list\" command,\n> we feed the result to get_remote_heads, and when we get a\n> \"fetch\" or \"push\" command, we feed it to fetch-pack or\n> send-pack, respectively.\n>\n> If the HTTP response from the server is truncated for any\n> reason,\n...\n> As a result, fetch-pack hangs, waiting for input. However,\n> remote-curl believes it has sent all of the advertisement,\n> and therefore waits for fetch-pack to speak. The two\n> processes end up in a deadlock.\n\nEek. Thanks for fixing this.\n\nOn Mon, Feb 18, 2013 at 2:50 AM, Jonathan Nieder wrote:\n> Jeff King wrote:\n>\n>> remote-curl.c | 23 +++++++++++++----------\n>> 1 file changed, 13 insertions(+), 10 deletions(-)\n>\n> I like.\n\nMe too.\n"},{"id":"209898","messageId":"20130220084754.GA17055@sigill.intra.peff.net","threadId":"32917","inReplyTo":"20130218213331.GA29200@sigill.intra.peff.net","subject":"Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T08:47:54Z","receivedAt":"2013-02-20T08:47:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 18, 2013 at 04:33:31PM -0500, Jeff King wrote:\n\n> On Mon, Feb 18, 2013 at 01:25:35PM -0800, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > But it's easy to do (1), and it starts the clock ticking for\n> > > the 1000-byte readers to become obsolete.\n> > \n> > Yup, I agree with that goal.\n> \n> Having just looked at the pkt-line callers a lot, I think most of them\n> could go for something like:\n> [...]\n> \n> That would actually simplify the callers a bit, and would harmonize the\n> buffer sizes at the same time. I'll look into doing a series tonight.\n\nJust a quick update on this series. It ended up taking more nights than\nI thought. :) The result looks much better than what I posted before, and\nI found several other corner cases and bugs in packet parsing, too.\n\nI'm going to hold off on posting it tonight, as I'm now up to 19\npatches, and after several rounds of \"rebase -i\", I need to give it a\nfinal read-through myself before inflicting it on anyone else. I'll do\nthat and post it tomorrow.\n\nIn the meantime, please hold off on what I've posted so far (that\nincludes the jk/smart-http-robustify topic).\n\n-Peff\n"},{"id":"209900","messageId":"9aaf7cad-3709-4140-b18d-3287227efd36@email.android.com","threadId":"32917","inReplyTo":"20130220084754.GA17055@sigill.intra.peff.net","subject":"Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-20T08:54:15Z","receivedAt":"2013-02-20T08:54:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\n\nJeff King wrote:\n>\n>In the meantime, please hold off on what I've posted so far (that\n>includes the jk/smart-http-robustify topic).\n\nSurely. I'm done for the night already. Looking forward to see the reroll tomorrow.\n\nThanks.\n-- \nPardon terseness, typo and HTML from a tablet.\n"}]}