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

Jeff King [off-list ref] writes:

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

quoted

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.

On Tue, Dec 9, 2014 at 2:41 PM, Junio C Hamano [off-list ref] wrote:

Jeff King [off-list ref] writes:

quoted

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

quoted

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?

quoted

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.

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

quoted

quoted

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 <redacted>
---
 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

On Wed, Dec 10, 2014 at 2:34 AM, Jeff King [off-list ref] 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.

On Wed, Dec 10, 2014 at 3:36 AM, Eric Sunshine [off-list ref] wrote:

On Wed, Dec 10, 2014 at 2:34 AM, Jeff King [off-list ref] wrote:

quoted

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).

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

On Wed, Dec 10, 2014 at 2:34 AM, Jeff King [off-list ref] wrote:

quoted

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 <redacted>
---
 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

On Wed, Dec 10, 2014 at 4:42 AM, Eric Sunshine [off-list ref] wrote:

On Wed, Dec 10, 2014 at 3:36 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Wed, Dec 10, 2014 at 2:34 AM, Jeff King [off-list ref] wrote:

quoted

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).

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

On Wed, Dec 10, 2014 at 4:42 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Wed, Dec 10, 2014 at 3:36 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Wed, Dec 10, 2014 at 2:34 AM, Jeff King [off-list ref] wrote:

quoted

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

On Wed, Dec 10, 2014 at 4:47 AM, Jeff King [off-list ref] wrote:

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

quoted

On Wed, Dec 10, 2014 at 2:34 AM, Jeff King [off-list ref] wrote:

quoted

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.

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

quoted

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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

On Wed, Dec 10, 2014 at 4:47 AM, Jeff King [off-list ref] wrote:

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

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

quoted

+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