Re: [PATCH v3 1/1] protocol: advertise multiple supported versions
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 13, 2018, 04:01 UTC
- Message-ID
- <xmqqr2fpwrqg.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <b9968e3fb005a00e9f78627467b4fc86a6259977.1542059029.git.steadmon@google.com>
steadmon@google.com writes:
Show 6 quoted lines
> 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.
Show 11 quoted lines
> 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.
Show 41 quoted lines
> +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?