Fix bugs in revision comparison

3 files changed, 121 insertions, 46 deletions

diff --git a/bpkg/package b/bpkg/package
index 8bd9546..3e8c989 100644
--- a/bpkg/package
+++ b/bpkg/package
@@ -508,17 +508,15 @@ namespace bpkg
   {
     // Since we don't quite know what T1 and T2 are (and where the resulting
     // expression will run), let's not push our luck with something like
-    // (revision || x.revision == y.revision).
+    // (!revision || x.revision == y.revision).
     //
-    if (revision)
-      return x.epoch == y.epoch &&
-        x.canonical_upstream == y.canonical_upstream &&
-        x.canonical_release == y.canonical_release &&
-        x.revision == y.revision;
-    else
-      return x.epoch == y.epoch &&
-        x.canonical_upstream == y.canonical_upstream &&
-        x.canonical_release == y.canonical_release;
+    auto r (x.epoch == y.epoch &&
+            x.canonical_upstream == y.canonical_upstream &&
+            x.canonical_release == y.canonical_release);
+
+    return revision
+      ? r && x.revision == y.revision
+      : r;
   }
 
   /*
@@ -528,66 +526,137 @@ namespace bpkg
   inline auto
   operator== (const T1& x, const T2& y) -> decltype (x.epoch == y.epoch)
   {
-    return compare_version_eq (x, y, true);
+  return compare_version_eq (x, y, true);
   }
   */
 
   template <typename T1, typename T2>
   inline auto
+  compare_version_ne (const T1& x, const T2& y, bool revision)
+    -> decltype (x.epoch == y.epoch)
+  {
+    auto r (x.epoch != y.epoch ||
+            x.canonical_upstream != y.canonical_upstream ||
+            x.canonical_release != y.canonical_release);
+
+    return revision
+      ? r || x.revision != y.revision
+      : r;
+  }
+
+  template <typename T1, typename T2>
+  inline auto
   operator!= (const T1& x, const T2& y) -> decltype (x.epoch != y.epoch)
   {
-    return x.epoch != y.epoch ||
-      x.canonical_upstream != y.canonical_upstream ||
-      x.canonical_release != y.canonical_release ||
-      x.revision != y.revision;
+    return compare_version_ne (x, y, true);
   }
 
   template <typename T1, typename T2>
   inline auto
-  operator< (const T1& x, const T2& y) -> decltype (x.epoch < y.epoch)
+  compare_version_lt (const T1& x, const T2& y, bool revision)
+    -> decltype (x.epoch == y.epoch)
   {
-    return x.epoch < y.epoch ||
+    auto r (
+      x.epoch < y.epoch ||
       (x.epoch == y.epoch && x.canonical_upstream < y.canonical_upstream) ||
       (x.epoch == y.epoch && x.canonical_upstream == y.canonical_upstream &&
-       x.canonical_release < y.canonical_release) ||
+       x.canonical_release < y.canonical_release));
+
+    return revision
+      ? r ||
       (x.epoch == y.epoch && x.canonical_upstream == y.canonical_upstream &&
-       x.canonical_release == y.canonical_release && x.revision < y.revision);
+       x.canonical_release == y.canonical_release && x.revision < y.revision)
+      : r;
   }
 
   template <typename T1, typename T2>
   inline auto
-  operator<= (const T1& x, const T2& y) -> decltype (x.epoch <= y.epoch)
+  operator< (const T1& x, const T2& y) -> decltype (x.epoch < y.epoch)
   {
-    return x.epoch < y.epoch ||
-      (x.epoch == y.epoch && x.canonical_upstream < y.canonical_upstream) ||
+    return compare_version_lt (x, y, true);
+  }
+
+  template <typename T1, typename T2>
+  inline auto
+  compare_version_le (const T1& x, const T2& y, bool revision)
+    -> decltype (x.epoch == y.epoch)
+  {
+    auto r (
+      x.epoch < y.epoch ||
+      (x.epoch == y.epoch && x.canonical_upstream < y.canonical_upstream));
+
+    return revision
+      ? r ||
       (x.epoch == y.epoch && x.canonical_upstream == y.canonical_upstream &&
        x.canonical_release < y.canonical_release) ||
       (x.epoch == y.epoch && x.canonical_upstream == y.canonical_upstream &&
-       x.canonical_release == y.canonical_release && x.revision <= y.revision);
+       x.canonical_release == y.canonical_release && x.revision <= y.revision)
+      : r ||
+      (x.epoch == y.epoch && x.canonical_upstream == y.canonical_upstream &&
+       x.canonical_release <= y.canonical_release);
   }
 
+  /*
+  Currently unused (and probably should stay that way).
+
   template <typename T1, typename T2>
   inline auto
-  operator> (const T1& x, const T2& y) -> decltype (x.epoch > y.epoch)
+  operator<= (const T1& x, const T2& y) -> decltype (x.epoch <= y.epoch)
+  {
+    return compare_version_le (x, y, true);
+  }
+  */
+
+  template <typename T1, typename T2>
+  inline auto
+  compare_version_gt (const T1& x, const T2& y, bool revision)
+    -> decltype (x.epoch == y.epoch)
   {
-    return x.epoch > y.epoch ||
+    auto r (
+      x.epoch > y.epoch ||
       (x.epoch == y.epoch && x.canonical_upstream > y.canonical_upstream) ||
       (x.epoch == y.epoch && x.canonical_upstream == y.canonical_upstream &&
-       x.canonical_release > y.canonical_release) ||
+       x.canonical_release > y.canonical_release));
+
+    return revision
+      ? r ||
       (x.epoch == y.epoch && x.canonical_upstream == y.canonical_upstream &&
-       x.canonical_release == y.canonical_release && x.revision > y.revision);
+       x.canonical_release == y.canonical_release && x.revision > y.revision)
+      : r;
   }
 
   template <typename T1, typename T2>
   inline auto
