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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Feb 11, 2019, 18:53 UTC
Message-ID: <20190211185323.75409-1-jonathantanmy@google.com>
In-Reply-To: <CANq=j3u-zdb_FvNJGPCmygNMScseav63GhVvBX3NcVS4f7TejA@mail.gmail.com>

Show 20 quoted lines

> > > +void get_client_protocol_version_advertisement(struct strbuf *advert)
> > > +{
> > > +   int i, tmp_nr = nr_allowed_versions;
> > > +   enum protocol_version *tmp_allowed_versions, config_version;
> > > +   strbuf_reset(advert);
> > > +
> > > +   version_registration_locked = 1;
> > > +
> > > +   config_version = get_protocol_version_config();
> > > +   if (config_version == protocol_v0) {
> > > +           strbuf_addstr(advert, "version=0");
> > > +           return;
> > > +   }
> >
> > Why is protocol v0 special-cased like this?
> 
> To semi-preserve the existing behavior that no version negotiation would
> happen when the config specifies version 0. Now we'll send out a version
> advertisement where we wouldn't have before, but we only advertise v0 so
> that no negotiation can happen.

Ah, I see.

This might be an argument for deciding that the protocol versions are strictly orderable. I don't recall any discussions around this, but it makes sense to me that someone specifying version 1 would be OK with version 0 but not version 2, and someone specifying version 2 would be OK with any of 0, 1, and 2. So I'm OK with making it effectively strictly orderable by introducing the concept of a maximum.

And if we have this concept of a maximum, then we can have the no-negotiation-if-v0-configured behavior without any special cases.

Show 25 quoted lines

> > > +   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;
> >
> > In this "else" block, wouldn't this mean that if we forget to declare
> > any versions, we might send an unsupported version to the server? I
> > didn't check this, but it might happen if we invoked
> > transport_fetch_refs() without going through any of the builtins (which
> > might happen in the case of a lazy fetch in a partial clone).
> 
> Hmm yeah. I changed the else{} here to die if we haven't registered any
> versions prior to creating the advertisement string. t0410 case 9 and
> t5702 case 20 both fail, and both are lazy fetches in partial clones.
> 
> In this case, defaulting to the version in the config works, because
> fetching is valid for all protocol versions. But if we ever add some
> version X where fetching hasn't been implemented this would break.
> 
> Should we change the else{} case here to be a BUG() and then fix
> fetch-object to register its supported versions?

Yes, I think so.

Show 27 quoted lines

> > > diff --git a/protocol.h b/protocol.h
> > > index 2ad35e433c..88330fd0ee 100644
> > > --- a/protocol.h
> > > +++ b/protocol.h
> > > @@ -14,7 +14,24 @@ enum protocol_version {
> > >   * 'protocol.version' config.  If unconfigured, a value of 'protocol_v0' is
> > >   * returned.
> > >   */
> > > -extern enum protocol_version get_protocol_version_config(void);
> > > +enum protocol_version get_protocol_version_config(void);
> > > +
> > > +/*
> > > + * Register an allowable protocol version for a given operation. Registration
> > > + * must occur before attempting to advertise a version to a server process.
> > > + */
> > > +void register_allowed_protocol_version(enum protocol_version version);
> >
> > I think the allowed protocol versions have to be transport-scoped. A
> > single builtin command may invoke multiple remote commands (as will be
> > the case if a lazy fetch in a partial clone is ever required).
> 
> Wouldn't it need to be per-transport per-command then? For example, if
> we ever added a hypothetical git-fetch-then-rebase-then-push builtin, we
> couldn't use the same version advertisement for the fetch and the push,
> even if they're still using the same transport. Or would we have to use
> separate transport objects for the fetch and the push in such a
> situation?

If we make each implementation of the vtable function read the config and compute its own list of allowed protocols (is this what you mean by "make each vtable entry declare its versions" below?), then we still leave open the possibility of using the same transport object for fetch and push.

> If we do move the version list into the transport, what would be the
> right way to register the allowed versions? Maybe we would make each
> vtable entry declare its versions?

This makes sense to me. I think that the implementations of transport_vtable.connect might need to take in a list of versions, but fetch and push can probably read the config by themselves so the API does not need to change.