Re: [RFC/WIP PATCH 11/11] Document protocol version 2

From: Junio C Hamano <gitster@pobox.com>
Date: May 29, 2015, 20:35 UTC
Message-ID: <xmqqsiafazr7.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1432677675-5118-12-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 6 quoted lines

> @@ -1,11 +1,11 @@
>  Packfile transfer protocols
>  ===========================
>  
> -Git supports transferring data in packfiles over the ssh://, git:// and
> +Git supports transferring data in packfiles over the ssh://, git://, http:// and

When you have chance, can you do things like this, which is a clear improvement of the current document even if we never had v2, as separate patches?

Show 7 quoted lines

> +Capability discovery (v2)
> +-------------------------
> ...
> +  capability-list  =  *(capability) [agent LF] flush-pkt
> +  capability       =  PKT-LINE("capability:" keyvaluepair LF)
> +  agent            =  keyvaluepair LF
> +  keyvaluepair     =  1*(LC_ALPHA / DIGIT / "-" / "_" / "=")

What is the "=" doing there? If you meant to cover things like "lang=en" with this, I do not think it is a good idea. Rather, it should be more like this:

	capability = 1*(LC_ALPHA / DIGIT / "-" / "_") [ "=" value ]
	value = 0*( any octet other than LF, NUL )

in order to leave us wiggle room to have more than very limited subset of US-ASCII in 'value'. I suspect that we may want to allow anything other than LF (unlike v1 that allowed anything other than SP and LF).

Show 11 quoted lines

> +  LC_ALPHA         =  %x61-7A
> +----
> +
> +The client MUST ignore any data on pkt-lines starting with anything
> +different than "capability" for future ease of extension.
> +
> +The client MUST NOT ask for capabilities the server did not say it
> +supports. The server MUST diagnose and abort if capabilities it does
> +not understand was requested. The server MUST NOT ignore capabilities
> +that client requested and server advertised.  As a consequence of these
> +rules, server MUST NOT advertise capabilities it does not understand.

I think it was already discussed that we shouldn't do the "capability:" and "cap:" prefixes in reviews of earlier parts, so the details of this part would be updated?

Show 16 quoted lines

> @@ -154,10 +203,14 @@ If HEAD is a valid ref, HEAD MUST appear as the first advertised
>  ref.  If HEAD is not a valid ref, HEAD MUST NOT appear in the
>  advertisement list at all, but other refs may still appear.
>  
> -The stream MUST include capability declarations behind a NUL on the
> -first ref. The peeled value of a ref (that is "ref^{}") MUST be
> -immediately after the ref itself, if presented. A conforming server
> -MUST peel the ref if it's an annotated tag.
> +In version 1 the stream MUST include capability declarations behind
> +a NUL on the first ref. The peeled value of a ref (that is "ref^{}")
> +MUST be immediately after the ref itself, if presented. A conforming
> +server MUST peel the ref if it's an annotated tag.
> +
> +In version 2 the capabilities are already negotiated, so the first ref
> +MUST NOT be followed by any capability advertisement, but it should be
> +treated as any other refs advertising line.

Sensible.

Show 6 quoted lines

> @@ -178,13 +231,28 @@ MUST peel the ref if it's an annotated tag.
>    shallow          =  PKT-LINE("shallow" SP obj-id)
>  
>    capability-list  =  capability *(SP capability)
> -  capability       =  1*(LC_ALPHA / DIGIT / "-" / "_")
> +  capability       =  1*(LC_ALPHA / DIGIT / "-" / "_" / "=")

Ditto.

Thanks.