Symbolic refs break ref advertisement on 1.8.4.3+

Symbolic refs break ref advertisement on 1.8.4.3+

[Bryan Turner]

According to a git bisect between the v1.8.4 and v1.8.4.3 tags, it
appears the changes in 5e7dcad, "upload-pack: send non-HEAD symbolic
refs", cause the ref advertisement to fail of the repository has more
than a handful of symbolic refs. Here's a simple reproduce case
(tested on Bash):

Aphrael:example bturner$ git version
git version 1.8.4.3
Aphrael:symbolic-refs bturner$ git init example
Initialized empty Git repository in /Users/bturner/example/.git/
Aphrael:symbolic-refs bturner$ cd example
Aphrael:example bturner$ echo "Testing..." > file.txt
Aphrael:example bturner$ git add file.txt
Aphrael:example bturner$ git commit -m "Initial commit"
[master (root-commit) b4c4b2a] Initial commit
 1 file changed, 1 insertion(+)
 create mode 100644 file.txt
Aphrael:example bturner$ for ((i=1;i<21;i++)); do git symbolic-ref
refs/heads/syms/$i refs/heads/master; done
Aphrael:example bturner$ git ls-remote .
fatal: protocol error: impossibly long line
fatal: Could not read from remote repository.

A symref= entry is written into the first packet of the ref
advertisement, right after the capabilities, for each symbolic ref in
the repository. Unfortunately, no splitting is done on that value and
so once you have 15-20 symbolic refs (more or less depending on path
lengths), you blow the 996 byte limit in format_packet (pkt-line.c)
and all further clone/fetch operations fail.

I've verified this same issue exists in all 1.8.5 RCs.

Best regards,
Bryan Turner

Re: Symbolic refs break ref advertisement on 1.8.4.3+

[Jeff King]

On Sun, Nov 17, 2013 at 01:39:52AM +1100, Bryan Turner wrote:

> Aphrael:example bturner$ for ((i=1;i<21;i++)); do git symbolic-ref
> refs/heads/syms/$i refs/heads/master; done
> Aphrael:example bturner$ git ls-remote .
> fatal: protocol error: impossibly long line
> fatal: Could not read from remote repository.
> 
> A symref= entry is written into the first packet of the ref
> advertisement, right after the capabilities, for each symbolic ref in
> the repository. Unfortunately, no splitting is done on that value and
> so once you have 15-20 symbolic refs (more or less depending on path
> lengths), you blow the 996 byte limit in format_packet (pkt-line.c)
> and all further clone/fetch operations fail.

Ick, yeah. I don't think there is a way around that with the way the
information is shoe-horned into the protocol.  We should probably just
revert 5e7dcad (upload-pack: send non-HEAD symbolic refs, 2013-09-17),
and assume the HEAD branch name is short enough to fit.

Another option would be to cap the number of non-HEAD symrefs we'd send
(by counting up the bytes and keeping below the limit). That at least
makes the "easy" cases work, but it's a bit too flaky for my taste.

-Peff

Re: Symbolic refs break ref advertisement on 1.8.4.3+

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Nov 17, 2013 at 01:39:52AM +1100, Bryan Turner wrote:
>
>> Aphrael:example bturner$ for ((i=1;i<21;i++)); do git symbolic-ref
>> refs/heads/syms/$i refs/heads/master; done
>> Aphrael:example bturner$ git ls-remote .
>> fatal: protocol error: impossibly long line
>> fatal: Could not read from remote repository.
>> 
>> A symref= entry is written into the first packet of the ref
>> advertisement, right after the capabilities, for each symbolic ref in
>> the repository. Unfortunately, no splitting is done on that value and
>> so once you have 15-20 symbolic refs (more or less depending on path
>> lengths), you blow the 996 byte limit in format_packet (pkt-line.c)
>> and all further clone/fetch operations fail.
>
> Ick, yeah. I don't think there is a way around that with the way the
> information is shoe-horned into the protocol.  We should probably just
> revert 5e7dcad (upload-pack: send non-HEAD symbolic refs, 2013-09-17),
> and assume the HEAD branch name is short enough to fit.
>
> Another option would be to cap the number of non-HEAD symrefs we'd send
> (by counting up the bytes and keeping below the limit). That at least
> makes the "easy" cases work, but it's a bit too flaky for my taste.

Thanks Bryan for an easy reproduction, and thanks Peff for a
suggestion.  Let's revert that one for now.