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

From: Brandon Williams <[email-masked]>
Date: Fri, 23 Feb 2018 14:56:37 -0800

On 02/23, Stefan Beller wrote:
> On Fri, Feb 23, 2018 at 1:30 PM, Brandon Williams <[email-masked]> 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