Re: [PATCH v3] connect: in ref advertisement, shallows are last

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

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

> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> Currently, get_remote_heads() parses the ref advertisement in one loop,
>> allowing refs and shallow lines to intersperse, despite this not being
>> allowed by the specification. Refactor get_remote_heads() to use two
>> loops instead, enforcing that refs come first, and then shallows.
>>
>> This also makes it easier to teach get_remote_heads() to interpret other
>> lines in the ref advertisement, which will be done in a subsequent
>> patch.
>
> Sounds sensible.  This still replaces the earlier 1.5?

Well, it does, but it also invalidates how the new "pick the version
offered and used" feature is integrated to this callchain.  I guess
we'd need a new "we are now expecting the version info" state in a
patch to replace "connect: teach client to recognize v1 server
response".

>> +static int process_ref(int *state, int len, struct ref ***list,
>> +		       unsigned int flags, struct oid_array *extra_have)
>> +{
>> +	struct object_id old_oid;
>> +	char *name;
>> +	int name_len;
>> +
>> +	if (len < GIT_SHA1_HEXSZ + 2 ||
>> +	    get_oid_hex(packet_buffer, &old_oid) ||
>> +	    packet_buffer[GIT_SHA1_HEXSZ] != ' ') {
>> +		*state = EXPECTING_SHALLOW;
>> +		return 0;
>> +	}
>> +
>> +	name = packet_buffer + GIT_SHA1_HEXSZ + 1;
>> +	name_len = strlen(name);
>> +	if (*state == EXPECTING_REF_WITH_CAPABILITIES &&
>> +	    len != name_len + GIT_SHA1_HEXSZ + 1) {
>> +		free(server_capabilities);

Is this free() still needed?  After hitting this block, you'd set
*state to EXPECTING_REF before you return, so nobody would set
server_capabilities by hitting this block twice, and an attempt to
do so will hit the die("unexpected cap") below, no?

Or it may be a signal that this patch tightens it too much and
breaks older or third-party implementations of the other side that
can emit more than one refs with capability advertisement?

>> +		server_capabilities = xstrdup(name + name_len + 1);
>> +	} else if (*state == EXPECTING_REF) {
>> +		if (len != name_len + GIT_SHA1_HEXSZ + 1)
>> +			die("unexpected capabilities after ref name");
>> +	}
>> +	...
>> +	}
>> +	*state = EXPECTING_REF;
>> +	return 1;
>> +}

>> @@ -123,76 +208,26 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>>  	 * willing to talk to us.  A hang-up before seeing any
>>  	 * response does not necessarily mean an ACL problem, though.
>>  	 */
>> -	int saw_response;
>> -	int got_dummy_ref_with_capabilities_declaration = 0;
>> +	int responded = 0;
>> +	int len;
>> +	int state = EXPECTING_REF_WITH_CAPABILITIES;
>>  
>>  	*list = NULL;

>> +	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
>> +		switch (state) {
>> +		case EXPECTING_REF_WITH_CAPABILITIES:
>> +		case EXPECTING_REF:
>> +			if (process_ref(&state, len, &list, flags, extra_have))
>> +				break;
>> +			/* fallthrough */

OK.  This fallthrough is because expecting-ref is really expecting
ref or shallow and once we see a shallow, we no longer expect ref
and expect only shallow.  So from that point of view, an assignment
to set state to EXPECTING_SHALLOW could happen here, not inside
process_ref.  I mention this because in general, passing state
around and let it be updated in helper functions would make the
state transition harder to follow, not easier, even though
refactoring the processing needed in different stages into helper
functions like this patch does ought to make it easier to see by
shrinking the outer loop (i.e. this one) that controls the whole
process.

I think if we split process_ref() further into two, then we no
longer need to pass &state to that function?  We start this loop
with "expecting the dummy ref (or other)" state, have a new
process_dummy_ref() function check if we got "capabilities^{}" thing
and do its thing if that is the case (otherwise we fall through to
the call to process_ref(), just like the above code falls through to
call process_shallow() when it realizes what it got is not a ref),
and after the first call to process_dummy_ref() we'd be in the
"expecting ref (or other)" state---and the state transition can
happen in this caller, not in process_dummy_ref() or process_ref().

Inside process_dummy_ref() and process_ref(), there would be a call
to the same helper that notices and extracts the server capability
and stores it (or barfs against the second line that advertises the
capability, by noticing that server_capabilities is not NULL).

Wouldn't that make the presentation of the state machine cleaner?