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

[Jeff King <peff@peff.net>]

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? Like:

diff --git a/connect.c b/connect.c
index c3a014c5ba..49eca46462 100644
--- a/connect.c
+++ b/connect.c
@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int unexpected)
+static NORETURN void die_initial_contact(int unexpected)
 {
 	if (unexpected)
 		die(_("The remote end hung up upon initial contact"));

That should let the callers know what's going on, and inside the
function itself, the compiler should confirm that all code paths hit
another NORETURN function.

-Peff