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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2017, 22:39 UTC
Message-ID: <xmqqh8t3fpm7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171204235823.63299-8-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 39 quoted lines

> @@ -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.

Show 30 quoted lines

> +#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.

Show 17 quoted lines

> -	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.