Skip to content

Commit

Permalink
"Pad" shorter versions with empty parts when comparing (#5001)
Browse files Browse the repository at this point in the history
<!-- 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)
  • Loading branch information
Trenly authored Nov 23, 2024
1 parent 25ccc2a commit 1deb4a8
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 28 deletions.
4 changes: 2 additions & 2 deletions src/AppInstallerCLITests/SQLiteIndex.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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") },
Expand Down Expand Up @@ -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") },
Expand Down
19 changes: 17 additions & 2 deletions src/AppInstallerCLITests/Versions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down
7 changes: 3 additions & 4 deletions src/AppInstallerSharedLib/Public/AppInstallerVersions.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
27 changes: 7 additions & 20 deletions src/AppInstallerSharedLib/Versions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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
Expand Down

0 comments on commit 1deb4a8

Please sign in to comment.