protocol version 2

* Capabilities were implemented as a hack and are hidden behind a NUL byte after the first ref sent from the server during the ref advertisement:
...

* Various other technical debt (e.g. abusing capabilities to communicate agent and symref data, service name set using a query parameter).

Special Packets
-----------------

In protocol v2 these special packets will have the following semantics:

* '0000' Flush Packet (flush-pkt) - indicates the end of a message
* '0001' End-of-List delimiter (delim-pkt) - indicates the end of a list

command-request = command
capability-list delim-pkt
(command specific parameters) flush-pkt
command = PKT-LINE("command=" key LF)

The server will then acknowledge the command and requested capabilities by echoing them back to the client and then launch into the command.

acknowledge-request = command
capability-list
delim-pkt
execute-command
execute-command = <defined by each command>

A particular command can last for as many rounds as are required to complete the service (multiple for negotiation during fetch and push or no additional trips in the case of ls-refs).

Commands in v2
~~~~~~~~~~~~~~~~

Services are the core actions that a client wants to perform (fetch, push, etc). Each service has its own set of capabilities and its own language of commands (think 'want' lines in fetch). Optionally a service can take in initial parameters or data when a client sends it service request.

Ls-refs
---------

Ls-refs can be looked at as the equivalent of the current ls-remote as it is a way to query a remote for the references that it has. Unlike the current ls-remote, the filtering of the output is done on the server side by passing a number of parameters to the server-side command instead of the filtering occurring on the client.

Ls-ref takes in the following parameters:

  --head, --tags: Limit to only refs/heads or refs/tags

--refs: Do not show peeled tags or pseudorefs like HEAD

--symref: In addition to the object pointed by it, show the underlying
ref pointed by it when showing a symbolic ref

<refspec>: When specified, only references matching the given patterns
are displayed.

The output of ls-refs is as follows:

output = (no-refs / list-of-refs)
*symref
*shallow
flush-pkt

no-refs = PKT-LINE(zero-id SP no-refs LF)

list-of-refs = *ref
ref = PKT-LINE((tip / peeled) LF)
tip = obj-id SP refname
peeled = obj-id SP refname "^{}"

symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF) shallow = PKT-LINE("shallow" SP obj-id LF)

Quoted 5 lines · Brandon Williams

* Capabilities were implemented as a hack and are hidden behind a NUL byte after the first ref sent from the server during the ref advertisement:
...

Quoted 3 lines

* Various other technical debt (e.g. abusing capabilities to communicate agent and symref data, service name set using a query parameter).

This sounds like a duplicate of the above.

Quoted 8 lines

Special Packets
-----------------

In protocol v2 these special packets will have the following semantics:

* '0000' Flush Packet (flush-pkt) - indicates the end of a message
* '0001' End-of-List delimiter (delim-pkt) - indicates the end of a list

After reading the remainder of the document twice, I do not think the reason why we want to introduce a new type delim-pkt is explained and justified well enough. If a command request takes a command packet, zero or more capability packets, a mandatory delimiter packet, zero or more parameter packets and a mandatory flush packet, then you can use the same "flush" as delimiter in the middle. The delimiter will of course become useful if you can omit it when not necessary (e.g. after seeing capabilities, you may see a flush and you will know there is no parameters and save the need to send one "delim").

I actually have a reasonable guess why you want to have a separate delimiter (which has nothing to do with "optional delim can be omitted"), but I want to see it explained in this document clearly by its designer(s).

Quoted 8 lines

command-request = command
capability-list delim-pkt
(command specific parameters) flush-pkt
command = PKT-LINE("command=" key LF)

Quoted 8 lines

The server will then acknowledge the command and requested capabilities by echoing them back to the client and then launch into the command.

acknowledge-request = command
capability-list
delim-pkt
execute-command
execute-command = <defined by each command>

It is not quite clear what the value of "echoing them back" is, especially if that is done by always echoing verbatim. A reader may naturally expect, when capabilities are exchanged between two parties, these are negotiated so that the only ones that are commonly supported by both ends would be used, or something like that.

Quoted 4 lines

A particular command can last for as many rounds as are required to complete the service (multiple for negotiation during fetch and push or no additional trips in the case of ls-refs).

OK.

Quoted 8 lines

Commands in v2
~~~~~~~~~~~~~~~~

Services are the core actions that a client wants to perform (fetch, push, etc). Each service has its own set of capabilities and its own language of commands (think 'want' lines in fetch). Optionally a service can take in initial parameters or data when a client sends it service request.

