Re: [PATCH] [PATCH] Fix upload_pack_v2 response ordering for shallow fetch when server has uploadpack.allowRefInWant=true

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

Junio C Hamano <gitster@pobox.com> writes:

>> diff --git a/upload-pack.c b/upload-pack.c
>> index a52856d869..a70d237ad3 100644
>> --- a/upload-pack.c
>> +++ b/upload-pack.c
>> @@ -1812,8 +1812,8 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request)
>>  				state = UPLOAD_DONE;
>>  			break;
>>  		case UPLOAD_SEND_PACK:
>> -			send_wanted_ref_info(&data);
>>  			send_shallow_info(&data);
>> +			send_wanted_ref_info(&data);
>>  
>>  			if (data.uri_protocols.nr) {
>>  				create_pack_file(&data, &data.uri_protocols);

I am merely guessing what your reasoning is, but is this meant to
match this part of the code on the other side of the connection?

		case FETCH_GET_PACK:
			trace2_region_leave("fetch-pack",
					    "negotiation_v2",
					    the_repository);
			trace2_data_intmax("negotiation_v2", the_repository,
					   "total_rounds", negotiation_round);
			/* Check for shallow-info section */
			if (process_section_header(&reader, "shallow-info", 1))
				receive_shallow_info(args, &reader, shallows, si);

			if (process_section_header(&reader, "wanted-refs", 1))
				receive_wanted_refs(&reader, sought, nr_sought);


These process_section_header() calls are made with the peek bit set,
signaling that it is OK if the packet we are about to receive is not
the one that is being checked, so what may happen is

 - upload-pack gives wanted-ref info and then shallow-info.

 - fetch-pack sees wanted-refs, notices that it is not shallow-info,
   ignores it, and the next process_section_header() call does
   notice it is wanted-refs and processes it.

But then, who consumes the shallow-info?  Does fetch-pack notices
shallow-info that it did not expect to see and crashes?  If so, that
is a very noteworthy thing to say in the proposed log message.  If
it does not crash and goes on but without utilizing what was carried
in the shallow-info packet, the resulting behaviour of fetch-pack
would be different from what we would expect, and if that is the
case, that difference is a noteworthy thing to decribe in the
proposed log message.

Thanks.