From c010ca31dfd32cc6255cfe8242302706a00a42a0 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Tue, 26 Jan 2016 20:36:15 +0200 Subject: Fix bugs in revision comparison --- bpkg/package | 131 +++++++++++++++++++++++++++++++++++++++------------- bpkg/pkg-build.cxx | 34 ++++++++------ bpkg/pkg-status.cxx | 2 + 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 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 + 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 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 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 + 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 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 + 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 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 + 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 + inline auto + operator>= (const T1& x, const T2& y) -> decltype (x.epoch >= y.epoch) + { + return compare_version_ge (x, y, true); } template 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. -- cgit v1.1