Use same priority for RC and RC snapshots (#1179)

Author: pkoenig10

readme.md

@@ -36,6 +36,7 @@ This specification describes _orderable_ and _non-orderable_ product version str
 Orderable version strings fall into one of 4 version types as defined by a category (release or release candidate)
 and whether it is a snapshot version or not (snapshot versions contain a commit hash at the end), the cross section
 of which produces the following:
+
  ```
 Version Type                        Example                 Format
 ------------                        -------                 ------
@@ -58,52 +59,52 @@ decisions about forward-vs-backwards product migrations. Further, it simplifies
 via version ranges; for instance, a product may declare that it is compatible with a second product with a version in
 `[1.2.3, 2.0.0)`.
 
-For any two orderable versions, v1 and v2, we can define whether v1 is a *bigger* (equivalently, *later*, *newer*, etc)
-than v2. For the four variants, there can be up to three numeric components identifying a version. From left to right,
-they are: the usual notation of the base version (e.g., for `1.2.3`, 1=major, 2=minor, 3=patch), an optional second
+For any two orderable versions, `v1` and `v2`, we can define whether `v1` is a *larger* (equivalently, *later*, *newer*, etc)
+than `v2`. For the four variants, there can be up to three numeric components identifying a version. From left to right,
+they are: the usual notation of the base version (e.g., for `1.2.3`, 1=major, 2=minor, 3=patch), an optional first
 numeric component to identify a release candidate (e.g. `-rc3`) or a snapshot version (e.g. `-5-gnm4s9ba`), and finally
-an optional third numeric component to identify a release candidate snapshot version (e.g. `-rc3-5-gnm4s9ba`).
+an optional second numeric component to identify a release candidate snapshot version (e.g. `-rc3-5-gnm4s9ba`).
+
+Intuitively, the version types define an ordering of versions for a given base version:
+- For a given base version, release snapshot versions are larger than release versions
+- For a given base version, release versions are larger than release candidate and release candidate snapshot versions
+- For a given base and rc version, release candidate snapshot versions are larger than release candidate versions
 
-Intuitively, given the same base version, snapshot versions are bigger than non-snapshot versions, normal release
-versions are bigger than release candidate versions, and a normal release snapshot version is bigger than a release
-candidate of any kind. The following top-down procedure determines whether v1 is bigger than v2, written `v1 > v2`;
+The following top-down procedure determines whether `v1` is larger than `v2`, written `v1 > v2`;
 comparisons like `major(v1) > major(v2)` are by integer ordering (not lexicographic ordering):
 
-- If `major(v1) > major(v2)`, then `v1 > v2`
-- If `minor(v1) > minor(v2)`, then `v1 > v2`
-- If `patch(v1) > patch(v2)`, then `v1 > v2`
-- From here on, let us assume that the base versions (major/minor/patch) are the same for v1 and v2
-- If v1 is a normal snapshot version and v2 is a normal release, then `v1 > v2`
-- If v1 is a normal release version and v2 is a rc version, then `v1 > v2`
-- If v1 and v2 are both normal snapshot versions and `snapshot(v1) > snapshot(v2)`, then `v1 > v2`
-- If v1 and v2 are both rc versions and `rc(v1) > rc(v2)`, then `v1 > v2`
-- From here on, let us assume that v1 and v2 are both rc versions of the same `rc()` number
-- If v1 is a snapshot rc version and v2 is a normal rc version, then `v1 > v2`
-- If v1 and v2 are both snapshot rc versions and `rcSnapshot(v1) > rcSnapshot(v2)`, then `v1 > v2`
-
-Further, v1 is as big as v2, written `v1 == v2`, iff neither `v1 > v2` nor `v2 > v1`.
-We write `v1 >= v2` if `v1 > v2` or `v1 == v2`.
+- We know nothing about `v1` and `v2`:
+  - If `major(v1) > major(v2)`, then `v1 > v2`
+  - If `minor(v1) > minor(v2)`, then `v1 > v2`
+  - If `patch(v1) > patch(v2)`, then `v1 > v2`
+- From here on, we know that `v1` and `v2` have the same base version:
+  - If both `v1` and `v2` are both release snapshot versions and `snapshot(v1) > snapshot(v2)`, then `v1 > v2`
+  - If `v1` is a release snapshot version and `v2` is not, then `v1 > v2`
+  - If both `v1` and `v2` are both release versions, then `v1 == v2`
+  - If `v1` is a release version and `v2` is not, then `v1 > v2`
+- From here on, we know that `v1` and `v2` have the same base version and are release candidate or release candidate snapshot version:
+  - If `rc(v1) > rc(v2)`, then `v1 > v2`
+  - If `v1` is a release candidate snapshot version and `v2` is not, then `v1 > v2`
+  - If `rcSnapshot(v1) > rcSnapshot(v2)`, then `v1 > v2`
 
 Examples, with each greater than all the previous:
-- RC: `1.0.0-rc1`
-- Bigger RC: `1.0.0-rc2`
-- RC Snapshot trumps RC: `1.0.0-rc2-4-gaaaaaaa`
-- Bigger RC Snapshot: `1.0.0-rc2-5-gccccccc`
-- Base trumps RC: `2.0.0`
-- Snapshot trumps all: `2.0.0-3-gaaaaaaa`
-- Bigger Snapshot: `2.0.0-4-gbbbbbbb`
-- Bigger Base: `2.1.0-rc1`
-- Release trumps RC: `2.1.0`
+- `1.0.0-rc1`
+- `1.0.0-rc1-1-gabcedf`
+- `1.0.0-rc1-2-gabcedf`
+- `1.0.0-rc2`
+- `1.0.0`
+- `1.0.0-1-gabcedf`
+- `1.0.0-2-gabcedf`
+- `1.0.1-rc1`
+- `1.0.1`
+- `1.1.0`
+- `2.0.0`
 
 Examples of equality:
-- `1.2.0 == 1.2.0`
-- `2.0.0-rc1 == 2.0.0-rc1`
-- `2.0.0-rc1-3-gaaaaaaa == 2.0.0-rc1-3-gbbbbbbb`
-- `2.0.0-5-gbbbbbbb == 2.0.0-5-gaaaaaaa1`
-
-Note that any two release and rc versions are equally big iff they are syntactically equal. As the second example
-demonstrates, this does not hold for snapshot versions.
-
+- `1.0.0-rc1 == 1.0.0-rc1`
+- `1.0.0-rc1-1-gaaaaaaa == 1.0.0-rc1-1-gbbbbbbb`
+- `1.0.0 == 1.0.0`
+- `1.0.0-1-gaaaaaaa == 1.0.0-1-gbbbbbbb`
 
 #### Version matchers
 

sls-versions/src/main/java/com/palantir/sls/versions/CompactVersion.java

@@ -42,7 +42,7 @@
  * 20 bits: major
  * </code>
  *
- * <p><b>Note</b>: the correctness of the implementation of {@link #compareTo(CompactVersion)} depends on the
+ * <p><b>Note</b>: The correctness of the implementation of {@link #compareTo(CompactVersion)} depends on the
  * constituent longs holding only positive values. This is partially accomplished by using less than the full number of
  * bits available, and thus by avoiding twos-complement representation issues when the highest bit in the long is set.
  */

sls-versions/src/main/java/com/palantir/sls/versions/SlsVersionType.java

@@ -23,9 +23,9 @@
  * {@link SlsVersion} objects.
  */
 public enum SlsVersionType {
-    RELEASE_SNAPSHOT(RegexParser.of("^([0-9]+)\\.([0-9]+)\\.([0-9]+)-([0-9]+)-g[a-f0-9]+$"), 4),
-    RELEASE(ReleaseVersionParser.INSTANCE, 3),
-    RELEASE_CANDIDATE_SNAPSHOT(RegexParser.of("^([0-9]+)\\.([0-9]+)\\.([0-9]+)-rc([0-9]+)-([0-9]+)-g[a-f0-9]+$"), 2),
+    RELEASE_SNAPSHOT(RegexParser.of("^([0-9]+)\\.([0-9]+)\\.([0-9]+)-([0-9]+)-g[a-f0-9]+$"), 3),
+    RELEASE(ReleaseVersionParser.INSTANCE, 2),
+    RELEASE_CANDIDATE_SNAPSHOT(RegexParser.of("^([0-9]+)\\.([0-9]+)\\.([0-9]+)-rc([0-9]+)-([0-9]+)-g[a-f0-9]+$"), 1),
     RELEASE_CANDIDATE(RegexParser.of("^([0-9]+)\\.([0-9]+)\\.([0-9]+)-rc([0-9]+)$"), 1),
     NON_ORDERABLE(RegexParser.of("^([0-9]+)\\.([0-9]+)\\.([0-9]+)(-[a-z0-9-]+)?(\\.dirty)?$"), 0);
 
@@ -45,6 +45,12 @@ Parser getParser() {
         this.priority = priority;
     }
 
+    /**
+     * Provides a first-level ordering of versions that have the same major/minor/patch versions. Version types with a
+     * higher priority are greater than version types with a lower priority.
+     *
+     * <p><b>Note</b>: RELEASE_CANDIDATE and RELEASE_CANDIDATE_SNAPSHOT have the same priority.
+     */
     public int getPriority() {
         return this.priority;
     }

sls-versions/src/main/java/com/palantir/sls/versions/VersionComparator.java

@@ -16,95 +16,54 @@
 
 package com.palantir.sls.versions;
 
-import static com.palantir.logsafe.Preconditions.checkArgument;
-
-import com.palantir.logsafe.SafeArg;
 import java.util.Comparator;
-import java.util.OptionalInt;
 
 /** Compares {@link OrderableSlsVersion}s by "newness", i.e., "1.4.0" is greater/newer/later than "1.2.1", etc.. */
 public enum VersionComparator implements Comparator<OrderableSlsVersion> {
     INSTANCE;
 
     @Override
-    public int compare(OrderableSlsVersion left, OrderableSlsVersion right) {
-        if (left.getValue().equals(right.getValue())) {
+    @SuppressWarnings("ImmutablesReferenceEquality")
+    public int compare(OrderableSlsVersion version1, OrderableSlsVersion version2) {
+        if (version1 == version2) {
             return 0;
         }
 
-        int mainVersionComparison = compareMainVersion(left, right);
-        if (mainVersionComparison != 0) {
-            return mainVersionComparison;
-        }
-
-        if ((left.getType() == SlsVersionType.RELEASE) || (right.getType() == SlsVersionType.RELEASE)) {
-            // Releases always compare correctly just by type now we know base version matches.
-            return Integer.compare(left.getType().getPriority(), right.getType().getPriority());
-        }
-
-        if ((left.getType() == SlsVersionType.RELEASE_SNAPSHOT)
-                && (right.getType() == SlsVersionType.RELEASE_SNAPSHOT)) {
-            // If both are snapshots, compare snapshot number.
-            return compareFirstSequenceVersions(left, right);
+        int result = Integer.compare(version1.getMajorVersionNumber(), version2.getMajorVersionNumber());
+        if (result != 0) {
+            return result;
         }
 
-        if (left.getType() == SlsVersionType.RELEASE_SNAPSHOT) {
-            // Snapshot is larger.
-            return 1;
+        result = Integer.compare(version1.getMinorVersionNumber(), version2.getMinorVersionNumber());
+        if (result != 0) {
+            return result;
         }
 
-        if (right.getType() == SlsVersionType.RELEASE_SNAPSHOT) {
-            // Snapshot is larger.
-            return -1;
+        result = Integer.compare(version1.getPatchVersionNumber(), version2.getPatchVersionNumber());
+        if (result != 0) {
+            return result;
         }
 
-        return compareSuffix(left, right);
-    }
-
-    private int compareMainVersion(OrderableSlsVersion left, OrderableSlsVersion right) {
-        if (left.getMajorVersionNumber() != right.getMajorVersionNumber()) {
-            return left.getMajorVersionNumber() > right.getMajorVersionNumber() ? 1 : -1;
+        result = Integer.compare(
+                version1.getType().getPriority(), version2.getType().getPriority());
+        if (result != 0) {
+            return result;
         }
 
-        if (left.getMinorVersionNumber() != right.getMinorVersionNumber()) {
-            return left.getMinorVersionNumber() > right.getMinorVersionNumber() ? 1 : -1;
+        result = Integer.compare(
+                version1.firstSequenceVersionNumber().orElse(0),
+                version2.firstSequenceVersionNumber().orElse(0));
+        if (result != 0) {
+            return result;
         }
 
-        if (left.getPatchVersionNumber() != right.getPatchVersionNumber()) {
-            return left.getPatchVersionNumber() > right.getPatchVersionNumber() ? 1 : -1;
+        result = Integer.compare(
+                version1.secondSequenceVersionNumber().orElse(0),
+                version2.secondSequenceVersionNumber().orElse(0));
+        if (result != 0) {
+            return result;
         }
 
         return 0;
     }
-
-    private int compareSuffix(OrderableSlsVersion left, OrderableSlsVersion right) {
-        // We know by this point that both are RCs or RC-snapshots.
-        // Compare RC number first.
-        int rcCompare = compareFirstSequenceVersions(left, right);
-        if (rcCompare != 0) {
-            return rcCompare;
-        }
-
-        // RC number is the same, compare snapshot versions.
-        // Substitute -1 if not present because snapshots are greater than non-snapshots.
-        OptionalInt leftInt = left.secondSequenceVersionNumber();
-        OptionalInt rightInt = right.secondSequenceVersionNumber();
-        return Integer.compare(leftInt.orElse(-1), rightInt.orElse(-1));
-    }
-
-    private int compareFirstSequenceVersions(OrderableSlsVersion left, OrderableSlsVersion right) {
-        OptionalInt leftInt = left.firstSequenceVersionNumber();
-        OptionalInt rightInt = right.firstSequenceVersionNumber();
-
-        checkArgument(
-                leftInt.isPresent(),
-                "Expected to find a first sequence number for version",
-                SafeArg.of("version", left.getValue()));
-        checkArgument(
-                rightInt.isPresent(),
-                "Expected to find a first sequence number for version",
-                SafeArg.of("version", right.getValue()));
-
-        return Integer.compare(leftInt.getAsInt(), rightInt.getAsInt());
-    }
 }