{"thread":{"id":"38152","subject":"[RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers","startedAt":"2014-12-09T17:49:58Z","lastAt":"2014-12-10T21:06:51Z","messageCount":20,"participants":["Jeff King","Johannes Sixt","Junio C Hamano","Michael Blume","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"253421","messageId":"20141209174958.GA26167@peff.net","threadId":"38152","inReplyTo":null,"subject":"[RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-09T17:49:58Z","receivedAt":"2014-12-09T17:49:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we send out pkt-lines with refnames, we use a static\n1000-byte buffer. This means that the maximum size of a ref\nover the git protocol is around 950 bytes (the exact size\ndepends on the protocol line being written, but figure on a sha1\nplus some boilerplate).\n\nThis is enough for any sane workflow, but occasionally odd\nthings happen (e.g., a bug may create a ref \"foo/foo/foo/...\"\naccidentally).  With the current code, you cannot even use\n\"push\" to delete such a ref from a remote.\n\nLet's bump the size of the buffer to LARGE_PACKET_MAX. This\nmatches the size of the readers, as of 74543a0 (pkt-line:\nprovide a LARGE_PACKET_MAX static buffer, 2013-02-20).\nVersions of git older than that will complain about our\nlarge packets, but it's really no worse than the current\nbehavior. Right now the sender barfs with \"impossibly long\nline\" trying to send the packet, and afterwards the reader\nwill barf with \"protocol error: bad line length %d\", which\nis arguably better anyway.\n\nNote that we're not really _solving_ the problem here, but\njust bumping the limits. In theory, the length of a ref is\nunbounded, and pkt-line can only represent sizes up to\n65531 bytes. So we are just bumping the limit, not removing.\nBut hopefully 64K should be enough for anyone.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis was inspired by a real-world case (with the \"foo/foo\" bug mentioned\nabove). It is rather an unlikely scenario, though (it literally was a\nhook on the receiving end creating the ref, and push-deletion would have\nbeen the simplest way to fix the situation). On the other hand, it's\nnice to eliminate as many arbitrary limits as possible.\n\nThis does waste 63K of BSS. I was tempted to just use the existing\nstatic packet_buffer (introduced by 74543a0) that the readers use. But\nthat is probably too magical; some callers may be surprised to find\nthe buffers they read invalidated by a call to packet_write. I think the\ncurrent callers are probably OK, but it's a little too error-prone for\nmy taste.\n\nAnother option would be to use a static strbuf. Then we're only wasting\nheap, and even then only as much as we need (we'd still manually cap it\nat LARGE_PACKET_MAX since that's what the protocol dictates). This would\nalso make packet_buf_write more efficient (right now it formats into a\nstatic buffer, and then copies the result into a strbuf; probably not\nmeasurably important, but silly nonetheless).\n\n pkt-line.c                |  2 +-\n t/t5527-fetch-odd-refs.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 8bc89b1..aa42fb5 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -64,7 +64,7 @@ void packet_buf_flush(struct strbuf *buf)\n }\n \n #define hex(a) (hexchar[(a) & 15])\n-static char buffer[1000];\n+static char buffer[LARGE_PACKET_MAX];\n static unsigned format_packet(const char *fmt, va_list args)\n {\n \tstatic char hexchar[] = \"0123456789abcdef\";\ndiff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh\nindex edea9f9..c86d38b 100755\n--- a/t/t5527-fetch-odd-refs.sh\n+++ b/t/t5527-fetch-odd-refs.sh\n@@ -26,4 +26,33 @@ test_expect_success 'suffix ref is ignored during fetch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'create repo with absurdly long refname' '\n+\tref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40\n+\tref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&\n+\tgit init long &&\n+\t(\n+\t\tcd long &&\n+\t\ttest_commit long &&\n+\t\ttest_commit master &&\n+\t\tgit update-ref refs/heads/$ref1440 long\n+\t)\n+'\n+\n+test_expect_success 'fetch handles extremely long refname' '\n+\tgit fetch long refs/heads/*:refs/remotes/long/* &&\n+\tcat >expect <<-\\EOF &&\n+\tlong\n+\tmaster\n+\tEOF\n+\tgit for-each-ref --format=\"%(subject)\" refs/remotes/long >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'push handles extremely long refname' '\n+\tgit push long :refs/heads/$ref1440 &&\n+\tgit -C long for-each-ref --format=\"%(subject)\" refs/heads >actual &&\n+\techo master >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253422","messageId":"20141209180916.GA26873@peff.net","threadId":"38152","inReplyTo":"20141209174958.GA26167@peff.net","subject":"Re: [RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-09T18:09:16Z","receivedAt":"2014-12-09T18:09:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 09, 2014 at 12:49:58PM -0500, Jeff King wrote:\n\n> Another option would be to use a static strbuf. Then we're only wasting\n> heap, and even then only as much as we need (we'd still manually cap it\n> at LARGE_PACKET_MAX since that's what the protocol dictates). This would\n> also make packet_buf_write more efficient (right now it formats into a\n> static buffer, and then copies the result into a strbuf; probably not\n> measurably important, but silly nonetheless).\n\nBelow is what that would look like. It's obviously a much more invasive\nchange, but I think the result is nice.\n\n-- >8 --\nSubject: pkt-line: allow writing of LARGE_PACKET_MAX buffers\n\nWhen we send out pkt-lines with refnames, we use a static\n1000-byte buffer. This means that the maximum size of a ref\nover the git protocol is around 950 bytes (the exact size\ndepends on the protocol line being written, but figure on a sha1\nplus some boilerplate).\n\nThis is enough for any sane workflow, but occasionally odd\nthings happen (e.g., a bug may create a ref \"foo/foo/foo/...\"\naccidentally).  With the current code, you cannot even use\n\"push\" to delete such a ref from a remote.\n\nLet's switch to using a strbuf, with a hard-limit of\nLARGE_PACKET_MAX (which is specified by the protocol).  This\nmatches the size of the readers, as of 74543a0 (pkt-line:\nprovide a LARGE_PACKET_MAX static buffer, 2013-02-20).\nVersions of git older than that will complain about our\nlarge packets, but it's really no worse than the current\nbehavior. Right now the sender barfs with \"impossibly long\nline\" trying to send the packet, and afterwards the reader\nwill barf with \"protocol error: bad line length %d\", which\nis arguably better anyway.\n\nNote that we're not really _solving_ the problem here, but\njust bumping the limits. In theory, the length of a ref is\nunbounded, and pkt-line can only represent sizes up to\n65531 bytes. So we are just bumping the limit, not removing\nit.  But hopefully 64K should be enough for anyone.\n\nAs a bonus, by using a strbuf for the formatting we can\neliminate an unnecessary copy in format_buf_write.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pkt-line.c                | 37 +++++++++++++++++++------------------\n t/t5527-fetch-odd-refs.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 18 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 8bc89b1..187a229 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -64,44 +64,45 @@ void packet_buf_flush(struct strbuf *buf)\n }\n \n #define hex(a) (hexchar[(a) & 15])\n-static char buffer[1000];\n-static unsigned format_packet(const char *fmt, va_list args)\n+static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n {\n \tstatic char hexchar[] = \"0123456789abcdef\";\n-\tunsigned n;\n+\tsize_t orig_len, n;\n \n-\tn = vsnprintf(buffer + 4, sizeof(buffer) - 4, fmt, args);\n-\tif (n >= sizeof(buffer)-4)\n+\torig_len = out->len;\n+\tstrbuf_addstr(out, \"0000\");\n+\tstrbuf_vaddf(out, fmt, args);\n+\tn = out->len - orig_len;\n+\n+\tif (n > LARGE_PACKET_MAX)\n \t\tdie(\"protocol error: impossibly long line\");\n-\tn += 4;\n-\tbuffer[0] = hex(n >> 12);\n-\tbuffer[1] = hex(n >> 8);\n-\tbuffer[2] = hex(n >> 4);\n-\tbuffer[3] = hex(n);\n-\tpacket_trace(buffer+4, n-4, 1);\n-\treturn n;\n+\n+\tout->buf[orig_len + 0] = hex(n >> 12);\n+\tout->buf[orig_len + 1] = hex(n >> 8);\n+\tout->buf[orig_len + 2] = hex(n >> 4);\n+\tout->buf[orig_len + 3] = hex(n);\n+\tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n }\n \n void packet_write(int fd, const char *fmt, ...)\n {\n+\tstatic struct strbuf buf = STRBUF_INIT;\n \tva_list args;\n-\tunsigned n;\n \n+\tstrbuf_reset(&buf);\n \tva_start(args, fmt);\n-\tn = format_packet(fmt, args);\n+\tformat_packet(&buf, fmt, args);\n \tva_end(args);\n-\twrite_or_die(fd, buffer, n);\n+\twrite_or_die(fd, buf.buf, buf.len);\n }\n \n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n {\n \tva_list args;\n-\tunsigned n;\n \n \tva_start(args, fmt);\n-\tn = format_packet(fmt, args);\n+\tformat_packet(buf, fmt, args);\n \tva_end(args);\n-\tstrbuf_add(buf, buffer, n);\n }\n \n static int get_packet_data(int fd, char **src_buf, size_t *src_size,\ndiff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh\nindex edea9f9..c86d38b 100755\n--- a/t/t5527-fetch-odd-refs.sh\n+++ b/t/t5527-fetch-odd-refs.sh\n@@ -26,4 +26,33 @@ test_expect_success 'suffix ref is ignored during fetch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'create repo with absurdly long refname' '\n+\tref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40\n+\tref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&\n+\tgit init long &&\n+\t(\n+\t\tcd long &&\n+\t\ttest_commit long &&\n+\t\ttest_commit master &&\n+\t\tgit update-ref refs/heads/$ref1440 long\n+\t)\n+'\n+\n+test_expect_success 'fetch handles extremely long refname' '\n+\tgit fetch long refs/heads/*:refs/remotes/long/* &&\n+\tcat >expect <<-\\EOF &&\n+\tlong\n+\tmaster\n+\tEOF\n+\tgit for-each-ref --format=\"%(subject)\" refs/remotes/long >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'push handles extremely long refname' '\n+\tgit push long :refs/heads/$ref1440 &&\n+\tgit -C long for-each-ref --format=\"%(subject)\" refs/heads >actual &&\n+\techo master >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253437","messageId":"54875462.3040305@kdbg.org","threadId":"38152","inReplyTo":"20141209174958.GA26167@peff.net","subject":"Re: [RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-12-09T19:58:26Z","receivedAt":"2014-12-09T19:58:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.12.2014 um 18:49 schrieb Jeff King:\n> +test_expect_success 'create repo with absurdly long refname' '\n> +\tref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40\n> +\tref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&\n> +\tgit init long &&\n> +\t(\n> +\t\tcd long &&\n> +\t\ttest_commit long &&\n> +\t\ttest_commit master &&\n> +\t\tgit update-ref refs/heads/$ref1440 long\n\nHaving this ref on the filesystem is going to fail on Windows, I presume\n(our limit is 260). Can we stuff it away as a packed ref right from the\nbeginning? (And turn off reflogs, BTW.)\n\n> +\t)\n> +'\n\n-- Hannes\n"},{"id":"253439","messageId":"20141209200058.GA11481@peff.net","threadId":"38152","inReplyTo":"54875462.3040305@kdbg.org","subject":"Re: [RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-09T20:00:58Z","receivedAt":"2014-12-09T20:00:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 09, 2014 at 08:58:26PM +0100, Johannes Sixt wrote:\n\n> Am 09.12.2014 um 18:49 schrieb Jeff King:\n> > +test_expect_success 'create repo with absurdly long refname' '\n> > +\tref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40\n> > +\tref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&\n> > +\tgit init long &&\n> > +\t(\n> > +\t\tcd long &&\n> > +\t\ttest_commit long &&\n> > +\t\ttest_commit master &&\n> > +\t\tgit update-ref refs/heads/$ref1440 long\n> \n> Having this ref on the filesystem is going to fail on Windows, I presume\n> (our limit is 260). Can we stuff it away as a packed ref right from the\n> beginning? (And turn off reflogs, BTW.)\n\nYeah, after sending that, I wondered if it would cause problems.\n\nI don't think it would be too hard to just cat it right into the\npacked-refs file. The other option would be to try creating it, and set\na prereq to skip other tests if it fails.\n\n-Peff\n"},{"id":"253459","messageId":"xmqqa92wla34.fsf@gitster.dls.corp.google.com","threadId":"38152","inReplyTo":"20141209180916.GA26873@peff.net","subject":"Re: [RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-09T22:41:51Z","receivedAt":"2014-12-09T22:41:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Dec 09, 2014 at 12:49:58PM -0500, Jeff King wrote:\n>\n>> Another option would be to use a static strbuf. Then we're only wasting\n>> heap, and even then only as much as we need (we'd still manually cap it\n>> at LARGE_PACKET_MAX since that's what the protocol dictates). This would\n>> also make packet_buf_write more efficient (right now it formats into a\n>> static buffer, and then copies the result into a strbuf; probably not\n>> measurably important, but silly nonetheless).\n>\n> Below is what that would look like. It's obviously a much more invasive\n> change, but I think the result is nice.\n\nYes, indeed.  Is there any reason why we shouldn't go with this\nvariant, other than \"it touches a bit more lines\" that I am not\nseeing?\n\n> Let's switch to using a strbuf, with a hard-limit of\n> LARGE_PACKET_MAX (which is specified by the protocol).  This\n> matches the size of the readers, as of 74543a0 (pkt-line:\n> provide a LARGE_PACKET_MAX static buffer, 2013-02-20).\n> Versions of git older than that will complain about our\n> large packets, but it's really no worse than the current\n> behavior. Right now the sender barfs with \"impossibly long\n> line\" trying to send the packet, and afterwards the reader\n> will barf with \"protocol error: bad line length %d\", which\n> is arguably better anyway.\n\nAnything older than 1.8.3 is affected by this, but only when the\nsending side has to send a large packet.  It is between failing\nbecause the sender cannot send a large packet and failing because\nthe receiver does not expect such a large packet to come, and either\nway the whole operation will fail anyway, so there is no net loss.\n"},{"id":"253475","messageId":"CAO2U3QjXvs1FsfsnW1wpgWbRAWLU3kJHYT45wJTG4S1yxxPT6Q@mail.gmail.com","threadId":"38152","inReplyTo":"xmqqa92wla34.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Michael Blume","fromEmail":"blume.mike@gmail.com","sentAt":"2014-12-10T04:36:22Z","receivedAt":"2014-12-10T04:36:22Z","isPatch":true,"sender":{"key":"blume.mike@gmail.com","avatar":"https://gravatar.com/avatar/1a7b440e1d942425ff4098ac7fc15b86b30cecaa56e1692a7ef8b5939ba25ea7?d=mp&s=160"},"body":"On Tue, Dec 9, 2014 at 2:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> On Tue, Dec 09, 2014 at 12:49:58PM -0500, Jeff King wrote:\n>>\n>>> Another option would be to use a static strbuf. Then we're only wasting\n>>> heap, and even then only as much as we need (we'd still manually cap it\n>>> at LARGE_PACKET_MAX since that's what the protocol dictates). This would\n>>> also make packet_buf_write more efficient (right now it formats into a\n>>> static buffer, and then copies the result into a strbuf; probably not\n>>> measurably important, but silly nonetheless).\n>>\n>> Below is what that would look like. It's obviously a much more invasive\n>> change, but I think the result is nice.\n>\n> Yes, indeed.  Is there any reason why we shouldn't go with this\n> variant, other than \"it touches a bit more lines\" that I am not\n> seeing?\n>\n>> Let's switch to using a strbuf, with a hard-limit of\n>> LARGE_PACKET_MAX (which is specified by the protocol).  This\n>> matches the size of the readers, as of 74543a0 (pkt-line:\n>> provide a LARGE_PACKET_MAX static buffer, 2013-02-20).\n>> Versions of git older than that will complain about our\n>> large packets, but it's really no worse than the current\n>> behavior. Right now the sender barfs with \"impossibly long\n>> line\" trying to send the packet, and afterwards the reader\n>> will barf with \"protocol error: bad line length %d\", which\n>> is arguably better anyway.\n>\n> Anything older than 1.8.3 is affected by this, but only when the\n> sending side has to send a large packet.  It is between failing\n> because the sender cannot send a large packet and failing because\n> the receiver does not expect such a large packet to come, and either\n> way the whole operation will fail anyway, so there is no net loss.\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\nI'm getting failures on my mac too, I assume filesystem-related.\n"},{"id":"253479","messageId":"20141210073447.GA20298@peff.net","threadId":"38152","inReplyTo":"xmqqa92wla34.fsf@gitster.dls.corp.google.com","subject":"[PATCH v3] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T07:34:47Z","receivedAt":"2014-12-10T07:34:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 09, 2014 at 02:41:51PM -0800, Junio C Hamano wrote:\n\n> >> Another option would be to use a static strbuf. Then we're only wasting\n> >> heap, and even then only as much as we need (we'd still manually cap it\n> >> at LARGE_PACKET_MAX since that's what the protocol dictates). This would\n> >> also make packet_buf_write more efficient (right now it formats into a\n> >> static buffer, and then copies the result into a strbuf; probably not\n> >> measurably important, but silly nonetheless).\n> >\n> > Below is what that would look like. It's obviously a much more invasive\n> > change, but I think the result is nice.\n> \n> Yes, indeed.  Is there any reason why we shouldn't go with this\n> variant, other than \"it touches a bit more lines\" that I am not\n> seeing?\n\nI don't think so. Mostly I wrote and sent the minimal one first, and\nonly later realized that the strbuf approach would be so neat.\n\n> Anything older than 1.8.3 is affected by this, but only when the\n> sending side has to send a large packet.  It is between failing\n> because the sender cannot send a large packet and failing because\n> the receiver does not expect such a large packet to come, and either\n> way the whole operation will fail anyway, so there is no net loss.\n\nExactly.\n\nBelow is a another iteration on the patch. The actual code changes are\nthe same as the strbuf one, but the tests take care to avoid assuming\nthe filesystem can handle such a long path. Testing on Windows and OS X\nis appreciated.\n\nNote that in addition to cheating on the creation of the long ref, I had\nto tweak the fetch test a little to avoid writing the loose ref there,\ntoo. That makes the test a little weaker (it is not as \"end to end\",\nchecking that all parts of fetch can handle it), but it does check the\nthing we are changing here, that the protocol code can handle it.\n\n-- >8 --\nSubject: [PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers\n\nWhen we send out pkt-lines with refnames, we use a static\n1000-byte buffer. This means that the maximum size of a ref\nover the git protocol is around 950 bytes (the exact size\ndepends on the protocol line being written, but figure on a sha1\nplus some boilerplate).\n\nThis is enough for any sane workflow, but occasionally odd\nthings happen (e.g., a bug may create a ref \"foo/foo/foo/...\"\naccidentally).  With the current code, you cannot even use\n\"push\" to delete such a ref from a remote.\n\nLet's switch to using a strbuf, with a hard-limit of\nLARGE_PACKET_MAX (which is specified by the protocol).  This\nmatches the size of the readers, as of 74543a0 (pkt-line:\nprovide a LARGE_PACKET_MAX static buffer, 2013-02-20).\nVersions of git older than that will complain about our\nlarge packets, but it's really no worse than the current\nbehavior. Right now the sender barfs with \"impossibly long\nline\" trying to send the packet, and afterwards the reader\nwill barf with \"protocol error: bad line length %d\", which\nis arguably better anyway.\n\nNote that we're not really _solving_ the problem here, but\njust bumping the limits. In theory, the length of a ref is\nunbounded, and pkt-line can only represent sizes up to\n65531 bytes. So we are just bumping the limit, not removing\nit.  But hopefully 64K should be enough for anyone.\n\nAs a bonus, by using a strbuf for the formatting we can\neliminate an unnecessary copy in format_buf_write.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pkt-line.c                | 37 +++++++++++++++++++------------------\n t/t5527-fetch-odd-refs.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 61 insertions(+), 18 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 8bc89b1..187a229 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -64,44 +64,45 @@ void packet_buf_flush(struct strbuf *buf)\n }\n \n #define hex(a) (hexchar[(a) & 15])\n-static char buffer[1000];\n-static unsigned format_packet(const char *fmt, va_list args)\n+static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n {\n \tstatic char hexchar[] = \"0123456789abcdef\";\n-\tunsigned n;\n+\tsize_t orig_len, n;\n \n-\tn = vsnprintf(buffer + 4, sizeof(buffer) - 4, fmt, args);\n-\tif (n >= sizeof(buffer)-4)\n+\torig_len = out->len;\n+\tstrbuf_addstr(out, \"0000\");\n+\tstrbuf_vaddf(out, fmt, args);\n+\tn = out->len - orig_len;\n+\n+\tif (n > LARGE_PACKET_MAX)\n \t\tdie(\"protocol error: impossibly long line\");\n-\tn += 4;\n-\tbuffer[0] = hex(n >> 12);\n-\tbuffer[1] = hex(n >> 8);\n-\tbuffer[2] = hex(n >> 4);\n-\tbuffer[3] = hex(n);\n-\tpacket_trace(buffer+4, n-4, 1);\n-\treturn n;\n+\n+\tout->buf[orig_len + 0] = hex(n >> 12);\n+\tout->buf[orig_len + 1] = hex(n >> 8);\n+\tout->buf[orig_len + 2] = hex(n >> 4);\n+\tout->buf[orig_len + 3] = hex(n);\n+\tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n }\n \n void packet_write(int fd, const char *fmt, ...)\n {\n+\tstatic struct strbuf buf = STRBUF_INIT;\n \tva_list args;\n-\tunsigned n;\n \n+\tstrbuf_reset(&buf);\n \tva_start(args, fmt);\n-\tn = format_packet(fmt, args);\n+\tformat_packet(&buf, fmt, args);\n \tva_end(args);\n-\twrite_or_die(fd, buffer, n);\n+\twrite_or_die(fd, buf.buf, buf.len);\n }\n \n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n {\n \tva_list args;\n-\tunsigned n;\n \n \tva_start(args, fmt);\n-\tn = format_packet(fmt, args);\n+\tformat_packet(buf, fmt, args);\n \tva_end(args);\n-\tstrbuf_add(buf, buffer, n);\n }\n \n static int get_packet_data(int fd, char **src_buf, size_t *src_size,\ndiff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh\nindex edea9f9..0921a2a 100755\n--- a/t/t5527-fetch-odd-refs.sh\n+++ b/t/t5527-fetch-odd-refs.sh\n@@ -26,4 +26,46 @@ test_expect_success 'suffix ref is ignored during fetch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'create repo with absurdly long refname' '\n+\tref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40\n+\tref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&\n+\tgit init long &&\n+\t(\n+\t\tcd long &&\n+\t\ttest_commit long &&\n+\t\ttest_commit master &&\n+\n+\t\t# not all filesystems can handle such long filenames, so\n+\t\t# we cheat a bit and explicitly create our long ref as a\n+\t\t# packed ref. Since we are doing it by hand, let us also\n+\t\t# double check that git respects what we wrote.\n+\t\tlong=$(git rev-parse long) &&\n+\t\techo \"$long refs/heads/$ref1440\" >.git/packed-refs &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\tlong\n+\t\tmaster\n+\t\tEOF\n+\t\tgit for-each-ref --format=\"%(subject)\" refs/heads >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'fetch handles extremely long refname' '\n+\t# do not fetch it verbatim, as many systems would barf not\n+\t# on the protocol, but on writing the loose ref locally.\n+\t# Instead, give it a sane local name and just make sure\n+\t# we got it.\n+\tgit fetch long refs/heads/$ref1440:refs/heads/long-ref &&\n+\techo long >expect &&\n+\tgit log -1 --format=%s long-ref >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'push handles extremely long refname' '\n+\tgit push long :refs/heads/$ref1440 &&\n+\tgit -C long for-each-ref --format=\"%(subject)\" refs/heads >actual &&\n+\techo master >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253482","messageId":"CAPig+cQQThA7wiz8iwkKX=ipg1n5w+gyeS8NqtbjGui986Hn+g@mail.gmail.com","threadId":"38152","inReplyTo":"20141210073447.GA20298@peff.net","subject":"Re: [PATCH v3] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-10T08:36:31Z","receivedAt":"2014-12-10T08:36:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:\n> Below is a another iteration on the patch. The actual code changes are\n> the same as the strbuf one, but the tests take care to avoid assuming\n> the filesystem can handle such a long path. Testing on Windows and OS X\n> is appreciated.\n\nAll three new tests fail on OS X. Thus far brief examination of the\nfirst failing tests shows that 'expect' and 'actual' differ:\n\nexpect:\n    long\n    master\n\nactual:\n    master\n\n> Note that in addition to cheating on the creation of the long ref, I had\n> to tweak the fetch test a little to avoid writing the loose ref there,\n> too. That makes the test a little weaker (it is not as \"end to end\",\n> checking that all parts of fetch can handle it), but it does check the\n> thing we are changing here, that the protocol code can handle it.\n"},{"id":"253488","messageId":"CAPig+cR4p9C46wU2-nNVy7rpXzbW0fGmqzik85UP_1j3YUEmjA@mail.gmail.com","threadId":"38152","inReplyTo":"CAPig+cQQThA7wiz8iwkKX=ipg1n5w+gyeS8NqtbjGui986Hn+g@mail.gmail.com","subject":"Re: [PATCH v3] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-10T09:42:47Z","receivedAt":"2014-12-10T09:42:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 10, 2014 at 3:36 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:\n>> Below is a another iteration on the patch. The actual code changes are\n>> the same as the strbuf one, but the tests take care to avoid assuming\n>> the filesystem can handle such a long path. Testing on Windows and OS X\n>> is appreciated.\n>\n> All three new tests fail on OS X. Thus far brief examination of the\n> first failing tests shows that 'expect' and 'actual' differ:\n>\n> expect:\n>     long\n>     master\n>\n> actual:\n>     master\n\nThe failure manifests as soon as the refname hits length 1024, at\nwhich point for-each-ref stops reporting it. MAX_PATH on OS X is 1024,\nso some part of the machinery invoked by for-each-ref likely is\nrejecting refnames longer than that (even when coming from\npacked-refs).\n"},{"id":"253489","messageId":"20141210094702.GA8917@peff.net","threadId":"38152","inReplyTo":"CAPig+cQQThA7wiz8iwkKX=ipg1n5w+gyeS8NqtbjGui986Hn+g@mail.gmail.com","subject":"[PATCH v4] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T09:47:02Z","receivedAt":"2014-12-10T09:47:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 10, 2014 at 03:36:31AM -0500, Eric Sunshine wrote:\n\n> On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:\n> > Below is a another iteration on the patch. The actual code changes are\n> > the same as the strbuf one, but the tests take care to avoid assuming\n> > the filesystem can handle such a long path. Testing on Windows and OS X\n> > is appreciated.\n> \n> All three new tests fail on OS X. Thus far brief examination of the\n> first failing tests shows that 'expect' and 'actual' differ:\n> \n> expect:\n>     long\n>     master\n> \n> actual:\n>     master\n\nUgh. It looks like the packed-refs reader uses a PATH_MAX-sized buffer\nto read each line, and simply omits the ref. That may be something we\nwant to work on, but it's a separate topic.  I think there are platforms\nwhose PATH_MAX is much smaller than what they can actually represent.\nSo ideally we would lift the arbitrary PATH_MAX limitations inside git,\nand just let the OS complain when we exceed its limits.\n\nEven if we fix that, though, I think the push test would still fail on\nsystems that have a true limit on the filesystem.  Writing to a ref\nrequires taking a lock in the filesystem, which will involve creating\nthe too-long path. I think there are simply some systems that will not\nsupport long refs well, and the tests need to be skipped there.\n\nSo here's a re-roll that uses prerequisites.\n\n-Peff\n\n-- >8 --\nSubject: pkt-line: allow writing of LARGE_PACKET_MAX buffers\n\nWhen we send out pkt-lines with refnames, we use a static\n1000-byte buffer. This means that the maximum size of a ref\nover the git protocol is around 950 bytes (the exact size\ndepends on the protocol line being written, but figure on a sha1\nplus some boilerplate).\n\nThis is enough for any sane workflow, but occasionally odd\nthings happen (e.g., a bug may create a ref \"foo/foo/foo/...\"\naccidentally).  With the current code, you cannot even use\n\"push\" to delete such a ref from a remote.\n\nLet's switch to using a strbuf, with a hard-limit of\nLARGE_PACKET_MAX (which is specified by the protocol).  This\nmatches the size of the readers, as of 74543a0 (pkt-line:\nprovide a LARGE_PACKET_MAX static buffer, 2013-02-20).\nVersions of git older than that will complain about our\nlarge packets, but it's really no worse than the current\nbehavior. Right now the sender barfs with \"impossibly long\nline\" trying to send the packet, and afterwards the reader\nwill barf with \"protocol error: bad line length %d\", which\nis arguably better anyway.\n\nNote that we're not really _solving_ the problem here, but\njust bumping the limits. In theory, the length of a ref is\nunbounded, and pkt-line can only represent sizes up to\n65531 bytes. So we are just bumping the limit, not removing\nit.  But hopefully 64K should be enough for anyone.\n\nAs a bonus, by using a strbuf for the formatting we can\neliminate an unnecessary copy in format_buf_write.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pkt-line.c                | 37 +++++++++++++++++++------------------\n t/t5527-fetch-odd-refs.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 18 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 8bc89b1..187a229 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -64,44 +64,45 @@ void packet_buf_flush(struct strbuf *buf)\n }\n \n #define hex(a) (hexchar[(a) & 15])\n-static char buffer[1000];\n-static unsigned format_packet(const char *fmt, va_list args)\n+static void format_packet(struct strbuf *out, const char *fmt, va_list args)\n {\n \tstatic char hexchar[] = \"0123456789abcdef\";\n-\tunsigned n;\n+\tsize_t orig_len, n;\n \n-\tn = vsnprintf(buffer + 4, sizeof(buffer) - 4, fmt, args);\n-\tif (n >= sizeof(buffer)-4)\n+\torig_len = out->len;\n+\tstrbuf_addstr(out, \"0000\");\n+\tstrbuf_vaddf(out, fmt, args);\n+\tn = out->len - orig_len;\n+\n+\tif (n > LARGE_PACKET_MAX)\n \t\tdie(\"protocol error: impossibly long line\");\n-\tn += 4;\n-\tbuffer[0] = hex(n >> 12);\n-\tbuffer[1] = hex(n >> 8);\n-\tbuffer[2] = hex(n >> 4);\n-\tbuffer[3] = hex(n);\n-\tpacket_trace(buffer+4, n-4, 1);\n-\treturn n;\n+\n+\tout->buf[orig_len + 0] = hex(n >> 12);\n+\tout->buf[orig_len + 1] = hex(n >> 8);\n+\tout->buf[orig_len + 2] = hex(n >> 4);\n+\tout->buf[orig_len + 3] = hex(n);\n+\tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n }\n \n void packet_write(int fd, const char *fmt, ...)\n {\n+\tstatic struct strbuf buf = STRBUF_INIT;\n \tva_list args;\n-\tunsigned n;\n \n+\tstrbuf_reset(&buf);\n \tva_start(args, fmt);\n-\tn = format_packet(fmt, args);\n+\tformat_packet(&buf, fmt, args);\n \tva_end(args);\n-\twrite_or_die(fd, buffer, n);\n+\twrite_or_die(fd, buf.buf, buf.len);\n }\n \n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n {\n \tva_list args;\n-\tunsigned n;\n \n \tva_start(args, fmt);\n-\tn = format_packet(fmt, args);\n+\tformat_packet(buf, fmt, args);\n \tva_end(args);\n-\tstrbuf_add(buf, buffer, n);\n }\n \n static int get_packet_data(int fd, char **src_buf, size_t *src_size,\ndiff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh\nindex edea9f9..85bcb2e 100755\n--- a/t/t5527-fetch-odd-refs.sh\n+++ b/t/t5527-fetch-odd-refs.sh\n@@ -26,4 +26,37 @@ test_expect_success 'suffix ref is ignored during fetch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'try to create repo with absurdly long refname' '\n+\tref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40\n+\tref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&\n+\tgit init long &&\n+\t(\n+\t\tcd long &&\n+\t\ttest_commit long &&\n+\t\ttest_commit master\n+\t) &&\n+\tif git -C long update-ref refs/heads/$ref1440 long; then\n+\t\ttest_set_prereq LONG_REF\n+\telse\n+\t\techo >&2 \"long refs not supported\"\n+\tfi\n+'\n+\n+test_expect_success LONG_REF 'fetch handles extremely long refname' '\n+\tgit fetch long refs/heads/*:refs/remotes/long/* &&\n+\tcat >expect <<-\\EOF &&\n+\tlong\n+\tmaster\n+\tEOF\n+\tgit for-each-ref --format=\"%(subject)\" refs/remotes/long >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success LONG_REF 'push handles extremely long refname' '\n+\tgit push long :refs/heads/$ref1440 &&\n+\tgit -C long for-each-ref --format=\"%(subject)\" refs/heads >actual &&\n+\techo master >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253490","messageId":"CAPig+cT9rRXdZ5OS8HPBuNOh2P-+PVYZkGR-74rBfXsc2nj_Zw@mail.gmail.com","threadId":"38152","inReplyTo":"CAPig+cR4p9C46wU2-nNVy7rpXzbW0fGmqzik85UP_1j3YUEmjA@mail.gmail.com","subject":"Re: [PATCH v3] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-10T09:49:38Z","receivedAt":"2014-12-10T09:49:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 10, 2014 at 4:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Dec 10, 2014 at 3:36 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:\n>>> Below is a another iteration on the patch. The actual code changes are\n>>> the same as the strbuf one, but the tests take care to avoid assuming\n>>> the filesystem can handle such a long path. Testing on Windows and OS X\n>>> is appreciated.\n>>\n>> All three new tests fail on OS X. Thus far brief examination of the\n>> first failing tests shows that 'expect' and 'actual' differ:\n>>\n>> expect:\n>>     long\n>>     master\n>>\n>> actual:\n>>     master\n>\n> The failure manifests as soon as the refname hits length 1024, at\n> which point for-each-ref stops reporting it. MAX_PATH on OS X is 1024,\n> so some part of the machinery invoked by for-each-ref likely is\n> rejecting refnames longer than that (even when coming from\n> packed-refs).\n\nClarification: for-each-ref ignores the ref when the full line read\nfrom packed-refs hits length 1024 (not when the refname itself hits\nlength 1024).\n"},{"id":"253491","messageId":"20141210095319.GA9099@peff.net","threadId":"38152","inReplyTo":"CAPig+cT9rRXdZ5OS8HPBuNOh2P-+PVYZkGR-74rBfXsc2nj_Zw@mail.gmail.com","subject":"Re: [PATCH v3] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T09:53:20Z","receivedAt":"2014-12-10T09:53:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 10, 2014 at 04:49:38AM -0500, Eric Sunshine wrote:\n\n> On Wed, Dec 10, 2014 at 4:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On Wed, Dec 10, 2014 at 3:36 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> >> On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:\n> >>> Below is a another iteration on the patch. The actual code changes are\n> >>> the same as the strbuf one, but the tests take care to avoid assuming\n> >>> the filesystem can handle such a long path. Testing on Windows and OS X\n> >>> is appreciated.\n> >>\n> >> All three new tests fail on OS X. Thus far brief examination of the\n> >> first failing tests shows that 'expect' and 'actual' differ:\n> >>\n> >> expect:\n> >>     long\n> >>     master\n> >>\n> >> actual:\n> >>     master\n> >\n> > The failure manifests as soon as the refname hits length 1024, at\n> > which point for-each-ref stops reporting it. MAX_PATH on OS X is 1024,\n> > so some part of the machinery invoked by for-each-ref likely is\n> > rejecting refnames longer than that (even when coming from\n> > packed-refs).\n> \n> Clarification: for-each-ref ignores the ref when the full line read\n> from packed-refs hits length 1024 (not when the refname itself hits\n> length 1024).\n\nYes, the problem is in read_packed_refs:\n\n    char refline[PATH_MAX];\n    ...\n    while (fgets(refline, sizeof(refline), f)) {\n        ...\n    }\n\nThis could be trivially converted to strbuf_getwholeline, but I am not\nsure what else would break, or whether such a system would actually be\n_usable_ with such long refs (e.g., would it break the first time you\n\nUsing fgets like this does shear lines, though. The next fgets call will\nsee the second half of the line. I think we are saved from doing\nanything stupid by parse_ref_line, but it is mostly luck. So perhaps for\nthat reason the trivial conversion to strbuf is worth it, even if it\ndoesn't help any practical cases.\n\n-Peff\n"},{"id":"253492","messageId":"CAPig+cS4461U4MH3+2UFJgm4K3puBfKg95zNCVYFq6SpwJUrag@mail.gmail.com","threadId":"38152","inReplyTo":"20141210094702.GA8917@peff.net","subject":"Re: [PATCH v4] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-10T09:56:05Z","receivedAt":"2014-12-10T09:56:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 10, 2014 at 4:47 AM, Jeff King <peff@peff.net> wrote:\n> On Wed, Dec 10, 2014 at 03:36:31AM -0500, Eric Sunshine wrote:\n>\n>> On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:\n>> > Below is a another iteration on the patch. The actual code changes are\n>> > the same as the strbuf one, but the tests take care to avoid assuming\n>> > the filesystem can handle such a long path. Testing on Windows and OS X\n>> > is appreciated.\n>>\n>> All three new tests fail on OS X. Thus far brief examination of the\n>> first failing tests shows that 'expect' and 'actual' differ:\n>>\n>> expect:\n>>     long\n>>     master\n>>\n>> actual:\n>>     master\n>\n> Ugh. It looks like the packed-refs reader uses a PATH_MAX-sized buffer\n> to read each line, and simply omits the ref. That may be something we\n> want to work on, but it's a separate topic.  I think there are platforms\n> whose PATH_MAX is much smaller than what they can actually represent.\n> So ideally we would lift the arbitrary PATH_MAX limitations inside git,\n> and just let the OS complain when we exceed its limits.\n>\n> Even if we fix that, though, I think the push test would still fail on\n> systems that have a true limit on the filesystem.  Writing to a ref\n> requires taking a lock in the filesystem, which will involve creating\n> the too-long path. I think there are simply some systems that will not\n> support long refs well, and the tests need to be skipped there.\n>\n> So here's a re-roll that uses prerequisites.\n\nOn OS X, this version correctly skips the two final tests as intended.\n"},{"id":"253493","messageId":"20141210103907.GA22186@peff.net","threadId":"38152","inReplyTo":"20141210095319.GA9099@peff.net","subject":"[PATCH 0/3] convert read_packed_refs to use strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T10:39:07Z","receivedAt":"2014-12-10T10:39:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 10, 2014 at 04:53:19AM -0500, Jeff King wrote:\n\n> > Clarification: for-each-ref ignores the ref when the full line read\n> > from packed-refs hits length 1024 (not when the refname itself hits\n> > length 1024).\n> \n> Yes, the problem is in read_packed_refs:\n> \n>     char refline[PATH_MAX];\n>     ...\n>     while (fgets(refline, sizeof(refline), f)) {\n>         ...\n>     }\n> \n> This could be trivially converted to strbuf_getwholeline, but I am not\n> sure what else would break, or whether such a system would actually be\n> _usable_ with such long refs (e.g., would it break the first time you\n\nI accidentally cut off the next line, but it was something like\n\"...first time you actually tried writing to the ref)\".\n\n> Using fgets like this does shear lines, though. The next fgets call will\n> see the second half of the line. I think we are saved from doing\n> anything stupid by parse_ref_line, but it is mostly luck. So perhaps for\n> that reason the trivial conversion to strbuf is worth it, even if it\n> doesn't help any practical cases.\n\nHere's a patch to do that. It still doesn't let you create long refs on\nOS X, as we get caught up in the PATH_MAX found in git_path() and\nfriends. Still, I think it's a step in the right direction, and it fixes\nthe shearing issue.\n\nPatches 2 and 3 are just follow-on cleanups.\n\n  [1/3]: read_packed_refs: use a strbuf for reading lines\n  [2/3]: read_packed_refs: pass strbuf to parse_ref_line\n  [3/3]: read_packed_refs: use skip_prefix instead of static array\n\nI checked, and this miraculously does not conflict with any of the refs\nwork in pu. :)\n\n-Peff\n"},{"id":"253494","messageId":"20141210104007.GA24514@peff.net","threadId":"38152","inReplyTo":"20141210103907.GA22186@peff.net","subject":"[PATCH 1/3] read_packed_refs: use a strbuf for reading lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T10:40:07Z","receivedAt":"2014-12-10T10:40:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently used a fixed PATH_MAX-sized buffer for reading\npacked-refs lines. This is a reasonable guess, in the sense\nthat git generally cannot work with refs larger than\nPATH_MAX. However, there are a few cases where it is not\ngreat:\n\n  1. Some systems may have a low value of PATH_MAX, but can\n     actually handle larger paths in practice. Fixing this\n     code path probably isn't enough to make them work\n     completely with long refs, but it is a step in the\n     right direction.\n\n  2. We use fgets, which will happily give us half a line on\n     the first read, and then the rest of the line on the\n     second. This is probably OK in practice, because our\n     refline parser is careful enough to look for the\n     trailing newline on the first line. The second line may\n     look like a peeled line to us, but since \"^\" is illegal\n     in refnames, it is not likely to come up.\n\n     Still, it does not hurt to be more careful.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..6f31935 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1126,16 +1126,16 @@ static const char *parse_ref_line(char *line, unsigned char *sha1)\n static void read_packed_refs(FILE *f, struct ref_dir *dir)\n {\n \tstruct ref_entry *last = NULL;\n-\tchar refline[PATH_MAX];\n+\tstruct strbuf line = STRBUF_INIT;\n \tenum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;\n \n-\twhile (fgets(refline, sizeof(refline), f)) {\n+\twhile (strbuf_getwholeline(&line, f, '\\n') != EOF) {\n \t\tunsigned char sha1[20];\n \t\tconst char *refname;\n \t\tstatic const char header[] = \"# pack-refs with:\";\n \n-\t\tif (!strncmp(refline, header, sizeof(header)-1)) {\n-\t\t\tconst char *traits = refline + sizeof(header) - 1;\n+\t\tif (!strncmp(line.buf, header, sizeof(header)-1)) {\n+\t\t\tconst char *traits = line.buf + sizeof(header) - 1;\n \t\t\tif (strstr(traits, \" fully-peeled \"))\n \t\t\t\tpeeled = PEELED_FULLY;\n \t\t\telse if (strstr(traits, \" peeled \"))\n@@ -1144,7 +1144,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t\t\tcontinue;\n \t\t}\n \n-\t\trefname = parse_ref_line(refline, sha1);\n+\t\trefname = parse_ref_line(line.buf, sha1);\n \t\tif (refname) {\n \t\t\tint flag = REF_ISPACKED;\n \n@@ -1160,10 +1160,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t\t\tcontinue;\n \t\t}\n \t\tif (last &&\n-\t\t    refline[0] == '^' &&\n-\t\t    strlen(refline) == PEELED_LINE_LENGTH &&\n-\t\t    refline[PEELED_LINE_LENGTH - 1] == '\\n' &&\n-\t\t    !get_sha1_hex(refline + 1, sha1)) {\n+\t\t    line.buf[0] == '^' &&\n+\t\t    line.len == PEELED_LINE_LENGTH &&\n+\t\t    line.buf[PEELED_LINE_LENGTH - 1] == '\\n' &&\n+\t\t    !get_sha1_hex(line.buf + 1, sha1)) {\n \t\t\thashcpy(last->u.value.peeled, sha1);\n \t\t\t/*\n \t\t\t * Regardless of what the file header said,\n@@ -1173,6 +1173,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t\t\tlast->flag |= REF_KNOWS_PEELED;\n \t\t}\n \t}\n+\n+\tstrbuf_release(&line);\n }\n \n /*\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253495","messageId":"20141210104019.GB24514@peff.net","threadId":"38152","inReplyTo":"20141210103907.GA22186@peff.net","subject":"[PATCH 2/3] read_packed_refs: pass strbuf to parse_ref_line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T10:40:19Z","receivedAt":"2014-12-10T10:40:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that we have a strbuf in read_packed_refs, we can pass\nit straight to the line parser, which saves us an extra\nstrlen.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6f31935..10f8247 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1068,8 +1068,10 @@ static const char PACKED_REFS_HEADER[] =\n  * Return a pointer to the refname within the line (null-terminated),\n  * or NULL if there was a problem.\n  */\n-static const char *parse_ref_line(char *line, unsigned char *sha1)\n+static const char *parse_ref_line(struct strbuf *line, unsigned char *sha1)\n {\n+\tconst char *ref;\n+\n \t/*\n \t * 42: the answer to everything.\n \t *\n@@ -1078,22 +1080,23 @@ static const char *parse_ref_line(char *line, unsigned char *sha1)\n \t *  +1 (space in between hex and name)\n \t *  +1 (newline at the end of the line)\n \t */\n-\tint len = strlen(line) - 42;\n-\n-\tif (len <= 0)\n+\tif (line->len <= 42)\n \t\treturn NULL;\n-\tif (get_sha1_hex(line, sha1) < 0)\n+\n+\tif (get_sha1_hex(line->buf, sha1) < 0)\n \t\treturn NULL;\n-\tif (!isspace(line[40]))\n+\tif (!isspace(line->buf[40]))\n \t\treturn NULL;\n-\tline += 41;\n-\tif (isspace(*line))\n+\n+\tref = line->buf + 41;\n+\tif (isspace(*ref))\n \t\treturn NULL;\n-\tif (line[len] != '\\n')\n+\n+\tif (line->buf[line->len - 1] != '\\n')\n \t\treturn NULL;\n-\tline[len] = 0;\n+\tline->buf[--line->len] = 0;\n \n-\treturn line;\n+\treturn ref;\n }\n \n /*\n@@ -1144,7 +1147,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t\t\tcontinue;\n \t\t}\n \n-\t\trefname = parse_ref_line(line.buf, sha1);\n+\t\trefname = parse_ref_line(&line, sha1);\n \t\tif (refname) {\n \t\t\tint flag = REF_ISPACKED;\n \n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253496","messageId":"20141210104035.GC24514@peff.net","threadId":"38152","inReplyTo":"20141210103907.GA22186@peff.net","subject":"[PATCH 3/3] read_packed_refs: use skip_prefix instead of static array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T10:40:36Z","receivedAt":"2014-12-10T10:40:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We want to recognize the packed-refs header and skip to the\n\"traits\" part of the line. We currently do it by feeding\nsizeof() a static const array to strncmp. However, it's a\nbit simpler to just skip_prefix, which expresses the\nintention more directly, and without remembering to account\nfor the NUL-terminator in each sizeof() call.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 10f8247..c71553f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1135,10 +1135,9 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \twhile (strbuf_getwholeline(&line, f, '\\n') != EOF) {\n \t\tunsigned char sha1[20];\n \t\tconst char *refname;\n-\t\tstatic const char header[] = \"# pack-refs with:\";\n+\t\tconst char *traits;\n \n-\t\tif (!strncmp(line.buf, header, sizeof(header)-1)) {\n-\t\t\tconst char *traits = line.buf + sizeof(header) - 1;\n+\t\tif (skip_prefix(line.buf, \"# pack-refs with:\", &traits)) {\n \t\t\tif (strstr(traits, \" fully-peeled \"))\n \t\t\t\tpeeled = PEELED_FULLY;\n \t\t\telse if (strstr(traits, \" peeled \"))\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253513","messageId":"xmqq7fxzjt96.fsf@gitster.dls.corp.google.com","threadId":"38152","inReplyTo":"20141210103907.GA22186@peff.net","subject":"Re: [PATCH 0/3] convert read_packed_refs to use strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-10T17:43:01Z","receivedAt":"2014-12-10T17:43:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here's a patch to do that. It still doesn't let you create long refs on\n> OS X, as we get caught up in the PATH_MAX found in git_path() and\n> friends. Still, I think it's a step in the right direction, and it fixes\n> the shearing issue.\n>\n> Patches 2 and 3 are just follow-on cleanups.\n>\n>   [1/3]: read_packed_refs: use a strbuf for reading lines\n>   [2/3]: read_packed_refs: pass strbuf to parse_ref_line\n>   [3/3]: read_packed_refs: use skip_prefix instead of static array\n>\n> I checked, and this miraculously does not conflict with any of the refs\n> work in pu. :)\n\nHeh, I suspect that is largely because Michael's \"reflog expire\" is\nkept out of my tree while it is still under discusson.\n"},{"id":"253533","messageId":"CAPig+cSx2dw7DsZgiebHqZUG7DbnQtpgxKbi2883Z42VajeAJA@mail.gmail.com","threadId":"38152","inReplyTo":"20141210094702.GA8917@peff.net","subject":"Re: [PATCH v4] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-10T20:14:17Z","receivedAt":"2014-12-10T20:14:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 10, 2014 at 4:47 AM, Jeff King <peff@peff.net> wrote:\n> Subject: pkt-line: allow writing of LARGE_PACKET_MAX buffers\n>\n> When we send out pkt-lines with refnames, we use a static\n> 1000-byte buffer. This means that the maximum size of a ref\n> over the git protocol is around 950 bytes (the exact size\n> depends on the protocol line being written, but figure on a sha1\n> plus some boilerplate).\n>\n> This is enough for any sane workflow, but occasionally odd\n> things happen (e.g., a bug may create a ref \"foo/foo/foo/...\"\n> accidentally).  With the current code, you cannot even use\n> \"push\" to delete such a ref from a remote.\n>\n> Let's switch to using a strbuf, with a hard-limit of\n> LARGE_PACKET_MAX (which is specified by the protocol).  This\n> matches the size of the readers, as of 74543a0 (pkt-line:\n> provide a LARGE_PACKET_MAX static buffer, 2013-02-20).\n> Versions of git older than that will complain about our\n> large packets, but it's really no worse than the current\n> behavior. Right now the sender barfs with \"impossibly long\n> line\" trying to send the packet, and afterwards the reader\n> will barf with \"protocol error: bad line length %d\", which\n> is arguably better anyway.\n>\n> Note that we're not really _solving_ the problem here, but\n> just bumping the limits. In theory, the length of a ref is\n> unbounded, and pkt-line can only represent sizes up to\n> 65531 bytes. So we are just bumping the limit, not removing\n> it.  But hopefully 64K should be enough for anyone.\n>\n> As a bonus, by using a strbuf for the formatting we can\n> eliminate an unnecessary copy in format_buf_write.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh\n> index edea9f9..85bcb2e 100755\n> --- a/t/t5527-fetch-odd-refs.sh\n> +++ b/t/t5527-fetch-odd-refs.sh\n> @@ -26,4 +26,37 @@ test_expect_success 'suffix ref is ignored during fetch' '\n>         test_cmp expect actual\n>  '\n>\n> +test_expect_success 'try to create repo with absurdly long refname' '\n> +       ref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40\n\nMaybe you want to keep the &&-chain intact here?\n\n> +       ref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&\n> +       git init long &&\n> +       (\n> +               cd long &&\n> +               test_commit long &&\n> +               test_commit master\n> +       ) &&\n> +       if git -C long update-ref refs/heads/$ref1440 long; then\n> +               test_set_prereq LONG_REF\n> +       else\n> +               echo >&2 \"long refs not supported\"\n> +       fi\n> +'\n> +\n> +test_expect_success LONG_REF 'fetch handles extremely long refname' '\n> +       git fetch long refs/heads/*:refs/remotes/long/* &&\n> +       cat >expect <<-\\EOF &&\n> +       long\n> +       master\n> +       EOF\n> +       git for-each-ref --format=\"%(subject)\" refs/remotes/long >actual &&\n> +       test_cmp expect actual\n> +'\n> +\n> +test_expect_success LONG_REF 'push handles extremely long refname' '\n> +       git push long :refs/heads/$ref1440 &&\n> +       git -C long for-each-ref --format=\"%(subject)\" refs/heads >actual &&\n> +       echo master >expect &&\n> +       test_cmp expect actual\n> +'\n> +\n>  test_done\n> --\n> 2.2.0.454.g7eca6b7\n>\n"},{"id":"253535","messageId":"20141210210651.GB31071@peff.net","threadId":"38152","inReplyTo":"CAPig+cSx2dw7DsZgiebHqZUG7DbnQtpgxKbi2883Z42VajeAJA@mail.gmail.com","subject":"Re: [PATCH v4] pkt-line: allow writing of LARGE_PACKET_MAX buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T21:06:51Z","receivedAt":"2014-12-10T21:06:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 10, 2014 at 03:14:17PM -0500, Eric Sunshine wrote:\n\n> > +test_expect_success 'try to create repo with absurdly long refname' '\n> > +       ref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40\n> \n> Maybe you want to keep the &&-chain intact here?\n\nThanks, yeah. It doesn't matter in practice, but we do try to &&-chain\neverything.\n\n-Peff\n"}]}