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

2 messages, 2 authors, 2017-09-28 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2017-09-27 05:17:46

Brandon Williams [off-list ref] writes:
+`GIT_PROTOCOL`::
+	For internal use only.  Used in handshaking the wire protocol.
+	Contains a colon ':' separated list of keys with optional values
+	'key[=value]'.  Presence of unknown keys must be tolerated.
Is this meant to be used only on the "server" end?  Am I correct to
interpret "handshaking" to mean the initial connection acceptor
(e.g. "git daemon") uses it to pass what it decided to the programs
that implement the service (e.g. "git receive-pack")?
+/*
+ * Environment variable used in handshaking the wire protocol.
+ * Contains a colon ':' separated list of keys with optional values
+ * 'key[=value]'.  Presence of unknown keys must be tolerated.
+ */
+#define GIT_PROTOCOL_ENVIRONMENT "GIT_PROTOCOL"
"Must be tolerated" feels a bit strange.  When somebody asks you to
use "version 3" or "version 1 variant 2", when you only know
"version 0" or "version 1" and you are not yet even aware of the
concept of "variant", we simply ignore "variant=2" as if it wasn't
there, even though "version=3" will be rejected (because we know of
"version"; it's just that we don't know "version=3").
+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?

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

From: Brandon Williams <hidden>
Date: 2017-09-28 21:59:05

On 09/27, Junio C Hamano wrote:
Brandon Williams [off-list ref] writes:
quoted
+`GIT_PROTOCOL`::
+	For internal use only.  Used in handshaking the wire protocol.
+	Contains a colon ':' separated list of keys with optional values
+	'key[=value]'.  Presence of unknown keys must be tolerated.
Is this meant to be used only on the "server" end?  Am I correct to
interpret "handshaking" to mean the initial connection acceptor
(e.g. "git daemon") uses it to pass what it decided to the programs
that implement the service (e.g. "git receive-pack")?
Yes, the idea is that the client will request a protocol version by
setting GIT_PROTOCOL (or indirectly set when using git:// or http://).
upload-pack and receive-pack will use the keys and values set in
GIT_PROTOCOL to determine which version of the protocol to use.  At some
point in the future they may even use other keys and values as a means
of sending more information in an initial request from the client.
quoted
+/*
+ * Environment variable used in handshaking the wire protocol.
+ * Contains a colon ':' separated list of keys with optional values
+ * 'key[=value]'.  Presence of unknown keys must be tolerated.
+ */
+#define GIT_PROTOCOL_ENVIRONMENT "GIT_PROTOCOL"
"Must be tolerated" feels a bit strange.  When somebody asks you to
use "version 3" or "version 1 variant 2", when you only know
"version 0" or "version 1" and you are not yet even aware of the
concept of "variant", we simply ignore "variant=2" as if it wasn't
there, even though "version=3" will be rejected (because we know of
"version"; it's just that we don't know "version=3").
By "Must be tolerated" I was trying to get across that if the server
seeing something it doesn't understand, it shouldn't choke.  Maybe a
better wording would be to use the word "ignored"?
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?
I envision this logic changing for newer servers once more protocol
versions are added because at some point a server may want to disallow a
particular version (because of a security issue or what have you).  So I
figured the easiest thing to do for now was to implement "Newest version
wins".

-- 
Brandon Williams
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help