Re: [PATCH v3 1/1] protocol: advertise multiple supported versions

[Junio C Hamano <gitster@pobox.com>]

steadmon@google.com writes:

> Currently the client advertises that it supports the wire protocol
> version set in the protocol.version config. However, not all services
> support the same set of protocol versions. When connecting to
> git-receive-pack, the client automatically downgrades to v0 if
> config.protocol is set to v2, but this check is not performed for other
> services.

"downgrades to v0 even if ... is set to v2" you mean?  Otherwise it
is unclear why asking for v2 leads to using v0.

> This patch creates a protocol version registry. Individual operations
> register all the protocol versions they support prior to communicating
> with a server. Versions should be listed in preference order; the
> version specified in protocol.version will automatically be moved to the
> front of the registry.
>
> The protocol version registry is passed to remote helpers via the
> GIT_PROTOCOL environment variable.
>
> Clients now advertise the full list of registered versions. Servers
> select the first recognized version from this advertisement.

Makes sense.

> +void get_client_protocol_version_advertisement(struct strbuf *advert)
> +{
> +	int tmp_nr = nr_allowed_versions;
> +	enum protocol_version *tmp_allowed_versions, config_version;
> +	strbuf_reset(advert);
> +
> +	have_advertised_versions_already = 1;
> +
> +	config_version = get_protocol_version_config();
> +	if (config_version == protocol_v0) {
> +		strbuf_addstr(advert, "version=0");
> +		return;
> +	}
> +
> +	if (tmp_nr > 0) {
> +		ALLOC_ARRAY(tmp_allowed_versions, tmp_nr);
> +		copy_array(tmp_allowed_versions, allowed_versions, tmp_nr,
> +			   sizeof(enum protocol_version));
> +	} else {
> +		ALLOC_ARRAY(tmp_allowed_versions, 1);
> +		tmp_nr = 1;
> +		tmp_allowed_versions[0] = config_version;
> +	}
> +
> +	if (tmp_allowed_versions[0] != config_version)
> +		for (int i = 1; i < nr_allowed_versions; i++)
> +			if (tmp_allowed_versions[i] == config_version) {
> +				enum protocol_version swap =
> +					tmp_allowed_versions[0];
> +				tmp_allowed_versions[0] =
> +					tmp_allowed_versions[i];
> +				tmp_allowed_versions[i] = swap;
> +			}
> +
> +	strbuf_addf(advert, "version=%s",
> +		    format_protocol_version(tmp_allowed_versions[0]));
> +	for (int i = 1; i < tmp_nr; i++)
> +		strbuf_addf(advert, ":version=%s",
> +			    format_protocol_version(tmp_allowed_versions[i]));
> +}
> +

So the idea is that the protocols the other end can talk come in
advert in their preferred order, and we take an intersection of them
and our "allowed-versions", but the preference is further skewed
with the "swap" thing if we have our own preference specified via
config?

I am wondering if the code added by this patch outside this
function, with if (strcmp(client_ad.buf, "version=0") sprinkled all
over the place, works sensibly when the other side says "I prefer
version=0 but I do not mind talking version=1".

Isn't tmp_allowed_versions[] leaking when we return from this
function?