So a service (like "fetch") employ a set of "command"s (like "want", "have, etc)? In the earlier part of the document, we did not see any mention of "service" and instead saw only "command" mentioned. Is the state machine on both ends and the transition between states implicit? E.g. when one side throws "want" command and the other side acknowledges, they understand implicitly that they are now in a "fetch" service session, even though there is nothing that said over the wire that they are doing so? One reason I am wondering about this is what we should do if a command verb may be applicable in multiple services.

After reading the earlier protocol exchange explanation, I was sort of expecting that "fetch" would be the command and "want", "have", and "ack" lines would be exchanged as "command specific parameters", so a sudden introduction of "services" here was a bit of impedance mismatch to me.

Quoted 10 lines

Ls-refs
---------

Ls-refs can be looked at as the equivalent of the current ls-remote as it is a way to query a remote for the references that it has. Unlike the current ls-remote, the filtering of the output is done on the server side by passing a number of parameters to the server-side command instead of the filtering occurring on the client.

Quoted 3 lines

Ls-ref takes in the following parameters:

  --head, --tags: Limit to only refs/heads or refs/tags

I see no need for the above two as long as "refs/heads/*", etc. are understood.

Quoted 1 line

--refs: Do not show peeled tags or pseudorefs like HEAD

So showing peeled tags is the default? Then I can sort-of see why "I am not interested in peeled result".

I do not see why "do not show HEAD, MERGE_HEAD, etc." is needed, though. It should be sufficient to just ask for refs/* if you are interested only in other things, no?

Quoted 2 lines

--symref: In addition to the object pointed by it, show the underlying
ref pointed by it when showing a symbolic ref

Sort of OK--it probably is easier to always send this, as symbolic refs are minority anyway, though.

Quoted 2 lines

<refspec>: When specified, only references matching the given patterns
are displayed.

I do not think you meant <refspec> here.

The side that is listing what it has has no reason to know what the recipient plans to do with the result, so you must be only sending the LHS of a refspec. If your explanation says "given patterns", then replace <refspec> with <pattern>. Do not abuse a term that has specific and established meaning for something else.

Quoted 8 lines

The output of ls-refs is as follows:

output = (no-refs / list-of-refs)
*symref
*shallow
flush-pkt

Quoted 1 line

no-refs = PKT-LINE(zero-id SP no-refs LF)

Can't your list-of-refs have 0 element? I do not see why you need no-refs here. It's not like you need a dummy line, to the end of which you need to append NUL plus hidden capabilities ;-)

Quoted 8 lines

list-of-refs = *ref
ref = PKT-LINE((tip / peeled) LF)
tip = obj-id SP refname
peeled = obj-id SP refname "^{}"

symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF) shallow = PKT-LINE("shallow" SP obj-id LF)

Thanks.

Quoted 4 lines

I actually have a reasonable guess why you want to have a separate delimiter (which has nothing to do with "optional delim can be omitted"), but I want to see it explained in this document clearly by its designer(s).

Jonathan Tan suggested that we tighten flush semantics in a newer protocol so that proxies are easier to work with. Currently proxies need to understand the protocol instead of simply waiting for a flush.

Also I've been told the smart http code is more complex because of the current semantics of flush packets.

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · Brandon Williams

<refspec>: When specified, only references matching the given patterns
are displayed.

Quoted 7 lines

I do not think you meant <refspec> here.

The side that is listing what it has has no reason to know what the recipient plans to do with the result, so you must be only sending the LHS of a refspec. If your explanation says "given patterns", then replace <refspec> with <pattern>. Do not abuse a term that has specific and established meaning for something else.

Yes, you're right i intended that to mean <pattern> instead so that the client could send "refs/heads/*" or some other such pattern and have the server limit its output.

Overview
==========

This document presents a specification for a version 2 of Git's wire protocol. Protocol v2 will improve upon v1 in the following ways:

* Instead of multiple service names, multiple commands will be supported by a single service
* Easily extendable as capabilities are moved into their own section of the protocol, no longer being hidden behind a NUL byte and limited by the size of a pkt-line (as there will be a single capability per pkt-line).
* Separate out other information hidden behind NUL bytes (e.g. agent string as a capability and symrefs can be requested using 'ls-ref')
* Ref advertisement will be omitted unless explicitly requested
* ls-ref command to explicitly request some refs

Ls-refs
---------

Ls-refs can be looked at as the equivalent of the current ls-remote as it is a way to query a remote for the references that it has. Unlike the current ls-remote, the filtering of the output is done on the server side by passing a number of parameters to the server-side command instead of the filtering occurring on the client.

Ls-ref takes in the following parameters:

   --head, --tags: Limit to only refs/heads or refs/tags

--refs: Do not show peeled tags or pseudorefs like HEAD

--symref: In addition to the object pointed by it, show the underlying
ref pointed by it when showing a symbolic ref
<refspec>: When specified, only references matching the given patterns
are displayed.

Fetch
-------

Fetch will need to be a modified version of the v1 fetch protocol. Some potential areas for improvement are: Ref-in-want, CDN offloading, Fetch-options.

Since we'll have an 'ls-ref' service we can eliminate the need of fetch to perform a ref-advertisement, instead a client can run the 'ls-refs' service first, in order to find out what refs the server has, and then request those refs directly using the fetch service.

//TODO Flush out the design

Fetch-object
--------------

This service could be used by partial clones in order to request missing objects.

//TODO Flush out the design

Push
------

Push will need to be a modified version of the v1 push protocol. Some potential areas for improvement are: Fix push-options, Negotiation for force push.

Other Considerations
======================

* Move away from pkt-line framing?
* Have responses structured in well known formats (e.g. JSON)
* Eliminate initial round-trip using 'GIT_PROTOCOL' side-channel
* Additional commands in a partial clone world (e.g. log, grep)

Quoted 6 lines · Brandon Williams

Overview
==========

This document presents a specification for a version 2 of Git's wire protocol. Protocol v2 will improve upon v1 in the following ways:

Quoted 10 lines

* Instead of multiple service names, multiple commands will be supported by a single service
* Easily extendable as capabilities are moved into their own section of the protocol, no longer being hidden behind a NUL byte and limited by the size of a pkt-line (as there will be a single capability per pkt-line).
* Separate out other information hidden behind NUL bytes (e.g. agent string as a capability and symrefs can be requested using 'ls-ref')
* Ref advertisement will be omitted unless explicitly requested
* ls-ref command to explicitly request some refs

Hi Brandon,

I'm very interested in your protocol as a former server-side dev for the VSTS Git server, and understand some of these headaches. We

built limited refs specifically to target the problem you are solving with ls-ref, but it requires knowledge about the authenticated user in order to work. I believe your suggestion is a better solution for the Git protocol.

The "easily extendable" part has specifically caught my interest, as we (Microsoft) would like to move most of the GVFS protocol into core Git, and this is a great way to do it. Even if not all features are accepted by upstream, we could use our GVFS-specific fork of Git to communicate to our servers without breaking normal users' interactions.

Please CC me in future versions of this proposal. Let me know if you want to chat directly about the "TODO" items below.

Speaking of TODOs, how much of this concept do you have working in a prototype? Do you have code that performs this version 2 handshake and communicates the ls-refs result?

Quoted 4 lines

Ls-refs
---------

Quoted 8 lines

Ls-refs can be looked at as the equivalent of the current ls-remote as it is a way to query a remote for the references that it has. Unlike the current ls-remote, the filtering of the output is done on the server side by passing a number of parameters to the server-side command instead of the filtering occurring on the client.

Ls-ref takes in the following parameters:

Quoted 1 line

  --head, --tags: Limit to only refs/heads or refs/tags

Nit: It would be better to use "--heads" to match refs/heads and your use of "--tags" for refs/tags.

Quoted 2 lines

--refs: Do not show peeled tags or pseudorefs like HEAD

Assuming we are in the case where the server has a HEAD ref, why would that ever be advertised? Also, does this imply that without the --refs option we would peel annotated tags until we find non-tag OIDs? Neither of these functions seem useful as default behavior.

Quoted 5 lines

--symref: In addition to the object pointed by it, show the underlying
ref pointed by it when showing a symbolic ref
<refspec>: When specified, only references matching the given patterns
are displayed.

Can you be specific about the patterns? For instance, it is not a good idea to allow the client to submit a regex for the server to compute. Instead, can we limit this pattern-matching to a prefix-set, such as the following list of prefixes:

    refs/heads/master
    refs/releases/*
    refs/heads/user/me/*

Quoted 14 lines

Fetch
-------

Fetch will need to be a modified version of the v1 fetch protocol. Some potential areas for improvement are: Ref-in-want, CDN offloading, Fetch-options.

Since we'll have an 'ls-ref' service we can eliminate the need of fetch to perform a ref-advertisement, instead a client can run the 'ls-refs' service first, in order to find out what refs the server has, and then request those refs directly using the fetch service.

//TODO Flush out the design

Quoted 7 lines

Fetch-object
--------------

This service could be used by partial clones in order to request missing objects.

//TODO Flush out the design

As you flesh our these "fetch" and "fetch-object" commands, keep in mind that partial clones could mean any of the following:

 * fetch all reachable objects except for blobs.

 * fetch all reachable objects except for blobs above a certain size.

 * fetch all commits, trees, (and blobs?) within a certain "cone" of the file system.

Quoted 6 lines

Push
------

Push will need to be a modified version of the v1 push protocol. Some potential areas for improvement are: Fix push-options, Negotiation for force push.

Negotiation is something to keep in mind for all pushes, especially in an ecosystem full of fork-based workflows. If you are working across forks and someone else syncs data between your remotes, you may re-push a large chunk of objects that are already present in a fork. Adding an ls-refs step before push would be a step in the right direction.

Quoted 7 lines

Other Considerations
======================

* Move away from pkt-line framing?
* Have responses structured in well known formats (e.g. JSON)
* Eliminate initial round-trip using 'GIT_PROTOCOL' side-channel
* Additional commands in a partial clone world (e.g. log, grep)

[Tangent]

I too have thought about making calls like "log" and "blame" available for calling remotes. One reason GVFS sends a "prefetch pack" of _all_ commits and trees is because one "git log -- path/to/file" command would start downloading thousands of objects one at a time as the walk moves through the history. If the remote can compute the commands that require historical data, then our partial clones can be more "pure" (i.e. only contain objects required for the user's changes).

One major caveat: if someone runs "log" from HEAD, then they may be working over data that is not on the remote, which means they would need to start the history walk until reaching commits that are known to be on the remote. If there are merges in the local history, then this could include multiple independent commits.

Further complicating this area, the server may not want to allow certain types of commands (i.e. regexes, expensive history options like "--simplify-merges").

In conclusion, I think it is a great idea to have the protocol allow these extensions, especially in a way that is easy to extend without breaking client/server compat scenarios (after both have v2 enabled).

[End Tangent]

Thanks,
-Stolee

Objective
===========

Replace Git's current wire protocol with a simpler, less wasteful protocol that can evolve over time.

Capability Advertisement
--------------------------

A server which decides to communicate (based on a request from a client) using protocol version 2, notifies the client by sending a version string in its initial response followed by an advertisement of its capabilities. Each capability is a key with an optional value. Clients must ignore all unknown keys.

Semantics of unknown values are left to
the definition of each key.

Some capabilities will describe commands
which can be requested to be executed by the client.

Ls-refs
---------

Ls-refs can be looked at as the equivalent of the current ls-remote as it is a way to query a remote for the references that it has. Unlike the current ls-remote, the filtering of the output is done on the server side by passing a number of parameters to the server-side command instead of the filtering occurring on the client.

Ls-ref takes in the following parameters:

--head, --tags: Limit to only refs/heads or refs/tags --refs: Do not show peeled tags or pseudorefs like HEAD --symref: In addition to the object pointed by it, show the underlying
ref pointed by it when showing a symbolic ref
<refspec>: When specified, only references matching the given patterns
are displayed.

The output of ls-refs is as follows:

output = (no-refs / list-of-refs)
*symref
*shallow
flush-pkt

no-refs = PKT-LINE(zero-id SP no-refs LF)
list-of-refs = *ref
ref = PKT-LINE((tip / peeled) LF)
tip = obj-id SP refname
peeled = obj-id SP refname "^{}"

symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF) shallow = PKT-LINE("shallow" SP obj-id LF)

Fetch
-------

Fetch will need to be a modified version of the v1 fetch protocol. Some potential areas for improvement are: Ref-in-want, CDN offloading, Fetch-options.

Since we'll have an 'ls-ref' service we can eliminate the need of fetch to perform a ref-advertisement, instead a client can run the 'ls-refs' service first, in order to find out what refs the server has, and then request those refs directly using the fetch service.

//TODO Flush out the design

Fetch-object
--------------

This service could be used by partial clones in order to request missing objects.

//TODO Flush out the design

Push
------

Push will need to be a modified version of the v1 push protocol. Some potential areas for improvement are: Fix push-options, Negotiation for force push.

One change that will need to happen is to improve how push-options are sent to the server (so that they aren't sent twice!!). Also the report-status needs to be better than it currently is in v1 so that tools like gerrit can explain what it did with the ref-update the client sent to it. Maybe have a push-rebase capability or command?

//TODO Flush out the design

Other Considerations
======================

* Move away from pkt-line framing?
* Have responses structured in well known formats (e.g. JSON)
* Eliminate initial round-trip using 'GIT_PROTOCOL' side-channel
* Additional commands in a partial clone world (e.g. log, grep)

From: "Brandon Williams" <[email protected]>
Sent: Friday, October 20, 2017 6:18 PM

Quoted 6 lines

Objective
===========

Replace Git's current wire protocol with a simpler, less wasteful protocol that can evolve over time.

<snip>

Quoted 8 lines

Capability Advertisement
--------------------------

A server which decides to communicate (based on a request from a client) using protocol version 2, notifies the client by sending a version string in its initial response followed by an advertisement of its capabilities. Each capability is a key with an optional value. Clients must ignore all unknown keys.

Quoted 2 lines

Semantics of unknown values are left to
the definition of each key.

This sounds odd. If the keys are known then their semantics are known. Or the keys are unknown and they and their values are ignored.

Maybe: Capability keys shall define their response to unknown key values.

Quoted 4 lines

Some capabilities will describe commands which can be requested to be executed by the client.

<snip>

Quoted 9 lines

Ls-refs
---------

Ls-refs can be looked at as the equivalent of the current ls-remote as it is a way to query a remote for the references that it has. Unlike the current ls-remote, the filtering of the output is done on the server side by passing a number of parameters to the server-side command instead of the filtering occurring on the client.

Quoted 8 lines

Ls-ref takes in the following parameters:

--head, --tags: Limit to only refs/heads or refs/tags --refs: Do not show peeled tags or pseudorefs like HEAD --symref: In addition to the object pointed by it, show the underlying
ref pointed by it when showing a symbolic ref
<refspec>: When specified, only references matching the given patterns
are displayed.

Does the --symref also the pseudorefs?

Isn't there a need somethimes to determine the ref that the remote's HEAD points to. This is an issue with the current clone and bundle code when there is a choice of refs/branches that could be the current HEAD ref and the wrong one is chosen, though this V2 change doesn't affect bundles.

Quoted 55 lines

The output of ls-refs is as follows:

output = (no-refs / list-of-refs)
*symref
*shallow
flush-pkt

no-refs = PKT-LINE(zero-id SP no-refs LF)
list-of-refs = *ref
ref = PKT-LINE((tip / peeled) LF)
tip = obj-id SP refname
peeled = obj-id SP refname "^{}"

symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF) shallow = PKT-LINE("shallow" SP obj-id LF)

Fetch
-------

Fetch will need to be a modified version of the v1 fetch protocol. Some potential areas for improvement are: Ref-in-want, CDN offloading, Fetch-options.

Since we'll have an 'ls-ref' service we can eliminate the need of fetch to perform a ref-advertisement, instead a client can run the 'ls-refs' service first, in order to find out what refs the server has, and then request those refs directly using the fetch service.

//TODO Flush out the design

Fetch-object
--------------

This service could be used by partial clones in order to request missing objects.

//TODO Flush out the design

Push
------

Push will need to be a modified version of the v1 push protocol. Some potential areas for improvement are: Fix push-options, Negotiation for force push.

One change that will need to happen is to improve how push-options are sent to the server (so that they aren't sent twice!!). Also the report-status needs to be better than it currently is in v1 so that tools like gerrit can explain what it did with the ref-update the client sent to it. Maybe have a push-rebase capability or command?

//TODO Flush out the design

Quoted 7 lines

Other Considerations
======================

* Move away from pkt-line framing?
* Have responses structured in well known formats (e.g. JSON)
* Eliminate initial round-trip using 'GIT_PROTOCOL' side-channel
* Additional commands in a partial clone world (e.g. log, grep)

--
Philip

Some of the pain points with the current protocol spec are:

* The server's initial response is the ref advertisement. This advertisement cannot be omitted and can become an issue due to the sheer number of refs that can be sent with large repositories. For example, when contacting the internal equivalent of https://android.googlesource.com/, the server will send approximately 1 million refs totaling 71MB. This is data that is sent during each and every fetch and is not scalable.

* Capabilities were implemented as a hack and are hidden behind a NUL byte after the first ref sent from the server during the ref advertisement:

<SHA1> <Ref Name>\0<capabilities space separated> <symref> <agent>

Since they are sent in the context of a pkt-line they are also subject
to the same length limitations (1k bytes with old clients). While we
may not be close to hitting this limitation with capabilities alone, it
has become a problem when trying to abuse capabilities for other
purposes (e.g. [symrefs](https://public-inbox.org/git/20160816161838.klvjhhoxsftvkfmd@x/)).

* Various other technical debt (e.g. abusing capabilities to communicate agent and symref data, service name set using a query parameter).

Special Packets
-----------------

In protocol v2 these special packets will have the following semantics:

* '0000' Flush Packet (flush-pkt) - indicates the end of a message
* '0001' End-of-List delimiter (delim-pkt) - indicates the end of a list

pkt-line.h +2 -0

index 3dad583e2..f1545929b 100644
@@ -60,8 +60,16 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
  * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if
  * present) is removed from the buffer before returning.
  */
+enum packet_read_status {
+       PACKET_READ_ERROR = -1,

Quoted 10 lines · Brandon Williams

pkt-line.h +2 -0

index 3dad583e2..f1545929b 100644
@@ -60,8 +60,16 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
  * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if
  * present) is removed from the buffer before returning.
  */
+enum packet_read_status {
+       PACKET_READ_ERROR = -1,

pkt-line.h int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); · line 74

Quoted 10 lines

pkt-line.h +6 -0

+       PACKET_READ_NORMAL,
+       PACKET_READ_FLUSH,
+};
 #define PACKET_READ_GENTLE_ON_EOF (1u<<0)
 #define PACKET_READ_CHOMP_NEWLINE (1u<<1)
+enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,
+                                               char *buffer, unsigned size, int *pktlen,
+                                               int options);
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
                *buffer, unsigned size, int options);

The documentation that is preceding these lines is very specific to packet_read, e.g.

If options does contain PACKET_READ_GENTLE_ON_EOF,
we will not die on condition 4 (truncated input), but instead return -1

which doesn't hold true for the _status version. Can you adapt the comment to explain both read functions?

Sometimes it is advantageous to be able to peek the next packet line without consuming it (e.g. to be able to determine the protocol version a server is speaking). In order to do that introduce 'struct packet_reader' which is an abstraction around the normal packet reading logic. This enables a caller to be able to peek a single line at a time using 'packet_reader_peek()' and having a caller consume a line by calling 'packet_reader_read()'.

Signed-off-by Brandon Williams <[email protected]>

---
 pkt-line.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 pkt-line.h | 20 ++++++++++++++++++++
 2 files changed, 81 insertions(+)

pkt-line.c +1 -0

index ac619f05b..518109bbe 100644
@@ -406,3 +406,64 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
        }
        return sb_out->len - orig_len;
 }
+

pkt-line.c +8 -0

+enum packet_read_status packet_reader_read(struct packet_reader *reader)
+{
+       if (reader->line_peeked) {
+               reader->line_peeked = 0;
+               return reader->status;
+       }
+
+       reader->status = packet_read_with_status(reader->fd,

Quoted 23 lines · Brandon Williams

Sometimes it is advantageous to be able to peek the next packet line without consuming it (e.g. to be able to determine the protocol version a server is speaking). In order to do that introduce 'struct packet_reader' which is an abstraction around the normal packet reading logic. This enables a caller to be able to peek a single line at a time using 'packet_reader_peek()' and having a caller consume a line by calling 'packet_reader_read()'.

Signed-off-by Brandon Williams <[email protected]>

---
 pkt-line.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 pkt-line.h | 20 ++++++++++++++++++++
 2 files changed, 81 insertions(+)

pkt-line.c +1 -0

index ac619f05b..518109bbe 100644
@@ -406,3 +406,64 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
        }
        return sb_out->len - orig_len;
 }
