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

From: Jonathan Tan <jonathantanmy@google.com>
Date: May 8, 2017, 21:27 UTC
Message-ID: <01f61fa4-d69d-8f0b-76e1-bca3444f4101@google.com>
In-Reply-To: <20170506002605.GM28740@aiede.svl.corp.google.com>

On 05/05/2017 05:26 PM, Jonathan Nieder wrote:

Show 16 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.

That wouldn't work fine because there is no mention of push options - hence the modification in c714e45f to talk about push options, but that is also not accurate because push options (and, more importantly, the associated flush-pkt) are not sent if the client doesn't have any.

Show 8 quoted lines

> [...]
>> -  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?

I've updated "update-request" to "update-requests" to show that this is the "list of reference update requests" mentioned in the previous paragraph.

This is only the ref part - I've chosen to describe push options and packfile in separate sections, because there are very specific rules that determine whether the push option section must be included or omitted.

Show 36 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?

Changed to "MUST be the same, modulo the prefix".