Re: [PATCH v2 6/9] connect: teach client to recognize v1 server response

[Brandon Williams <bmwill@google.com>]

On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > +/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. */
> > +static int process_protocol_version(void)
> > +{
> > +	switch (determine_protocol_version_client(packet_buffer)) {
> > +		case protocol_v1:
> > +			return 1;
> > +		case protocol_v0:
> > +			return 0;
> > +		default:
> > +			die("server is speaking an unknown protocol");
> > +	}
> > +}
> 
> For the purpose of "technology demonstration" v1 protocol, it is OK
> to discard the result of "determine_pvc()" like the above code, but
> in a real application, we would do a bit more than just ignoring an
> extra "version #" packet that appears at the beginning, no?
> 
> It would be sensible to design how the result of determien_pvc()
> call is propagated to the remainder of the program in this patch and
> implement it.  Perhaps add a new global (like server_capabilities
> already is) and store the value there, or something?  Or pass a
> pointer to enum protocol_version as a return-location parameter to
> this helper function so that the process_capabilities() can pass a
> pointer to its local variable?

Yes, once we actually implement a v2 we would need to not throw away the
result of 'determine_pvc()' and instead do control flow based on the
resultant version.  I was trying to implement 'v1' as simply as possible
so that I wouldn't have to do a large amount of refactoring when
proposing this transition, though it seems Jonathan ended up doing more
than I planned, as I figured we didn't really know what the code will
need to be refactored to, in order to handle another protocol version.
I would suspect that we maybe wouldn't want to determine which version a
server is speaking in 'get_remote_heads()' but rather at some point
before that so we can branch off to do v2 like things, for example,
capability discovery and not ref discovery.

If you do think we need to do more of that refactoring now, before a v2,
I can most certainly work on that.


> 
> >  static void process_capabilities(int *len)
> >  {
> > @@ -224,12 +239,19 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
> >  	 */
> >  	int responded = 0;
> >  	int len;
> > -	int state = EXPECTING_FIRST_REF;
> > +	int state = EXPECTING_PROTOCOL_VERSION;
> >  
> >  	*list = NULL;
> >  
> >  	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
> >  		switch (state) {
> > +		case EXPECTING_PROTOCOL_VERSION:
> > +			if (process_protocol_version()) {
> > +				state = EXPECTING_FIRST_REF;
> > +				break;
> > +			}
> > +			state = EXPECTING_FIRST_REF;
> > +			/* fallthrough */
> >  		case EXPECTING_FIRST_REF:
> >  			process_capabilities(&len);
> >  			if (process_dummy_ref()) {

-- 
Brandon Williams