add multi-ack client support (clone broken on VisualStudio Team Services, repo returns 500 error)

[chrisdostert]

Scenario: I pass a visual studio team services (VSTS) repo url ('https://tenant.visualstudio.com/project/_git/repo' for example) to PlainClone w/ basic http auth credentials

Expected: The repo is cloned successfully

Actual: I get back an unexpected client error: unexpected requesting "https://tenant.visualstudio.com/project/_git/repo/git-upload-pack" status code: 500 error

[smola]

@chrisdostert Does it fail with public repositories too?

[chrisdostert]

@smola I'm unable to find "official" docs stating as much but search results (& failing to make one) seem to suggest it doesn't offer public repo support, only private.

stackoverflow

[smola]

@chrisdostert Thanks. I'll get an account and test it.

[smola]

@chrisdostert Got it. VSTS does not support clients without multi_ack capability (see protocol spec).

The server HTTP response contains the following body: TF401041: The Git protocol sent is not as expected (Clients must support multi-ack.).

Lack of multi_ack support has been a long standing issue in go-git. However, VSTS is the first server we found not supporting clients without multi_ack.

Note that the git protocol does not have any standarized way of signaling an error for this case, since capability negotiation assumes that lack of capabilities by the client is always valid.

[smola]

If anyone wants to help, here's (most of) the relevant code: https://github.com/src-d/go-git/blob/master/plumbing/protocol/packp/srvresp.go#L22 https://github.com/src-d/go-git/blob/932ced9f55f556de02610425cfa161c35c6a758b/plumbing/transport/common.go#L131

[chrisdostert]

@smola thanks for finding that. Very helpful. opctl has users on VSTS eagerly awaiting this functionality so I feel obligated, but I feel like it's going to take me alot of research to know enough to knock this out. I've been reading through the git-internals docs & go-git codebase trying to get an idea of what it would take; any help/guidance is greatly appreciated.

[smola]

@chrisdostert I'm still not sure abouyt how to implement it. Right now ServerResponse.Decode just decodes the answer from the server with non-multi-ack. But doing multi-ack that part is bidirectional, so it should probably not called decode, get an io.Writer in addition to the io.Reader and maybe a new interface Acknowledger or something like that to call back.

Here's how multi_ack works: https://github.com/git/git/blob/4fa66c85f11bc5a541462ca5ae3246aa0ce02e74/Documentation/technical/pack-protocol.txt#L282

I'm sorry I cannot provide much more detail without actually diving into the task. In any case, this is something I'll be able to work on soon.

[smola]

Given that this is a pretty big change, I propose splitting it in at least 3 PR to make it easier to review (and parallelize the work).

This is a summary of what I plan to do:

1. Encoding/Decoding

Add support in plumbing/protocol/packp to encode and decode server responses with multi_ack (and multi_ack_detailed if it's feasible while we are at it). This is documented here: https://github.com/git/git/blob/master/Documentation/technical/pack-protocol.txt#L282 And it would affect srvresp.go and maybe uppackresp.go on our side.

2. Adding dumb support for multi_ack/multi_ack_detailed

Make our transports support multi_ack/multi_ack_detailed encoding. This might be separated in 2 PR, one for http tansport, and one for the others (see plumbing/transport/internal/common). This would be dumb support in the sense that it would not leverage multi_ack/multi_ack_detailed to improve performance. It would rely on the same API as now, on the full computed list of haves, but talk to the server with multi_ack messages.

At this point, this issue would be fixed, since we would formally support multi_ack/multi_ack_detailed at protocol level, and so it would work with VSTS.

3. Create an object iterator to walk haves

In order to implement multi_ack logic, we would need an iterator implementing the same walking logic as revlist.Objects, but in the form of an iterator that allows marking objects as ignored during the iteration process.

4. Implement multi_ack/multi_ack_detailed logic

Given everything else is done, we can implement the multi_ack/multi_ack_detailed logic, which probably requires changes in the API of protocol/packp or transport, or maybe both, and it would also affect the Remote implementation.