Re: [PATCH] protocol upload-pack-v2

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> @@ -67,7 +74,6 @@ gracefully with an error message.
>    error-line     =  PKT-LINE("ERR" SP explanation-text)
>  ----
>  
> -
>  SSH Transport

Noise?

> @@ -124,9 +130,56 @@ has, the first can 'fetch' from the second.  This operation determines
>  what data the server has that the client does not then streams that
>  data down to the client in packfile format.
>  
> +Capability discovery (v2)
> +-------------------------
>  
> +In version 1, capability discovery is part of reference discovery and
> +covered in reference discovery section.
> +
> +In version 2, when the client initially connects, the server
> +immediately sends its capabilities to the client. Then the client must
> +send the list of server capabilities it wants to use to the server.
> +
> +   S: 00XXcapabilities 4\n
> +   S: 00XXcap:lang\n
> +   S: 00XXcap:thin-pack\n
> +   S: 00XXcap:ofs-delta\n
> +   S: 00XXagent:agent=git/2:3.4.5+custom-739-gb850f98\n
> +
> +   C: 00XXcapabilities 3
> +   C: 00XXcap:thin-pack\n
> +   C: 00XXcap:ofs-delta\n
> +   C: 00XXcap:lang=en\n
> +   C: 00XXagent:agent=git/custom_string\n

I do not see a good reason why we want "I am sending N caps"
upfront, instead of "this, that, and here is the end of the group".
If you expect the recipient to benefit by being able to pre-allocate
N slots, then that might make sense, but otherwise, I'd rather see
us stick to a (weaker) flush that says "group ends here".

Besides, I do not know how you counted 4 on the S: side and 3 on
the C: side in the above example and missing LF after 3 ;-).

> +----
> +  cap              =  PKT-LINE("capabilities" SP size LF list)

Isn't a cap packet terminated by LF without any "list" following it?
The notation PKT-LINE(<blah>) is "wrap <blah> in a single packet",
and I do not think you meant to send the capability line and a series
of cap:foo entries in a single packet.

> +  size             =  *DIGIT
> +  capability-list  =  *(capability) [agent LF]
> +  capability       =  "cap:" keyvaluepair LF
> +  agent            =  keyvaluepair LF

I do not see a reason to make 'agent' as part of capability.  That
was an implementation detail of v1 but v2 does not have an
obligation to consider agent announcement as capability.

server-announcement = PKT-LINE("capabilities" ...) capability-list [agent-announcement]
capability-list = as you have it w/o "[agent LF]"
agent-announcement = PKT-LINE("agent=" agent-token LF)

or something, perhaps?

> +The client MUST ignore any data before the pkt-line starting with "capabilities"
> +for future easy of extension.

s/easy/ease/; but I am not sure if this makes sense.  Without
knowing the extended preamble, you cannot even tell if a packet line
that happens to start with "capabilities" is a proper beginning of
0-th iteration of v2 protocol, or an embedded data in the preamble,
no?

> +Reference Discovery (v2)
> +------------------------
> +
> +In version 2, reference discovery is initiated by the client with
> +"want-refs" line. The client may skip reference discovery phase
> +entirely by not sending "want-refs" (e.g. the client has another way
> +to retrieve ref list).
> +
> +----
> +  want-refs  =  PKT-LINE("want-refs" SP mode *argument)
> +  mode       =  "all"
> +  argument   =  SP arg
> +  arg        =  1*(LC_ALPHA / DIGIT / "-" / "_" / "=")
> +----
> +
> +Mode "all" sends all visible refs to the client like in version 1. No
> +arguments are accepted in this mode. More modes may be available based
> +on capabilities.

I tend to agree with Duy that the protocol must anticipate that args
can become longer.

> +----
> +  advertised-refs  =  (no-refs / list-of-refs)
> +		      *shallow
> +		      flush-pkt

I am not sure if defining "shallow" as part of "refs advertisement"
is a good idea.  The latter lives in the same place in the v1
protocol merely because that was how it was later bolted onto the
protocol.  But this concern can easily be allayed by retitling
"advertised-refs" to something else.