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

[Stefan Beller <sbeller@google.com>]

On Thu, Oct 11, 2018 at 6:02 PM <steadmon@google.com> wrote:
>
> From: Josh Steadmon <steadmon@google.com>
>
> 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?