stead...@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?