-  operator>= (const T1& x, const T2& y) -> decltype (x.epoch >= y.epoch)
+  operator> (const T1& x, const T2& y) -> decltype (x.epoch > y.epoch)
   {
-    return x.epoch > y.epoch ||
-      (x.epoch == y.epoch && x.canonical_upstream > y.canonical_upstream) ||
+    return compare_version_gt (x, y, true);
+  }
+
+  template <typename T1, typename T2>
+  inline auto
+  compare_version_ge (const T1& x, const T2& y, bool revision)
+    -> decltype (x.epoch == y.epoch)
+  {
+    auto r (
+      x.epoch > y.epoch ||
+      (x.epoch == y.epoch && x.canonical_upstream > y.canonical_upstream));
+
+    return revision
+      ? r ||
       (x.epoch == y.epoch && x.canonical_upstream == y.canonical_upstream &&
        x.canonical_release > y.canonical_release) ||
       (x.epoch == y.epoch && x.canonical_upstream == y.canonical_upstream &&
-       x.canonical_release == y.canonical_release && x.revision >= y.revision);
+       x.canonical_release == y.canonical_release && x.revision >= y.revision)
+      : r ||
+      (x.epoch == y.epoch && x.canonical_upstream == y.canonical_upstream &&
+       x.canonical_release >= y.canonical_release);
+  }
+
+  template <typename T1, typename T2>
+  inline auto
+  operator>= (const T1& x, const T2& y) -> decltype (x.epoch >= y.epoch)
+  {
+    return compare_version_ge (x, y, true);
   }
 
   template <typename T>
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index 0d663bd..9c3914c 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -66,20 +66,20 @@ namespace bpkg
     //
     if (c)
     {
-      // If the constraint is equality and the revision is not explicitly
-      // specified, then compare ignoring the revision. The idea is that when
-      // the user runs 'bpkg build libfoo/1.0.0' and there is 1.0.0+1
-      // available, it should just work. The user shouldn't have to spell the
-      // revision explicitly. Similarly, when we have 'depends: libfoo ==
-      // 1.0.0', then it would be strange if 1.0.0+1 did not satisfy this
-      // constraint.
+      // If the revision is not explicitly specified, then compare ignoring the
+      // revision. The idea is that when the user runs 'bpkg build libfoo/1'
+      // and there is 1+1 available, it should just work. The user shouldn't
+      // have to spell the revision explicitly. Similarly, when we have
+      // 'depends: libfoo == 1', then it would be strange if 1+1 did not
+      // satisfy this constraint. The same for libfoo <= 1 -- 1+1 should
+      // satisfy.
       //
       // Note that strictly speaking 0 doesn't mean unspecified. Which means
       // with this implementation there is no way to say "I really mean
-      // revision 0" since 1.0.0 == 1.0.0+0. One can, in the current model, say
-      // libfoo == 1.0.0+1, though. This is probably ok since one would assume
-      // any subsequent revision of a package is only better than the ones
-      // before.
+      // revision 0" since 1 == 1+0. One can, in the current model, say libfoo
+      // == 1+1, though. This is probably ok since one would assume any
+      // subsequent revision of a package version are just as (un)satisfactory
+      // as the first one.
       //
       if (c->min_version &&
           c->max_version &&
@@ -96,18 +96,22 @@ namespace bpkg
       {
         if (c->min_version)
         {
+          const version& v (*c->min_version);
+
           if (c->min_open)
-            q = q && vm > *c->min_version;
+            q = q && compare_version_gt (vm, v, v.revision != 0);
           else
-            q = q && vm >= *c->min_version;
+            q = q && compare_version_ge (vm, v, v.revision != 0);
         }
 
         if (c->max_version)
         {
+          const version& v (*c->max_version);
+
           if (c->max_open)
-            q = q && vm < *c->max_version;
+            q = q && compare_version_lt (vm, v, v.revision != 0);
           else
-            q = q && vm <= *c->max_version;
+            q = q && compare_version_le (vm, v, v.revision != 0);
         }
 
         q += order_by_version_desc (vm);
diff --git a/bpkg/pkg-status.cxx b/bpkg/pkg-status.cxx
index 896ffe9..1f890c4 100644
--- a/bpkg/pkg-status.cxx
+++ b/bpkg/pkg-status.cxx
@@ -87,6 +87,8 @@ namespace bpkg
           q = q && compare_version_eq (query::id.version, v, v.revision != 0);
           q += order_by_revision_desc (query::id.version);
         }
+        else
+          q += order_by_version_desc (query::id.version);
 
         // Only consider packages that are in repositories that were explicitly
         // added to the configuration and their complements, recursively.