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

Junio C Hamano [off-list ref] writes:

quoted

+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.

On 09/27, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

quoted

+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