Subject: Re: [WIP 08/15] connect: discover protocol version outside of get_remote_heads

From: Junio C Hamano <[email-masked]>
Date: Thu, 07 Dec 2017 10:50:58 -0800

Brandon Williams <[email-masked]> writes:

> @@ -193,7 +195,17 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  		if (!conn)
>  			return args.diag_url ? 0 : 1;
>  	}
> -	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
> +
> +	packet_reader_init(&reader, fd[0], NULL, 0);
> +
> +	switch (discover_version(&reader)) {
> +	case protocol_v1:
> +	case protocol_v0:
> +		get_remote_heads(&reader, &ref, 0, NULL, &shallow);
> +		break;
> +	case protocol_unknown_version:
> +		BUG("unknown protocol version");
> +	}

We see quite a few hunks just like this one appear in this patch.
The repetition is a bit disturbing.  I wonder if we want a wrapper
around the "reader-init && discover-version && return an error if
the protocol version is not known" dance.  That would allow us to
write this part of the code like so:

	struct packet_reader reader;

	if (connection_preamble(&reader, fd[0]))
		die("unknown protocol version");
	get_remote_heads(&reader, &ref, 0, NULL, &shallow);

or something like that.

By the way, is that really a BUG()?  Getting a connection and the
other end declaring a protocol version you do not yet understand is
not a bug in your program---it is a normal runtime error, no?