+

pkt-line.c ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out) · line 419

Quoted 10 lines

pkt-line.c +10 -0

+/* Packet Reader Functions */
+void packet_reader_init(struct packet_reader *reader, int fd,
+                       char *src_buffer, size_t src_len)
+{
+       reader->fd = fd;
+       reader->src_buffer = src_buffer;
+       reader->src_len = src_len;
+       reader->buffer = packet_buffer;
+       reader->buffer_size = sizeof(packet_buffer);
+       reader->options = PACKET_READ_CHOMP_NEWLINE | PACKET_READ_GENTLE_ON_EOF;

I assume the future user of this packet reader will need exactly these options coincidentally. ;)

I think it might be ok for now and later we can extend the reader if needed to also take the flags as args. However given this set of args, this is a gentle packet reader, as it corresponds to the _gently version of reading packets AFAICT.

Unlike the pkt_read function this constructor of a packet reader doesn't need the arguments for its buffer (packet_buffer and sizeof thereof), which packet_read
unfortunately needs. We pass in packet_buffer all the time except in builtin/receive-pack
for obtaining the gpg cert. I think that's ok here.

Quoted 17 lines

pkt-line.c +16 -0

+enum packet_read_status packet_reader_read(struct packet_reader *reader)
+{
+       if (reader->line_peeked) {
+               reader->line_peeked = 0;
+               return reader->status;
+       }
+
+       reader->status = packet_read_with_status(reader->fd,
+                                                &reader->src_buffer,
+                                                &reader->src_len,
+                                                reader->buffer,
+                                                reader->buffer_size,
+                                                &reader->pktlen,
+                                                reader->options);
+
+       switch (reader->status) {

pkt-line.c ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out) · line 444

Quoted 2 lines

pkt-line.c +2 -0

+       case PACKET_READ_ERROR:
+               reader->pktlen = -1;

In case of error the read function might not
have assigned the pktlen as requested, so we assign it to -1/NULL here. Though the caller ought to already know that they handle bogus, as the state is surely the first thing they'd inspect?

pkt-line.c ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out) · line 454

