[RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Jeff King]

When we send out pkt-lines with refnames, we use a static
1000-byte buffer. This means that the maximum size of a ref
over the git protocol is around 950 bytes (the exact size
depends on the protocol line being written, but figure on a sha1
plus some boilerplate).

This is enough for any sane workflow, but occasionally odd
things happen (e.g., a bug may create a ref "foo/foo/foo/..."
accidentally).  With the current code, you cannot even use
"push" to delete such a ref from a remote.

Let's bump the size of the buffer to LARGE_PACKET_MAX. This
matches the size of the readers, as of 74543a0 (pkt-line:
provide a LARGE_PACKET_MAX static buffer, 2013-02-20).
Versions of git older than that will complain about our
large packets, but it's really no worse than the current
behavior. Right now the sender barfs with "impossibly long
line" trying to send the packet, and afterwards the reader
will barf with "protocol error: bad line length %d", which
is arguably better anyway.

Note that we're not really _solving_ the problem here, but
just bumping the limits. In theory, the length of a ref is
unbounded, and pkt-line can only represent sizes up to
65531 bytes. So we are just bumping the limit, not removing.
But hopefully 64K should be enough for anyone.

Signed-off-by: Jeff King <peff@peff.net>
---
This was inspired by a real-world case (with the "foo/foo" bug mentioned
above). It is rather an unlikely scenario, though (it literally was a
hook on the receiving end creating the ref, and push-deletion would have
been the simplest way to fix the situation). On the other hand, it's
nice to eliminate as many arbitrary limits as possible.

This does waste 63K of BSS. I was tempted to just use the existing
static packet_buffer (introduced by 74543a0) that the readers use. But
that is probably too magical; some callers may be surprised to find
the buffers they read invalidated by a call to packet_write. I think the
current callers are probably OK, but it's a little too error-prone for
my taste.

Another option would be to use a static strbuf. Then we're only wasting
heap, and even then only as much as we need (we'd still manually cap it
at LARGE_PACKET_MAX since that's what the protocol dictates). This would
also make packet_buf_write more efficient (right now it formats into a
static buffer, and then copies the result into a strbuf; probably not
measurably important, but silly nonetheless).

 pkt-line.c                |  2 +-
 t/t5527-fetch-odd-refs.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/pkt-line.c b/pkt-line.c
index 8bc89b1..aa42fb5 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -64,7 +64,7 @@ void packet_buf_flush(struct strbuf *buf)
 }
 
 #define hex(a) (hexchar[(a) & 15])
