Re: [WIP 07/15] connect: convert get_remote_heads to use struct packet_reader

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

Brandon Williams <bmwill@google.com> writes:

> @@ -56,6 +62,41 @@ static void die_initial_contact(int unexpected)
>  		      "and the repository exists."));
>  }
>  
> +static enum protocol_version discover_version(struct packet_reader *reader)
> +{
> +	enum protocol_version version = protocol_unknown_version;
> +
> +	/*
> +	 * Peek the first line of the server's response to
> +	 * determine the protocol version the server is speaking.
> +	 */
> +	switch (packet_reader_peek(reader)) {
> +	case PACKET_READ_ERROR:
> +		die_initial_contact(0);
> +	case PACKET_READ_FLUSH:
> +	case PACKET_READ_DELIM:
> +		version = protocol_v0;
> +		break;
> +	case PACKET_READ_NORMAL:
> +		version = determine_protocol_version_client(reader->line);
> +		break;
> +	}
> +
> +	/* Maybe process capabilities here, at least for v2 */
> +	switch (version) {
> +	case protocol_v1:
> +		/* Read the peeked version line */
> +		packet_reader_read(reader);
> +		break;
> +	case protocol_v0:
> +		break;
> +	case protocol_unknown_version:
> +		BUG("ERROR");
> +	}
> +
> +	return version;
> +}
> +

This discovers and consumes the "version" thing, but for an older
protocol that does not have the "version" thing, it does not clobber
the first "ref", thanks to its use of peek.  Makes sense.

> +#define EXPECTING_FIRST_REF 0
> +#define EXPECTING_REF 1
> +#define EXPECTING_SHALLOW 2
> +#define EXPECTING_DONE 3
>  
>  static void process_capabilities(int *len)
>  {
> @@ -230,28 +237,34 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  			      struct oid_array *shallow_points)
>  {
>  	struct ref **orig_list = list;
> +	int len = 0;
> +	int state = EXPECTING_FIRST_REF;
> +	struct packet_reader reader;
> +	const char *arg;
>  
> -	/*
> -	 * A hang-up after seeing some response from the other end
> -	 * means that it is unexpected, as we know the other end is
> -	 * willing to talk to us.  A hang-up before seeing any
> -	 * response does not necessarily mean an ACL problem, though.
> -	 */
> -	int responded = 0;
> -	int len;
> -	int state = EXPECTING_PROTOCOL_VERSION;
> +	packet_reader_init(&reader, in, src_buf, src_len);
> +
> +	discover_version(&reader);
>  
>  	*list = NULL;

And thanks to the "peeking" implementation, the version handling is
cleanly separated from the rest of the exchange, which is good.

> -	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
> +	while (state != EXPECTING_DONE) {
> +		switch (packet_reader_read(&reader)) {
> +		case PACKET_READ_ERROR:
> +			die_initial_contact(1);
> +		case PACKET_READ_NORMAL:
> +			len = reader.pktlen;
> +			if (len > 4 && skip_prefix(packet_buffer, "ERR ", &arg))
> +				die("remote error: %s", arg);
> +			break;
> +		case PACKET_READ_FLUSH:
> +			state = EXPECTING_DONE;
> +			break;
> +		case PACKET_READ_DELIM:
> +			die("invalid packet\n");
> +		}
> +

EXPECTING_DONE sounded like we are expecting to see 'done' packet
sent from the other side, but I was mistaken.  It is the state
where we are "done" expecting anything ;-).

Having an (unconditional) assignment to 'state' in the above switch
makes me feel somewhat uneasy, as the next "switch (state)" is what
is meant as the state machine that would allow us to say things like
"from this state, transition to that state is impossible".  When we
get a flush while we are expecting the first ref, for example, we'd
just go into the "done" state.  There is no provision for a future
update to say "no, getting a flush in this state is an error".

That is no different from the current code; when read_remote_ref()
notices that it got a flush, it just leaves the loop without even
touching 'state' variable.  But at least, I find that the current
code is more honest---it does not even touch 'state' and allows the
code after the loop to inspect it, if needed.  From that point of
vhew, the way the new code uses 'state' to leave the loop upon
seeing a flush is a regression---it makes it harder to notice and
report when we got a flush in a wrong state.

Perhaps getting rid of "EXPECTING_DONE" from the enum and then:

	int got_flush = 0;
	while (1) {
		switch (reader_read()) {
		case PACKET_READ_FLUSH:
			got_flush = 1;
			break;
		... other cases ...
		}

		if (got_flush)
			break;

		switch (state) {
		... current code ...
		}
	}

would be an improvement; we can later extend "if (got_flush)" part
to check what state we are in if we wanted to notice and report an
error there before breaking out of the loop.