Re: [PATCH v3 07/35] connect: convert get_remote_heads to use struct packet_reader
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
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
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
> +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
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