{"thread":{"id":"32952","subject":"[PATCHv3 0/19] pkt-line cleanups and fixes","startedAt":"2013-02-20T19:51:47Z","lastAt":"2014-03-28T20:35:52Z","messageCount":40,"participants":["Jeff King","Jonathan Nieder","Junio C Hamano","Eric Sunshine","Marat Radchenko","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"209917","messageId":"20130220195147.GA25332@sigill.intra.peff.net","threadId":"32952","inReplyTo":null,"subject":"[PATCHv3 0/19] pkt-line cleanups and fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T19:51:47Z","receivedAt":"2013-02-20T19:51:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's another round of my pkt-line fixes. The more I dug, the more\ninteresting corners I found. :)\n\nThere are really several potentially independent topics rolled together\nhere. There are dependencies between some of them, so I tried to float\nthe most independent and non-controversial bits to the beginning. We may\nwant those as a separate topic to merge sooner, and have the rest as a\ntopic build on top.\n\nOverall, the diffstat shows a reduction in lines (and I even added a few\ndozen lines of comments), which is nice. The intent was to fix some bugs\nand corner cases, but I found a lot of cleanup opportunities in the\nmiddle.\n\n builtin/archive.c | 17 ++--\n builtin/fetch-pack.c | 11 +-\n builtin/receive-pack.c | 10 +-\n builtin/send-pack.c | 4 +-\n builtin/upload-archive.c | 45 +++------\n cache.h | 4 +-\n connect.c | 13 +--\n daemon.c | 4 +-\n fetch-pack.c | 18 ++--\n http-backend.c | 8 +-\n http.c | 1 +\n pkt-line.c | 126 ++++++++++-------------\n pkt-line.h | 72 +++++++++++++-\n remote-curl.c | 188 ++++++++++++++++-------------------\n send-pack.c | 22 ++--\n sideband.c | 11 +-\n sideband.h | 3 -\n t/t5503-tagfollow.sh | 38 ++++---\n t/t5700-clone-reference.sh | 10 +-\n transport.c | 6 +-\n upload-pack.c | 40 +++-----\n write_or_die.c | 19 ++--\n 22 files changed, 321 insertions(+), 349 deletions(-)\n\nThe patches are:\n\n [01/19]: upload-pack: use get_sha1_hex to parse \"shallow\" lines\n\n New in this round; fixes a potential interoperability problem.\n\n [02/19]: upload-pack: do not add duplicate objects to shallow list\n\n New. Fixes a potential memory-consumption denial-of-service.\n\n [03/19]: upload-pack: remove packet debugging harness\n\n New. Optional cleanup, but later patches textually depend on it.\n\n [04/19]: fetch-pack: fix out-of-bounds buffer offset in get_ack\n\n New. Fixes a potential interoperability problem.\n\n [05/19]: send-pack: prefer prefixcmp over memcmp in receive_status\n\n New. Optional cleanup.\n\n [06/19]: upload-archive: do not copy repo name\n [07/19]: upload-archive: use argv_array to store client arguments\n\n New. Optional cleanup.\n\n [08/19]: write_or_die: raise SIGPIPE when we get EPIPE\n [09/19]: pkt-line: move a misplaced comment\n [10/19]: pkt-line: drop safe_write function\n\n The latter two were in the last round; but it's 08/19 that makes\n doing 10/19 safe. I think it's also a sane thing to be doing in\n general for existing callers of write_or_die.\n\n These can really be pulled into a separate topic if we want, as\n there isn't even a lot of textual dependency.\n\n [11/19]: pkt-line: provide a generic reading function with options\n\n This is an alternative to the proliferation of different reading\n functions that round 2 had. I think it ends up cleaner. It also\n addresses Jonathan's function-signature concerns.\n\n [12/19]: pkt-line: teach packet_read_line to chomp newlines\n\n New. A convenience cleanup that drops a lot of lines. Technically\n optional, but later patches depend heavily on it (textually, and for\n splitting line-readers from binary-readers).\n\n [13/19]: pkt-line: move LARGE_PACKET_MAX definition from sideband\n [14/19]: pkt-line: provide a LARGE_PACKET_MAX static buffer\n\n New. Another cleanup that makes packet_read_line callers a bit\n simpler, and bumps the packet size limits throughout git, as we\n discussed.\n\n [15/19]: pkt-line: share buffer/descriptor reading implementation\n [16/19]: teach get_remote_heads to read from a memory buffer\n [17/19]: remote-curl: pass buffer straight to get_remote_heads\n\n These are more or less ported from v2's patches 6-8, except that the\n earlier pkt-line changes make the first one way more pleasant.\n\n [18/19]: remote-curl: move ref-parsing code up in file\n [19/19]: remote-curl: always parse incoming refs\n\n ...and the yak is shaved. More or less a straight rebase of their v2\n counterparts, and the thing that actually started me on this topic.\n\nI know it's a big series, but I tried hard to break it down into\nbite-sized chunks. Thanks for your reviewing patience.\n\n-Peff\n"},{"id":"209918","messageId":"20130220195333.GA25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 01/19] upload-pack: use get_sha1_hex to parse \"shallow\" lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T19:53:33Z","receivedAt":"2013-02-20T19:53:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we receive a line like \"shallow \" from the\nclient, we feed the part to get_sha1. This is a\nmistake, as the argument on a shallow line is defined by\nDocumentation/technical/pack-protocol.txt to contain an\n\"obj-id\". This is never defined in the BNF, but it is clear\nfrom the text and from the other uses that it is meant to be\na hex sha1, not an arbitrary identifier (and that is what\nfetch-pack has always sent).\n\nWe should be using get_sha1_hex instead, which doesn't allow\nthe client to request arbitrary junk like \"HEAD@{yesterday}\".\nBecause this is just marking shallow objects, the client\ncouldn't actually do anything interesting (like fetching\nobjects from unreachable reflog entries), but we should keep\nour parsing tight to be on the safe side.\n\nBecause get_sha1 is for the most part a superset of\nget_sha1_hex, in theory the only behavior change should be\ndisallowing non-hex object references. However, there is\none interesting exception: get_sha1 will only parse\na 40-character hex sha1 if the string has exactly 40\ncharacters, whereas get_sha1_hex will just eat the first 40\ncharacters, leaving the rest. That means that current\nversions of git-upload-pack will not accept a \"shallow\"\npacket that has a trailing newline, even though the protocol\ndocumentation is clear that newlines are allowed (even\nencouraged) in non-binary parts of the protocol.\n\nThis never mattered in practice, though, because fetch-pack,\ncontrary to the protocol documentation, does not include a\nnewline in its shallow lines. JGit follows its lead (though\nit correctly is strict on the parsing end about wanting a\nhex object id).\n\nWe do not adjust fetch-pack to send newlines here, as it\nwould break communication with older versions of git (and\nthere is no actual benefit to doing so, except for\nconsistency with other parts of the protocol).\n\nSigned-off-by: Jeff King \n---\nI couldn't trigger anything interestingly malicious from this, but I\ndidn't look very hard. Maybe somebody who knows the shallow protocol\nbetter could think of something clever (not that it matters much).\n\n upload-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 30146a0..b058e8d 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -596,7 +596,7 @@ static void receive_needs(void)\n \t\tif (!prefixcmp(line, \"shallow \")) {\n \t\t\tunsigned char sha1[20];\n \t\t\tstruct object *object;\n-\t\t\tif (get_sha1(line + 8, sha1))\n+\t\t\tif (get_sha1_hex(line + 8, sha1))\n \t\t\t\tdie(\"invalid shallow line: %s\", line);\n \t\t\tobject = parse_object(sha1);\n \t\t\tif (!object)\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209919","messageId":"20130220195457.GB25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 02/19] upload-pack: do not add duplicate objects to shallow list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T19:54:57Z","receivedAt":"2013-02-20T19:54:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the client tells us it has a shallow object via\n\"shallow \", we make sure we have the object, mark it\nwith a flag, then add it to a dynamic array of shallow\nobjects. This means that a client can get us to allocate\narbitrary amounts of memory just by flooding us with shallow\nlines (whether they have the objects or not). You can\ndemonstrate it easily with:\n\n yes '0035shallow e83c5163316f89bfbde7d9ab23ca2e25604af290' |\n git-upload-pack git.git\n\nWe already protect against duplicates in want lines by\nchecking if our flag is already set; let's do the same thing\nhere. Note that a client can still get us to allocate some\namount of memory by marking every object in the repo as\n\"shallow\" (or \"want\"). But this at least bounds it with the\nnumber of objects in the repository, which is not under the\ncontrol of an upload-pack client.\n\nSigned-off-by: Jeff King \n---\nLooking over upload-pack, I think this is the only \"consume arbitrary\nmemory\" spot. Since you can convince git to go to quite a bit of work\njust processing a big repo, the distinction may not be important, but\ndrawing the line between \"large\" and \"arbitrarily large\" seemed\nreasonable to me (and it's a trivial fix).\n\n upload-pack.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex b058e8d..1aee407 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -603,8 +603,10 @@ static void receive_needs(void)\n \t\t\t\tdie(\"did not find object for %s\", line);\n \t\t\tif (object->type != OBJ_COMMIT)\n \t\t\t\tdie(\"invalid shallow object %s\", sha1_to_hex(sha1));\n-\t\t\tobject->flags |= CLIENT_SHALLOW;\n-\t\t\tadd_object_array(object, NULL, &shallows);\n+\t\t\tif (!(object->flags & CLIENT_SHALLOW)) {\n+\t\t\t object->flags |= CLIENT_SHALLOW;\n+\t\t\t add_object_array(object, NULL, &shallows);\n+\t\t\t}\n \t\t\tcontinue;\n \t\t}\n \t\tif (!prefixcmp(line, \"deepen \")) {\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209920","messageId":"20130220195528.GC25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 03/19] upload-pack: remove packet debugging harness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T19:55:28Z","receivedAt":"2013-02-20T19:55:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you set the GIT_DEBUG_SEND_PACK environment variable,\nupload-pack will dump lines it receives in the receive_needs\nphase to a descriptor. This debugging harness is a strict\nsubset of what GIT_TRACE_PACKET can do. Let's just drop it\nin favor of that.\n\nA few tests used GIT_DEBUG_SEND_PACK to confirm which\nobjects get sent; we have to adapt them to the new output\nformat.\n\nSigned-off-by: Jeff King \n---\n t/t5503-tagfollow.sh | 38 +++++++++++++++++---------------------\n t/t5700-clone-reference.sh | 10 +++++-----\n upload-pack.c | 9 ---------\n 3 files changed, 22 insertions(+), 35 deletions(-)\n\ndiff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\nindex 60de2d6..d181c96 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -5,7 +5,7 @@ if ! test_have_prereq NOT_MINGW; then\n . ./test-lib.sh\n \n if ! test_have_prereq NOT_MINGW; then\n-\tsay \"GIT_DEBUG_SEND_PACK not supported - skipping tests\"\n+\tsay \"GIT_TRACE_PACKET not supported - skipping tests\"\n fi\n \n # End state of the repository:\n@@ -42,21 +42,26 @@ test_expect_success NOT_MINGW 'fetch A (new commit : 1 connection)' '\n \n test_expect_success NOT_MINGW 'setup expect' '\n cat - <expect\n-#S\n want $A\n-#E\n EOF\n '\n \n+get_needs () {\n+\tperl -alne '\n+\t\tnext unless $F[1] eq \"upload-pack<\";\n+\t\tlast if $F[2] eq \"0000\";\n+\t\tprint $F[2], \" \", $F[3];\n+\t' \"$@\"\n+}\n+\n test_expect_success NOT_MINGW 'fetch A (new commit : 1 connection)' '\n \trm -f $U &&\n \t(\n \t\tcd cloned &&\n-\t\tGIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&\n+\t\tGIT_TRACE_PACKET=3 git fetch 3>../$U &&\n \t\ttest $A = $(git rev-parse --verify origin/master)\n \t) &&\n-\ttest -s $U &&\n-\tcut -d\" \" -f1,2 $U >actual &&\n+\tget_needs $U >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -74,10 +79,8 @@ want $T\n \n test_expect_success NOT_MINGW 'setup expect' '\n cat - <expect\n-#S\n want $C\n want $T\n-#E\n EOF\n '\n \n@@ -85,13 +88,12 @@ test_expect_success NOT_MINGW 'fetch C, T (new branch, tag : 1 connection)' '\n \trm -f $U &&\n \t(\n \t\tcd cloned &&\n-\t\tGIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&\n+\t\tGIT_TRACE_PACKET=3 git fetch 3>../$U &&\n \t\ttest $C = $(git rev-parse --verify origin/cat) &&\n \t\ttest $T = $(git rev-parse --verify tag1) &&\n \t\ttest $A = $(git rev-parse --verify tag1^0)\n \t) &&\n-\ttest -s $U &&\n-\tcut -d\" \" -f1,2 $U >actual &&\n+\tget_needs $U >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -113,10 +115,8 @@ want $S\n \n test_expect_success NOT_MINGW 'setup expect' '\n cat - <expect\n-#S\n want $B\n want $S\n-#E\n EOF\n '\n \n@@ -124,22 +124,19 @@ want $S\n \trm -f $U &&\n \t(\n \t\tcd cloned &&\n-\t\tGIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&\n+\t\tGIT_TRACE_PACKET=3 git fetch 3>../$U &&\n \t\ttest $B = $(git rev-parse --verify origin/master) &&\n \t\ttest $B = $(git rev-parse --verify tag2^0) &&\n \t\ttest $S = $(git rev-parse --verify tag2)\n \t) &&\n-\ttest -s $U &&\n-\tcut -d\" \" -f1,2 $U >actual &&\n+\tget_needs $U >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success NOT_MINGW 'setup expect' '\n cat - <expect\n-#S\n want $B\n want $S\n-#E\n EOF\n '\n \n@@ -151,15 +148,14 @@ test_expect_success NOT_MINGW 'new clone fetch master and tags' '\n \t\tcd clone2 &&\n \t\tgit init &&\n \t\tgit remote add origin .. &&\n-\t\tGIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&\n+\t\tGIT_TRACE_PACKET=3 git fetch 3>../$U &&\n \t\ttest $B = $(git rev-parse --verify origin/master) &&\n \t\ttest $S = $(git rev-parse --verify tag2) &&\n \t\ttest $B = $(git rev-parse --verify tag2^0) &&\n \t\ttest $T = $(git rev-parse --verify tag1) &&\n \t\ttest $A = $(git rev-parse --verify tag1^0)\n \t) &&\n-\ttest -s $U &&\n-\tcut -d\" \" -f1,2 $U >actual &&\n+\tget_needs $U >actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\nindex c47d450..9cd3b4d 100755\n--- a/t/t5700-clone-reference.sh\n+++ b/t/t5700-clone-reference.sh\n@@ -55,10 +55,10 @@ test_expect_success 'fetched no objects' \\\n rm -f \"$U.D\"\n \n test_expect_success 'cloning with reference (no -l -s)' \\\n-'GIT_DEBUG_SEND_PACK=3 git clone --reference B \"file://$(pwd)/A\" D 3>\"$U.D\"'\n+'GIT_TRACE_PACKET=3 git clone --reference B \"file://$(pwd)/A\" D 3>\"$U.D\"'\n \n test_expect_success 'fetched no objects' \\\n-'! grep \"^want\" \"$U.D\"'\n+'! grep \" want\" \"$U.D\"'\n \n cd \"$base_dir\"\n \n@@ -173,12 +173,12 @@ test_expect_success 'fetch with incomplete alternates' '\n \t(\n \t\tcd K &&\n \t\tgit remote add J \"file://$base_dir/J\" &&\n-\t\tGIT_DEBUG_SEND_PACK=3 git fetch J 3>\"$U.K\"\n+\t\tGIT_TRACE_PACKET=3 git fetch J 3>\"$U.K\"\n \t) &&\n \tmaster_object=$(cd A && git for-each-ref --format=\"%(objectname)\" refs/heads/master) &&\n-\t! grep \"^want $master_object\" \"$U.K\" &&\n+\t! grep \" want $master_object\" \"$U.K\" &&\n \ttag_object=$(cd A && git for-each-ref --format=\"%(objectname)\" refs/tags/HEAD) &&\n-\t! grep \"^want $tag_object\" \"$U.K\"\n+\t! grep \" want $tag_object\" \"$U.K\"\n '\n \n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 1aee407..63cea91 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -42,7 +42,6 @@ static int use_sideband;\n * otherwise maximum packet size (up to 65520 bytes).\n */\n static int use_sideband;\n-static int debug_fd;\n static int advertise_refs;\n static int stateless_rpc;\n \n@@ -580,8 +579,6 @@ static void receive_needs(void)\n \tint has_non_tip = 0;\n \n \tshallow_nr = 0;\n-\tif (debug_fd)\n-\t\twrite_str_in_full(debug_fd, \"#S\\n\");\n \tfor (;;) {\n \t\tstruct object *o;\n \t\tconst char *features;\n@@ -590,8 +587,6 @@ static void receive_needs(void)\n \t\treset_timeout();\n \t\tif (!len)\n \t\t\tbreak;\n-\t\tif (debug_fd)\n-\t\t\twrite_in_full(debug_fd, line, len);\n \n \t\tif (!prefixcmp(line, \"shallow \")) {\n \t\t\tunsigned char sha1[20];\n@@ -653,8 +648,6 @@ static void receive_needs(void)\n \t\t\tadd_object_array(o, NULL, &want_obj);\n \t\t}\n \t}\n-\tif (debug_fd)\n-\t\twrite_str_in_full(debug_fd, \"#E\\n\");\n \n \t/*\n \t * We have sent all our refs already, and the other end\n@@ -845,8 +838,6 @@ int main(int argc, char **argv)\n \tif (is_repository_shallow())\n \t\tdie(\"attempt to fetch/clone from a shallow repository\");\n \tgit_config(upload_pack_config, NULL);\n-\tif (getenv(\"GIT_DEBUG_SEND_PACK\"))\n-\t\tdebug_fd = atoi(getenv(\"GIT_DEBUG_SEND_PACK\"));\n \tupload_pack();\n \treturn 0;\n }\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209921","messageId":"20130220200028.GD25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 04/19] fetch-pack: fix out-of-bounds buffer offset in get_ack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:00:28Z","receivedAt":"2013-02-20T20:00:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we read acks from the remote, we expect either:\n\n ACK \n\nor\n\n ACK \n\nWe parse the \"ACK \" bit from the line, and then start\nlooking for the flag strings at \"line+45\"; if we don't have\nthem, we assume it's of the first type. But if we do have\nthe first type, then line+45 is not necessarily inside our\nstring at all!\n\nIt turns out that this works most of the time due to the way\nwe parse the packets. They should come in with a newline,\nand packet_read puts an extra NUL into the buffer, so we end\nup with:\n\n ACK \\n\\0\n\nwith the newline at offset 44 and the NUL at offset 45. We\nthen strip the newline, putting a NUL at offset 44. So\nwhen we look at \"line+45\", we are looking past the end of\nour string; but it's OK, because we hit the terminator from\nthe original string.\n\nThis breaks down, however, if the other side does not\nterminate their packets with a newline. In that case, our\npacket is one character shorter, and we start looking\nthrough uninitialized memory for the flag. No known\nimplementation sends such a packet, so it has never come up\nin practice.\n\nThis patch tightens the check by looking for a short,\nflagless ACK before trying to parse the flag.\n\nSigned-off-by: Jeff King \n---\nThis is the absolute minimal fix, which just checks for the no-flag case\nearly; we still treat arbitrary crud in the flag field as just an ACK.\nFrom my understanding of the protocol, a saner parsing scheme would be:\n\n const char *flag = line + 44; /* we already parsed \"ACK \" */\n if (!*flag)\n return ACK;\n if (!strcmp(flag, \" continue\"))\n return ACK_continue;\n if (!strcmp(flag, \" common\"))\n return ACK_continue;\n if (!strcmp(flag, \" ready\"))\n return ACK_ready;\n die(\"fetch-pack expected multi-ack flag, got: %s\", line);\n\nBut that is much tighter, and I wasn't sure if the looseness was there\nto facilitate future expansion or something (though I'd think we would\nneed a new capability for that).\n\n fetch-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 6d8926a..27a3e80 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -226,6 +226,8 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n \t\treturn NAK;\n \tif (!prefixcmp(line, \"ACK \")) {\n \t\tif (!get_sha1_hex(line+4, result_sha1)) {\n+\t\t\tif (len < 45)\n+\t\t\t\treturn ACK;\n \t\t\tif (strstr(line+45, \"continue\"))\n \t\t\t\treturn ACK_continue;\n \t\t\tif (strstr(line+45, \"common\"))\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209922","messageId":"20130220200043.GE25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 05/19] send-pack: prefer prefixcmp over memcmp in receive_status","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:00:43Z","receivedAt":"2013-02-20T20:00:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This code predates prefixcmp, so it used memcmp along with\nstatic sizes. Replacing these memcmps with prefixcmp makes\nthe code much more readable, and the lack of static sizes\nwill make refactoring it in future patches simpler.\n\nNote that we used to be unnecessarily liberal in parsing the\n\"unpack\" status line, and would accept \"unpack ok\\njunk\". No\nversion of git has ever produced that, and it violates the\nBNF in Documentation/technical/pack-protocol.txt. Let's take\nthis opportunity to tighten the check by converting the\nprefix comparison into a strcmp.\n\nWhile we're in the area, let's also fix a vague error\nmessage that does not follow our usual conventions (it\nwrites directly to stderr and does not use the \"error:\"\nprefix).\n\nSigned-off-by: Jeff King \n---\n send-pack.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 97ab336..e91cbe2 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -109,9 +109,9 @@ static int receive_status(int in, struct ref *refs)\n \tchar line[1000];\n \tint ret = 0;\n \tint len = packet_read_line(in, line, sizeof(line));\n-\tif (len < 10 || memcmp(line, \"unpack \", 7))\n+\tif (prefixcmp(line, \"unpack \"))\n \t\treturn error(\"did not receive remote status\");\n-\tif (memcmp(line, \"unpack ok\\n\", 10)) {\n+\tif (strcmp(line, \"unpack ok\\n\")) {\n \t\tchar *p = line + strlen(line) - 1;\n \t\tif (*p == '\\n')\n \t\t\t*p = '\\0';\n@@ -125,9 +125,8 @@ static int receive_status(int in, struct ref *refs)\n \t\tlen = packet_read_line(in, line, sizeof(line));\n \t\tif (!len)\n \t\t\tbreak;\n-\t\tif (len < 3 ||\n-\t\t (memcmp(line, \"ok \", 3) && memcmp(line, \"ng \", 3))) {\n-\t\t\tfprintf(stderr, \"protocol error: %s\\n\", line);\n+\t\tif (prefixcmp(line, \"ok \") && prefixcmp(line, \"ng \")) {\n+\t\t\terror(\"invalid ref status from remote: %s\", line);\n \t\t\tret = -1;\n \t\t\tbreak;\n \t\t}\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209923","messageId":"20130220200059.GF25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 06/19] upload-archive: do not copy repo name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:00:59Z","receivedAt":"2013-02-20T20:00:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"According to the comment, enter_repo will modify its input.\nHowever, this has not been the case since 1c64b48\n(enter_repo: do not modify input, 2011-10-04). Drop the\nnow-useless copy.\n\nSigned-off-by: Jeff King \n---\n builtin/upload-archive.c | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex b928beb..c3d134e 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -27,13 +27,8 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \tif (argc != 2)\n \t\tusage(upload_archive_usage);\n \n-\tif (strlen(argv[1]) + 1 > sizeof(buf))\n-\t\tdie(\"insanely long repository name\");\n-\n-\tstrcpy(buf, argv[1]); /* enter-repo smudges its argument */\n-\n-\tif (!enter_repo(buf, 0))\n-\t\tdie(\"'%s' does not appear to be a git repository\", buf);\n+\tif (!enter_repo(argv[1], 0))\n+\t\tdie(\"'%s' does not appear to be a git repository\", argv[1]);\n \n \t/* put received options in sent_argv[] */\n \tsent_argc = 1;\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209924","messageId":"20130220200126.GG25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 07/19] upload-archive: use argv_array to store client arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:01:26Z","receivedAt":"2013-02-20T20:01:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The current parsing scheme for upload-archive is to pack\narguments into a fixed-size buffer, separated by NULs, and\nput a pointer to each argument in the buffer into a\nfixed-size argv array.\n\nThis works fine, and the limits are high enough that nobody\nreasonable is going to hit them, but it makes the code hard\nto follow. Instead, let's just stuff the arguments into an\nargv_array, which is much simpler. That lifts the \"all\narguments must fit inside 4K together\" limit.\n\nWe could also trivially lift the MAX_ARGS limitation (in\nfact, we have to keep extra code to enforce it). But that\nwould mean a client could force us to allocate an arbitrary\namount of memory simply by sending us \"argument\" lines. By\nlimiting the MAX_ARGS, we limit an attacker to about 4\nmegabytes (64 times a maximum 64K packet buffer). That may\nsound like a lot compared to the 4K limit, but it's not a\nbig deal compared to what git-archive will actually allocate\nwhile working (e.g., to load blobs into memory). The\nimportant thing is that it is bounded.\n\nSigned-off-by: Jeff King \n---\n builtin/upload-archive.c | 35 ++++++++++++++---------------------\n 1 file changed, 14 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex c3d134e..3393cef 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -7,6 +7,7 @@\n #include \"pkt-line.h\"\n #include \"sideband.h\"\n #include \"run-command.h\"\n+#include \"argv-array.h\"\n \n static const char upload_archive_usage[] =\n \t\"git upload-archive \";\n@@ -18,10 +19,9 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \n int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n {\n-\tconst char *sent_argv[MAX_ARGS];\n+\tstruct argv_array sent_argv = ARGV_ARRAY_INIT;\n \tconst char *arg_cmd = \"argument \";\n-\tchar *p, buf[4096];\n-\tint sent_argc;\n+\tchar buf[4096];\n \tint len;\n \n \tif (argc != 2)\n@@ -31,33 +31,26 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \t\tdie(\"'%s' does not appear to be a git repository\", argv[1]);\n \n \t/* put received options in sent_argv[] */\n-\tsent_argc = 1;\n-\tsent_argv[0] = \"git-upload-archive\";\n-\tfor (p = buf;;) {\n+\targv_array_push(&sent_argv, \"git-upload-archive\");\n+\tfor (;;) {\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_line(0, buf, sizeof(buf));\n \t\tif (len == 0)\n \t\t\tbreak;\t/* got a flush */\n-\t\tif (sent_argc > MAX_ARGS - 2)\n-\t\t\tdie(\"Too many options (>%d)\", MAX_ARGS - 2);\n+\t\tif (sent_argv.argc > MAX_ARGS)\n+\t\t die(\"Too many options (>%d)\", MAX_ARGS - 1);\n \n-\t\tif (p[len-1] == '\\n') {\n-\t\t\tp[--len] = 0;\n+\t\tif (buf[len-1] == '\\n') {\n+\t\t\tbuf[--len] = 0;\n \t\t}\n-\t\tif (len < strlen(arg_cmd) ||\n-\t\t strncmp(arg_cmd, p, strlen(arg_cmd)))\n-\t\t\tdie(\"'argument' token or flush expected\");\n \n-\t\tlen -= strlen(arg_cmd);\n-\t\tmemmove(p, p + strlen(arg_cmd), len);\n-\t\tsent_argv[sent_argc++] = p;\n-\t\tp += len;\n-\t\t*p++ = 0;\n+\t\tif (prefixcmp(buf, arg_cmd))\n+\t\t\tdie(\"'argument' token or flush expected\");\n+\t\targv_array_push(&sent_argv, buf + strlen(arg_cmd));\n \t}\n-\tsent_argv[sent_argc] = NULL;\n \n \t/* parse all options sent by the client */\n-\treturn write_archive(sent_argc, sent_argv, prefix, 0, NULL, 1);\n+\treturn write_archive(sent_argv.argc, sent_argv.argv, prefix, 0, NULL, 1);\n }\n \n __attribute__((format (printf, 1, 2)))\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209925","messageId":"20130220200136.GH25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:01:36Z","receivedAt":"2013-02-20T20:01:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The write_or_die function will always die on an error,\nincluding EPIPE. However, it currently treats EPIPE\nspecially by suppressing any error message, and by exiting\nwith exit code 0.\n\nSuppressing the error message makes some sense; a pipe death\nmay just be a sign that the other side is not interested in\nwhat we have to say. However, exiting with a successful\nerror code is not a good idea, as write_or_die is frequently\nused in cases where we want to be careful about having\nwritten all of the output, and we may need to signal to our\ncaller that we have done so (e.g., you would not want a push\nwhose other end has hung up to report success).\n\nThis distinction doesn't typically matter in git, because we\ndo not ignore SIGPIPE in the first place. Which means that\nwe will not get EPIPE, but instead will just die when we get\na SIGPIPE. But it's possible for a default handler to be set\nby a parent process, or for us to add a callsite inside one\nof our few SIGPIPE-ignoring blocks of code.\n\nThis patch converts write_or_die to actually raise SIGPIPE\nwhen we see EPIPE, rather than exiting with zero. This\nbrings the behavior in line with the \"normal\" case that we\ndie from SIGPIPE (and any callers who want to check why we\ndied will see the same thing). We also give the same\ntreatment to other related functions, including\nwrite_or_whine_pipe and maybe_flush_or_die.\n\nSigned-off-by: Jeff King \n---\n write_or_die.c | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/write_or_die.c b/write_or_die.c\nindex 960f448..b50f99a 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -1,5 +1,15 @@\n #include \"cache.h\"\n \n+static void check_pipe(int err)\n+{\n+\tif (err == EPIPE) {\n+\t\tsignal(SIGPIPE, SIG_DFL);\n+\t\traise(SIGPIPE);\n+\t\t/* Should never happen, but just in case... */\n+\t\texit(141);\n+\t}\n+}\n+\n /*\n * Some cases use stdio, but want to flush after the write\n * to get error handling (and to get better interactive\n@@ -34,8 +44,7 @@ void maybe_flush_or_die(FILE *f, const char *desc)\n \t\t\treturn;\n \t}\n \tif (fflush(f)) {\n-\t\tif (errno == EPIPE)\n-\t\t\texit(0);\n+\t\tcheck_pipe(errno);\n \t\tdie_errno(\"write failure on '%s'\", desc);\n \t}\n }\n@@ -50,8 +59,7 @@ void write_or_die(int fd, const void *buf, size_t count)\n void write_or_die(int fd, const void *buf, size_t count)\n {\n \tif (write_in_full(fd, buf, count) < 0) {\n-\t\tif (errno == EPIPE)\n-\t\t\texit(0);\n+\t\tcheck_pipe(errno);\n \t\tdie_errno(\"write error\");\n \t}\n }\n@@ -59,8 +67,7 @@ int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)\n int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)\n {\n \tif (write_in_full(fd, buf, count) < 0) {\n-\t\tif (errno == EPIPE)\n-\t\t\texit(0);\n+\t\tcheck_pipe(errno);\n \t\tfprintf(stderr, \"%s: write error (%s)\\n\",\n \t\t\tmsg, strerror(errno));\n \t\treturn 0;\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209926","messageId":"20130220200146.GI25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 09/19] pkt-line: move a misplaced comment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:01:46Z","receivedAt":"2013-02-20T20:01:46Z","isPatch":true,"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---\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.2.rc0.9.g352092c\n"},{"id":"209927","messageId":"20130220200156.GJ25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 10/19] pkt-line: drop safe_write function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:01:56Z","receivedAt":"2013-02-20T20:01:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is just write_or_die by another name. The one\ndistinction is that write_or_die will treat EPIPE specially\nby suppressing error messages. That's fine, as we die by\nSIGPIPE anyway (and in the off chance that it is disabled,\nwrite_or_die will simulate it).\n\nSigned-off-by: Jeff King \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 pkt-line.h | 1 -\n remote-curl.c | 4 ++--\n send-pack.c | 2 +-\n sideband.c | 9 +++++----\n upload-pack.c | 3 ++-\n 10 files changed, 19 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 62ba6e7..9129563 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -932,7 +932,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 27a3e80..b53a18f 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -247,7 +247,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/pkt-line.h b/pkt-line.h\nindex 7a67e9c..3b6c19c 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -27,6 +27,5 @@ int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);\n int packet_read_line(int fd, char *buffer, unsigned size);\n int packet_read(int fd, char *buffer, unsigned size);\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 #endif\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 e91cbe2..bde796b 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -280,7 +280,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 63cea91..c2b2c61 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.2.rc0.9.g352092c\n"},{"id":"209928","messageId":"20130220200210.GK25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 11/19] pkt-line: provide a generic reading function with options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:02:10Z","receivedAt":"2013-02-20T20:02:10Z","isPatch":true,"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, packet_read, that returns an error instead of dying\nupon reading a truncated input stream. However, it is not\nclear from the names which should be called, or what the\ndifference is.\n\nLet's instead make packet_read be a generic public interface\nthat can take option flags, and update the single callsite\nthat uses it. This is less code, more clear, and paves the\nway for introducing more options into the generic interface\nlater. The function signature is changed, so there should be\nno hidden conflicts with topics in flight.\n\nWhile we're at it, we'll document how error conditions are\nhandled based on the options, and rename the confusing\n\"return_line_fail\" option to \"gentle_on_eof\". While we are\ncleaning up the names, we can drop the \"return_line_fail\"\nchecks in packet_read_internal entirely. They look like\nthis:\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 return_line_fail was true\nin the first place.\n\nSigned-off-by: Jeff King \n---\n connect.c | 3 ++-\n pkt-line.c | 21 ++++++++-------------\n pkt-line.h | 27 ++++++++++++++++++++++++++-\n 3 files changed, 36 insertions(+), 15 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 49e56ba..0aa202f 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -76,7 +76,8 @@ 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(in, buffer, sizeof(buffer),\n+\t\t\t\t PACKET_READ_GENTLE_ON_EOF);\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..8700cf8 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 options)\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 (options & PACKET_READ_GENTLE_ON_EOF)\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+int packet_read(int fd, char *buffer, unsigned size, int options)\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, options);\n+\tif (ret < 0)\n \t\treturn ret;\n \tlen = packet_length(linelen);\n \tif (len < 0)\n@@ -161,22 +161,17 @@ int packet_read_line(int fd, char *buffer, unsigned size)\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, options);\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-{\n-\treturn packet_read_internal(fd, buffer, size, 1);\n-}\n-\n int packet_read_line(int fd, char *buffer, unsigned size)\n {\n-\treturn packet_read_internal(fd, buffer, size, 0);\n+\treturn packet_read(fd, buffer, size, 0);\n }\n \n int packet_get_line(struct strbuf *out,\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 3b6c19c..8cd326c 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -24,8 +24,33 @@ 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, which must be at\n+ * least size bytes long. The return value specifies the number of bytes read\n+ * into the buffer.\n+ *\n+ * If options does not contain PACKET_READ_GENTLE_ON_EOF, we will die under any\n+ * 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+ * If options does contain PACKET_READ_GENTLE_ON_EOF, we will not die on\n+ * condition 4 (truncated input), but instead return -1. However, we will still\n+ * die for the other 3 conditions.\n+ */\n+#define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n+int packet_read(int fd, char *buffer, unsigned size, int options);\n+\n+/* Historical convenience wrapper for packet_read that sets no options */\n int packet_read_line(int fd, char *buffer, unsigned size);\n-int packet_read(int fd, char *buffer, unsigned size);\n+\n int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);\n \n #endif\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209929","messageId":"20130220200228.GL25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 12/19] pkt-line: teach packet_read_line to chomp newlines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:02:28Z","receivedAt":"2013-02-20T20:02:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The packets sent during ref negotiation are all terminated\nby newline; even though the code to chomp these newlines is\nshort, we end up doing it in a lot of places.\n\nThis patch teaches packet_read_line to auto-chomp the\ntrailing newline; this lets us get rid of a lot of inline\nchomping code.\n\nAs a result, some call-sites which are not reading\nline-oriented data (e.g., when reading chunks of packfiles\nalongside sideband) transition away from packet_read_line to\nthe generic packet_read interface. This patch converts all\nof the existing callsites.\n\nSince the function signature of packet_read_line does not\nchange (but its behavior does), there is a possibility of\nnew callsites being introduced in later commits, silently\nintroducing an incompatibility. However, since a later\npatch in this series will change the signature, such a\ncommit would have to be merged directly into this commit,\nnot to the tip of the series; we can therefore ignore the\nissue.\n\nThis is an internal cleanup and should produce no change of\nbehavior in the normal case. However, there is one corner\ncase to note. Callers of packet_read_line have never been\nable to tell the difference between a flush packet (\"0000\")\nand an empty packet (\"0004\"), as both cause packet_read_line\nto return a length of 0. Readers treat them identically,\neven though Documentation/technical/protocol-common.txt says\nwe must not; it also says that implementations should not\nsend an empty pkt-line.\n\nBy stripping out the newline before the result gets to the\ncaller, we will now treat the newline-only packet (\"0005\\n\")\nthe same as an empty packet, which in turn gets treated like\na flush packet. In practice this doesn't matter, as neither\nempty nor newline-only packets are part of git's protocols\n(at least not for the line-oriented bits, and readers who\nare not expecting line-oriented packets will be calling\npacket_read directly, anyway). But even if we do decide to\ncare about the distinction later, it is orthogonal to this\npatch. The right place to tighten would be to stop treating\nempty packets as flush packets, and this change does not\nmake doing so any harder.\n\nSigned-off-by: Jeff King \n---\n builtin/archive.c | 2 --\n builtin/fetch-pack.c | 2 --\n builtin/receive-pack.c | 2 --\n builtin/upload-archive.c | 4 ----\n connect.c | 5 ++---\n daemon.c | 2 +-\n fetch-pack.c | 2 --\n pkt-line.c | 7 ++++++-\n pkt-line.h | 9 ++++++++-\n remote-curl.c | 6 +++---\n send-pack.c | 6 +-----\n sideband.c | 2 +-\n upload-pack.c | 8 --------\n 13 files changed, 22 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 9a1cfd3..d381ac4 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -56,8 +56,6 @@ static int run_remote_archiver(int argc, const char **argv,\n \tlen = packet_read_line(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-\t\tbuf[--len] = 0;\n \tif (strcmp(buf, \"ACK\")) {\n \t\tif (len > 5 && !prefixcmp(buf, \"NACK \"))\n \t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 940ae35..f73664f 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -105,8 +105,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tint n = packet_read_line(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')\n-\t\t\t\t\tn--;\n \t\t\t\tstring_list_append(&sought, xmemdupz(line, n));\n \t\t\t}\n \t\t}\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9129563..6679e63 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -763,8 +763,6 @@ static struct command *read_head_info(void)\n \t\tlen = packet_read_line(0, line, sizeof(line));\n \t\tif (!len)\n \t\t\tbreak;\n-\t\tif (line[len-1] == '\\n')\n-\t\t\tline[--len] = 0;\n \t\tif (len < 83 ||\n \t\t line[40] != ' ' ||\n \t\t line[81] != ' ' ||\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 3393cef..7d367b5 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -40,10 +40,6 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \t\tif (sent_argv.argc > MAX_ARGS)\n \t\t die(\"Too many options (>%d)\", MAX_ARGS - 1);\n \n-\t\tif (buf[len-1] == '\\n') {\n-\t\t\tbuf[--len] = 0;\n-\t\t}\n-\n \t\tif (prefixcmp(buf, arg_cmd))\n \t\t\tdie(\"'argument' token or flush expected\");\n \t\targv_array_push(&sent_argv, buf + strlen(arg_cmd));\ndiff --git a/connect.c b/connect.c\nindex 0aa202f..fe8eb01 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -77,14 +77,13 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \t\tint len, name_len;\n \n \t\tlen = packet_read(in, buffer, sizeof(buffer),\n-\t\t\t\t PACKET_READ_GENTLE_ON_EOF);\n+\t\t\t\t PACKET_READ_GENTLE_ON_EOF |\n+\t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n \t\tif (len < 0)\n \t\t\tdie_initial_contact(got_at_least_one_head);\n \n \t\tif (!len)\n \t\t\tbreak;\n-\t\tif (buffer[len-1] == '\\n')\n-\t\t\tbuffer[--len] = 0;\n \n \t\tif (len > 4 && !prefixcmp(buffer, \"ERR \"))\n \t\t\tdie(\"remote error: %s\", buffer + 4);\ndiff --git a/daemon.c b/daemon.c\nindex 4602b46..4f5cd61 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), 0);\n \talarm(0);\n \n \tlen = strlen(line);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b53a18f..f830db2 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -220,8 +220,6 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n \n \tif (!len)\n \t\tdie(\"git fetch-pack: expected ACK/NAK, got EOF\");\n-\tif (line[len-1] == '\\n')\n-\t\tline[--len] = 0;\n \tif (!strcmp(line, \"NAK\"))\n \t\treturn NAK;\n \tif (!prefixcmp(line, \"ACK \")) {\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 8700cf8..dc11c40 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -164,6 +164,11 @@ int packet_read(int fd, char *buffer, unsigned size, int options)\n \tret = safe_read(fd, buffer, len, options);\n \tif (ret < 0)\n \t\treturn ret;\n+\n+\tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n+\t len && buffer[len-1] == '\\n')\n+\t\tlen--;\n+\n \tbuffer[len] = 0;\n \tpacket_trace(buffer, len, 0);\n \treturn len;\n@@ -171,7 +176,7 @@ int packet_read_line(int fd, char *buffer, unsigned size)\n \n int packet_read_line(int fd, char *buffer, unsigned size)\n {\n-\treturn packet_read(fd, buffer, size, 0);\n+\treturn packet_read(fd, buffer, size, PACKET_READ_CHOMP_NEWLINE);\n }\n \n int packet_get_line(struct strbuf *out,\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 8cd326c..5d2fb42 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -44,11 +44,18 @@ int packet_read(int fd, char *buffer, unsigned size, int options);\n * If options does contain PACKET_READ_GENTLE_ON_EOF, we will not die on\n * condition 4 (truncated input), but instead return -1. However, we will still\n * die for the other 3 conditions.\n+ *\n+ * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if\n+ * present) is removed from the buffer before returning.\n */\n #define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n+#define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n int packet_read(int fd, char *buffer, unsigned size, int options);\n \n-/* Historical convenience wrapper for packet_read that sets no options */\n+/*\n+ * Convenience wrapper for packet_read that is not gentle, and sets the\n+ * CHOMP_NEWLINE option.\n+ */\n int packet_read_line(int fd, char *buffer, unsigned size);\n \n int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 7be4b53..b28f965 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, 0);\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, 0);\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, 0);\n \t\tif (!n)\n \t\t\tbreak;\n \t\trpc->pos = 0;\ndiff --git a/send-pack.c b/send-pack.c\nindex bde796b..8c230bf 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -111,10 +111,7 @@ static int receive_status(int in, struct ref *refs)\n \tint len = packet_read_line(in, line, sizeof(line));\n \tif (prefixcmp(line, \"unpack \"))\n \t\treturn error(\"did not receive remote status\");\n-\tif (strcmp(line, \"unpack ok\\n\")) {\n-\t\tchar *p = line + strlen(line) - 1;\n-\t\tif (*p == '\\n')\n-\t\t\t*p = '\\0';\n+\tif (strcmp(line, \"unpack ok\")) {\n \t\terror(\"unpack failed: %s\", line + 7);\n \t\tret = -1;\n \t}\n@@ -131,7 +128,6 @@ static int receive_status(int in, struct ref *refs)\n \t\t\tbreak;\n \t\t}\n \n-\t\tline[strlen(line)-1] = '\\0';\n \t\trefname = line + 3;\n \t\tmsg = strchr(refname, ' ');\n \t\tif (msg)\ndiff --git a/sideband.c b/sideband.c\nindex 8f7b25b..15cc1ae 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, 0);\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 c2b2c61..7446cb7 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -50,13 +50,6 @@ static void reset_timeout(void)\n \talarm(timeout);\n }\n \n-static int strip(char *line, int len)\n-{\n-\tif (len && line[len-1] == '\\n')\n-\t\tline[--len] = 0;\n-\treturn len;\n-}\n-\n static ssize_t send_client_data(int fd, const char *data, ssize_t sz)\n {\n \tif (use_sideband)\n@@ -447,7 +440,6 @@ static int get_common_commits(void)\n \t\t\tgot_other = 0;\n \t\t\tcontinue;\n \t\t}\n-\t\tstrip(line, len);\n \t\tif (!prefixcmp(line, \"have \")) {\n \t\t\tswitch (got_sha1(line+5, sha1)) {\n \t\t\tcase -1: /* they have what we do not */\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209930","messageId":"20130220200245.GM25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 13/19] pkt-line: move LARGE_PACKET_MAX definition from sideband","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:02:45Z","receivedAt":"2013-02-20T20:02:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Having the packet sizes defined near the packet read/write\nfunctions makes more sense.\n\nSigned-off-by: Jeff King \n---\n http.c | 1 +\n pkt-line.h | 3 +++\n sideband.h | 3 ---\n 3 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex d9d1aad..8803c70 100644\n--- a/http.c\n+++ b/http.c\n@@ -5,6 +5,7 @@\n #include \"url.h\"\n #include \"credential.h\"\n #include \"version.h\"\n+#include \"pkt-line.h\"\n \n int active_requests;\n int http_is_verbose;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 5d2fb42..6927ea5 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -58,6 +58,9 @@ int packet_read_line(int fd, char *buffer, unsigned size);\n */\n int packet_read_line(int fd, char *buffer, unsigned size);\n \n+#define DEFAULT_PACKET_MAX 1000\n+#define LARGE_PACKET_MAX 65520\n+\n int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);\n \n #endif\ndiff --git a/sideband.h b/sideband.h\nindex d72db35..e46bed0 100644\n--- a/sideband.h\n+++ b/sideband.h\n@@ -4,9 +4,6 @@\n #define SIDEBAND_PROTOCOL_ERROR -2\n #define SIDEBAND_REMOTE_ERROR -1\n \n-#define DEFAULT_PACKET_MAX 1000\n-#define LARGE_PACKET_MAX 65520\n-\n int recv_sideband(const char *me, int in_stream, int out);\n ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);\n \n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209931","messageId":"20130220200257.GN25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 14/19] pkt-line: provide a LARGE_PACKET_MAX static buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:02:57Z","receivedAt":"2013-02-20T20:02:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most of the callers of packet_read_line just read into a\nstatic 1000-byte buffer (callers which handle arbitrary\nbinary data already use LARGE_PACKET_MAX). This works fine\nin practice, because:\n\n 1. The only variable-sized data in these lines is a ref\n name, and refs tend to be a lot shorter than 1000\n characters.\n\n 2. When sending ref lines, git-core always limits itself\n to 1000 byte packets.\n\nHowever, the only limit given in the protocol specification\nin Documentation/technical/protocol-common.txt is\nLARGE_PACKET_MAX; the 1000 byte limit is mentioned only in\npack-protocol.txt, and then only describing what we write,\nnot as a specific limit for readers.\n\nThis patch lets us bump the 1000-byte limit to\nLARGE_PACKET_MAX. Even though git-core will never write a\npacket where this makes a difference, there are two good\nreasons to do this:\n\n 1. Other git implementations may have followed\n protocol-common.txt and used a larger maximum size. We\n don't bump into it in practice because it would involve\n very long ref names.\n\n 2. We may want to increase the 1000-byte limit one day.\n Since packets are transferred before any capabilities,\n it's difficult to do this in a backwards-compatible\n way. But if we bump the size of buffer the readers can\n handle, eventually older versions of git will be\n obsolete enough that we can justify bumping the\n writers, as well. We don't have plans to do this\n anytime soon, but there is no reason not to start the\n clock ticking now.\n\nJust bumping all of the reading bufs to LARGE_PACKET_MAX\nwould waste memory. Instead, since most readers just read\ninto a temporary buffer anyway, let's provide a single\nstatic buffer that all callers can use. We can further wrap\nthis detail away by having the packet_read_line wrapper just\nuse the buffer transparently and return a pointer to the\nstatic storage. That covers most of the cases, and the\nremaining ones already read into their own LARGE_PACKET_MAX\nbuffers.\n\nSigned-off-by: Jeff King \n---\n builtin/archive.c | 15 +++++++--------\n builtin/fetch-pack.c | 7 +++----\n builtin/receive-pack.c | 6 +++---\n builtin/upload-archive.c | 7 ++-----\n connect.c | 4 ++--\n daemon.c | 4 ++--\n fetch-pack.c | 12 ++++++------\n pkt-line.c | 9 +++++++--\n pkt-line.h | 9 +++++++--\n send-pack.c | 7 +++----\n upload-pack.c | 12 +++++-------\n 11 files changed, 47 insertions(+), 45 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex d381ac4..49178f1 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -27,8 +27,8 @@ static int run_remote_archiver(int argc, const char **argv,\n \t\t\t const char *remote, const char *exec,\n \t\t\t const char *name_hint)\n {\n-\tchar buf[LARGE_PACKET_MAX];\n-\tint fd[2], i, len, rv;\n+\tchar *buf;\n+\tint fd[2], i, rv;\n \tstruct transport *transport;\n \tstruct remote *_remote;\n \n@@ -53,19 +53,18 @@ 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-\tif (!len)\n+\tbuf = packet_read_line(fd[0], NULL);\n+\tif (!buf)\n \t\tdie(_(\"git archive: expected ACK/NAK, got EOF\"));\n \tif (strcmp(buf, \"ACK\")) {\n-\t\tif (len > 5 && !prefixcmp(buf, \"NACK \"))\n+\t\tif (!prefixcmp(buf, \"NACK \"))\n \t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n-\t\tif (len > 4 && !prefixcmp(buf, \"ERR \"))\n+\t\tif (!prefixcmp(buf, \"ERR \"))\n \t\t\tdie(_(\"remote error: %s\"), buf + 4);\n \t\tdie(_(\"git archive: protocol error\"));\n \t}\n \n-\tlen = packet_read_line(fd[0], buf, sizeof(buf));\n-\tif (len)\n+\tif (packet_read_line(fd[0], NULL))\n \t\tdie(_(\"git archive: expected a flush\"));\n \n \t/* Now, start reading from fd[0] and spit it out to stdout */\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex f73664f..c21cc2c 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -100,12 +100,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t/* in stateless RPC mode we use pkt-line to read\n \t\t\t * from stdin, until we get a flush packet\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\tif (!n)\n+\t\t\t\tchar *line = packet_read_line(0, NULL);\n+\t\t\t\tif (!line)\n \t\t\t\t\tbreak;\n-\t\t\t\tstring_list_append(&sought, xmemdupz(line, n));\n+\t\t\t\tstring_list_append(&sought, xstrdup(line));\n \t\t\t}\n \t\t}\n \t\telse {\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 6679e63..ccebd74 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -754,14 +754,14 @@ static struct command *read_head_info(void)\n \tstruct command *commands = NULL;\n \tstruct command **p = &commands;\n \tfor (;;) {\n-\t\tstatic char line[1000];\n+\t\tchar *line;\n \t\tunsigned char old_sha1[20], new_sha1[20];\n \t\tstruct command *cmd;\n \t\tchar *refname;\n \t\tint len, reflen;\n \n-\t\tlen = packet_read_line(0, line, sizeof(line));\n-\t\tif (!len)\n+\t\tline = packet_read_line(0, &len);\n+\t\tif (!line)\n \t\t\tbreak;\n \t\tif (len < 83 ||\n \t\t line[40] != ' ' ||\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 7d367b5..2a94675 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -21,8 +21,6 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n {\n \tstruct argv_array sent_argv = ARGV_ARRAY_INIT;\n \tconst char *arg_cmd = \"argument \";\n-\tchar buf[4096];\n-\tint len;\n \n \tif (argc != 2)\n \t\tusage(upload_archive_usage);\n@@ -33,9 +31,8 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \t/* put received options in sent_argv[] */\n \targv_array_push(&sent_argv, \"git-upload-archive\");\n \tfor (;;) {\n-\t\t/* This will die if not enough free space in buf */\n-\t\tlen = packet_read_line(0, buf, sizeof(buf));\n-\t\tif (len == 0)\n+\t\tchar *buf = packet_read_line(0, NULL);\n+\t\tif (!buf)\n \t\t\tbreak;\t/* got a flush */\n \t\tif (sent_argv.argc > MAX_ARGS)\n \t\t die(\"Too many options (>%d)\", MAX_ARGS - 1);\ndiff --git a/connect.c b/connect.c\nindex fe8eb01..611ffb4 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -72,11 +72,11 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \tfor (;;) {\n \t\tstruct ref *ref;\n \t\tunsigned char old_sha1[20];\n-\t\tstatic char buffer[1000];\n \t\tchar *name;\n \t\tint len, name_len;\n+\t\tchar *buffer = packet_buffer;\n \n-\t\tlen = packet_read(in, buffer, sizeof(buffer),\n+\t\tlen = packet_read(in, packet_buffer, sizeof(packet_buffer),\n \t\t\t\t PACKET_READ_GENTLE_ON_EOF |\n \t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n \t\tif (len < 0)\ndiff --git a/daemon.c b/daemon.c\nindex 4f5cd61..3f70e79 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -604,7 +604,7 @@ static int execute(void)\n \n static int execute(void)\n {\n-\tstatic char line[1000];\n+\tchar *line = packet_buffer;\n \tint pktlen, len, i;\n \tchar *addr = getenv(\"REMOTE_ADDR\"), *port = getenv(\"REMOTE_PORT\");\n \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(0, line, sizeof(line), 0);\n+\tpktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);\n \talarm(0);\n \n \tlen = strlen(line);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f830db2..66ff9ad 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -172,8 +172,8 @@ static void consume_shallow_list(struct fetch_pack_args *args, int fd)\n \t\t * shallow and unshallow commands every time there\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\tchar *line;\n+\t\twhile ((line = packet_read_line(fd, NULL))) {\n \t\t\tif (!prefixcmp(line, \"shallow \"))\n \t\t\t\tcontinue;\n \t\t\tif (!prefixcmp(line, \"unshallow \"))\n@@ -215,8 +215,8 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n \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;\n+\tchar *line = packet_read_line(fd, &len);\n \n \tif (!len)\n \t\tdie(\"git fetch-pack: expected ACK/NAK, got EOF\");\n@@ -346,11 +346,11 @@ static int find_common(struct fetch_pack_args *args,\n \tstate_len = req_buf.len;\n \n \tif (args->depth > 0) {\n-\t\tchar line[1024];\n+\t\tchar *line;\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 ((line = packet_read_line(fd[0], NULL))) {\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 dc11c40..55fb688 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"pkt-line.h\"\n \n+char packet_buffer[LARGE_PACKET_MAX];\n static const char *packet_trace_prefix = \"git\";\n static const char trace_key[] = \"GIT_TRACE_PACKET\";\n \n@@ -174,9 +175,13 @@ int packet_read_line(int fd, char *buffer, unsigned size)\n \treturn len;\n }\n \n-int packet_read_line(int fd, char *buffer, unsigned size)\n+char *packet_read_line(int fd, int *len_p)\n {\n-\treturn packet_read(fd, buffer, size, PACKET_READ_CHOMP_NEWLINE);\n+\tint len = packet_read(fd, packet_buffer, sizeof(packet_buffer),\n+\t\t\t PACKET_READ_CHOMP_NEWLINE);\n+\tif (len_p)\n+\t\t*len_p = len;\n+\treturn len ? packet_buffer : NULL;\n }\n \n int packet_get_line(struct strbuf *out,\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 6927ea5..fa93e32 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -54,12 +54,17 @@ int packet_read_line(int fd, char *buffer, unsigned size);\n \n /*\n * Convenience wrapper for packet_read that is not gentle, and sets the\n- * CHOMP_NEWLINE option.\n+ * CHOMP_NEWLINE option. The return value is NULL for a flush packet,\n+ * and otherwise points to a static buffer (that may be overwritten by\n+ * subsequent calls). If the size parameter is not NULL, the length of the\n+ * packet is written to it.\n */\n-int packet_read_line(int fd, char *buffer, unsigned size);\n+char *packet_read_line(int fd, int *size);\n+\n \n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n+extern char packet_buffer[LARGE_PACKET_MAX];\n \n int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);\n \ndiff --git a/send-pack.c b/send-pack.c\nindex 8c230bf..7d172ef 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -106,9 +106,8 @@ static int receive_status(int in, struct ref *refs)\n static int receive_status(int in, struct ref *refs)\n {\n \tstruct ref *hint;\n-\tchar line[1000];\n \tint ret = 0;\n-\tint len = packet_read_line(in, line, sizeof(line));\n+\tchar *line = packet_read_line(in, NULL);\n \tif (prefixcmp(line, \"unpack \"))\n \t\treturn error(\"did not receive remote status\");\n \tif (strcmp(line, \"unpack ok\")) {\n@@ -119,8 +118,8 @@ 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\tif (!len)\n+\t\tline = packet_read_line(in, NULL);\n+\t\tif (!line)\n \t\t\tbreak;\n \t\tif (prefixcmp(line, \"ok \") && prefixcmp(line, \"ng \")) {\n \t\t\terror(\"invalid ref status from remote: %s\", line);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7446cb7..bc241ba 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -408,7 +408,6 @@ static int get_common_commits(void)\n \n static int get_common_commits(void)\n {\n-\tstatic char line[1000];\n \tunsigned char sha1[20];\n \tchar last_hex[41];\n \tint got_common = 0;\n@@ -418,10 +417,10 @@ 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\tchar *line = packet_read_line(0, NULL);\n \t\treset_timeout();\n \n-\t\tif (!len) {\n+\t\tif (!line) {\n \t\t\tif (multi_ack == 2 && got_common\n \t\t\t && !got_other && ok_to_give_up()) {\n \t\t\t\tsent_ready = 1;\n@@ -567,8 +566,7 @@ static void receive_needs(void)\n static void receive_needs(void)\n {\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n-\tstatic char line[1000];\n-\tint len, depth = 0;\n+\tint depth = 0;\n \tint has_non_tip = 0;\n \n \tshallow_nr = 0;\n@@ -576,9 +574,9 @@ 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\tchar *line = packet_read_line(0, NULL);\n \t\treset_timeout();\n-\t\tif (!len)\n+\t\tif (!line)\n \t\t\tbreak;\n \n \t\tif (!prefixcmp(line, \"shallow \")) {\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209933","messageId":"20130220200430.GO25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 15/19] pkt-line: share buffer/descriptor reading implementation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:04:30Z","receivedAt":"2013-02-20T20:04:30Z","isPatch":true,"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 generic packet_read\nfunction to accept either source, and we can do away with\npacket_get_line's implementation.\n\nThere are two other differences to account for between the\nold and new functions. The first is that we used to read\ninto a strbuf, but now read into a fixed size buffer. The\nonly two callers are fine with that, and in fact it\nsimplifies their code, since they can use the same\nstatic-buffer interface as the rest of the packet_read_line\ncallers (and we provide a similar convenience wrapper for\nreading from a buffer rather than a descriptor).\n\nThis is technically an externally-visible behavior change in\nthat we used to accept arbitrary sized packets up to 65532\nbytes, and now cap out at LARGE_PACKET_MAX, 65520. In\npractice this doesn't matter, as we use it only for parsing\nsmart-http headers (of which there is exactly one defined,\nand it is small and fixed-size). And any extension headers\nwould be breaking the protocol to go over LARGE_PACKET_MAX\nanyway.\n\nThe other difference is that packet_get_line would return\non error rather than dying. However, both callers of\nstrbuf_get_line are actually improved by dying.\n\nThe first caller does its own error checking, but we can\ndrop that; as a result, we'll actually get more specific\nreporting about protocol breakage when packet_read dies\ninternally. The only downside is that packet_read will not\nprint the smart-http URL that failed, but that's not a big\ndeal; anybody not debugging can already see the remote's URL\nalready, and anybody debugging would want to run with\nGIT_CURL_VERBOSE anyway to see way more information.\n\nThe second caller, which is just trying to skip past any\nextra smart-http headers (of which there are none defined,\nbut which we allow to keep room for future expansion), did\nnot error check at all. As a result, it would treat an error\njust like a flush packet. The resulting mess would generally\ncause an error later in get_remote_heads, but now we get\nerror reporting much closer to the source of the problem.\n\nSigned-off-by: Jeff King \n---\nThis adds two options to the generic packet_read interface for which\nmany callers will just pass (NULL, 0). We can hide that behind a\nwrapper, but I was annoyed with the proliferation of wrappers from the\nlast round. Pick your poison.\n\n connect.c | 3 ++-\n daemon.c | 2 +-\n pkt-line.c | 77 ++++++++++++++++++++++++++++++-----------------------------\n pkt-line.h | 23 +++++++++++++-----\n remote-curl.c | 22 ++++++++---------\n sideband.c | 2 +-\n 6 files changed, 70 insertions(+), 59 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 611ffb4..061aa5b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -76,7 +76,8 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \t\tint len, name_len;\n \t\tchar *buffer = packet_buffer;\n \n-\t\tlen = packet_read(in, packet_buffer, sizeof(packet_buffer),\n+\t\tlen = packet_read(in, NULL, 0,\n+\t\t\t\t packet_buffer, sizeof(packet_buffer),\n \t\t\t\t PACKET_READ_GENTLE_ON_EOF |\n \t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n \t\tif (len < 0)\ndiff --git a/daemon.c b/daemon.c\nindex 3f70e79..9a241d9 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(0, packet_buffer, sizeof(packet_buffer), 0);\n+\tpktlen = packet_read(0, NULL, 0, packet_buffer, sizeof(packet_buffer), 0);\n \talarm(0);\n \n \tlen = strlen(line);\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 55fb688..2c47052 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -104,12 +104,29 @@ static int safe_read(int fd, void *buffer, unsigned size, int options)\n \tstrbuf_add(buf, buffer, n);\n }\n \n-static int safe_read(int fd, void *buffer, unsigned size, int options)\n+static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n+\t\t\t void *dst, unsigned size, int options)\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+\tif (fd >= 0 && src_buf && *src_buf)\n+\t\tdie(\"BUG: multiple sources given to packet_read\");\n+\n+\t/* Read up to \"size\" bytes from our source, whatever it is. */\n+\tif (src_buf && *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 (options & PACKET_READ_GENTLE_ON_EOF)\n \t\t\treturn -1;\n \n@@ -144,12 +161,13 @@ int packet_read(int fd, char *buffer, unsigned size, int options)\n \treturn len;\n }\n \n-int packet_read(int fd, char *buffer, unsigned size, int options)\n+int packet_read(int fd, char **src_buf, size_t *src_len,\n+\t\tchar *buffer, unsigned size, int options)\n {\n \tint len, ret;\n \tchar linelen[4];\n \n-\tret = safe_read(fd, linelen, 4, options);\n+\tret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);\n \tif (ret < 0)\n \t\treturn ret;\n \tlen = packet_length(linelen);\n@@ -162,7 +180,7 @@ int packet_read(int fd, char *buffer, unsigned size, int options)\n \tlen -= 4;\n \tif (len >= size)\n \t\tdie(\"protocol error: bad line length %d\", len);\n-\tret = safe_read(fd, buffer, len, options);\n+\tret = get_packet_data(fd, src_buf, src_len, buffer, len, options);\n \tif (ret < 0)\n \t\treturn ret;\n \n@@ -175,41 +193,24 @@ int packet_get_line(struct strbuf *out,\n \treturn len;\n }\n \n-char *packet_read_line(int fd, int *len_p)\n+static char *packet_read_line_generic(int fd,\n+\t\t\t\t char **src, size_t *src_len,\n+\t\t\t\t int *dst_len)\n {\n-\tint len = packet_read(fd, packet_buffer, sizeof(packet_buffer),\n+\tint len = packet_read(fd, src, src_len,\n+\t\t\t packet_buffer, sizeof(packet_buffer),\n \t\t\t PACKET_READ_CHOMP_NEWLINE);\n-\tif (len_p)\n-\t\t*len_p = len;\n+\tif (dst_len)\n+\t\t*dst_len = len;\n \treturn len ? packet_buffer : NULL;\n }\n \n-int packet_get_line(struct strbuf *out,\n-\tchar **src_buf, size_t *src_len)\n+char *packet_read_line(int fd, int *len_p)\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_line_generic(fd, NULL, 0, len_p);\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+char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)\n+{\n+\treturn packet_read_line_generic(-1, src, src_len, dst_len);\n }\ndiff --git a/pkt-line.h b/pkt-line.h\nindex fa93e32..47361f5 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,9 +25,16 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f\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, which must be at\n- * least size bytes long. The return value specifies the number of bytes read\n- * into the buffer.\n+ * Read a packetized line into the buffer, which must be at least size bytes\n+ * long. The return value specifies the number of bytes read into the buffer.\n+ *\n+ * If src_buffer is not NULL (and nor is *src_buffer), it should point to a\n+ * buffer containing the packet data to parse, of at least *src_len bytes.\n+ * After the function returns, src_buf will be increments and src_len\n+ * decremented by the number of bytes consumed.\n+ *\n+ * If src_buffer (or *src_buffer) is NULL, then data is read from the\n+ * descriptor \"fd\".\n *\n * If options does not contain PACKET_READ_GENTLE_ON_EOF, we will die under any\n * of the following conditions:\n@@ -50,7 +57,8 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f\n */\n #define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n-int packet_read(int fd, char *buffer, unsigned size, int options);\n+int packet_read(int fd, char **src_buffer, size_t *src_len, char\n+\t\t*buffer, unsigned size, int options);\n \n /*\n * Convenience wrapper for packet_read that is not gentle, and sets the\n@@ -61,11 +69,14 @@ extern char packet_buffer[LARGE_PACKET_MAX];\n */\n char *packet_read_line(int fd, int *size);\n \n+/*\n+ * Same as packet_read_line, but read from a buf rather than a descriptor;\n+ * see packet_read for details on how src_* is used.\n+ */\n+char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);\n \n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n extern char packet_buffer[LARGE_PACKET_MAX];\n \n-int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);\n-\n #endif\ndiff --git a/remote-curl.c b/remote-curl.c\nindex b28f965..c0edd4c 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -138,28 +138,26 @@ 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;\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\tline = packet_read_line_buf(&last->buf, &last->len, NULL);\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 (strcmp(line, exp.buf))\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_line_buf(&last->buf, &last->len, NULL) > 0)\n+\t\t\t;\n \n \t\tlast->proto_git = 1;\n \t}\n@@ -308,7 +306,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(rpc->out, rpc->buf, rpc->alloc, 0);\n+\t\tavail = packet_read(rpc->out, NULL, 0, rpc->buf, rpc->alloc, 0);\n \t\tif (!avail)\n \t\t\treturn 0;\n \t\trpc->pos = 0;\n@@ -425,7 +423,7 @@ static int post_rpc(struct rpc_state *rpc)\n \t\t\tbreak;\n \t\t}\n \n-\t\tn = packet_read(rpc->out, buf, left, 0);\n+\t\tn = packet_read(rpc->out, 0, NULL, buf, left, 0);\n \t\tif (!n)\n \t\t\tbreak;\n \t\trpc->len += n;\n@@ -579,7 +577,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(rpc->out, rpc->buf, rpc->alloc, 0);\n+\t\tint n = packet_read(rpc->out, 0, NULL, rpc->buf, rpc->alloc, 0);\n \t\tif (!n)\n \t\t\tbreak;\n \t\trpc->pos = 0;\ndiff --git a/sideband.c b/sideband.c\nindex 15cc1ae..857954c 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(in_stream, buf + pf, LARGE_PACKET_MAX, 0);\n+\t\tlen = packet_read(in_stream, NULL, 0, buf + pf, LARGE_PACKET_MAX, 0);\n \t\tif (len == 0)\n \t\t\tbreak;\n \t\tif (len < 1) {\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209934","messageId":"20130220200645.GP25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 16/19] teach get_remote_heads to read from a memory buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:06:45Z","receivedAt":"2013-02-20T20:06:45Z","isPatch":true,"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---\nAnother \"wrapper vs NULL argument\" opportunity. I could go either way if\nwe feel strongly in one direction. A third option is:\n\n struct packet_source {\n /* Choose one. */\n int fd;\n char *buf;\n int len;\n };\n\nbut then each caller has to be bothered to define and fill in the\nstruct, which ends up even uglier.\n\n builtin/fetch-pack.c | 2 +-\n builtin/send-pack.c | 2 +-\n cache.h | 4 +++-\n connect.c | 6 +++---\n remote-curl.c | 2 +-\n transport.c | 6 +++---\n 6 files changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c21cc2c..03ed2ca 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -125,7 +125,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 061aa5b..f57efd0 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,7 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \t\tint len, name_len;\n \t\tchar *buffer = packet_buffer;\n \n-\t\tlen = packet_read(in, NULL, 0,\n+\t\tlen = packet_read(in, &src_buf, &src_len,\n \t\t\t\t packet_buffer, sizeof(packet_buffer),\n \t\t\t\t PACKET_READ_GENTLE_ON_EOF |\n \t\t\t\t PACKET_READ_CHOMP_NEWLINE);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex c0edd4c..3bc6cb5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -192,7 +192,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.2.rc0.9.g352092c\n"},{"id":"209935","messageId":"20130220200702.GQ25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 17/19] remote-curl: pass buffer straight to get_remote_heads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:07:02Z","receivedAt":"2013-02-20T20:07:02Z","isPatch":true,"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---\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 3bc6cb5..e07f654 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -170,33 +170,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.2.rc0.9.g352092c\n"},{"id":"209936","messageId":"20130220200711.GR25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 18/19] remote-curl: move ref-parsing code up in file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:07:11Z","receivedAt":"2013-02-20T20:07:11Z","isPatch":true,"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---\n remote-curl.c | 118 +++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 59 insertions(+), 59 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex e07f654..856decc 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@@ -170,65 +229,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 \tstruct discovery *heads;\n-- \n1.8.2.rc0.9.g352092c\n"},{"id":"209937","messageId":"20130220200719.GS25647@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220195147.GA25332@sigill.intra.peff.net","subject":"[PATCH v3 19/19] remote-curl: always parse incoming refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T20:07:19Z","receivedAt":"2013-02-20T20:07:19Z","isPatch":true,"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---\n remote-curl.c | 22 +++++++++++++---------\n 1 file changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 856decc..3d2b194 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@@ -221,6 +223,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@@ -234,13 +241,11 @@ static struct ref *get_refs(int for_push)\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@@ -254,7 +259,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@@ -670,7 +674,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@@ -789,7 +793,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.2.rc0.9.g352092c\n"},{"id":"209940","messageId":"20130220215043.GA24236@google.com","threadId":"32952","inReplyTo":"20130220200136.GH25647@sigill.intra.peff.net","subject":"Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-20T21:51:11Z","receivedAt":"2013-02-20T21:51:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> The write_or_die function will always die on an error,\n> including EPIPE. However, it currently treats EPIPE\n> specially by suppressing any error message, and by exiting\n> with exit code 0.\n>\n> Suppressing the error message makes some sense; a pipe death\n> may just be a sign that the other side is not interested in\n> what we have to say. However, exiting with a successful\n> error code is not a good idea, as write_or_die is frequently\n> used in cases where we want to be careful about having\n> written all of the output, and we may need to signal to our\n> caller that we have done so (e.g., you would not want a push\n> whose other end has hung up to report success).\n>\n> This distinction doesn't typically matter in git, because we\n> do not ignore SIGPIPE in the first place. Which means that\n> we will not get EPIPE, but instead will just die when we get\n> a SIGPIPE. But it's possible for a default handler to be set\n> by a parent process,\n\nNot so much \"default\" as \"insane inherited\", as in the example\nof old versions of Python's subprocess.Popen.\n\nI suspect this used exit(0) instead of raise(SIGPIPE) in the first\nplace to work around a bash bug (too much verbosity about SIGPIPE).\nIf any programs still have that kind of bug, I'd rather put pressure\non them to fix it by *not* working around it. So the basic idea here\nlooks good to me.\n\n[...]\n> --- a/write_or_die.c\n> +++ b/write_or_die.c\n> @@ -1,5 +1,15 @@\n> #include \"cache.h\"\n> \n> +static void check_pipe(int err)\n> +{\n> +\tif (err == EPIPE) {\n> +\t\tsignal(SIGPIPE, SIG_DFL);\n> +\t\traise(SIGPIPE);\n> +\t\t/* Should never happen, but just in case... */\n> +\t\texit(141);\n\nHow about\n\n\t\tdie(\"BUG: another thread changed SIGPIPE handling behind my back!\");\n\nto make it easier to find and fix such problems?\n\nThanks,\nJonathan\n"},{"id":"209941","messageId":"20130220215845.GB817@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220215043.GA24236@google.com","subject":"Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T21:58:45Z","receivedAt":"2013-02-20T21:58:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 20, 2013 at 01:51:11PM -0800, Jonathan Nieder wrote:\n\n> > This distinction doesn't typically matter in git, because we\n> > do not ignore SIGPIPE in the first place. Which means that\n> > we will not get EPIPE, but instead will just die when we get\n> > a SIGPIPE. But it's possible for a default handler to be set\n> > by a parent process,\n> \n> Not so much \"default\" as \"insane inherited\", as in the example\n> of old versions of Python's subprocess.Popen.\n\nIt's possible that somebody could have a legitimate reason for doing so.\nI just can't think of one. :)\n\n> I suspect this used exit(0) instead of raise(SIGPIPE) in the first\n> place to work around a bash bug (too much verbosity about SIGPIPE).\n> If any programs still have that kind of bug, I'd rather put pressure\n> on them to fix it by *not* working around it. So the basic idea here\n> looks good to me.\n\nYeah, if you look for old discussions on SIGPIPE in the git list, it is\nmostly Linus complaining about the bash behavior, and this code does\ndate back to that era. The bash bug is long since fixed.\n\n> > +\tif (err == EPIPE) {\n> > +\t\tsignal(SIGPIPE, SIG_DFL);\n> > +\t\traise(SIGPIPE);\n> > +\t\t/* Should never happen, but just in case... */\n> > +\t\texit(141);\n> \n> How about\n> \n> \t\tdie(\"BUG: another thread changed SIGPIPE handling behind my back!\");\n> \n> to make it easier to find and fix such problems?\n\nYou mean for the \"should never happen\" bit, not the first part, right? I\nactually wonder if we should simply exit(141) in the first place. That\nis shell exit-code for SIGPIPE death already (so it's what our\nrun_command would show us, and what anybody running us through shell\nwould see).\n\n-Peff\n"},{"id":"209942","messageId":"20130220220114.GB24236@google.com","threadId":"32952","inReplyTo":"20130220215845.GB817@sigill.intra.peff.net","subject":"Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-20T22:01:14Z","receivedAt":"2013-02-20T22:01:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Wed, Feb 20, 2013 at 01:51:11PM -0800, Jonathan Nieder wrote:\n\n>>> +\tif (err == EPIPE) {\n>>> +\t\tsignal(SIGPIPE, SIG_DFL);\n>>> +\t\traise(SIGPIPE);\n>>> +\t\t/* Should never happen, but just in case... */\n>>> +\t\texit(141);\n>>\n>> How about\n>>\n>> \t\tdie(\"BUG: another thread changed SIGPIPE handling behind my back!\");\n>>\n>> to make it easier to find and fix such problems?\n>\n> You mean for the \"should never happen\" bit, not the first part, right? I\n> actually wonder if we should simply exit(141) in the first place. That\n> is shell exit-code for SIGPIPE death already (so it's what our\n> run_command would show us, and what anybody running us through shell\n> would see).\n\nYes, for the \"should never happen\" part. Raising a signal is nice\nbecause it means the wait()-ing process can see what happened by\nchecking WIFSIGNALED(status).\n\nJonathan\n"},{"id":"209943","messageId":"20130220220359.GA1417@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220220114.GB24236@google.com","subject":"Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T22:03:59Z","receivedAt":"2013-02-20T22:03:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 20, 2013 at 02:01:14PM -0800, Jonathan Nieder wrote:\n\n> >> How about\n> >>\n> >> \t\tdie(\"BUG: another thread changed SIGPIPE handling behind my back!\");\n> >>\n> >> to make it easier to find and fix such problems?\n> >\n> > You mean for the \"should never happen\" bit, not the first part, right? I\n> > actually wonder if we should simply exit(141) in the first place. That\n> > is shell exit-code for SIGPIPE death already (so it's what our\n> > run_command would show us, and what anybody running us through shell\n> > would see).\n> \n> Yes, for the \"should never happen\" part. Raising a signal is nice\n> because it means the wait()-ing process can see what happened by\n> checking WIFSIGNALED(status).\n\nRight. My point is that only happens if there's no shell in the way. But\nI guess it doesn't hurt to make the attempt to help the people using\nwait() directly.\n\nI don't mind adding a \"BUG: \" message like you described, but we should\nstill try to exit(141) as the backup, since that is the shell-equivalent\ncode to the SIGPIPE signal death.\n\n-Peff\n"},{"id":"209944","messageId":"20130220220637.GC24236@google.com","threadId":"32952","inReplyTo":"20130220220359.GA1417@sigill.intra.peff.net","subject":"Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-20T22:06:37Z","receivedAt":"2013-02-20T22:06:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Wed, Feb 20, 2013 at 02:01:14PM -0800, Jonathan Nieder wrote:\n\n>>>> How about\n>>>>\n>>>> \t\tdie(\"BUG: another thread changed SIGPIPE handling behind my back!\");\n>>>>\n>>>> to make it easier to find and fix such problems?\n>>>\n>>> You mean for the \"should never happen\" bit, not the first part, right? I\n>>> actually wonder if we should simply exit(141) in the first place. That\n>>> is shell exit-code for SIGPIPE death already (so it's what our\n>>> run_command would show us, and what anybody running us through shell\n>>> would see).\n>>\n>> Yes, for the \"should never happen\" part.\n[...]\n> I don't mind adding a \"BUG: \" message like you described, but we should\n> still try to exit(141) as the backup, since that is the shell-equivalent\n> code to the SIGPIPE signal death.\n\nIf you want. :)\n\nI think caring about graceful degradation of behavior in the case of\nan assertion failure is overengineering, but it's mostly harmless.\n"},{"id":"209945","messageId":"20130220221248.GC817@sigill.intra.peff.net","threadId":"32952","inReplyTo":"20130220220637.GC24236@google.com","subject":"Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-20T22:12:48Z","receivedAt":"2013-02-20T22:12:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 20, 2013 at 02:06:37PM -0800, Jonathan Nieder wrote:\n\n> > I don't mind adding a \"BUG: \" message like you described, but we should\n> > still try to exit(141) as the backup, since that is the shell-equivalent\n> > code to the SIGPIPE signal death.\n> \n> If you want. :)\n> \n> I think caring about graceful degradation of behavior in the case of\n> an assertion failure is overengineering, but it's mostly harmless.\n\nI am more concerned that the assertion is not \"oops, another thread is\ndoing something crazy, and it is a bug\", but rather that there is some\nweird platform where SIG_DFL does not kill the program under SIGPIPE.\nThat seems pretty crazy, though. I think I'd squash in something like\nthis:\n\ndiff --git a/write_or_die.c b/write_or_die.c\nindex b50f99a..abb64db 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -5,7 +5,9 @@ static void check_pipe(int err)\n \tif (err == EPIPE) {\n \t\tsignal(SIGPIPE, SIG_DFL);\n \t\traise(SIGPIPE);\n+\n \t\t/* Should never happen, but just in case... */\n+\t\terror(\"BUG: SIGPIPE on SIG_DFL handler did not kill us.\");\n \t\texit(141);\n \t}\n }\n\nwhich more directly reports the assertion that failed, and degrades\nreasonably gracefully. Yeah, it's probably overengineering, but it's\neasy enough to do.\n\n-Peff\n"},{"id":"209947","messageId":"7vfw0qy6g7.fsf@alter.siamese.dyndns.org","threadId":"32952","inReplyTo":"20130220221248.GC817@sigill.intra.peff.net","subject":"Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-20T22:19:04Z","receivedAt":"2013-02-20T22:19:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I am more concerned that the assertion is not \"oops, another thread is\n> doing something crazy, and it is a bug\", but rather that there is some\n> weird platform where SIG_DFL does not kill the program under SIGPIPE.\n> That seems pretty crazy, though. I think I'd squash in something like\n> this:\n>\n> diff --git a/write_or_die.c b/write_or_die.c\n> index b50f99a..abb64db 100644\n> --- a/write_or_die.c\n> +++ b/write_or_die.c\n> @@ -5,7 +5,9 @@ static void check_pipe(int err)\n> \tif (err == EPIPE) {\n> \t\tsignal(SIGPIPE, SIG_DFL);\n> \t\traise(SIGPIPE);\n> +\n> \t\t/* Should never happen, but just in case... */\n> +\t\terror(\"BUG: SIGPIPE on SIG_DFL handler did not kill us.\");\n> \t\texit(141);\n> \t}\n> }\n>\n> which more directly reports the assertion that failed, and degrades\n> reasonably gracefully. Yeah, it's probably overengineering, but it's\n> easy enough to do.\n\nYeah, that sounds like a sensible thing to do, as it is cheap even\nthough we do not expect it to trigger.\n"},{"id":"210023","messageId":"CAPig+cQ0kxUpuXe3Pj01xnPmFX4T61hpROrUwCf7R9Y9e35M1A@mail.gmail.com","threadId":"32952","inReplyTo":"20130220200430.GO25647@sigill.intra.peff.net","subject":"Re: [PATCH v3 15/19] pkt-line: share buffer/descriptor reading implementation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-02-22T11:22:02Z","receivedAt":"2013-02-22T11:22:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 20, 2013 at 3:04 PM, Jeff King wrote:\n> diff --git a/pkt-line.h b/pkt-line.h\n> index fa93e32..47361f5 100644\n> --- a/pkt-line.h\n> +++ b/pkt-line.h\n> @@ -25,9 +25,16 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f\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, which must be at\n> - * least size bytes long. The return value specifies the number of bytes read\n> - * into the buffer.\n> + * Read a packetized line into the buffer, which must be at least size bytes\n> + * long. The return value specifies the number of bytes read into the buffer.\n> + *\n> + * If src_buffer is not NULL (and nor is *src_buffer), it should point to a\n> + * buffer containing the packet data to parse, of at least *src_len bytes.\n> + * After the function returns, src_buf will be increments and src_len\n\ns/increments/incremented/\n\n> + * decremented by the number of bytes consumed.\n"},{"id":"237989","messageId":"loom.20140328T093203-852@post.gmane.org","threadId":"32952","inReplyTo":"20130220200136.GH25647@sigill.intra.peff.net","subject":"[BUG] MSVC: error box when interrupting `gitlog` by quitting less","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-28T08:35:31Z","receivedAt":"2014-03-28T08:35:31Z","isPatch":false,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"Jeff King peff.net> writes:\n\n> \n> The write_or_die function will always die on an error,\n> including EPIPE. However, it currently treats EPIPE\n> specially by suppressing any error message, and by exiting\n> with exit code 0.\n\nThis causes error box on Windows in MSVC=1 build:\n\ngit.exe!_invoke_watson(...) Line 132\tC++\ngit.exe!_invalid_parameter(...) Line 85\tC++\ngit.exe!_invalid_parameter_noinfo() Line 97\tC++\ngit.exe!raise(int signum) Line 499\tC\ngit.exe!mingw_raise(int sig) Line 1745\tC\ngit.exe!check_pipe(int err) Line 9\tC\ngit.exe!maybe_flush_or_die(_iobuf * f, const char * desc) Line 48\tC\ngit.exe!log_tree_commit(rev_info * opt, commit * commit) Line 820\tC\ngit.exe!cmd_log_walk(rev_info * rev) Line 344\tC\ngit.exe!cmd_log(int argc, const char * * argv, const char * prefix) Line 637\tC\ngit.exe!run_builtin(cmd_struct * p, int argc, const char * * argv) Line 314\tC\ngit.exe!handle_builtin(int argc, const char * * argv) Line 487\tC\ngit.exe!run_argv(int * argcp, const char * * * argv) Line 536\tC\ngit.exe!mingw_main(int argc, char * * av) Line 616\tC\ngit.exe!main(int argc, char * * argv) Line 551\tC\n\n\"Should never happen\", ha-ha.\n"},{"id":"237990","messageId":"loom.20140328T101113-154@post.gmane.org","threadId":"32952","inReplyTo":"loom.20140328T093203-852@post.gmane.org","subject":"Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-28T09:14:07Z","receivedAt":"2014-03-28T09:14:07Z","isPatch":false,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"Marat Radchenko slonopotamus.org> writes:\n\n> \n> Jeff King peff.net> writes:\n> \n> > \n> > The write_or_die function will always die on an error,\n> > including EPIPE. However, it currently treats EPIPE\n> > specially by suppressing any error message, and by exiting\n> > with exit code 0.\n> \n> This causes error box on Windows in MSVC=1 build:\n\nAfter deeper investigation it turned out that Windows supports\nmuch less signals [1] than POSIX and \"If the argument is not a valid signal \nas specified above, the invalid parameter handler is invoked\".\n\nThe question is - what is the proper way to fix this?\nPatch mingw_raise in compat/mingw.c to map unsupported signals into\nsupported ones like SIGPIPE -> SIGTERM?\n\n[1]: http://msdn.microsoft.com/en-us/library/dwwzkt4c.aspx\n"},{"id":"237991","messageId":"20140328094443.GA16370@sigill.intra.peff.net","threadId":"32952","inReplyTo":"loom.20140328T101113-154@post.gmane.org","subject":"Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-28T09:44:43Z","receivedAt":"2014-03-28T09:44:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 28, 2014 at 09:14:07AM +0000, Marat Radchenko wrote:\n\n> > Jeff King peff.net> writes:\n> > \n> > > \n> > > The write_or_die function will always die on an error,\n> > > including EPIPE. However, it currently treats EPIPE\n> > > specially by suppressing any error message, and by exiting\n> > > with exit code 0.\n> > \n> > This causes error box on Windows in MSVC=1 build:\n> \n> After deeper investigation it turned out that Windows supports\n> much less signals [1] than POSIX and \"If the argument is not a valid signal \n> as specified above, the invalid parameter handler is invoked\".\n> \n> The question is - what is the proper way to fix this?\n> Patch mingw_raise in compat/mingw.c to map unsupported signals into\n> supported ones like SIGPIPE -> SIGTERM?\n> \n> [1]: http://msdn.microsoft.com/en-us/library/dwwzkt4c.aspx\n\nI'm not sure what an actual SIGPIPE death looks like on Windows. What\nhappens if git is still writing data to the pager and the pager exits?\nDoes it receive a signal of some sort?\n\nThe point of the code in check_pipe is to simulate that death. So\nwhatever happens to git in that case is what we would want to happen\nwhen we call raise(SIGPIPE).\n\nA possibly simpler option would be to just have the MSVC build skip the\nraise() call, and do the exit(141) that comes just after. That is\nprobably close enough simulation of SIGPIPE death.\n\n-Peff\n"},{"id":"237994","messageId":"loom.20140328T105136-494@post.gmane.org","threadId":"32952","inReplyTo":"20140328094443.GA16370@sigill.intra.peff.net","subject":"Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-28T10:07:22Z","receivedAt":"2014-03-28T10:07:22Z","isPatch":false,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"Jeff King peff.net> writes:\n\n> \n> I'm not sure what an actual SIGPIPE death looks like on Windows.\n\nThere is no SIGPIPE death on Windows due to total absence of SIGPIPE.\nraise(unsupported int) just causes ugly \"git.exe has stopped working\"\nwindow and possibly ends up as SIGABT (I don't know how to check this).\n\n> What\n> happens if git is still writing data to the pager and the pager exits?\n> Does it receive a signal of some sort?\n\nI'm not sure what you mean, sorry. check_pipe properly detects pager exit.\nThe problem is with the way it tries to die.\n\n> The point of the code in check_pipe is to simulate that death. So\n> whatever happens to git in that case is what we would want to happen\n> when we call raise(SIGPIPE).\n\nThat's what I'm talking about. On Windows, you can't raise(SIGPIPE).\nYou can only raise(Windows_supported_signal) where signal is one of:\nSIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM as MSDN tells us.\n\n> A possibly simpler option would be to just have the MSVC build skip the\n> raise() call, and do the exit(141) that comes just after. That is\n> probably close enough simulation of SIGPIPE death.\n\nIsn't raise(SIGTERM/SIGINT) good enough?\n"},{"id":"237995","messageId":"20140328101940.GA27601@sigill.intra.peff.net","threadId":"32952","inReplyTo":"loom.20140328T105136-494@post.gmane.org","subject":"Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-28T10:19:41Z","receivedAt":"2014-03-28T10:19:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 28, 2014 at 10:07:22AM +0000, Marat Radchenko wrote:\n\n> > What\n> > happens if git is still writing data to the pager and the pager exits?\n> > Does it receive a signal of some sort?\n> \n> I'm not sure what you mean, sorry. check_pipe properly detects pager exit.\n> The problem is with the way it tries to die.\n\nRight, but check_pipe shouldn't trigger in most cases on Unix because\nthe process will be killed by SIGPIPE automatically. It's only there to\ncatch the case where we have disabled SIGPIPE.\n\nOn Windows, what happens to \"yes\" if you run:\n\n yes | (exit 0)\n\nOn Unix, \"yes\" receives SIGPIPE and dies. Does it run forever on\nWindows? If it dies, what does the death look like (does it have a\nsignal death, or exit with a specific code?).\n\n> > The point of the code in check_pipe is to simulate that death. So\n> > whatever happens to git in that case is what we would want to happen\n> > when we call raise(SIGPIPE).\n> \n> That's what I'm talking about. On Windows, you can't raise(SIGPIPE).\n> You can only raise(Windows_supported_signal) where signal is one of:\n> SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM as MSDN tells us.\n\nRight, I understand that you don't have SIGPIPE. But we want to emulate\nwhatever happens in the case I described above.\n\n> > A possibly simpler option would be to just have the MSVC build skip the\n> > raise() call, and do the exit(141) that comes just after. That is\n> > probably close enough simulation of SIGPIPE death.\n> \n> Isn't raise(SIGTERM/SIGINT) good enough?\n\nPerhaps. It is a slight lie. We _didn't_ get a SIGTERM, and anybody\nlooking at our exit code to find out why we died would be misled. But\nthe most important thing is that we die and that the exit status is\nnon-zero.\n\n-Peff\n"},{"id":"237996","messageId":"53354EE3.2050908@viscovery.net","threadId":"32952","inReplyTo":"loom.20140328T105136-494@post.gmane.org","subject":"Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-03-28T10:28:51Z","receivedAt":"2014-03-28T10:28:51Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Please do not cull the Cc list.\n\nAm 3/28/2014 11:07, schrieb Marat Radchenko:\n> Jeff King peff.net> writes:\n> \n>>\n>> I'm not sure what an actual SIGPIPE death looks like on Windows.\n> \n> There is no SIGPIPE death on Windows due to total absence of SIGPIPE.\n> raise(unsupported int) just causes ugly \"git.exe has stopped working\"\n> window and possibly ends up as SIGABT (I don't know how to check this).\n\nThis happens \"only\" with newer Microsoft C runtime libraries. They do not\nreturn EINVAL (because that usually indicates a bug caused by insufficient\nchecks before the function call), but crash the program by default in the\nway that you observed.\n\n>> What\n>> happens if git is still writing data to the pager and the pager exits?\n>> Does it receive a signal of some sort?\n\nNo; the write attempt returns with EPIPE.\n\n> \n> I'm not sure what you mean, sorry. check_pipe properly detects pager exit.\n> The problem is with the way it tries to die.\n> \n>> The point of the code in check_pipe is to simulate that death. So\n>> whatever happens to git in that case is what we would want to happen\n>> when we call raise(SIGPIPE).\n> \n> That's what I'm talking about. On Windows, you can't raise(SIGPIPE).\n> You can only raise(Windows_supported_signal) where signal is one of:\n> SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM as MSDN tells us.\n\nCorrect. All other signal number should return EINVAL. But, as I said,\nthat does not happen by default.\n\nThe correct solution is to link against invalidcontinue.obj in the MSVC\nbuild. This is a compiler-provided object file that changes the default\nbehavior to the \"expected\" kind, i.e., C runtime functions return EINVAL\nwhen appropriate instead of crashing the application.\n\n>> A possibly simpler option would be to just have the MSVC build skip the\n>> raise() call, and do the exit(141) that comes just after. That is\n>> probably close enough simulation of SIGPIPE death.\n\nCorrect. The MinGW build uses an older C runtime library, which does not\nhave the strange default behavior, and we do use that exit(141). And with\nthe fix to the MSVC build suggested above, that version would do likewise.\n\n-- Hannes\n"},{"id":"237998","messageId":"1396005570-948-1-git-send-email-marat@slonopotamus.org","threadId":"32952","inReplyTo":"53354EE3.2050908@viscovery.net","subject":"[PATCH] MSVC: link in invalidcontinue.obj for better POSIX compatibility","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-28T11:19:30Z","receivedAt":"2014-03-28T11:19:30Z","isPatch":true,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"This patch fixes crashes caused by quitting from PAGER.\n\nSigned-off-by: Marat Radchenko \n---\n\n> Please do not cull the Cc list.\n\nThat was gmane web interface.\n\n> The correct solution is to link against invalidcontinue.obj in the MSVC\n> build. This is a compiler-provided object file that changes the default\n> behavior to the \"expected\" kind, i.e., C runtime functions return EINVAL\n> when appropriate instead of crashing the application.\n\nThanks for a hint.\n\n config.mak.uname | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 38c60af..8e7ec6e 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -366,7 +366,7 @@ ifeq ($(uname_S),Windows)\n \t\tcompat/win32/dirent.o\n \tCOMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\\\".exe\\\"\n \tBASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib\n-\tEXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib\n+\tEXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj\n \tPTHREAD_LIBS =\n \tlib =\n ifndef DEBUG\n-- \n1.9.1\n"},{"id":"238027","messageId":"xmqqy4zu9o0j.fsf@gitster.dls.corp.google.com","threadId":"32952","inReplyTo":"1396005570-948-1-git-send-email-marat@slonopotamus.org","subject":"Re: [PATCH] MSVC: link in invalidcontinue.obj for better POSIX compatibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-28T18:27:56Z","receivedAt":"2014-03-28T18:27:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marat Radchenko writes:\n\n> This patch fixes crashes caused by quitting from PAGER.\n\nCan you elaborate a bit more on the underlying cause, summarizing\nwhat you learned from this discussion, so that those who read \"git\nlog\" output two weeks from now do not have to come back to this\nthread in the mail archive in order to figure out why we suddenly\nneeds to link with yet another library?\n\nThanks.\n\n> Signed-off-by: Marat Radchenko \n> ---\n>\n>> Please do not cull the Cc list.\n>\n> That was gmane web interface.\n>\n>> The correct solution is to link against invalidcontinue.obj in the MSVC\n>> build. This is a compiler-provided object file that changes the default\n>> behavior to the \"expected\" kind, i.e., C runtime functions return EINVAL\n>> when appropriate instead of crashing the application.\n>\n> Thanks for a hint.\n>\n> config.mak.uname | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 38c60af..8e7ec6e 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -366,7 +366,7 @@ ifeq ($(uname_S),Windows)\n> \t\tcompat/win32/dirent.o\n> \tCOMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\\\".exe\\\"\n> \tBASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib\n> -\tEXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib\n> +\tEXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj\n> \tPTHREAD_LIBS =\n> \tlib =\n> ifndef DEBUG\n"},{"id":"238030","messageId":"loom.20140328T193620-235@post.gmane.org","threadId":"32952","inReplyTo":"xmqqy4zu9o0j.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] MSVC: link in invalidcontinue.obj for better POSIX compatibility","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-28T18:46:55Z","receivedAt":"2014-03-28T18:46:55Z","isPatch":true,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"Junio C Hamano pobox.com> writes:\n\n> > This patch fixes crashes caused by quitting from PAGER.\n> \n> Can you elaborate a bit more on the underlying cause, summarizing\n> what you learned from this discussion, so that those who read \"git\n> log\" output two weeks from now do not have to come back to this\n> thread in the mail archive in order to figure out why we suddenly\n> needs to link with yet another library?\n\nWithout linking to that obj, Windows abort()'s instead of setting\nerrno=EINVAL when invalid arguments are passed to standard functions.\nIn this particular case, when PAGER quits and git detects it with\nerrno=EPIPE on write(), git tries raise(SIGPIPE) but since there is no\nSIGPIPE on Windows, it is treated as invalid argument, causing abort()\nand crash report window.\n\nLinking in invalidcontinue.obj (provided along with MS compiler) allows\nraise(SIGPIPE) to return with errno=EINVAL. While testing MSVC=1 git,\nI found several more cases with same sympthoms (and also fixed by\ngiven patch).\n"},{"id":"238035","messageId":"xmqqlhvu9m8x.fsf@gitster.dls.corp.google.com","threadId":"32952","inReplyTo":"xmqqy4zu9o0j.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] MSVC: link in invalidcontinue.obj for better POSIX compatibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-28T19:06:06Z","receivedAt":"2014-03-28T19:06:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Marat Radchenko writes:\n>\n>> This patch fixes crashes caused by quitting from PAGER.\n>\n> Can you elaborate a bit more on the underlying cause, summarizing\n> what you learned from this discussion, so that those who read \"git\n> log\" output two weeks from now do not have to come back to this\n> thread in the mail archive in order to figure out why we suddenly\n> needs to link with yet another library?\n>\n> Thanks.\n\nJust to avoid getting misunderstood, I am not asking it to be\nexplained to me in an e-mail. I want to see a patch with its\nproposed commit log message to explain it to readers of \"git log\".\n\nThanks.\n"},{"id":"238043","messageId":"1396037282-26081-1-git-send-email-marat@slonopotamus.org","threadId":"32952","inReplyTo":"xmqqlhvu9m8x.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] MSVC: link in invalidcontinue.obj for better POSIX compatibility","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-28T20:08:02Z","receivedAt":"2014-03-28T20:08:02Z","isPatch":true,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"By default, Windows abort()'s instead of setting\nerrno=EINVAL when invalid arguments are passed to standard functions.\n\nFor example, when PAGER quits and git detects it with\nerrno=EPIPE on write(), check_pipe() in write_or_die.c tries raise(SIGPIPE)\nbut since there is no SIGPIPE on Windows, it is treated as invalid argument,\ncausing abort() and crash report window.\n\nLinking in invalidcontinue.obj (provided along with MS compiler) allows\nraise(SIGPIPE) to return with errno=EINVAL.\n\nSigned-off-by: Marat Radchenko \n---\n config.mak.uname | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 38c60af..8e7ec6e 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -366,7 +366,7 @@ ifeq ($(uname_S),Windows)\n \t\tcompat/win32/dirent.o\n \tCOMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\\\".exe\\\"\n \tBASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib\n-\tEXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib\n+\tEXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj\n \tPTHREAD_LIBS =\n \tlib =\n ifndef DEBUG\n-- \n1.8.3.2\n"},{"id":"238044","messageId":"xmqq4n2i9i3b.fsf@gitster.dls.corp.google.com","threadId":"32952","inReplyTo":"1396037282-26081-1-git-send-email-marat@slonopotamus.org","subject":"Re: [PATCH v2] MSVC: link in invalidcontinue.obj for better POSIX compatibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-28T20:35:52Z","receivedAt":"2014-03-28T20:35:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marat Radchenko writes:\n\n> By default, Windows abort()'s instead of setting\n> errno=EINVAL when invalid arguments are passed to standard functions.\n>\n> For example, when PAGER quits and git detects it with\n> errno=EPIPE on write(), check_pipe() in write_or_die.c tries raise(SIGPIPE)\n> but since there is no SIGPIPE on Windows, it is treated as invalid argument,\n> causing abort() and crash report window.\n>\n> Linking in invalidcontinue.obj (provided along with MS compiler) allows\n> raise(SIGPIPE) to return with errno=EINVAL.\n>\n> Signed-off-by: Marat Radchenko \n> ---\n\nThanks; will queue.\n\n> config.mak.uname | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 38c60af..8e7ec6e 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -366,7 +366,7 @@ ifeq ($(uname_S),Windows)\n> \t\tcompat/win32/dirent.o\n> \tCOMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\\\".exe\\\"\n> \tBASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib\n> -\tEXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib\n> +\tEXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj\n> \tPTHREAD_LIBS =\n> \tlib =\n> ifndef DEBUG\n"}]}