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

[Junio C Hamano <gitster@pobox.com>]

Josh Steadmon <steadmon@google.com> writes:

> On 2018.11.13 13:01, Junio C Hamano wrote:
>> 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.

OK, then it will still be confusing unless "we downgrade v2 to v0
because ..."gives the reason.

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

That sounds like a very sensible semantics.

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

Yes, that sounds like a sensible behaviour.

What I was alludding to was a lot simpler, though.  An advert string
"version=0:version=1" from a client that prefers version 0 won't be
!strcmp("version=0", advert) but seeing many strcmp() in the patch
made me wonder.