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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 7, 2017, 18:50 UTC
Message-ID: <xmqq7etye5il.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171204235823.63299-9-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 16 quoted lines

> @@ -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?