New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added initial support for git wire protocol v2 #876
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create a const for 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!