Quoted 11 lines

pkt-line.c +10 -0

+               reader->line = NULL;
+               break;
+       case PACKET_READ_NORMAL:
+               reader->line = reader->buffer;
+               break;
+       case PACKET_READ_FLUSH:
+               reader->pktlen = 0;
+               reader->line = NULL;
+               break;
+       }

Oh, this gives an interesting interface for someone who is just inspecting the len/line instead of the state, so it might be worth keeping it this way.

Can we have an API documentation in the header file, explaining what to expect in each field given the state of the (read, peaked) packet?

One of the design goals of protocol-v2 is to improve the semantics of flush packets. Currently in protocol-v1, flush packets are used both to indicate a break in a list of packet lines as well as an indication that one side has finished speaking. This makes it particularly difficult to implement proxies as a proxy would need to completely understand git protocol instead of simply looking for a flush packet.

To do this, introduce the special deliminator packet '0001'. A delim packet can then be used as a deliminator between lists of packet lines while flush packets can be reserved to indicate the end of a response.

Signed-off-by Brandon Williams <[email protected]>

Quoted 7 lines · Brandon Williams

One of the design goals of protocol-v2 is to improve the semantics of flush packets. Currently in protocol-v1, flush packets are used both to indicate a break in a list of packet lines as well as an indication that one side has finished speaking. This makes it particularly difficult to implement proxies as a proxy would need to completely understand git protocol instead of simply looking for a flush packet.

