git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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?

Previous: Junio C HamanoNext: Josh Steadmon
Message 25 of 52 in “Limit client version advertisements”
  1. 0/1 Limit client version advertisementsJosh Steadmon, Oct 2, 2018
  2. 1/1 protocol: limit max protocol version per serviceJosh Steadmon, Oct 2, 2018
  3. Stefan BellerOct 2, 2018
  4. Josh SteadmonOct 3, 2018
  5. Stefan BellerOct 3, 2018
  6. Josh SteadmonOct 5, 2018
  7. Stefan BellerOct 5, 2018
  8. Josh SteadmonOct 10, 2018
  9. Jonathan NiederOct 12, 2018
  10. Jonathan NiederOct 12, 2018
  11. Josh SteadmonOct 12, 2018
  12. Jonathan NiederOct 12, 2018
  13. Junio C HamanoOct 13, 2018
  14. 0/1 Advertise multiple supported proto versionssteadmon@google.com, Oct 12, 2018
  15. 1/1 protocol: advertise multiple supported versionssteadmon@google.com, Oct 12, 2018
  16. Stefan BellerOct 12, 2018
  17. Josh SteadmonOct 22, 2018
  18. Stefan BellerOct 23, 2018
  19. 0/1 Advertise multiple supported proto versionssteadmon@google.com, Nov 12, 2018
  20. 1/1 protocol: advertise multiple supported versionssteadmon@google.com, Nov 12, 2018
  21. Stefan BellerNov 12, 2018
  22. Re* [PATCH v3 1/1] protocol: advertise multiple supported versionsJunio C Hamano, Nov 13, 2018
  23. Stefan BellerNov 13, 2018
  24. Junio C HamanoNov 14, 2018
  25. Junio C HamanoNov 13, 2018
  26. Josh SteadmonNov 13, 2018
  27. Junio C HamanoNov 14, 2018
  28. Josh SteadmonNov 14, 2018
  29. Junio C HamanoNov 16, 2018
  30. Josh SteadmonNov 16, 2018
  31. Junio C HamanoNov 13, 2018
  32. SZEDER GáborNov 13, 2018
  33. Josh SteadmonNov 13, 2018
  34. SZEDER GáborNov 14, 2018
  35. Junio C HamanoNov 14, 2018
  36. SZEDER GáborNov 14, 2018
  37. 0/1 Advertise multiple supported proto versionsJosh Steadmon, Nov 14, 2018
  38. 1/1 protocol: advertise multiple supported versionsJosh Steadmon, Nov 14, 2018
  39. Junio C HamanoNov 14, 2018
  40. Josh SteadmonNov 14, 2018
  41. Junio C HamanoNov 16, 2018
  42. 0/1 Advertise multiple supported proto versionsJosh Steadmon, Nov 16, 2018
  43. 1/1 protocol: advertise multiple supported versionsJosh Steadmon, Nov 16, 2018
  44. Ævar Arnfjörð BjarmasonDec 14, 2018
  45. Josh SteadmonDec 14, 2018
  46. Ævar Arnfjörð BjarmasonDec 14, 2018
  47. Josh SteadmonDec 18, 2018
  48. 0/1 Advertise multiple supported proto versionsJosh Steadmon, Dec 20, 2018
  49. 1/1 protocol: advertise multiple supported versionsJosh Steadmon, Dec 20, 2018
  50. Jonathan TanFeb 5, 2019
  51. Josh SteadmonFeb 7, 2019
  52. Jonathan TanFeb 11, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.