Re: [PATCH v3 07/35] connect: convert get_remote_heads to use struct packet_reader

From: Stefan Beller <sbeller@google.com>
Date: Feb 22, 2018, 19:52 UTC
Message-ID: <CAGZ79ka_tYYhFE5OPfSgpuvssuivCTj7x3aXzLYLO22WGugTRA@mail.gmail.com>
In-Reply-To: <20180207011312.189834-8-bmwill@google.com>

On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams <bmwill@google.com> wrote:

Show 25 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_EOF:
> +               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 */

We do not (yet) react to v2, so this comment only makes sense after a later patch? If so please include it later, as this is confusing for now.

> +       switch (version) {
> +       case protocol_v1:
> +               /* Read the peeked version line */
> +               packet_reader_read(reader);

I wonder if we want to assign version to v0 here, as now all v1 is done and we could treat the remaining communication as a v0. Not sure if that helps with some switch/cases, but as we'd give all cases to have the compiler not yell at us, this would be no big deal. So I guess we can keep it v1.

With or without the comment nit, this patch is
Reviewed-by: Stefan Beller <sbeller@google.com>