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

[Brandon Williams]

On 02/23, Stefan Beller wrote:
> On Fri, Feb 23, 2018 at 1:30 PM, Brandon Williams  wrote:
> > On 02/22, Stefan Beller wrote:
> >> > +static enum protocol_version discover_version(struct packet_reader 
> >> > *reader)
> >> > +{
> >> ...
> >> > +
> >> > +   /* 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:
> >> > +   die("unknown protocol version: '%s'\n", reader->line);
> >>
> >> The following patches introduce more of the switch(version) cases.
> >> And there it actually is a
> >> BUG("protocol version unknown? should have been set in 
> >> discover_version")
> >> but here it is a mere
> >>   die (_("The server uses a different protocol version than we can
> >> speak: %s\n"),
> >>   reader->line);
> >> so I would think here it is reasonable to add _(translation).
> >
> > This should be a BUG as it shouldn't ever be unknown at this point.  And
> > I'll also drop that comment.
> 
> Huh?
> Then I miss-understood the flow of code. When the server announces its
> answer is version 42, but the client cannot handle it, which die call is
> responsible for reporting it to the user?
> (That is technically a BUG on the server side, as we probably never
> asked for v42, so I would not want to print BUG locally at the client?)

This is handled in 
`determine_protocol_version_client(const char *server_response)`,
which is just a few lines out of context here.

-- 
Brandon Williams

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

[Stefan Beller]

On Fri, Feb 23, 2018 at 1:30 PM, Brandon Williams  wrote:
> On 02/22, Stefan Beller wrote:
>> > +static enum protocol_version discover_version(struct packet_reader 
>> > *reader)
>> > +{
>> ...
>> > +
>> > +   /* 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:
>> > +   die("unknown protocol version: '%s'\n", reader->line);
>>
>> The following patches introduce more of the switch(version) cases.
>> And there it actually is a
>> BUG("protocol version unknown? should have been set in discover_version")
>> but here it is a mere
>>   die (_("The server uses a different protocol version than we can
>> speak: %s\n"),
>>   reader->line);
>> so I would think here it is reasonable to add _(translation).
>
> This should be a BUG as it shouldn't ever be unknown at this point.  And
> I'll also drop that comment.

Huh?
Then I miss-understood the flow of code. When the server announces its
answer is version 42, but the client cannot handle it, which die call is
responsible for reporting it to the user?
(That is technically a BUG on the server side, as we probably never
asked for v42, so I would not want to print BUG locally at the client?)

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

[Brandon Williams]

On 02/22, Stefan Beller wrote:
> > +static enum protocol_version discover_version(struct packet_reader *reader)
> > +{
> ...
> > +
> > +   /* 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:
> > +   die("unknown protocol version: '%s'\n", reader->line);
> 
> The following patches introduce more of the switch(version) cases.
> And there it actually is a
> BUG("protocol version unknown? should have been set in discover_version")
> but here it is a mere
>   die (_("The server uses a different protocol version than we can
> speak: %s\n"),
>   reader->line);
> so I would think here it is reasonable to add _(translation).

This should be a BUG as it shouldn't ever be unknown at this point.  And
I'll also drop that comment.

-- 
Brandon Williams

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

[Stefan Beller]

> +static enum protocol_version discover_version(struct packet_reader *reader)
> +{
...
> +
> +   /* 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:
> +   die("unknown protocol version: '%s'\n", reader->line);

The following patches introduce more of the switch(version) cases.
And there it actually is a
BUG("protocol version unknown? should have been set in discover_version")
but here it is a mere
  die (_("The server uses a different protocol version than we can
speak: %s\n"),
  reader->line);
so I would think here it is reasonable to add _(translation).

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

[Stefan Beller]

On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:

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

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

[Brandon Williams]

In order to allow for better control flow when protocol_v2 is introduced
convert 'get_remote_heads()' to use 'struct packet_reader' to read
packet lines.  This enables a client to be able to peek the first line
of a server's response (without consuming it) in order to determine the
protocol version its speaking and then passing control to the
appropriate handler.

This is needed because the initial response from a server speaking
protocol_v0 includes the first ref, while subsequent protocol versions
respond with a version line.  We want to be able to read this first line
without consuming the first ref sent in the protocol_v0 case so that the
protocol version the server is speaking can be determined outside of
'get_remote_heads()' in a future patch.

Signed-off-by: Brandon Williams 
---
 connect.c | 174 ++
 1 file changed, 96 insertions(+), 78 deletions(-)

diff --git a/connect.c b/connect.c
index c3a014c5b..00e90075c 100644
--- a/connect.c
+++ b/connect.c
@@ -48,6 +48,12 @@ int check_ref_type(const struct ref *ref, int flags)
 
 static void die_initial_contact(int unexpected)
 {
+   /*
+* 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.
+*/
if (unexpected)
die(_("The remote end hung up upon initial contact"));
else
@@ -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 */
+   switch (version) {
+   case protocol_v1:
+   /* Read the peeked version line */
+   packet_reader_read(reader);
+   break;
+   case protocol_v0:
+   break;
+   case protocol_unknown_version:
+   die("unknown protocol version: '%s'\n", reader->line);
+   }
+
+   return version;
+}
+
 static void parse_one_symref_info(struct string_list *symref, const char *val, 
int len)
 {
char *sym, *target;
@@ -109,60 +150,21 @@ static void annotate_refs_with_symref_info(struct ref 
*ref)
string_list_clear(, 0);
 }
 
-/*
- * Read one line of a server's ref advertisement into packet_buffer.
- */
-static int read_remote_ref(int in, char **src_buf, size_t *src_len,
-  int *responded)
+static void process_capabilities(const char *line, int *len)
 {
-   int len = packet_read(in, src_buf, src_len,
- packet_buffer, sizeof(packet_buffer),
- PACKET_READ_GENTLE_ON_EOF |
- PACKET_READ_CHOMP_NEWLINE);
-   const char *arg;
-   if (len < 0)
-   die_initial_contact(*responded);
-   if (len > 4 && skip_prefix(packet_buffer, "ERR ", ))
-   die("remote error: %s", arg);
-
-   *responded = 1;
-
-   return len;
-}
-
-#define EXPECTING_PROTOCOL_VERSION 0
-#define EXPECTING_FIRST_REF 1
-#define EXPECTING_REF 2
-#define EXPECTING_SHALLOW 3
-
-/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. */
-static int process_protocol_version(void)
-{
-   switch (determine_protocol_version_client(packet_buffer)) {
-   case protocol_v1:
-   return 1;
-   case protocol_v0:
-   return 0;
-   default:
-   die("server is speaking an unknown protocol");
-   }
-}
-
-static void process_capabilities(int *len)
-{
-   int nul_location = strlen(packet_buffer);
+   int nul_location = strlen(line);
if (nul_location == *len)
return;
-   server_capabilities = xstrdup(packet_buffer + nul_location + 1);
+   server_capabilities = xstrdup(line + nul_location + 1);
*len = nul_location;
 }
 
-static int process_dummy_ref(void)
+static int process_dummy_ref(const char *line)
 {
struct object_id oid;
const char *name;
 
-   if (parse_oid_hex(packet_buffer, , ))
+   if (parse_oid_hex(line, , ))
return 0;
if (*name != ' ')
return 0;
@@