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

[Eric Sunshine <sunshine@sunshineco.com>]

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
>