Added initial support for git wire protocol v2 #876
Conversation
- Added v2 functions alongside v1
- Added LsRefs command request and response
- Fixed typo - Added delim and end functions to pktline
There was a problem hiding this comment.
@blmayer great job progressing the v2 support. 👏
I will take a look over the weekend on the failing tests - but my initial impression is that the session needs to be aware of the protocol version and at times have different flows, more on this once I get to the bottom of the issues.
| // PktType returns the type of packet, use this for special cases like | ||
| // flush, delim and end. | ||
| func (s *Scanner) PktType() PktType { | ||
| return s.pktType | ||
| } |
There was a problem hiding this comment.
Please add a comment around any diff in behaviour calling this on v0/v2? Assuming the former should never really happen.
| if len(vals) > 0 { | ||
| pe.EncodeString(c.String() + "=" + strings.Join(vals, " ") + "\n") | ||
| } else { | ||
| pe.EncodeString(c.String() + "\n") | ||
| } |
There was a problem hiding this comment.
| if len(vals) > 0 { | |
| pe.EncodeString(c.String() + "=" + strings.Join(vals, " ") + "\n") | |
| } else { | |
| pe.EncodeString(c.String() + "\n") | |
| } | |
| line := c.String() | |
| if len(vals) > 0 { | |
| line = fmt.Sprintf("%s=%s, line, strings.Join(vals, " ")) | |
| } | |
| pe.EncodeString(line + "\n") |
There was a problem hiding this comment.
Not sure if this suggestion works, since each invocation of EncodeString will create the line size prefix (part of the pktline protocol). And on v2 all that payload must be on the same line.
There was a problem hiding this comment.
You are absolutely right, updated it to handle that as a var of its own.
|
|
||
| // if done was sent skip acks section | ||
| if len(res.Haves) > 0 { | ||
| if res.Args.Supports("done") { |
| @@ -79,6 +80,7 @@ func (h *handler) NewReceivePackSession(s storer.Storer) (transport.ReceivePackS | |||
| } | |||
|
|
|||
| type session struct { | |||
| version int | |||
There was a problem hiding this comment.
Let's create a type for version which defaults to v1 and gets populated based on the protocol negotiation.
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
|
Thanks for reviewing this. I'll work on your suggestions on the following days. |
There was a problem hiding this comment.
Great job here! This is going to be a big update 🙂
It might be a good idea also to support Protocol V1 which is simply V0 plus version line before advertising references.
See: https://github.com/git/git/blob/master/protocol.h#L25, https://github.com/git/git/blob/master/builtin/upload-pack.c#L60-L68, https://stackoverflow.com/questions/66743099/whats-the-difference-between-git-protocol-v1-and-v0-v2
Let's treat v1 as its own thing, once v2 is implemented. But I agree that by the looks of v1 upstream, it should be fairly straight forward to implement it. |
| // decode # SP service=<service> LF | ||
| s.Scan() | ||
| f := string(s.Bytes()) | ||
| if i := strings.Index(f, "service="); i < 0 { |
There was a problem hiding this comment.
| if i := strings.Index(f, "service="); i < 0 { | |
| i := strings.Index(f, "service=") | |
| if i < 0 { |
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
| // CommandResponse values represent the information transmitted on a | ||
| // command response message used in wire protocol v2. Values from this type | ||
| // are not zero-value safe, use the New function instead. | ||
| type CommandResponse struct { |
There was a problem hiding this comment.
Perhaps we could make CommandResponse an interface with a private method, and then define different command responses to implement this interface. From what i can tell, each v2 command has a different response structure
Hi. In this PR I implemented some parts of the v2 protocol. I have created some tests, but the overall work needs improvements. As this is my first time coding I think some feedback here is cool, since I'm not sure if I coded this in the best place.
The v2 protocol uses commands to negotiate packfiles, I added the initial capability advertisement, ls-refs and fetch commands. Inside each command capabilities change how it is done, I added only the necessary to make it work.
I used this repo to test: https://github.com/blmayer/gwi
So, I'd like some help so I can wrap this up and merge.
Thanks!