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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 16, 2018, 02:45 UTC
Message-ID: <xmqq4lchn3jy.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181114195757.GJ126896@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 9 quoted lines

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

OK, let me try to see if I understand correctly by rephrasing.

If the client does not say anything about which version it prefers (because it only knows about version 0 without even realizing that there is a version negotiation exchange), we substitute the lack of proposed versions with string "version=0", and the strcmp()s I saw and was puzzled by are all about special casing such a client. But we would end up behaving the same way (at least when judged only by externally visible effects) to a client that is aware of version negotiation and tells us it prefers version 0 (and nothing else) with the selection logic anyway.

Did I get it right? If so, yeah, I think it makes sense to avoid two codepaths that ends up doing the same thing by removing the special case.

Show 5 quoted lines

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