Re: [PATCH 3/3] protocol docs: explain receive-pack push options

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 6, 2017, 00:26 UTC
Message-ID: <20170506002605.GM28740@aiede.svl.corp.google.com>
In-Reply-To: <615007182a5e205a7074317a4c8788aa4eb009ff.1494027001.git.jonathantanmy@google.com>

Hi,

Jonathan Tan wrote:

> Support for push options in the receive-pack protocol (and all Git
> components that speak it) have been added over a few commits, but not
> fully documented (especially its interaction with signed pushes). Update
> the protocol documentation to include the relevant details.

Thanks. This could be combined with the previous patch, since that patch is enforcing what this patch documents.

[...]

Show 5 quoted lines

> -This list is followed by a flush-pkt. Then the push options are transmitted
> -one per packet followed by another flush-pkt. After that the packfile that
> -should contain all the objects that the server will need to complete the new
> -references will be sent.
> +This list is followed by a flush-pkt.

I think this removed more than intended.

Before c714e45f (receive-pack: implement advertising and receiving push options, 2016-07-14), this said

	This list is followed by a flush-pkt and then the packfile that should
	contain all the objects that the server will need to complete the new
	references.

which I think would work fine.

[...]

> -  update-request    =  *shallow ( command-list | push-cert ) [packfile]
> +  update-request    =  *shallow ( command-list | push-cert )

This seems confusing to me both before and after. How does update-request get used? Is it supposed to include the packfile or not?

Where do push-options fit into an unsigned request?

[...]

Show 24 quoted lines

> @@ -500,12 +497,35 @@ references will be sent.
>  		      PKT-LINE("pusher" SP ident LF)
>  		      PKT-LINE("pushee" SP url LF)
>  		      PKT-LINE("nonce" SP nonce LF)
> +		      *PKT-LINE("push-option" SP push-option LF)
>  		      PKT-LINE(LF)
>  		      *PKT-LINE(command LF)
>  		      *PKT-LINE(gpg-signature-lines LF)
>  		      PKT-LINE("push-cert-end" LF)
>  
> -  packfile          = "PACK" 28*(OCTET)
> +  push-option       =  1*CHAR
> +----
> +
> +If the server has advertised the 'push-options' capability and the client has
> +specified 'push-options' as part of the capability list above, the client then
> +sends its push options followed by a flush-pkt.
> +
> +----
> +  push-options      =  *PKT-LINE(push-option) flush-pkt
> +----
> +
> +For backwards compatibility with older Git servers, if the client sends a push
> +cert and push options, it SHOULD send its push options both embedded within the

This needs to be a MUST, not SHOULD.

> +push cert and after the push cert. (Note that the push options within the cert
> +are prefixed, but the push options after the cert are not.) Both these lists
> +SHOULD be consistent.

Likewise this one.

What does it mean to be consistent?

Show 8 quoted lines

> +
> +After that the packfile that
> +should contain all the objects that the server will need to complete the new
> +references will be sent.
> +
> +----
> +  packfile          =  "PACK" 28*(OCTET)
>  ----

Thanks, Jonathan