threads / discuss / 50521

2.21: Change in ref advertisement handling?

Subject: 2.21: Change in ref advertisement handling?

## tl;dr

2 messages between Feb 16, 2019 and Feb 16, 2019.

replies: 1people: 2as markdown or json

Bryan Turner· Feb 16, 2019, 02:45 UTC · lore

In Bitbucket Server's Git test suite, we have some lazy tests that use a stub HTTP server with mocked payloads to feed a Git client, to test its behavior.

For some of those tests, we have the server configured to send a response body like this for a ref advertisement: 76d28b849be7fb3301e4bd14fb28328947b4f5a1<tab>HEAD<newline> 76d28b849be7fb3301e4bd14fb28328947b4f5a1<tab>refs/heads/master<newline>

There are headers we set, like "Content-Type: application/x-git-upload-pack-advertisement", but that's the response. It's not packeted or anything.

If I use (for example) Git 2.17.1 and run "git ls-remote <url>" with that response, Git happily displays essentially identical output, HEAD and master at 76d28b849be7fb3301e4bd14fb28328947b4f5a1.

If I run that "git ls-remote" with 2.21.0-rc1, Git displays "fatal: the remote end hung up unexpectedly" and dies.

If I change the mocked response to: 001e# service=git-upload-pack<newline> 0000<no newline; wrapping just for display> 003276d28b849be7fb3301e4bd14fb28328947b4f5a1<sp>HEAD<newline> 003f76d28b849be7fb3301e4bd14fb28328947b4f5a1<sp>refs/heads/master<newline> 0000

This works in both 2.17.1 and 2.21.0-rc1. (I was a little surprised Git didn't care about the lack of a NUL byte after "HEAD" with the capabilities after it, before the newline, but even 2.21 doesn't seem to mind.)

First let me say, I had no idea these tests of ours existed, and I'm surprised that they worked on Git 2.20 and older, given that the response isn't in proper pkt-line format.

I'm not looking for anything to change here, let me hasten to add; I'm just interested in some clarification. Why did our invalid ref advertisement response work in older Git versions? Why has it stopped working in 2.21?

Best regards, Bryan Turner

Jeff King· Feb 16, 2019, 06:45 UTC · re: Bryan Turner · lore

Re: 2.21: Change in ref advertisement handling?

On Fri, Feb 15, 2019 at 06:45:42PM -0800, Bryan Turner wrote:

Show 18 quoted lines

> In Bitbucket Server's Git test suite, we have some lazy tests that use
> a stub HTTP server with mocked payloads to feed a Git client, to test
> its behavior.
> 
> For some of those tests, we have the server configured to send a
> response body like this for a ref advertisement:
> 76d28b849be7fb3301e4bd14fb28328947b4f5a1<tab>HEAD<newline>
> 76d28b849be7fb3301e4bd14fb28328947b4f5a1<tab>refs/heads/master<newline>
> 
> There are headers we set, like "Content-Type:
> application/x-git-upload-pack-advertisement", but that's the response.
> It's not packeted or anything.
>
> [...]
>
> First let me say, I had no idea these tests of ours existed, and I'm
> surprised that they worked on Git 2.20 and older, given that the
> response isn't in proper pkt-line format.

I believe older versions of Git would have fallen back to dumb-http here. Do your tests actually follow-up with a real object fetch, or are they just listing the refs? If the latter, I think it would "work" even if your server does not otherwise implement dumb-http.

> I'm not looking for anything to change here, let me hasten to add; I'm
> just interested in some clarification. Why did our invalid ref
> advertisement response work in older Git versions? Why has it stopped
> working in 2.21?

This is almost certainly due to my 8ee3e120cd (remote-curl: refactor smart-http discovery, 2019-02-06), which intentionally tightened a few corner cases to match the spec. In particular, once you respond with "application/x-git-upload-pack-advertisement", we fully commit to smart-http and complain if you further violate the spec, rather than falling back to dumb-http.

-Peff