-static char buffer[1000];
+static char buffer[LARGE_PACKET_MAX];
 static unsigned format_packet(const char *fmt, va_list args)
 {
 	static char hexchar[] = "0123456789abcdef";
diff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh
index edea9f9..c86d38b 100755
--- a/t/t5527-fetch-odd-refs.sh
+++ b/t/t5527-fetch-odd-refs.sh
@@ -26,4 +26,33 @@ test_expect_success 'suffix ref is ignored during fetch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'create repo with absurdly long refname' '
+	ref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40
+	ref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&
+	git init long &&
+	(
+		cd long &&
+		test_commit long &&
+		test_commit master &&
+		git update-ref refs/heads/$ref1440 long
+	)
+'
+
+test_expect_success 'fetch handles extremely long refname' '
+	git fetch long refs/heads/*:refs/remotes/long/* &&
+	cat >expect <<-\EOF &&
+	long
+	master
+	EOF
+	git for-each-ref --format="%(subject)" refs/remotes/long >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push handles extremely long refname' '
+	git push long :refs/heads/$ref1440 &&
+	git -C long for-each-ref --format="%(subject)" refs/heads >actual &&
+	echo master >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.2.0.454.g7eca6b7

Re: [RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Jeff King]

On Tue, Dec 09, 2014 at 12:49:58PM -0500, Jeff King wrote:

> Another option would be to use a static strbuf. Then we're only wasting
> heap, and even then only as much as we need (we'd still manually cap it
> at LARGE_PACKET_MAX since that's what the protocol dictates). This would
> also make packet_buf_write more efficient (right now it formats into a
> static buffer, and then copies the result into a strbuf; probably not
> measurably important, but silly nonetheless).

Below is what that would look like. It's obviously a much more invasive
change, but I think the result is nice.

-- >8 --
Subject: pkt-line: allow writing of LARGE_PACKET_MAX buffers

When we send out pkt-lines with refnames, we use a static
1000-byte buffer. This means that the maximum size of a ref
over the git protocol is around 950 bytes (the exact size
depends on the protocol line being written, but figure on a sha1
plus some boilerplate).

This is enough for any sane workflow, but occasionally odd
things happen (e.g., a bug may create a ref "foo/foo/foo/..."
accidentally).  With the current code, you cannot even use
"push" to delete such a ref from a remote.

Let's switch to using a strbuf, with a hard-limit of
LARGE_PACKET_MAX (which is specified by the protocol).  This
matches the size of the readers, as of 74543a0 (pkt-line:
provide a LARGE_PACKET_MAX static buffer, 2013-02-20).
Versions of git older than that will complain about our
large packets, but it's really no worse than the current
behavior. Right now the sender barfs with "impossibly long
line" trying to send the packet, and afterwards the reader
will barf with "protocol error: bad line length %d", which
is arguably better anyway.

Note that we're not really _solving_ the problem here, but
just bumping the limits. In theory, the length of a ref is
unbounded, and pkt-line can only represent sizes up to
65531 bytes. So we are just bumping the limit, not removing
it.  But hopefully 64K should be enough for anyone.

As a bonus, by using a strbuf for the formatting we can
eliminate an unnecessary copy in format_buf_write.

Signed-off-by: Jeff King <peff@peff.net>
---
 pkt-line.c                | 37 +++++++++++++++++++------------------
 t/t5527-fetch-odd-refs.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 8bc89b1..187a229 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -64,44 +64,45 @@ void packet_buf_flush(struct strbuf *buf)
 }
 
 #define hex(a) (hexchar[(a) & 15])
-static char buffer[1000];
-static unsigned format_packet(const char *fmt, va_list args)
+static void format_packet(struct strbuf *out, const char *fmt, va_list args)
 {
 	static char hexchar[] = "0123456789abcdef";
-	unsigned n;
+	size_t orig_len, n;
 
-	n = vsnprintf(buffer + 4, sizeof(buffer) - 4, fmt, args);
-	if (n >= sizeof(buffer)-4)
+	orig_len = out->len;
+	strbuf_addstr(out, "0000");
+	strbuf_vaddf(out, fmt, args);
+	n = out->len - orig_len;
+
+	if (n > LARGE_PACKET_MAX)
 		die("protocol error: impossibly long line");
-	n += 4;
-	buffer[0] = hex(n >> 12);
-	buffer[1] = hex(n >> 8);
-	buffer[2] = hex(n >> 4);
-	buffer[3] = hex(n);
-	packet_trace(buffer+4, n-4, 1);
-	return n;
+
+	out->buf[orig_len + 0] = hex(n >> 12);
+	out->buf[orig_len + 1] = hex(n >> 8);
+	out->buf[orig_len + 2] = hex(n >> 4);
+	out->buf[orig_len + 3] = hex(n);
+	packet_trace(out->buf + orig_len + 4, n - 4, 1);
 }
 
 void packet_write(int fd, const char *fmt, ...)
 {
+	static struct strbuf buf = STRBUF_INIT;
 	va_list args;
-	unsigned n;
 
+	strbuf_reset(&buf);
 	va_start(args, fmt);
-	n = format_packet(fmt, args);
+	format_packet(&buf, fmt, args);
 	va_end(args);
-	write_or_die(fd, buffer, n);
+	write_or_die(fd, buf.buf, buf.len);
 }
 
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 {
 	va_list args;
-	unsigned n;
 
 	va_start(args, fmt);
-	n = format_packet(fmt, args);
+	format_packet(buf, fmt, args);
 	va_end(args);
-	strbuf_add(buf, buffer, n);
 }
 
 static int get_packet_data(int fd, char **src_buf, size_t *src_size,
diff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh
index edea9f9..c86d38b 100755
--- a/t/t5527-fetch-odd-refs.sh
+++ b/t/t5527-fetch-odd-refs.sh
@@ -26,4 +26,33 @@ test_expect_success 'suffix ref is ignored during fetch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'create repo with absurdly long refname' '
+	ref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40
+	ref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&
+	git init long &&
+	(
+		cd long &&
+		test_commit long &&
+		test_commit master &&
+		git update-ref refs/heads/$ref1440 long
+	)
+'
+
+test_expect_success 'fetch handles extremely long refname' '
+	git fetch long refs/heads/*:refs/remotes/long/* &&
+	cat >expect <<-\EOF &&
+	long
+	master
+	EOF
+	git for-each-ref --format="%(subject)" refs/remotes/long >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push handles extremely long refname' '
+	git push long :refs/heads/$ref1440 &&
+	git -C long for-each-ref --format="%(subject)" refs/heads >actual &&
+	echo master >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.2.0.454.g7eca6b7

Re: [RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Johannes Sixt]

Am 09.12.2014 um 18:49 schrieb Jeff King:
> +test_expect_success 'create repo with absurdly long refname' '
> +	ref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40
> +	ref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&
> +	git init long &&
> +	(
> +		cd long &&
> +		test_commit long &&
> +		test_commit master &&
> +		git update-ref refs/heads/$ref1440 long

Having this ref on the filesystem is going to fail on Windows, I presume
(our limit is 260). Can we stuff it away as a packed ref right from the
beginning? (And turn off reflogs, BTW.)

> +	)
> +'

-- Hannes

Re: [RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Jeff King]

On Tue, Dec 09, 2014 at 08:58:26PM +0100, Johannes Sixt wrote:

> Am 09.12.2014 um 18:49 schrieb Jeff King:
> > +test_expect_success 'create repo with absurdly long refname' '
> > +	ref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40
> > +	ref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&
> > +	git init long &&
> > +	(
> > +		cd long &&
> > +		test_commit long &&
> > +		test_commit master &&
> > +		git update-ref refs/heads/$ref1440 long
> 
> Having this ref on the filesystem is going to fail on Windows, I presume
> (our limit is 260). Can we stuff it away as a packed ref right from the
> beginning? (And turn off reflogs, BTW.)

Yeah, after sending that, I wondered if it would cause problems.

I don't think it would be too hard to just cat it right into the
packed-refs file. The other option would be to try creating it, and set
a prereq to skip other tests if it fails.

-Peff

Re: [RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Dec 09, 2014 at 12:49:58PM -0500, Jeff King wrote:
>
>> Another option would be to use a static strbuf. Then we're only wasting
>> heap, and even then only as much as we need (we'd still manually cap it
>> at LARGE_PACKET_MAX since that's what the protocol dictates). This would
>> also make packet_buf_write more efficient (right now it formats into a
>> static buffer, and then copies the result into a strbuf; probably not
>> measurably important, but silly nonetheless).
>
> Below is what that would look like. It's obviously a much more invasive
> change, but I think the result is nice.

Yes, indeed.  Is there any reason why we shouldn't go with this
variant, other than "it touches a bit more lines" that I am not
seeing?

> Let's switch to using a strbuf, with a hard-limit of
> LARGE_PACKET_MAX (which is specified by the protocol).  This
> matches the size of the readers, as of 74543a0 (pkt-line:
> provide a LARGE_PACKET_MAX static buffer, 2013-02-20).
> Versions of git older than that will complain about our
> large packets, but it's really no worse than the current
> behavior. Right now the sender barfs with "impossibly long
> line" trying to send the packet, and afterwards the reader
> will barf with "protocol error: bad line length %d", which
> is arguably better anyway.

Anything older than 1.8.3 is affected by this, but only when the
sending side has to send a large packet.  It is between failing
because the sender cannot send a large packet and failing because
the receiver does not expect such a large packet to come, and either
way the whole operation will fail anyway, so there is no net loss.

Re: [RFC/PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Michael Blume]

On Tue, Dec 9, 2014 at 2:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Tue, Dec 09, 2014 at 12:49:58PM -0500, Jeff King wrote:
>>
>>> Another option would be to use a static strbuf. Then we're only wasting
>>> heap, and even then only as much as we need (we'd still manually cap it
>>> at LARGE_PACKET_MAX since that's what the protocol dictates). This would
>>> also make packet_buf_write more efficient (right now it formats into a
>>> static buffer, and then copies the result into a strbuf; probably not
>>> measurably important, but silly nonetheless).
>>
>> Below is what that would look like. It's obviously a much more invasive
>> change, but I think the result is nice.
>
> Yes, indeed.  Is there any reason why we shouldn't go with this
> variant, other than "it touches a bit more lines" that I am not
> seeing?
>
>> Let's switch to using a strbuf, with a hard-limit of
>> LARGE_PACKET_MAX (which is specified by the protocol).  This
>> matches the size of the readers, as of 74543a0 (pkt-line:
>> provide a LARGE_PACKET_MAX static buffer, 2013-02-20).
>> Versions of git older than that will complain about our
>> large packets, but it's really no worse than the current
>> behavior. Right now the sender barfs with "impossibly long
>> line" trying to send the packet, and afterwards the reader
>> will barf with "protocol error: bad line length %d", which
>> is arguably better anyway.
>
> Anything older than 1.8.3 is affected by this, but only when the
> sending side has to send a large packet.  It is between failing
> because the sender cannot send a large packet and failing because
> the receiver does not expect such a large packet to come, and either
> way the whole operation will fail anyway, so there is no net loss.
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

I'm getting failures on my mac too, I assume filesystem-related.

[PATCH v3] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Jeff King]

On Tue, Dec 09, 2014 at 02:41:51PM -0800, Junio C Hamano wrote:

> >> Another option would be to use a static strbuf. Then we're only wasting
> >> heap, and even then only as much as we need (we'd still manually cap it
> >> at LARGE_PACKET_MAX since that's what the protocol dictates). This would
> >> also make packet_buf_write more efficient (right now it formats into a
> >> static buffer, and then copies the result into a strbuf; probably not
> >> measurably important, but silly nonetheless).
> >
> > Below is what that would look like. It's obviously a much more invasive
> > change, but I think the result is nice.
> 
> Yes, indeed.  Is there any reason why we shouldn't go with this
> variant, other than "it touches a bit more lines" that I am not
> seeing?

I don't think so. Mostly I wrote and sent the minimal one first, and
only later realized that the strbuf approach would be so neat.

> Anything older than 1.8.3 is affected by this, but only when the
> sending side has to send a large packet.  It is between failing
> because the sender cannot send a large packet and failing because
> the receiver does not expect such a large packet to come, and either
> way the whole operation will fail anyway, so there is no net loss.

Exactly.

Below is a another iteration on the patch. The actual code changes are
the same as the strbuf one, but the tests take care to avoid assuming
the filesystem can handle such a long path. Testing on Windows and OS X
is appreciated.

Note that in addition to cheating on the creation of the long ref, I had
to tweak the fetch test a little to avoid writing the loose ref there,
too. That makes the test a little weaker (it is not as "end to end",
checking that all parts of fetch can handle it), but it does check the
thing we are changing here, that the protocol code can handle it.

-- >8 --
Subject: [PATCH] pkt-line: allow writing of LARGE_PACKET_MAX buffers

When we send out pkt-lines with refnames, we use a static
1000-byte buffer. This means that the maximum size of a ref
over the git protocol is around 950 bytes (the exact size
depends on the protocol line being written, but figure on a sha1
plus some boilerplate).

This is enough for any sane workflow, but occasionally odd
things happen (e.g., a bug may create a ref "foo/foo/foo/..."
accidentally).  With the current code, you cannot even use
"push" to delete such a ref from a remote.

Let's switch to using a strbuf, with a hard-limit of
LARGE_PACKET_MAX (which is specified by the protocol).  This
matches the size of the readers, as of 74543a0 (pkt-line:
provide a LARGE_PACKET_MAX static buffer, 2013-02-20).
Versions of git older than that will complain about our
large packets, but it's really no worse than the current
behavior. Right now the sender barfs with "impossibly long
line" trying to send the packet, and afterwards the reader
will barf with "protocol error: bad line length %d", which
is arguably better anyway.

Note that we're not really _solving_ the problem here, but
just bumping the limits. In theory, the length of a ref is
unbounded, and pkt-line can only represent sizes up to
65531 bytes. So we are just bumping the limit, not removing
it.  But hopefully 64K should be enough for anyone.

As a bonus, by using a strbuf for the formatting we can
eliminate an unnecessary copy in format_buf_write.

Signed-off-by: Jeff King <peff@peff.net>
---
 pkt-line.c                | 37 +++++++++++++++++++------------------
 t/t5527-fetch-odd-refs.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 18 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 8bc89b1..187a229 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -64,44 +64,45 @@ void packet_buf_flush(struct strbuf *buf)
 }
 
 #define hex(a) (hexchar[(a) & 15])
-static char buffer[1000];
-static unsigned format_packet(const char *fmt, va_list args)
+static void format_packet(struct strbuf *out, const char *fmt, va_list args)
 {
 	static char hexchar[] = "0123456789abcdef";
-	unsigned n;
+	size_t orig_len, n;
 
-	n = vsnprintf(buffer + 4, sizeof(buffer) - 4, fmt, args);
-	if (n >= sizeof(buffer)-4)
+	orig_len = out->len;
+	strbuf_addstr(out, "0000");
+	strbuf_vaddf(out, fmt, args);
+	n = out->len - orig_len;
+
+	if (n > LARGE_PACKET_MAX)
 		die("protocol error: impossibly long line");
-	n += 4;
-	buffer[0] = hex(n >> 12);
-	buffer[1] = hex(n >> 8);
-	buffer[2] = hex(n >> 4);
-	buffer[3] = hex(n);
-	packet_trace(buffer+4, n-4, 1);
-	return n;
+
+	out->buf[orig_len + 0] = hex(n >> 12);
+	out->buf[orig_len + 1] = hex(n >> 8);
+	out->buf[orig_len + 2] = hex(n >> 4);
+	out->buf[orig_len + 3] = hex(n);
+	packet_trace(out->buf + orig_len + 4, n - 4, 1);
 }
 
 void packet_write(int fd, const char *fmt, ...)
 {
+	static struct strbuf buf = STRBUF_INIT;
 	va_list args;
-	unsigned n;
 
+	strbuf_reset(&buf);
 	va_start(args, fmt);
-	n = format_packet(fmt, args);
+	format_packet(&buf, fmt, args);
 	va_end(args);
-	write_or_die(fd, buffer, n);
+	write_or_die(fd, buf.buf, buf.len);
 }
 
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 {
 	va_list args;
-	unsigned n;
 
 	va_start(args, fmt);
-	n = format_packet(fmt, args);
+	format_packet(buf, fmt, args);
 	va_end(args);
-	strbuf_add(buf, buffer, n);
 }
 
 static int get_packet_data(int fd, char **src_buf, size_t *src_size,
diff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh
index edea9f9..0921a2a 100755
--- a/t/t5527-fetch-odd-refs.sh
+++ b/t/t5527-fetch-odd-refs.sh
@@ -26,4 +26,46 @@ test_expect_success 'suffix ref is ignored during fetch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'create repo with absurdly long refname' '
+	ref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40
+	ref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&
+	git init long &&
+	(
+		cd long &&
+		test_commit long &&
+		test_commit master &&
+
+		# not all filesystems can handle such long filenames, so
+		# we cheat a bit and explicitly create our long ref as a
+		# packed ref. Since we are doing it by hand, let us also
+		# double check that git respects what we wrote.
+		long=$(git rev-parse long) &&
+		echo "$long refs/heads/$ref1440" >.git/packed-refs &&
+		cat >expect <<-\EOF &&
+		long
+		master
+		EOF
+		git for-each-ref --format="%(subject)" refs/heads >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'fetch handles extremely long refname' '
+	# do not fetch it verbatim, as many systems would barf not
+	# on the protocol, but on writing the loose ref locally.
+	# Instead, give it a sane local name and just make sure
+	# we got it.
+	git fetch long refs/heads/$ref1440:refs/heads/long-ref &&
+	echo long >expect &&
+	git log -1 --format=%s long-ref >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push handles extremely long refname' '
+	git push long :refs/heads/$ref1440 &&
+	git -C long for-each-ref --format="%(subject)" refs/heads >actual &&
+	echo master >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.2.0.454.g7eca6b7

Re: [PATCH v3] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Eric Sunshine]

On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:
> Below is a another iteration on the patch. The actual code changes are
> the same as the strbuf one, but the tests take care to avoid assuming
> the filesystem can handle such a long path. Testing on Windows and OS X
> is appreciated.

All three new tests fail on OS X. Thus far brief examination of the
first failing tests shows that 'expect' and 'actual' differ:

expect:
    long
    master

actual:
    master

> Note that in addition to cheating on the creation of the long ref, I had
> to tweak the fetch test a little to avoid writing the loose ref there,
> too. That makes the test a little weaker (it is not as "end to end",
> checking that all parts of fetch can handle it), but it does check the
> thing we are changing here, that the protocol code can handle it.

Re: [PATCH v3] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Eric Sunshine]

On Wed, Dec 10, 2014 at 3:36 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:
>> Below is a another iteration on the patch. The actual code changes are
>> the same as the strbuf one, but the tests take care to avoid assuming
>> the filesystem can handle such a long path. Testing on Windows and OS X
>> is appreciated.
>
> All three new tests fail on OS X. Thus far brief examination of the
> first failing tests shows that 'expect' and 'actual' differ:
>
> expect:
>     long
>     master
>
> actual:
>     master

The failure manifests as soon as the refname hits length 1024, at
which point for-each-ref stops reporting it. MAX_PATH on OS X is 1024,
so some part of the machinery invoked by for-each-ref likely is
rejecting refnames longer than that (even when coming from
packed-refs).

[PATCH v4] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Jeff King]

On Wed, Dec 10, 2014 at 03:36:31AM -0500, Eric Sunshine wrote:

> On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:
> > Below is a another iteration on the patch. The actual code changes are
> > the same as the strbuf one, but the tests take care to avoid assuming
> > the filesystem can handle such a long path. Testing on Windows and OS X
> > is appreciated.
> 
> All three new tests fail on OS X. Thus far brief examination of the
> first failing tests shows that 'expect' and 'actual' differ:
> 
> expect:
>     long
>     master
> 
> actual:
>     master

Ugh. It looks like the packed-refs reader uses a PATH_MAX-sized buffer
to read each line, and simply omits the ref. That may be something we
want to work on, but it's a separate topic.  I think there are platforms
whose PATH_MAX is much smaller than what they can actually represent.
So ideally we would lift the arbitrary PATH_MAX limitations inside git,
and just let the OS complain when we exceed its limits.

Even if we fix that, though, I think the push test would still fail on
systems that have a true limit on the filesystem.  Writing to a ref
requires taking a lock in the filesystem, which will involve creating
the too-long path. I think there are simply some systems that will not
support long refs well, and the tests need to be skipped there.

So here's a re-roll that uses prerequisites.

-Peff

-- >8 --
Subject: pkt-line: allow writing of LARGE_PACKET_MAX buffers

When we send out pkt-lines with refnames, we use a static
1000-byte buffer. This means that the maximum size of a ref
over the git protocol is around 950 bytes (the exact size
depends on the protocol line being written, but figure on a sha1
plus some boilerplate).

This is enough for any sane workflow, but occasionally odd
things happen (e.g., a bug may create a ref "foo/foo/foo/..."
accidentally).  With the current code, you cannot even use
"push" to delete such a ref from a remote.

Let's switch to using a strbuf, with a hard-limit of
LARGE_PACKET_MAX (which is specified by the protocol).  This
matches the size of the readers, as of 74543a0 (pkt-line:
provide a LARGE_PACKET_MAX static buffer, 2013-02-20).
Versions of git older than that will complain about our
large packets, but it's really no worse than the current
behavior. Right now the sender barfs with "impossibly long
line" trying to send the packet, and afterwards the reader
will barf with "protocol error: bad line length %d", which
is arguably better anyway.

Note that we're not really _solving_ the problem here, but
just bumping the limits. In theory, the length of a ref is
unbounded, and pkt-line can only represent sizes up to
65531 bytes. So we are just bumping the limit, not removing
it.  But hopefully 64K should be enough for anyone.

As a bonus, by using a strbuf for the formatting we can
eliminate an unnecessary copy in format_buf_write.

Signed-off-by: Jeff King <peff@peff.net>
---
 pkt-line.c                | 37 +++++++++++++++++++------------------
 t/t5527-fetch-odd-refs.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 8bc89b1..187a229 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -64,44 +64,45 @@ void packet_buf_flush(struct strbuf *buf)
 }
 
 #define hex(a) (hexchar[(a) & 15])
-static char buffer[1000];
-static unsigned format_packet(const char *fmt, va_list args)
+static void format_packet(struct strbuf *out, const char *fmt, va_list args)
 {
 	static char hexchar[] = "0123456789abcdef";
-	unsigned n;
+	size_t orig_len, n;
 
-	n = vsnprintf(buffer + 4, sizeof(buffer) - 4, fmt, args);
-	if (n >= sizeof(buffer)-4)
+	orig_len = out->len;
+	strbuf_addstr(out, "0000");
+	strbuf_vaddf(out, fmt, args);
+	n = out->len - orig_len;
+
+	if (n > LARGE_PACKET_MAX)
 		die("protocol error: impossibly long line");
-	n += 4;
-	buffer[0] = hex(n >> 12);
-	buffer[1] = hex(n >> 8);
-	buffer[2] = hex(n >> 4);
-	buffer[3] = hex(n);
-	packet_trace(buffer+4, n-4, 1);
-	return n;
+
+	out->buf[orig_len + 0] = hex(n >> 12);
+	out->buf[orig_len + 1] = hex(n >> 8);
+	out->buf[orig_len + 2] = hex(n >> 4);
+	out->buf[orig_len + 3] = hex(n);
+	packet_trace(out->buf + orig_len + 4, n - 4, 1);
 }
 
 void packet_write(int fd, const char *fmt, ...)
 {
+	static struct strbuf buf = STRBUF_INIT;
 	va_list args;
-	unsigned n;
 
+	strbuf_reset(&buf);
 	va_start(args, fmt);
-	n = format_packet(fmt, args);
+	format_packet(&buf, fmt, args);
 	va_end(args);
-	write_or_die(fd, buffer, n);
+	write_or_die(fd, buf.buf, buf.len);
 }
 
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 {
 	va_list args;
-	unsigned n;
 
 	va_start(args, fmt);
-	n = format_packet(fmt, args);
+	format_packet(buf, fmt, args);
 	va_end(args);
-	strbuf_add(buf, buffer, n);
 }
 
 static int get_packet_data(int fd, char **src_buf, size_t *src_size,
diff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh
index edea9f9..85bcb2e 100755
--- a/t/t5527-fetch-odd-refs.sh
+++ b/t/t5527-fetch-odd-refs.sh
@@ -26,4 +26,37 @@ test_expect_success 'suffix ref is ignored during fetch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'try to create repo with absurdly long refname' '
+	ref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40
+	ref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&
+	git init long &&
+	(
+		cd long &&
+		test_commit long &&
+		test_commit master
+	) &&
+	if git -C long update-ref refs/heads/$ref1440 long; then
+		test_set_prereq LONG_REF
+	else
+		echo >&2 "long refs not supported"
+	fi
+'
+
+test_expect_success LONG_REF 'fetch handles extremely long refname' '
+	git fetch long refs/heads/*:refs/remotes/long/* &&
+	cat >expect <<-\EOF &&
+	long
+	master
+	EOF
+	git for-each-ref --format="%(subject)" refs/remotes/long >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success LONG_REF 'push handles extremely long refname' '
+	git push long :refs/heads/$ref1440 &&
+	git -C long for-each-ref --format="%(subject)" refs/heads >actual &&
+	echo master >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.2.0.454.g7eca6b7

Re: [PATCH v3] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Eric Sunshine]

On Wed, Dec 10, 2014 at 4:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 10, 2014 at 3:36 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:
>>> Below is a another iteration on the patch. The actual code changes are
>>> the same as the strbuf one, but the tests take care to avoid assuming
>>> the filesystem can handle such a long path. Testing on Windows and OS X
>>> is appreciated.
>>
>> All three new tests fail on OS X. Thus far brief examination of the
>> first failing tests shows that 'expect' and 'actual' differ:
>>
>> expect:
>>     long
>>     master
>>
>> actual:
>>     master
>
> The failure manifests as soon as the refname hits length 1024, at
> which point for-each-ref stops reporting it. MAX_PATH on OS X is 1024,
> so some part of the machinery invoked by for-each-ref likely is
> rejecting refnames longer than that (even when coming from
> packed-refs).

Clarification: for-each-ref ignores the ref when the full line read
from packed-refs hits length 1024 (not when the refname itself hits
length 1024).

Re: [PATCH v3] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Jeff King]

On Wed, Dec 10, 2014 at 04:49:38AM -0500, Eric Sunshine wrote:

> On Wed, Dec 10, 2014 at 4:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Wed, Dec 10, 2014 at 3:36 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> >> On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:
> >>> Below is a another iteration on the patch. The actual code changes are
> >>> the same as the strbuf one, but the tests take care to avoid assuming
> >>> the filesystem can handle such a long path. Testing on Windows and OS X
> >>> is appreciated.
> >>
> >> All three new tests fail on OS X. Thus far brief examination of the
> >> first failing tests shows that 'expect' and 'actual' differ:
> >>
> >> expect:
> >>     long
> >>     master
> >>
> >> actual:
> >>     master
> >
> > The failure manifests as soon as the refname hits length 1024, at
> > which point for-each-ref stops reporting it. MAX_PATH on OS X is 1024,
> > so some part of the machinery invoked by for-each-ref likely is
> > rejecting refnames longer than that (even when coming from
> > packed-refs).
> 
> Clarification: for-each-ref ignores the ref when the full line read
> from packed-refs hits length 1024 (not when the refname itself hits
> length 1024).

Yes, the problem is in read_packed_refs:

    char refline[PATH_MAX];
    ...
    while (fgets(refline, sizeof(refline), f)) {
        ...
    }

This could be trivially converted to strbuf_getwholeline, but I am not
sure what else would break, or whether such a system would actually be
_usable_ with such long refs (e.g., would it break the first time you

Using fgets like this does shear lines, though. The next fgets call will
see the second half of the line. I think we are saved from doing
anything stupid by parse_ref_line, but it is mostly luck. So perhaps for
that reason the trivial conversion to strbuf is worth it, even if it
doesn't help any practical cases.

-Peff

Re: [PATCH v4] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Eric Sunshine]

On Wed, Dec 10, 2014 at 4:47 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Dec 10, 2014 at 03:36:31AM -0500, Eric Sunshine wrote:
>
>> On Wed, Dec 10, 2014 at 2:34 AM, Jeff King <peff@peff.net> wrote:
>> > Below is a another iteration on the patch. The actual code changes are
>> > the same as the strbuf one, but the tests take care to avoid assuming
>> > the filesystem can handle such a long path. Testing on Windows and OS X
>> > is appreciated.
>>
>> All three new tests fail on OS X. Thus far brief examination of the
>> first failing tests shows that 'expect' and 'actual' differ:
>>
>> expect:
>>     long
>>     master
>>
>> actual:
>>     master
>
> Ugh. It looks like the packed-refs reader uses a PATH_MAX-sized buffer
> to read each line, and simply omits the ref. That may be something we
> want to work on, but it's a separate topic.  I think there are platforms
> whose PATH_MAX is much smaller than what they can actually represent.
> So ideally we would lift the arbitrary PATH_MAX limitations inside git,
> and just let the OS complain when we exceed its limits.
>
> Even if we fix that, though, I think the push test would still fail on
> systems that have a true limit on the filesystem.  Writing to a ref
> requires taking a lock in the filesystem, which will involve creating
> the too-long path. I think there are simply some systems that will not
> support long refs well, and the tests need to be skipped there.
>
> So here's a re-roll that uses prerequisites.

On OS X, this version correctly skips the two final tests as intended.

[PATCH 0/3] convert read_packed_refs to use strbuf

[Jeff King]

On Wed, Dec 10, 2014 at 04:53:19AM -0500, Jeff King wrote:

> > Clarification: for-each-ref ignores the ref when the full line read
> > from packed-refs hits length 1024 (not when the refname itself hits
> > length 1024).
> 
> Yes, the problem is in read_packed_refs:
> 
>     char refline[PATH_MAX];
>     ...
>     while (fgets(refline, sizeof(refline), f)) {
>         ...
>     }
> 
> This could be trivially converted to strbuf_getwholeline, but I am not
> sure what else would break, or whether such a system would actually be
> _usable_ with such long refs (e.g., would it break the first time you

I accidentally cut off the next line, but it was something like
"...first time you actually tried writing to the ref)".

> Using fgets like this does shear lines, though. The next fgets call will
> see the second half of the line. I think we are saved from doing
> anything stupid by parse_ref_line, but it is mostly luck. So perhaps for
> that reason the trivial conversion to strbuf is worth it, even if it
> doesn't help any practical cases.

Here's a patch to do that. It still doesn't let you create long refs on
OS X, as we get caught up in the PATH_MAX found in git_path() and
friends. Still, I think it's a step in the right direction, and it fixes
the shearing issue.

Patches 2 and 3 are just follow-on cleanups.

  [1/3]: read_packed_refs: use a strbuf for reading lines
  [2/3]: read_packed_refs: pass strbuf to parse_ref_line
  [3/3]: read_packed_refs: use skip_prefix instead of static array

I checked, and this miraculously does not conflict with any of the refs
work in pu. :)

-Peff

[PATCH 1/3] read_packed_refs: use a strbuf for reading lines

[Jeff King]

We currently used a fixed PATH_MAX-sized buffer for reading
packed-refs lines. This is a reasonable guess, in the sense
that git generally cannot work with refs larger than
PATH_MAX. However, there are a few cases where it is not
great:

  1. Some systems may have a low value of PATH_MAX, but can
     actually handle larger paths in practice. Fixing this
     code path probably isn't enough to make them work
     completely with long refs, but it is a step in the
     right direction.

  2. We use fgets, which will happily give us half a line on
     the first read, and then the rest of the line on the
     second. This is probably OK in practice, because our
     refline parser is careful enough to look for the
     trailing newline on the first line. The second line may
     look like a peeled line to us, but since "^" is illegal
     in refnames, it is not likely to come up.

     Still, it does not hurt to be more careful.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 5ff457e..6f31935 100644
--- a/refs.c
+++ b/refs.c
@@ -1126,16 +1126,16 @@ static const char *parse_ref_line(char *line, unsigned char *sha1)
 static void read_packed_refs(FILE *f, struct ref_dir *dir)
 {
 	struct ref_entry *last = NULL;
-	char refline[PATH_MAX];
+	struct strbuf line = STRBUF_INIT;
 	enum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;
 
-	while (fgets(refline, sizeof(refline), f)) {
+	while (strbuf_getwholeline(&line, f, '\n') != EOF) {
 		unsigned char sha1[20];
 		const char *refname;
 		static const char header[] = "# pack-refs with:";
 
-		if (!strncmp(refline, header, sizeof(header)-1)) {
-			const char *traits = refline + sizeof(header) - 1;
+		if (!strncmp(line.buf, header, sizeof(header)-1)) {
+			const char *traits = line.buf + sizeof(header) - 1;
 			if (strstr(traits, " fully-peeled "))
 				peeled = PEELED_FULLY;
 			else if (strstr(traits, " peeled "))
@@ -1144,7 +1144,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
 			continue;
 		}
 
-		refname = parse_ref_line(refline, sha1);
+		refname = parse_ref_line(line.buf, sha1);
 		if (refname) {
 			int flag = REF_ISPACKED;
 
@@ -1160,10 +1160,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
 			continue;
 		}
 		if (last &&
-		    refline[0] == '^' &&
-		    strlen(refline) == PEELED_LINE_LENGTH &&
-		    refline[PEELED_LINE_LENGTH - 1] == '\n' &&
-		    !get_sha1_hex(refline + 1, sha1)) {
+		    line.buf[0] == '^' &&
+		    line.len == PEELED_LINE_LENGTH &&
+		    line.buf[PEELED_LINE_LENGTH - 1] == '\n' &&
+		    !get_sha1_hex(line.buf + 1, sha1)) {
 			hashcpy(last->u.value.peeled, sha1);
 			/*
 			 * Regardless of what the file header said,
@@ -1173,6 +1173,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
 			last->flag |= REF_KNOWS_PEELED;
 		}
 	}
+
+	strbuf_release(&line);
 }
 
 /*
-- 
2.2.0.454.g7eca6b7

[PATCH 2/3] read_packed_refs: pass strbuf to parse_ref_line

[Jeff King]

Now that we have a strbuf in read_packed_refs, we can pass
it straight to the line parser, which saves us an extra
strlen.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index 6f31935..10f8247 100644
--- a/refs.c
+++ b/refs.c
@@ -1068,8 +1068,10 @@ static const char PACKED_REFS_HEADER[] =
  * Return a pointer to the refname within the line (null-terminated),
  * or NULL if there was a problem.
  */
-static const char *parse_ref_line(char *line, unsigned char *sha1)
+static const char *parse_ref_line(struct strbuf *line, unsigned char *sha1)
 {
+	const char *ref;
+
 	/*
 	 * 42: the answer to everything.
 	 *
@@ -1078,22 +1080,23 @@ static const char *parse_ref_line(char *line, unsigned char *sha1)
 	 *  +1 (space in between hex and name)
 	 *  +1 (newline at the end of the line)
 	 */
-	int len = strlen(line) - 42;
-
-	if (len <= 0)
+	if (line->len <= 42)
 		return NULL;
-	if (get_sha1_hex(line, sha1) < 0)
+
+	if (get_sha1_hex(line->buf, sha1) < 0)
 		return NULL;
-	if (!isspace(line[40]))
+	if (!isspace(line->buf[40]))
 		return NULL;
-	line += 41;
-	if (isspace(*line))
+
+	ref = line->buf + 41;
+	if (isspace(*ref))
 		return NULL;
-	if (line[len] != '\n')
+
+	if (line->buf[line->len - 1] != '\n')
 		return NULL;
-	line[len] = 0;
+	line->buf[--line->len] = 0;
 
-	return line;
+	return ref;
 }
 
 /*
@@ -1144,7 +1147,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
 			continue;
 		}
 
-		refname = parse_ref_line(line.buf, sha1);
+		refname = parse_ref_line(&line, sha1);
 		if (refname) {
 			int flag = REF_ISPACKED;
 
-- 
2.2.0.454.g7eca6b7

[PATCH 3/3] read_packed_refs: use skip_prefix instead of static array

[Jeff King]

We want to recognize the packed-refs header and skip to the
"traits" part of the line. We currently do it by feeding
sizeof() a static const array to strncmp. However, it's a
bit simpler to just skip_prefix, which expresses the
intention more directly, and without remembering to account
for the NUL-terminator in each sizeof() call.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 10f8247..c71553f 100644
--- a/refs.c
+++ b/refs.c
@@ -1135,10 +1135,9 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
 	while (strbuf_getwholeline(&line, f, '\n') != EOF) {
 		unsigned char sha1[20];
 		const char *refname;
-		static const char header[] = "# pack-refs with:";
+		const char *traits;
 
-		if (!strncmp(line.buf, header, sizeof(header)-1)) {
-			const char *traits = line.buf + sizeof(header) - 1;
+		if (skip_prefix(line.buf, "# pack-refs with:", &traits)) {
 			if (strstr(traits, " fully-peeled "))
 				peeled = PEELED_FULLY;
 			else if (strstr(traits, " peeled "))
-- 
2.2.0.454.g7eca6b7

Re: [PATCH 0/3] convert read_packed_refs to use strbuf

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's a patch to do that. It still doesn't let you create long refs on
> OS X, as we get caught up in the PATH_MAX found in git_path() and
> friends. Still, I think it's a step in the right direction, and it fixes
> the shearing issue.
>
> Patches 2 and 3 are just follow-on cleanups.
>
>   [1/3]: read_packed_refs: use a strbuf for reading lines
>   [2/3]: read_packed_refs: pass strbuf to parse_ref_line
>   [3/3]: read_packed_refs: use skip_prefix instead of static array
>
> I checked, and this miraculously does not conflict with any of the refs
> work in pu. :)

Heh, I suspect that is largely because Michael's "reflog expire" is
kept out of my tree while it is still under discusson.

Re: [PATCH v4] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Eric Sunshine]

On Wed, Dec 10, 2014 at 4:47 AM, Jeff King <peff@peff.net> wrote:
> Subject: pkt-line: allow writing of LARGE_PACKET_MAX buffers
>
> When we send out pkt-lines with refnames, we use a static
> 1000-byte buffer. This means that the maximum size of a ref
> over the git protocol is around 950 bytes (the exact size
> depends on the protocol line being written, but figure on a sha1
> plus some boilerplate).
>
> This is enough for any sane workflow, but occasionally odd
> things happen (e.g., a bug may create a ref "foo/foo/foo/..."
> accidentally).  With the current code, you cannot even use
> "push" to delete such a ref from a remote.
>
> Let's switch to using a strbuf, with a hard-limit of
> LARGE_PACKET_MAX (which is specified by the protocol).  This
> matches the size of the readers, as of 74543a0 (pkt-line:
> provide a LARGE_PACKET_MAX static buffer, 2013-02-20).
> Versions of git older than that will complain about our
> large packets, but it's really no worse than the current
> behavior. Right now the sender barfs with "impossibly long
> line" trying to send the packet, and afterwards the reader
> will barf with "protocol error: bad line length %d", which
> is arguably better anyway.
>
> Note that we're not really _solving_ the problem here, but
> just bumping the limits. In theory, the length of a ref is
> unbounded, and pkt-line can only represent sizes up to
> 65531 bytes. So we are just bumping the limit, not removing
> it.  But hopefully 64K should be enough for anyone.
>
> As a bonus, by using a strbuf for the formatting we can
> eliminate an unnecessary copy in format_buf_write.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh
> index edea9f9..85bcb2e 100755
> --- a/t/t5527-fetch-odd-refs.sh
> +++ b/t/t5527-fetch-odd-refs.sh
> @@ -26,4 +26,37 @@ test_expect_success 'suffix ref is ignored during fetch' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'try to create repo with absurdly long refname' '
> +       ref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40

Maybe you want to keep the &&-chain intact here?

> +       ref1440=$ref240/$ref240/$ref240/$ref240/$ref240/$ref240 &&
> +       git init long &&
> +       (
> +               cd long &&
> +               test_commit long &&
> +               test_commit master
> +       ) &&
> +       if git -C long update-ref refs/heads/$ref1440 long; then
> +               test_set_prereq LONG_REF
> +       else
> +               echo >&2 "long refs not supported"
> +       fi
> +'
> +
> +test_expect_success LONG_REF 'fetch handles extremely long refname' '
> +       git fetch long refs/heads/*:refs/remotes/long/* &&
> +       cat >expect <<-\EOF &&
> +       long
> +       master
> +       EOF
> +       git for-each-ref --format="%(subject)" refs/remotes/long >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success LONG_REF 'push handles extremely long refname' '
> +       git push long :refs/heads/$ref1440 &&
> +       git -C long for-each-ref --format="%(subject)" refs/heads >actual &&
> +       echo master >expect &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.2.0.454.g7eca6b7
>

Re: [PATCH v4] pkt-line: allow writing of LARGE_PACKET_MAX buffers

[Jeff King]

On Wed, Dec 10, 2014 at 03:14:17PM -0500, Eric Sunshine wrote:

> > +test_expect_success 'try to create repo with absurdly long refname' '
> > +       ref240=$_z40/$_z40/$_z40/$_z40/$_z40/$_z40
> 
> Maybe you want to keep the &&-chain intact here?

Thanks, yeah. It doesn't matter in practice, but we do try to &&-chain
everything.

-Peff