Closed jezek closed 4 years ago
The version numbers and comparisons are quite messy. It's good that you're trying to make them better.
if (!is_older_than(...
is bad from readability perspective. I think we need a new function that does the "newer or equal than" comparison. It could be called is_atleast. Ideally people should stick to using either >= (newer or equal) or < (older) comparisons.
One problem with this pull request is that it breaks our old 4.7.2.0.0.1 test client release. That may have been a bad idea so breaking it would be fine. I'm not sure what the best way to release test clients would be. One option would be to release the first test client as 4.7.2.1.0.0. After that the build number could be increased. We often don't know what release number the next release will have. For example, we could have done a 4.7.2a release instead of 4.7.3. It depends on the number of changes in a release usually.
The readability was improved by a is_atleast
macro.
Oh, I see now, why there was the 4, 7, 2, 0, 0, 1
. But if it is ok, to break the old test client...
I think there should be some rules stated and communicated somewhere, like:
1) if you change client-server communication, bump up at least patch* version
2) use only is_atleast
or is_older_than
for version comparing
3) try to construct version comparsion conditions in decreasing order
4) keep in mind, there could be a release with increased extra (branch, build) for previous stable version
*) my opinion is that patch is not sufficient and you should increase at least minor version.
Hmm... I don't know where is the best place for is_atleast
macro. Appended to common/defines.h
common/defines.h is usually where all the macros end up. So it's probably the best place for the is_atleast macro.
Anyway, the code looks good so I'm gonna merge it now.
When connecting with an client with versions between 4.7.2 and 4.7.3, server should handle the communication, as if it was a 4.7.2 client.
Also the 4.7.3 client should not check if server is 'newer' than 4.7.2, but if server is 'newer or equal' (or 'not older') than 4.7.3, to allow possible 4.7.2 server extensions.