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