commit 1deb4a8b11a116ef902e8b0ac5fa3d3ec38dc008
parent 25ccc2a6db72b88daab6447668f1437eb61649ad
Author: Kaleb Luedtke <trenlymc@gmail.com>
Date: Fri, 22 Nov 2024 22:34:08 -0600
"Pad" shorter versions with empty parts when comparing (#5001)
<!-- To check a checkbox place an "x" between the brackets. e.g: [x] -->
- [x] I have signed the [Contributor License
Agreement](https://cla.opensource.microsoft.com/microsoft/winget-pkgs).
- [x] This pull request is related to an issue.
- Resolves #4998
When versions are created, any empty parts are trimmed off. This means
that `22.0.0` is stored as `[22]` in memory. This causes an issue when
comparing to a version that had additional parts, but should be sorted
lower than the version which is trimmed, such as `22.0.0-rc`. This is
due to the comparator seeing that `[22]` has no more parts to compare
after the first iteration, while `[22, 0, 0-rc]` does, causing it to hit
the condition where whichever version has more parts is considered to be
greater.
This PR updates the logic so that the version comparison effectively
contains as many empty parts as are needed to fully complete the
comparison. This retains the logic that if a part is not equal to the
part it is being compared against, the result of that comparison will be
returned but eliminates the need for checking if the versions have the
same number of parts since, effectively, they do; it's just that the
extra parts needed to complete the comparison are all empty.
Additional tests have been added to cover this corner case.
If possible, I would ask that this also be cherry-picked into any future
1.9 releases (assuming this PR passes review and merges)
###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/microsoft/winget-cli/pull/5001)
Diffstat:
4 files changed, 29 insertions(+), 28 deletions(-)
diff --git a/src/AppInstallerCLITests/SQLiteIndex.cpp b/src/AppInstallerCLITests/SQLiteIndex.cpp
@@ -1911,8 +1911,8 @@ TEST_CASE("SQLiteIndex_Search_VersionSorting", "[sqliteindex]")
{
{ UtilityVersion("15.0.0"), Channel("") },
{ UtilityVersion("14.0.0"), Channel("") },
- { UtilityVersion("13.2.0-bugfix"), Channel("") },
{ UtilityVersion("13.2.0"), Channel("") },
+ { UtilityVersion("13.2.0-bugfix"), Channel("") },
{ UtilityVersion("13.0.0"), Channel("") },
{ UtilityVersion("16.0.0"), Channel("alpha") },
{ UtilityVersion("15.8.0"), Channel("alpha") },
@@ -1966,8 +1966,8 @@ TEST_CASE("SQLiteIndex_PathString_VersionSorting", "[sqliteindex]")
{
{ UtilityVersion("15.0.0"), Channel("") },
{ UtilityVersion("14.0.0"), Channel("") },
- { UtilityVersion("13.2.0-bugfix"), Channel("") },
{ UtilityVersion("13.2.0"), Channel("") },
+ { UtilityVersion("13.2.0-bugfix"), Channel("") },
{ UtilityVersion("13.0.0"), Channel("") },
{ UtilityVersion("16.0.0"), Channel("alpha") },
{ UtilityVersion("15.8.0"), Channel("alpha") },
diff --git a/src/AppInstallerCLITests/Versions.cpp b/src/AppInstallerCLITests/Versions.cpp
@@ -154,6 +154,20 @@ TEST_CASE("VersionCompare", "[versions]")
RequireLessThan("0.0.1-beta", "0.0.2-alpha");
RequireLessThan("13.9.8", "14.1");
+ // Ensure that versions with non-digit characters in their parts are sorted correctly
+ RequireLessThan("1-rc", "1");
+ RequireLessThan("1.2-rc", "1.2");
+ RequireLessThan("1.0-rc", "1.0");
+ RequireLessThan("1.0.0-rc", "1");
+ RequireLessThan("22.0.0-rc.1", "22.0.0");
+ RequireLessThan("22.0.0-rc.1", "22.0.0.1");
+ RequireLessThan("22.0.0-rc.1", "22.0.0.1-rc");
+
+ // Ensure that Sub-RC versions are sorted correctly
+ RequireLessThan("22.0.0-rc.1", "22.0.0-rc.1.1");
+ RequireLessThan("22.0.0-rc.1.1", "22.0.0-rc.1.2");
+ RequireLessThan("22.0.0-rc.1.2", "22.0.0-rc.2");
+
RequireEqual("1.0", "1.0.0");
// Ensure whitespace doesn't affect equality
@@ -175,15 +189,16 @@ TEST_CASE("VersionAndChannelSort", "[versions]")
{
{ Version("15.0.0"), Channel("") },
{ Version("14.0.0"), Channel("") },
- { Version("13.2.0-bugfix"), Channel("") },
+ { Version("13.2.1-bugfix"), Channel("") },
{ Version("13.2.0"), Channel("") },
+ { Version("13.2.0-rc"), Channel("") },
{ Version("13.0.0"), Channel("") },
{ Version("16.0.0"), Channel("alpha") },
{ Version("15.8.0"), Channel("alpha") },
{ Version("15.1.0"), Channel("beta") },
};
- std::vector<size_t> reorderList = { 4, 2, 1, 7, 6, 3, 5, 0 };
+ std::vector<size_t> reorderList = { 4, 2, 1, 7, 6, 3, 8, 5, 0 };
REQUIRE(sortedList.size() == reorderList.size());
std::vector<VersionAndChannel> jumbledList;
diff --git a/src/AppInstallerSharedLib/Public/AppInstallerVersions.h b/src/AppInstallerSharedLib/Public/AppInstallerVersions.h
@@ -18,12 +18,11 @@ namespace AppInstaller::Utility
//
// Versions are compared by:
// for each part in each version
- // if both sides have no more parts, return equal
- // else if one side has no more parts, it is less
- // else if integers not equal, return comparison of integers
+ // if one side has no more parts, perform the remaining comparisons against an empty part
+ // if integers not equal, return comparison of integers
// else if only one side has a non-empty string part, it is less
// else if string parts not equal, return comparison of strings
- // if all parts are same, use approximate comparator if applicable
+ // if each part has been compared, use approximate comparator if applicable
//
// Note: approximate to another approximate version is invalid.
// approximate to Unknown is invalid.
diff --git a/src/AppInstallerSharedLib/Versions.cpp b/src/AppInstallerSharedLib/Versions.cpp
@@ -137,16 +137,12 @@ namespace AppInstaller::Utility
return (thisIsUnknown && !otherIsUnknown);
}
- for (size_t i = 0; i < m_parts.size(); ++i)
+ const Part emptyPart{};
+ for (size_t i = 0; i < std::max(m_parts.size(), other.m_parts.size()); ++i)
{
- if (i >= other.m_parts.size())
- {
- // All parts equal to this point
- break;
- }
-
- const Part& partA = m_parts[i];
- const Part& partB = other.m_parts[i];
+ // Whichever version is shorter, we need to pad it with empty parts
+ const Part& partA = (i >= m_parts.size()) ? emptyPart : m_parts[i];
+ const Part& partB = (i >= other.m_parts.size()) ? emptyPart : other.m_parts[i];
if (partA < partB)
{
@@ -159,17 +155,8 @@ namespace AppInstaller::Utility
// else parts are equal, so continue to next part
}
- // All parts tested were equal
- if (m_parts.size() == other.m_parts.size())
- {
- return ApproximateCompareLessThan(other);
- }
- else
- {
- // Else this is only less if there are more parts in other.
- return m_parts.size() < other.m_parts.size();
- }
-
+ // All parts were compared and found to be equal
+ return ApproximateCompareLessThan(other);
}
bool Version::operator>(const Version& other) const
