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

[Josh Steadmon <steadmon@google.com>]

On 2018.10.12 15:30, Stefan Beller wrote:
> 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?

Hmm, well it wouldn't be quite as clean as the argv_* versions, since we
can't take the pointer of an enum value, so we don't have a good
stop-value for the va_list. I suppose we could use
protocol_unknown_version, but that seems unintuitive to me. We could
also pass in an explicit argument count, but... ugh.

Am I missing some other way to do this cleanly? I'll admit I'm not very
familiar with va_lists.

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

Yeah, my understanding is that we don't want to assume that version
identifiers will always be simple integers. We could get away with
sprintf() for now, but I figured I didn't want to cause future pain
if/when the version identifiers become complex.

> > +
> >  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?

Ack, will fix in the next version.