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

> Brandon Williams <bmwill@google.com> 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?