Skip to content
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

[WIP] Implement Git Wire Protocol v2 #420

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ulugbekna
Copy link
Contributor

This is a WIP implementation of git wire protocol v2 (spec available here).

It's not really ready for a review, but if you still want to see how things are going, I'd recommend going from the oldest commit to the newest. Things should make sense.

@dinosaure
Copy link
Member

So the PR seems fine as is, I need to go to some details but these commits should be proposed into another PR:
09a28e4
f24b46e & c65712c

ulugbekna and others added 6 commits February 8, 2021 17:02
* make client, server capabilities management more explicit
* rename 'decoder_from' to more conventional 'of_string'
* add safer version of peek pkt
	that also support git wire protocol v2 pkts:
	delim-pkt and response-end-pkt
* move 'prompt_pkt' to 'decoder.mli'
	to reuse it in git wire proto v2
* add 'read_pkt'
* add 'junk_chars' fn to 'Decoder' to
	increase 'decoder.pos' by 'n'
* move 'bind' and '>>='
	from nss/protocol.ml to pkt_line.decoder
* [wip] support wire proto v2 capabilities
* add 'Ls_refs', 'Fetch_command' modules to
  represent commands 'ls-refs' and 'fetch' respectively;
* add 'Encoder' module to wire-proto-v2 with support for encoding
  command requests and copy-paste NSS's 'encode_proto_request'
* add some comments to better define parts of a packet line:
  specific names for 4 bytes that encode packet length,
  the bytes that follow the length bytes, etc.
* rename length calculating function 'pkt_len' to
  'encoded_pkt_len' that returns the value hex-encoded in the
  first 4 bytes of the packet line and 'pkt_len_at_least_4'
  returns 'max 4 (encoded_pkt_len pkt)'
* copy-paste 'Proto_request' module from NSS
* update 'response' type in proto-v2 'Protocol'
* add 'Extended_pkt_line_decoder' that provides
	more functionality than 'Pkt_line.Decoder' but not specific to the protocol
* add decoding for all commands of wire proto v2
* reflect changes after 'mimic' lib introduction
* make 'smart_flow' more understandable
* reduce dup code, e.g., (>>=)
* reorganize stuff closer to its use
* rename stuff for more clarity
* move smart protocol (wire proto v1)-based 'fetch'
	code to separate modules
* functorize 'Smart_flow'
* rename 'Smart_flow' to 'State_flow'
* add mli file to 'State_flow'
* improve 'nss/state.ml' API:
	- it improves cases when we want to "open" the module to get infix/syntax operators
	- it also make the API more uniform and rich, e.g., adds "map" fn
* rename "fail" to "io_raise":
	  1) avoid clash with "fail" from "smart"/"wire_proto_v2"
	  2) to highlight that it causes "exception"al behavior
* add support for "ls-refs" command (without args)
* fix log.debug use and its message
a module that translates state read-write monad into 'flow' operations,
to be usable both by 'Smart' and 'Wire_proto_v2'; hence, they
shouldn't have own copies of Context
let rec prompt_pkt ?strict k decoder =
if at_least_one_pkt decoder then k decoder
else prompt ?strict (prompt_pkt ?strict k) decoder

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about such deletion, I know that usually, I re-bind an OCaml value with the same name several such as:

let prompt_pkt = ...
let prompt_pkt = ... prompt_pkt ...
let prompt_pkt = ... prompt_pkt ...
...

So, in my mind, such deletion will disturb behaviors of functions defined then. Why you deleted this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved prompt_pkt from protocol.ml to decoder.ml because

  1. protocol.ml contains Decoder module, which implements decoding for values of the pack protocol. But prompt_pkt is a function that is not specific to protocol and could be defined on a lower and more general layer such as decoder.ml.
  2. I also needed it in proto_vals_v2.ml, so it made sense to move this general and low-level function to decoder.ml which specifically deals with pkt-line decoding
  3. Moving prompt_pkt to decoder.ml should be safe because all references to prompt_pkt in protocol.ml still refer to that value but "imported" from decoder.ml.

@dinosaure
Copy link
Member

The pull-request has some style updates which should be out of that. I don't have strong opinion about style but they should be done with some others PRs (for example, I saw some update about push.ml). Is it possible to split again the PR with style and the impl. of the protocol v2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants