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

From: Josh Steadmon <steadmon@google.com>
Date: Nov 13, 2018, 22:53 UTC
Message-ID: <20181113225320.GG126896@google.com>
In-Reply-To: <xmqqr2fpwrqg.fsf@gitster-ct.c.googlers.com>

On 2018.11.13 13:01, Junio C Hamano wrote:

Show 11 quoted lines

> steadmon@google.com writes:
> 
> > 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.

The downgrade on push happens only when the the configured version is v2. If v1 is requested, no downgrade is triggered. I'll clarify the commit message in the next version.

Show 61 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.
> 
> > +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?

We currently don't intersect with our own allowed list, we just accept the first version that we recognize. This introduces its own version negotiation bug; if we add v2 push in the future, a new client talking to an old server would try to use v2 even though the server may not have the corresponding v2 push implementation. I'll fix this in the next version.

In any case, the ordering of the server's allowed versions won't matter; we'll pick the the first version in the client's list which is also allowed on the server.

> 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".

Depends on what you mean by "sensibly" :). In the current case, if the client prefers v0, it will always end up using v0. After the fixes described above, it will always use v0 unless the server no longer supports v0. Is that what you would expect?

> Isn't tmp_allowed_versions[] leaking when we return from this
> function?

Yes, sorry about that. Will fix.