Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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);
> }
> }
