On Thu, Oct 11, 2018 at 6:02 PM <steadmon@xxxxxxxxxx> wrote:
>
> From: Josh Steadmon <steadmon@xxxxxxxxxx>
>
> 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.
>
> This patch creates a protocol version registry. Individual commands
> 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.
I like this patch from the users point of view (i.e. inside fetch/push etc),
and I need to through the details in connect/protocol as that seems
to be a lot of new code, but hides the complexity very well.
> + register_allowed_protocol_version(protocol_v2);
> + register_allowed_protocol_version(protocol_v1);
> + register_allowed_protocol_version(protocol_v0);
Would it make sense to have a
register_allowed_protocol_versions(protocol_v2, protocol_v1,
protocol_v0, NULL);
similar to argv_array_pushl or would that be overengineered?
> @@ -1085,10 +1085,9 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
> target_host, 0);
>
> /* If using a new version put that stuff here after a second null byte */
> - if (version > 0) {
> + if (strcmp(version_advert->buf, "version=0")) {
> strbuf_addch(&request, '\0');
> - strbuf_addf(&request, "version=%d%c",
> - version, '\0');
> + strbuf_addf(&request, "%s%c", version_advert->buf, '\0');
It's a bit unfortunate to pass around the string, but reading through
the following
lines seems like it is easiest.
> @@ -1226,16 +1226,10 @@ struct child_process *git_connect(int fd[2], const char *url,
> {
> char *hostandport, *path;
> struct child_process *conn;
> + struct strbuf version_advert = STRBUF_INIT;
> enum protocol protocol;
> - enum protocol_version version = get_protocol_version_config();
>
> - /*
> - * NEEDSWORK: If we are trying to use protocol v2 and we are planning
> - * to perform a push, then fallback to v0 since the client doesn't know
> - * how to push yet using v2.
> - */
> - if (version == protocol_v2 && !strcmp("git-receive-pack", prog))
> - version = protocol_v0;
> + get_client_protocol_version_advertisement(&version_advert);
Nice to have this special casing gone!
> diff --git a/protocol.c b/protocol.c
> index 5e636785d1..f788269c47 100644
> +
> +static const char protocol_v0_string[] = "0";
> +static const char protocol_v1_string[] = "1";
> +static const char protocol_v2_string[] = "2";
> +
...
> +/* Return the text representation of a wire protocol version. */
> +static const char *format_protocol_version(enum protocol_version version)
> +{
> + switch (version) {
> + case protocol_v0:
> + return protocol_v0_string;
> + case protocol_v1:
> + return protocol_v1_string;
> + case protocol_v2:
> + return protocol_v2_string;
> + case protocol_unknown_version:
> + die(_("Unrecognized protocol version"));
> + }
> + die(_("Unrecognized protocol_version"));
> +}
Up to now we have the textual representation that can easily
be constructed from protocol_version by using e.g.
sprintf(buf, "%d", version);
which is why I initially thought this could be worded way
shorter, but I guess this is more future proof?
> +
> enum protocol_version get_protocol_version_config(void)
> {
> const char *value;
> @@ -30,6 +55,79 @@ enum protocol_version get_protocol_version_config(void)
> return protocol_v0;
> }
>
> +void register_allowed_protocol_version(enum protocol_version version)
> +{
> + if (have_advertised_versions_already)
> + error(_("attempting to register an allowed protocol version after advertisement"));
This would be a
BUG(...)
instead?
