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

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

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.
>
> As part of this change, this patch interprets capabilities only on the
> first line in the ref advertisement, ignoring all others.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> I've updated state transitions to occur in get_remote_heads() instead,
> as suggested. I didn't want to do that previously because each step in
> the state machine needed to communicate if (i) the line is "consumed"
> and (ii) the state needed to be advanced, but with Junio's suggestion to
> reorganize the methods, that is no longer true.
>
> As Junio said, the free(server_capabilities) can be removed.
>
> As for whether how capabilities on subsequent lines are handled, I think
> it's better to ignore them - they are behind NULs, after all.

Not even a diagnosis?  When the other side clearly wants to tell us
something and we deliberately ignore, I'd think we want to at least
know about it---that may lead us to notify the implementators of the
other side of a protocol violation, or rethink the design decision
(i.e. only the first one matters) ourselves.

> This change does have the side effect that if the server sends a ref
> advertisement with "shallow"s only (and no refs), things will still
> work, and the server can even tuck capabilities on the first "shallow"
> line. I think that's fine, and it does make the client code cleaner.

I am ambivalent on this aspect of the change.

The change makes the resulting state transition logic quite easy to
follow.  Very nicely done.

> +	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
> +		switch (state) {
> +		case EXPECTING_FIRST_REF:
> +			process_capabilities(len);
> +			if (process_dummy_ref()) {
> +				state = EXPECTING_SHALLOW;
> +				break;
> +			}
> +			state = EXPECTING_REF;
> +			/* fallthrough */
> +		case EXPECTING_REF:
> +			if (process_ref(&list, flags, extra_have))
> +				break;
> +			state = EXPECTING_SHALLOW;
> +			/* fallthrough */
> +		case EXPECTING_SHALLOW:
> +			if (process_shallow(shallow_points))
> +				break;
> +			die("protocol error: unexpected '%s'", packet_buffer);
> +		default:
> +			die("unexpected state %d", state);
>  		}
>  	}