Re: [PATCH] protocol upload-pack-v2

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2015, 00:28 UTC
Message-ID: <xmqqr3t1vefz.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1425685087-21633-1-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 6 quoted lines

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

Noise?

Show 25 quoted lines

> @@ -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?

Show 18 quoted lines

> +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.