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

> On Tue, Mar 27, 2018 at 6:25 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> > On Tue, Mar 27, 2018 at 6:11 PM, Jeff King <peff@peff.net> wrote:
> >> On Tue, Mar 27, 2018 at 05:27:14PM +0200, Duy Nguyen wrote:
> >>
> >>> On Thu, Mar 15, 2018 at 10:31:14AM -0700, Brandon Williams wrote:
> >>> > In order to allow for better control flow when protocol_v2 is introduced
> >>> > +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:
> >>>
> >>> gcc is dumb. When -Werror and -Wimplicit-fallthrough are enabled (on
> >>> at least gcc 7.x), it fails to realize that this die_initial_contact()
> >>> will not fall through (even though we do tell it about die() not
> >>> returning, but I guess that involves more flow analysis to realize
> >>> die_initial_contact is in the same boat).
> >>> [...]
> >>> @@ -124,6 +124,7 @@ enum protocol_version discover_version(struct packet_reader *reader)
> >>>       switch (packet_reader_peek(reader)) {
> >>>       case PACKET_READ_EOF:
> >>>               die_initial_contact(0);
> >>> +             break;
> >>
> >> Would it make sense just to annotate that function to help the flow
> >> analysis?
> >
> > Yes that works wonderfully with my gcc-7.3.0
> 
> And this changes things. Since this series is 35 patches and there's
> no sign of reroll needed, I'm going to make this change separately.
> Don't reroll just because of this
> -- 
> Duy