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

[Josh Steadmon <steadmon@google.com>]

On 2018.11.14 11:38, Junio C Hamano wrote:
> 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.