Quoted 5 lines

To do this, introduce the special deliminator packet '0001'. A delim packet can then be used as a deliminator between lists of packet lines while flush packets can be reserved to indicate the end of a response.

Signed-off-by Brandon Williams <[email protected]>

I presume the update for Documentation/technical/* comes at a later patch in the series, clarifying the exact semantic difference between the packet types?

In order to allow for code sharing with the server-side of fetch in protocol-v2 convert upload-pack to be a builtin.

Signed-off-by Brandon Williams <[email protected]>

---

 Makefile      | 3 ++-
 builtin.h     | 1 +
 git.c         | 1 +
 upload-pack.c | 2 +-
 4 files changed, 5 insertions(+), 2 deletions(-)

...

upload-pack.c +0 -0

index ef99a029c..2d16952a3 100644

Quoted 3 lines · Brandon Williams

In order to allow for code sharing with the server-side of fetch in protocol-v2 convert upload-pack to be a builtin.

Quoted 2 lines

Signed-off-by Brandon Williams <[email protected]>

---

This looks obvious and straight-forward to a cursory look.

I vaguely recalled and feared that we on purpose kept this program separate from the rest of the system for a reason, but my mailing list search is coming up empty.

On Wed, 6 Dec 2017, Junio C Hamano wrote:
...

Quoted 3 lines

I vaguely recalled and feared that we on purpose kept this program separate from the rest of the system for a reason, but my mailing list search is coming up empty.

I only recall that we kept it in the bin/ directory (as opposed to mlibexec/git-core/) to help with fetching via SSH.

Quoted 5 lines · Brandon Williams

In order to allow for code sharing with the server-side of fetch in protocol-v2 convert upload-pack to be a builtin.

Signed-off-by Brandon Williams <[email protected]>

---

This looks obvious and straight-forward to a cursory look.

I vaguely recalled and feared that we on purpose kept this program separate from the rest of the system for a reason, but my mailing list search is coming up empty.

Quoted 14 lines

 Makefile      | 3 ++-
 builtin.h     | 1 +
 git.c         | 1 +
 upload-pack.c | 2 +-
 4 files changed, 5 insertions(+), 2 deletions(-)

...

upload-pack.c +0 -0

index ef99a029c..2d16952a3 100644
@@ -1033,7 +1033,7 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 

upload-pack.c static int upload_pack_config(const char *var, const char *value, void *unused) · line 1039

Quoted 5 lines

upload-pack.c +1 -1

-int cmd_main(int argc, const char **argv)
+int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 {
 	const char *dir;
 	int strict = 0;

Shouldn't this file be moved to builtin/ directory, though?

Remove code duplication and use the existing 'get_refs_via_connect()' function to retrieve a remote's heads in 'fetch_refs_via_pack()' and 'git_transport_push()'.

Signed-off-by Brandon Williams <[email protected]>

---
 transport.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

transport.c +0 -0

index d75ff0514..7c969f285 100644
@@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.cloning = transport->cloning;

Quoted 15 lines · Brandon Williams

Remove code duplication and use the existing 'get_refs_via_connect()' function to retrieve a remote's heads in 'fetch_refs_via_pack()' and 'git_transport_push()'.

Signed-off-by Brandon Williams <[email protected]>

---
 transport.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

transport.c +0 -0

index d75ff0514..7c969f285 100644
@@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.cloning = transport->cloning;

transport.c static int fetch_refs_via_pack(struct transport *transport, · line 234

Quoted 10 lines

transport.c +2 -6

 	args.update_shallow = data->options.update_shallow;
 
-	if (!data->got_remote_heads) {
-		connect_setup(transport, 0);
-		get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0,
-				 NULL, &data->shallow);
-		data->got_remote_heads = 1;
-	}
+	if (!data->got_remote_heads)
+		refs_tmp = get_refs_via_connect(transport, 0);

The updated version is equivalent to the original as long as transport->data->extra_have is empty at this point. Were we deliberately sending NULL, instead of &data->extra_have, in the original, or is it a mere oversight?

The same comment applies to the other hunk of this patch.

connect.c +25 -0

@@ -56,6 +62,41 @@ static void die_initial_contact(int unexpected)
 		      "and the repository exists."));
 }
 
+static enum protocol_version discover_version(struct packet_reader *reader)
+{
+	enum protocol_version version = protocol_unknown_version;
+
+	/*
+	 * Peek the first line of the server's response to
+	 * determine the protocol version the server is speaking.
+	 */
+	switch (packet_reader_peek(reader)) {
+	case PACKET_READ_ERROR:
+		die_initial_contact(0);
+	case PACKET_READ_FLUSH:
+	case PACKET_READ_DELIM:
+		version = protocol_v0;
+		break;
+	case PACKET_READ_NORMAL:
+		version = determine_protocol_version_client(reader->line);
+		break;
+	}
+
+	/* Maybe process capabilities here, at least for v2 */
+	switch (version) {
+	case protocol_v1:
+		/* Read the peeked version line */
+		packet_reader_read(reader);

connect.c +8 -4

+#define EXPECTING_FIRST_REF 0
+#define EXPECTING_REF 1
+#define EXPECTING_SHALLOW 2
+#define EXPECTING_DONE 3
 
 static void process_capabilities(int *len)
 {

@@ -230,28 +237,34 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			      struct oid_array *shallow_points)
 {
 	struct ref **orig_list = list;
+	int len = 0;
+	int state = EXPECTING_FIRST_REF;
+	struct packet_reader reader;
+	const char *arg;
 
-	/*
-	 * A hang-up after seeing some response from the other end
-	 * means that it is unexpected, as we know the other end is
-	 * willing to talk to us.  A hang-up before seeing any

connect.c +6 -1

-	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
+	while (state != EXPECTING_DONE) {
+		switch (packet_reader_read(&reader)) {
+		case PACKET_READ_ERROR:
+			die_initial_contact(1);
+		case PACKET_READ_NORMAL:
+			len = reader.pktlen;

Brandon Williams <[email protected]> writes:

EXPECTING_DONE sounded like we are expecting to see 'done' packet sent from the other side, but I was mistaken. It is the state where we are "done" expecting anything ;-).

Having an (unconditional) assignment to 'state' in the above switch makes me feel somewhat uneasy, as the next "switch (state)" is what is meant as the state machine that would allow us to say things like "from this state, transition to that state is impossible". When we get a flush while we are expecting the first ref, for example, we'd just go into the "done" state. There is no provision for a future update to say "no, getting a flush in this state is an error".

That is no different from the current code; when read_remote_ref() notices that it got a flush, it just leaves the loop without even touching 'state' variable. But at least, I find that the current code is more honest---it does not even touch 'state' and allows the code after the loop to inspect it, if needed. From that point of vhew, the way the new code uses 'state' to leave the loop upon seeing a flush is a regression---it makes it harder to notice and report when we got a flush in a wrong state.

Perhaps getting rid of "EXPECTING_DONE" from the enum and then:

	int got_flush = 0;
	while (1) {
		switch (reader_read()) {
		case PACKET_READ_FLUSH:
			got_flush = 1;
			break;
		... other cases ...
		}

		if (got_flush)
			break;

		switch (state) {
		... current code ...
		}
	}

would be an improvement; we can later extend "if (got_flush)" part to check what state we are in if we wanted to notice and report an error there before breaking out of the loop.

builtin/fetch-pack.c +1 -1

@@ -193,7 +195,17 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		if (!conn)
 			return args.diag_url ? 0 : 1;
 	}
-	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
+

Quoted 6 lines · Brandon Williams

builtin/fetch-pack.c +1 -1

@@ -193,7 +195,17 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		if (!conn)
 			return args.diag_url ? 0 : 1;
 	}
-	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
+

builtin/fetch-pack.c int cmd_fetch_pack(int argc, const char **argv, const char *prefix) · line 208

Quoted 10 lines

builtin/fetch-pack.c +10 -0

+	packet_reader_init(&reader, fd[0], NULL, 0);
+
+	switch (discover_version(&reader)) {
+	case protocol_v1:
+	case protocol_v0:
+		get_remote_heads(&reader, &ref, 0, NULL, &shallow);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}

We see quite a few hunks just like this one appear in this patch. The repetition is a bit disturbing. I wonder if we want a wrapper around the "reader-init && discover-version && return an error if the protocol version is not known" dance. That would allow us to write this part of the code like so:

	struct packet_reader reader;

	if (connection_preamble(&reader, fd[0]))
		die("unknown protocol version");
	get_remote_heads(&reader, &ref, 0, NULL, &shallow);

or something like that.

By the way, is that really a BUG()? Getting a connection and the other end declaring a protocol version you do not yet understand is not a bug in your program---it is a normal runtime error, no?

While we could wrap the preamble into a function it sort of defeats the purpose since you want to be able to call different functions based on the protocol version you're speaking. That way you can have hard separations between the code paths which operate on v0/v1 and v2.

Quoted 4 lines · Brandon Williams

While we could wrap the preamble into a function it sort of defeats the purpose since you want to be able to call different functions based on the protocol version you're speaking. That way you can have hard separations between the code paths which operate on v0/v1 and v2.

As I envision that the need for different processing across protocol versions in real code would become far greater than it would fit in cases within a single switch() statement, I imagined that the reader structure would have a field that says which version of the protocol it is, so that the caller of the preamble thing can inspect it later to switch on it.

builtin/serve.c new file +17 -0

+int cmd_serve(int argc, const char **argv, const char *prefix)
+{
+
+	struct option options[] = {
+		OPT_END()
+	};
+
+	/* ignore all unknown cmdline switches for now */
+	argc = parse_options(argc, argv, prefix, options, grep_usage,
+			     PARSE_OPT_KEEP_DASHDASH |
+			     PARSE_OPT_KEEP_UNKNOWN);
+	serve();
+
+	return 0;
+}
...
+/* Main serve loop for protocol version 2 */
+void serve(void)

serve.c line 73

Quoted 12 lines · Brandon Williams

serve.c new file +12 -0

+static struct protocol_capability *get_capability(const char *key)
+{
+	int i;
+
+	if (!key)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(capabilities); i++) {
+		struct protocol_capability *c = &capabilities[i];
+		const char *out;
+		if (skip_prefix(key, c->name, &out) && (!*out || *out == '='))
+			return c;

Looks familiar and resembles what was recently discussed on list ;-)

Quoted 5 lines

builtin/serve.c new file +5 -0

+int cmd_serve(int argc, const char **argv, const char *prefix)
+{
+
+	struct option options[] = {
+		OPT_END()

builtin/serve.c line 25

Quoted 10 lines

builtin/serve.c new file +10 -0

+	};
+
+	/* ignore all unknown cmdline switches for now */
+	argc = parse_options(argc, argv, prefix, options, grep_usage,
+			     PARSE_OPT_KEEP_DASHDASH |
+			     PARSE_OPT_KEEP_UNKNOWN);
+	serve();
+
+	return 0;
+}

Quoted 11 lines

...

serve.c new file +10 -0

+/* Main serve loop for protocol version 2 */
+void serve(void)
+{
+	/* serve by default supports v2 */
+	packet_write_fmt(1, "version 2\n");
+
+	advertise_capabilities();
+
+	for (;;)
+		if (process_request())

Review

Quoted 2 lines

(quoted diff) +2 -0

+			break;
+}

I am guessing that this would be run just like upload-pack, i.e. invoked via ssh or via git-daemon, and that is why it can just assume that fd#0/fd#1 are already connected to the other end. It may be helpful to document somewhere how we envision to invoke this program.

Introduce the ls-refs server command. In protocol v2, the ls-refs command is used to request the ref advertisement from the server. Since it is a command which can be requested (as opposed to manditory in v1), a clinet can sent a number of parameters in its request to limit the ref

advertisement based on provided ref-patterns.

Signed-off-by Brandon Williams <[email protected]>

---