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

Jonathan Tan [off-list ref] 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 <redacted>
---
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);
 		}
 	}