Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Pad" shorter versions with empty parts when comparing #5001

Merged
merged 7 commits into from
Nov 23, 2024
Merged

"Pad" shorter versions with empty parts when comparing #5001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
12b8b08
Pad shorter versions with empty parts when comparing
Trenly Nov 22, 2024
a19d706
Add requested test
Trenly Nov 22, 2024
dac5fd5
Move the test to a more logical spot
Trenly Nov 22, 2024
bcd8201
The index tests were wrong too
Trenly Nov 22, 2024
7a24063
More tests = Better
Trenly Nov 23, 2024
0945daa
Update comment in header
Trenly Nov 23, 2024
93c9dcf
Typo
Trenly Nov 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/AppInstallerCLITests/SQLiteIndex.cpp
View file
Open in desktop
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
View file
Open in desktop
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");
Trenly marked this conversation as resolved.
Show resolved Hide resolved
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");
Trenly marked this conversation as resolved.
Show resolved Hide resolved

// 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
View file
Open in desktop
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
View file
Open in desktop
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
Loading