Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 10, 2017, 19:51 UTC
Message-ID: <20171010125142.87959e72be8ea5f4abdda0e6@google.com>
In-Reply-To: <20171003201507.3589-4-bmwill@google.com>

On Tue, 3 Oct 2017 13:15:00 -0700 Brandon Williams <bmwill@google.com> wrote:

Show 22 quoted lines

> +enum protocol_version determine_protocol_version_server(void)
> +{
> +	const char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT);
> +	enum protocol_version version = protocol_v0;
> +
> +	/*
> +	 * Determine which protocol version the client has requested.  Since
> +	 * multiple 'version' keys can be sent by the client, indicating that
> +	 * the client is okay to speak any of them, select the greatest version
> +	 * that the client has requested.  This is due to the assumption that
> +	 * the most recent protocol version will be the most state-of-the-art.
> +	 */
> +	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)) {

After writing some protocol docs [1], I wonder if this is also too lenient. The code should probably die if a lone "version" (without the equals sign) is given.

[1] https://public-inbox.org/git/20171010193956.168385-1-jonathantanmy@google.com/

Show 11 quoted lines

> +				v = parse_protocol_version(value);
> +				if (v > version)
> +					version = v;
> +			}
> +		}
> +
> +		string_list_clear(&list, 0);
> +	}
> +
> +	return version;
> +}

Also, in your commit title, it is "extension", not "extention".