No sideband data stored for push

[rickharris]

This seems like it could be a regression since #573. I haven't been able to see any sideband data for push for a few months now, while clone stores sideband data just fine. I should've reported this earlier, I thought it was user error at first (hey, maybe it still is!)

Steps to reproduce:

1. Run the progress example, and see that sideband works fine with clone.
2. Add Progress: os.Stdout to the push options in the push example and see no sideband data written to os.Stdout.
3. Push to GitHub using the original implementation's example and see no sideband data written to os.Stdout.

Let me know if I can provide any more details. Thanks!

[Werelds]

I'm experiencing this as well and I think I know where it comes from.

The offending line is plumbing/protocol/packp/report_status.go:77.

ReportStatus breaks out on the first flush (30 30 30 30 which is 0000 in ASCII).

Here's an example response I get from my personal GitLab instance when pushing a branch that is not the default, in which case GL sends a message over band 2 (the verbose band) with a link to immediately create a merge request:

30 30 33 31 01 30 30 30 65 75 6E 70 61 63 6B 20 6F 6B 0A 30 30 31 61 6F 6B 20 72 65 66 73 2F 68 65 61 64 73 2F 64 65 76 65 6C 6F 70 0A 30 30 30 30 30 30 38 35 02 0A 54 6F 20 63 72 65 61 74 65 20 61 20 6D 65 72 67 65 20 72 65 71 75 65 73 74 20 66 6F 72 20 64 65 76 65 6C 6F 70 2C 20 76 69 73 69 74 3A 0A 20 20 68 74 74 70 73 3A 2F 2F 6D 79 2E 67 69 74 6C 61 62 2E 69 6F 2F 72 65 70 6F 2F 61 62 63 64 65 66 67 68 69 2F 6D 65 72 67 65 5F 72 65 71 75 65 73 74 73 2F 6E 65 77 3F 6D 65 72 67 65 5F 72 65 71 75 65 73 74 25 35 42 73 6F 75 72 63 65 5F 62 30 30 31 37 02 72 61 6E 63 68 25 35 44 3D 64 65 76 65 6C 6F 70 0A 0A 30 30 30 30

Fully translated to ASCII this is:

0031�000eunpack ok
001aok refs/heads/develop
00000085�
To create a merge request for develop, visit:
  https://my.gitlab.io/repo/abcdefghi/merge_requests/new?merge_request%5Bsource_b0017�ranch%5D=develop

0000

I've prepared a gist that breaks the above down byte-by-byte and runs it through the demuxer raw. The demuxer itself handles this correctly, the issue definitely lies somewhere within the status report bits. Comments indicate number of bytes, running total in parentheses, followed by the meaning of those bytes. For the most part this'll just be the ASCII above ;)

Gist in question: https://gist.github.com/Werelds/437dc82b0ff755187e44ae61d380663d

Note how the first flush is part of the band 1 payload (as in, those 4 bytes are counted towards band 1's length), the last flush however is not part of the payload.

I'm going to step through the code with delve a bit to identify exactly where it goes wrong; whether it's the scanner misreading the four zeroes as an empty string (resulting in len(b) == 0) or if it really is the line above on its own.

[Werelds]

Replace that break with continue and everything will work as it should; sidebands will be decoded correctly in full and without errors. Tests also still pass. I can create a PR for this, however it would perhaps be good if one of the maintainers took a look at this and perhaps remembers if there was a specific reason to have this in there. From my point of view it just seems to assume "flush means done, so quit", but that's not technically the case :)

I took a quick look at the "real" git code and it looks like recv_sideband just runs until EOF, flushes don't abort anything.

[smola]

@Werelds We probably missed this. A PR would be welcome, but we'll need to add a specific test for this to verify the change. Thank you!

[Werelds]

@smola I had a go at implementing this a while back (see the PR linked above), but because I'm not that familiar with the project I couldn't figure it all out. Unfortunately I haven't had time to look at it again.

In a nutshell though, the problem is one of design. This library doesn't have a single entrypoint to decode incoming streams like Git itself does. It's handled differently for different commands, whereas Git itself simply has a single receiver that sends the bands' data to the appropriate callbacks. It should be easy enough to fix by either using callbacks too or a channel to pass the data on.

So rather than ReportStatus (in this case, but the same goes for update requests for example) doing its own decoding of a response, the transports should do this. The transport should decode all data at all times and pass it to receivers; at the moment the library attempts to save time by dropping out too early. That's how Git itself does it anyway. Right now band 2 decoding requires a 2nd decoding pass as well, which makes the time saved moot.

[Werelds]

For reference: both send-pack and fetch-pack use the same recv_sideband.

We'd obviously have io.Writer parameters for each band instead, which is then more than flexible enough to discard bands if necessary, use a byte buffer or decode on the fly as necessary.

[henvic]

@WEREDS,
Are you still working on this?

Thanks!

[Werelds]

Unfortunately, not right now, no! Being the only remaining backend developer in a team has some downsides ;)

Hopefully soon I'll have a bit more time again to look at a better solution.

I still stand by what I said above: to handle sidebands correctly, the ideal fix should follow Git's example and not attempt to save a few hundred microseconds by not always fully decoding.

[jonohart]

Hi - we'd like to try and find a resolution to this issue.

We have a problem where it looks like this is causing a functional issue rather than simply a cosmetic issue of no output being displayed.

We have a git server which is configured with some hooks that perform actions when commits are pushed to the server, and the server is returning sideband information to the client while processing the hooks. We see issues with the hooks not being executed which only occur when we are pushing to our server using the go-git library (everything works correctly using the command line git client). Our suspicion is that the client is breaking the connection early (due to the break statement mentioned above) and this is interrupting the server's processing of the hooks. If I simply change this break to a continue as @Werelds suggested, pushing using the library works correctly and our server is able to process its hooks.

Is someone able to take a look at this? Is PR #821 on the right track?