Re: [PATCH v2 3/9] protocol: introduce protocol extention mechanisms

[Brandon Williams <bmwill@google.com>]

On 09/27, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> +enum protocol_version determine_protocol_version_server(void)
> >> +{
> >> +	const char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT);
> >> +	enum protocol_version version = protocol_v0;
> >> +
> >> +	if (git_protocol) {
> >> +		struct string_list list = STRING_LIST_INIT_DUP;
> >> +		const struct string_list_item *item;
> >> +		string_list_split(&list, git_protocol, ':', -1);
> >> +
> >> +		for_each_string_list_item(item, &list) {
> >> +			const char *value;
> >> +			enum protocol_version v;
> >> +
> >> +			if (skip_prefix(item->string, "version=", &value)) {
> >> +				v = parse_protocol_version(value);
> >> +				if (v > version)
> >> +					version = v;
> >> +			}
> >> +		}
> >> +
> >> +		string_list_clear(&list, 0);
> >> +	}
> >> +
> >> +	return version;
> >> +}
> >
> > This implements "the largest one wins", not "the last one wins".  Is
> > there a particular reason why the former is chosen?
> 
> Let me give my version of why the usual "the last one wins" would
> not necessarily a good idea.  I would imagine that a client
> contacting the server may want to say "I understand v3, v2 (but not
> v1 nor v0)" and in order to influence the server's choice between
> the available two, it may want to somehow say it prefers v3 over v2
> (or v2 over v3).  
> 
> One way to implement such a behaviour would be "the first one that
> is understood is used", i.e. something along this line:
> 
>         enum protocol_version version = protocol_unknown;
> 
> 	for_each_string_list_item(item, &list) {
> 		const char *value;
> 		enum protocol_version v;
> 		if (skip_prefix(item->string, "version=", &value)) {
>                 	if (version == protocol_unknown) {
>                         	v = parse_protocol_version(value);
> 			        if (v != protocol_unknown)
> 					version = v;
> 			}
> 		}
> 	}
> 
> 	if (version == protocol_unknown)
> 		version = protocol_v0;
> 
> and not "the largest one wins" nor "the last one wins".
> 
> I am not saying your code or the choice of "the largest one wins" is
> necessarily wrong.  I am just illlustrating the way to explain
> "because I want to support a usecase like _this_, I define the way
> in which multiple values to the version variable is parsed like so,
> hence this code".  IOW, I think this commit should mention how the
> "largest one wins" rule would be useful to the clients and the
> servers when they want to achieve X---and that X is left unexplained.

I believe I mentioned this elsewhere but I think that at some point this
logic will probably have to be tweaked again at some point so that a
server may be able to prefer one version to another.

That being said I can definitely add a comment indicating how this code
selects the version and that it can be used to ensure that the latest
and greatest protocol version is used.

-- 
Brandon Williams