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

From: Josh Steadmon <steadmon@google.com>
Date: Nov 14, 2018, 19:57 UTC
Message-ID: <20181114195757.GJ126896@google.com>
In-Reply-To: <xmqq1s7os7rz.fsf@gitster-ct.c.googlers.com>

On 2018.11.14 11:38, Junio C Hamano wrote:

Show 19 quoted lines

> Josh Steadmon <steadmon@google.com> writes:
> 
> > On 2018.11.13 13:01, Junio C Hamano wrote:
> >> 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.

Ah I see. The strcmp()s against "version=0" are special cases for where it looks like the client does not understand any sort of version negotiation. If we see multiple versions listed in the advert, then the rest of the selection logic should do the right thing.

However, I think that it might work to remove the special cases. In the event that the client is so old that it doesn't understand any form of version negotiation, it should just ignore the version fields / environment vars. If you think it's cleaner to remove the special cases, let me know.