From a301369868472cdfeb9e39c911d1bb636f6b9866 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Thu, 23 May 2019 22:30:09 +0100 Subject: [PATCH] Fix filename comparison on Windows boost::locale::to_lower uses locale-dependent lowercasing, which doesn't match filesystem case insensitivity when running in the Turkish locale, as then lowercase "I" is not "i", and uppercase "i" is not "I". To avoid this issue on Windows, use CompareStringOrdinal to use the same rules as the filesystem when comparing strings, and CharUpperBuffW to uppercase strings before comparing hashes. It's less clear that CharUpperBuffW uses the same rules as the filesystem, but that's what I understand from reading around. I haven't figured out how to get similar behaviour on Linux, though it probably involves using ICU. For now, boost::locale::to_upper is used before comparison. This is inaccurate, but at least avoids the problem of some characters not round-tripping when lowercased. --- include/loot/metadata/plugin_metadata.h | 9 ++- src/api/game/game_cache.cpp | 11 +-- src/api/helpers/text.cpp | 57 ++++++++++++++ src/api/helpers/text.h | 8 ++ src/api/metadata/file.cpp | 5 +- src/api/metadata/plugin_metadata.cpp | 7 +- src/api/metadata_list.cpp | 3 +- src/api/plugin.cpp | 2 +- src/api/sorting/plugin_sorter.cpp | 25 +++--- src/api/sorting/plugin_sorting_data.cpp | 4 - src/api/sorting/plugin_sorting_data.h | 1 - src/tests/api/internals/helpers/text_test.h | 87 +++++++++++++++++++++ 12 files changed, 188 insertions(+), 31 deletions(-) diff --git a/include/loot/metadata/plugin_metadata.h b/include/loot/metadata/plugin_metadata.h index 4f6fc825..c6de32d2 100644 --- a/include/loot/metadata/plugin_metadata.h +++ b/include/loot/metadata/plugin_metadata.h @@ -95,6 +95,13 @@ public: */ LOOT_API std::string GetLowercasedName() const; + /** + * Get the plugin name, normalized to be suitable for case-insensitive + * filename comparison. + * @return The normalized plugin name. + */ + LOOT_API std::string GetNormalizedName() const; + /** * Check if the plugin metadata is enabled for use during sorting. * @return True if the metadata will be used during sorting, false otherwise. @@ -312,7 +319,7 @@ struct hash { * @return The hash generated from the plugin's lowercased filename. */ size_t operator()(const loot::PluginMetadata& plugin) const { - return hash()(plugin.GetLowercasedName()); + return hash()(plugin.GetNormalizedName()); } }; } diff --git a/src/api/game/game_cache.cpp b/src/api/game/game_cache.cpp index 4d7cf310..3e2c402c 100644 --- a/src/api/game/game_cache.cpp +++ b/src/api/game/game_cache.cpp @@ -28,7 +28,8 @@ #include -using boost::locale::to_lower; +#include "api/helpers/text.h" + using std::lock_guard; using std::mutex; using std::pair; @@ -63,7 +64,7 @@ std::set> GameCache::GetPlugins() const { std::shared_ptr GameCache::GetPlugin( const std::string& pluginName) const { - auto it = plugins_.find(to_lower(pluginName)); + auto it = plugins_.find(NormalizeFilename(pluginName)); if (it != end(plugins_)) return it->second; @@ -73,13 +74,13 @@ std::shared_ptr GameCache::GetPlugin( void GameCache::AddPlugin(const Plugin&& plugin) { lock_guard lock(mutex_); - auto lowercasedName = to_lower(plugin.GetName()); + auto normalizedName = NormalizeFilename(plugin.GetName()); - auto it = plugins_.find(lowercasedName); + auto it = plugins_.find(normalizedName); if (it != end(plugins_)) plugins_.erase(it); - plugins_.emplace(lowercasedName, + plugins_.emplace(normalizedName, std::make_shared(std::move(plugin))); } diff --git a/src/api/helpers/text.cpp b/src/api/helpers/text.cpp index 3c59121a..00928fa8 100644 --- a/src/api/helpers/text.cpp +++ b/src/api/helpers/text.cpp @@ -27,6 +27,12 @@ #include +#ifdef _WIN32 +#include "windows.h" +#else +#include +#endif + using std::regex; namespace loot { @@ -107,4 +113,55 @@ std::optional ExtractVersion(const std::string& text) { return std::nullopt; } + + +#ifdef _WIN32 +std::wstring ToWinWide(const std::string& str) { + size_t len = MultiByteToWideChar(CP_UTF8, 0, str.c_str(), str.length(), 0, 0); + std::wstring wstr(len, 0); + MultiByteToWideChar(CP_UTF8, 0, str.c_str(), str.length(), &wstr[0], len); + return wstr; +} + +std::string FromWinWide(const std::wstring& wstr) { + size_t len = WideCharToMultiByte( + CP_UTF8, 0, wstr.c_str(), wstr.length(), NULL, 0, NULL, NULL); + std::string str(len, 0); + WideCharToMultiByte( + CP_UTF8, 0, wstr.c_str(), wstr.length(), &str[0], len, NULL, NULL); + return str; +} +#endif + +int CompareFilenames(const std::string& lhs, const std::string& rhs) { +#ifdef _WIN32 + // On Windows, use CompareStringOrdinal as that will perform case conversion + // using the operating system uppercase table information, which (I think) + // will give results that match the filesystem, and is not locale-dependent. + int result = CompareStringOrdinal(ToWinWide(lhs).c_str(), -1, ToWinWide(rhs).c_str(), -1, true); + switch (result) { + case CSTR_LESS_THAN: + return -1; + case CSTR_EQUAL: + return 0; + case CSTR_GREATER_THAN: + return 1; + default: + throw std::invalid_argument("One of the filenames to compare was invalid."); + } +#else + using boost::locale::to_upper; + return to_upper(lhs).compare(to_upper(rhs)); +#endif +} + +std::string NormalizeFilename(const std::string& filename) { +#ifdef _WIN32 + auto wideString = ToWinWide(filename); + CharUpperBuffW(&wideString[0], wideString.length()); + return FromWinWide(wideString); +#else + return boost::locale::to_upper(filename); +#endif +} } diff --git a/src/api/helpers/text.h b/src/api/helpers/text.h index 1e3e65d4..a8c90416 100644 --- a/src/api/helpers/text.h +++ b/src/api/helpers/text.h @@ -35,6 +35,14 @@ namespace loot { std::set ExtractBashTags(const std::string& description); std::optional ExtractVersion(const std::string& text); + +// Compare strings as if they're filenames, respecting filesystem case +// insensitivity on Windows. Returns -1 if lhs < rhs, 0 if lhs == rhs, and 1 if +// lhs > rhs. +int CompareFilenames(const std::string& lhs, const std::string& rhs); + +// Uppercase the given filename using an invariant locale on Windows. +std::string NormalizeFilename(const std::string& filename); } #endif diff --git a/src/api/metadata/file.cpp b/src/api/metadata/file.cpp index a50e7f2e..667ec639 100644 --- a/src/api/metadata/file.cpp +++ b/src/api/metadata/file.cpp @@ -27,6 +27,7 @@ #include #include "api/metadata/yaml/file.h" +#include "api/helpers/text.h" namespace loot { File::File() {} @@ -39,11 +40,11 @@ File::File(const std::string& name, ConditionalMetadata(condition) {} bool File::operator<(const File& rhs) const { - return boost::locale::to_lower(name_) < boost::locale::to_lower(rhs.name_); + return CompareFilenames(name_, rhs.name_) < 0; } bool File::operator==(const File& rhs) const { - return boost::locale::to_lower(name_) == boost::locale::to_lower(rhs.name_); + return CompareFilenames(name_, rhs.name_) == 0; } std::string File::GetName() const { return name_; } diff --git a/src/api/metadata/plugin_metadata.cpp b/src/api/metadata/plugin_metadata.cpp index f820f1cf..3444a9e3 100644 --- a/src/api/metadata/plugin_metadata.cpp +++ b/src/api/metadata/plugin_metadata.cpp @@ -32,6 +32,7 @@ #include "api/game/game.h" #include "api/helpers/logging.h" +#include "api/helpers/text.h" using std::inserter; using std::regex; @@ -175,6 +176,10 @@ std::string PluginMetadata::GetLowercasedName() const { return boost::locale::to_lower(name_); } +std::string PluginMetadata::GetNormalizedName() const { + return NormalizeFilename(name_); +} + bool PluginMetadata::IsEnabled() const { return enabled_; } std::optional PluginMetadata::GetGroup() const { return group_; } @@ -271,7 +276,7 @@ bool PluginMetadata::IsRegexPlugin() const { bool PluginMetadata::operator==(const PluginMetadata& rhs) const { if (IsRegexPlugin() == rhs.IsRegexPlugin()) { - return GetLowercasedName() == rhs.GetLowercasedName(); + return CompareFilenames(name_, rhs.name_) == 0; } if (IsRegexPlugin()) diff --git a/src/api/metadata_list.cpp b/src/api/metadata_list.cpp index 3364ee17..50bd32c7 100644 --- a/src/api/metadata_list.cpp +++ b/src/api/metadata_list.cpp @@ -29,6 +29,7 @@ #include "api/game/game.h" #include "api/helpers/logging.h" +#include "api/helpers/text.h" #include "api/metadata/condition_evaluator.h" #include "api/metadata/yaml/group.h" #include "api/metadata/yaml/plugin_metadata.h" @@ -100,7 +101,7 @@ void MetadataList::Save(const std::filesystem::path& filepath) const { auto plugins = Plugins(); plugins.sort([](const PluginMetadata& p1, const PluginMetadata& p2) { - return p1.GetLowercasedName() < p2.GetLowercasedName(); + return CompareFilenames(p1.GetName(), p2.GetName()) < 0; }); if (!plugins.empty()) diff --git a/src/api/plugin.cpp b/src/api/plugin.cpp index 547e247f..d6522b32 100644 --- a/src/api/plugin.cpp +++ b/src/api/plugin.cpp @@ -226,7 +226,7 @@ uintmax_t Plugin::GetFileSize(std::filesystem::path pluginPath) { } bool Plugin::operator<(const Plugin& rhs) const { - return boost::locale::to_lower(name_) < boost::locale::to_lower(rhs.name_); + return CompareFilenames(name_, rhs.name_) < 0; } void Plugin::Load(const std::filesystem::path& path, diff --git a/src/api/sorting/plugin_sorter.cpp b/src/api/sorting/plugin_sorter.cpp index e77cce3f..9105e7ea 100644 --- a/src/api/sorting/plugin_sorter.cpp +++ b/src/api/sorting/plugin_sorter.cpp @@ -35,6 +35,7 @@ #include "api/game/game.h" #include "api/helpers/logging.h" +#include "api/helpers/text.h" #include "api/metadata/condition_evaluator.h" #include "api/sorting/group_sort.h" #include "loot/exception/cyclic_interaction_error.h" @@ -237,10 +238,9 @@ void PluginSorter::AddPluginVertices(Game& game) { } std::optional PluginSorter::GetVertexByName(const std::string& name) const { - auto lowercasedName = boost::locale::to_lower(name); for (const auto& vertex : boost::make_iterator_range(boost::vertices(graph_))) { - if (graph_[vertex].GetLowercasedName() == lowercasedName) { + if (CompareFilenames(graph_[vertex].GetName(), name) == 0) { return vertex; } } @@ -678,22 +678,17 @@ int PluginSorter::ComparePlugins(const std::string& plugin1, // comparison to get an ordering. // Compare plugin basenames. - string name1 = boost::locale::to_lower(plugin1); - name1 = name1.substr(0, name1.length() - 4); - string name2 = boost::locale::to_lower(plugin2); - name2 = name2.substr(0, name2.length() - 4); + auto basename1 = plugin1.substr(0, plugin1.length() - 4); + auto basename2 = plugin2.substr(0, plugin2.length() - 4); - if (name1 < name2) - return -1; - else if (name2 < name1) - return 1; - else { + int result = CompareFilenames(basename1, basename2); + + if (result != 0) { + return result; + } else { // Could be a .esp and .esm plugin with the same basename, // compare whole filenames. - if (plugin1 < plugin2) - return -1; - else - return 1; + return CompareFilenames(plugin1, plugin2); } } return 0; diff --git a/src/api/sorting/plugin_sorting_data.cpp b/src/api/sorting/plugin_sorting_data.cpp index a9b0ca03..c3659975 100644 --- a/src/api/sorting/plugin_sorting_data.cpp +++ b/src/api/sorting/plugin_sorting_data.cpp @@ -49,10 +49,6 @@ PluginSortingData::PluginSortingData(const Plugin& plugin, std::string PluginSortingData::GetName() const { return plugin_.GetName(); } -std::string PluginSortingData::GetLowercasedName() const { - return boost::locale::to_lower(plugin_.GetName()); -} - bool PluginSortingData::IsMaster() const { return plugin_.IsMaster() || (plugin_.IsLightMaster() && !boost::iends_with(plugin_.GetName(), ".esp")); diff --git a/src/api/sorting/plugin_sorting_data.h b/src/api/sorting/plugin_sorting_data.h index ffd2fec2..7196c3d8 100644 --- a/src/api/sorting/plugin_sorting_data.h +++ b/src/api/sorting/plugin_sorting_data.h @@ -36,7 +36,6 @@ public: const PluginMetadata& userMetadata); std::string GetName() const; - std::string GetLowercasedName() const; bool IsMaster() const; bool LoadsArchive() const; std::vector GetMasters() const; diff --git a/src/tests/api/internals/helpers/text_test.h b/src/tests/api/internals/helpers/text_test.h index 31a7d9b7..fa0c9837 100644 --- a/src/tests/api/internals/helpers/text_test.h +++ b/src/tests/api/internals/helpers/text_test.h @@ -176,6 +176,93 @@ TEST(ExtractVersion, shouldPreferVersionPrefixedNumbersOverVPrefixedNumber) { "later),\nPatch Version: 1.0"); EXPECT_EQ("1.0", text.value()); } + +#ifdef _WIN32 +// MSVC interprets source files in the default code page, so +// for me u8"\xC3\x9C" != u8"\u00DC", which is a lot of fun. +// To avoid insanity, write non-ASCII characters as \uXXXX escapes. +// \u03a1 is greek rho uppercase 'Ρ' +// \u03c1 is greek rho lowercase 'ρ' +// \u03f1 is greek rho 'ϱ' +// \u0130 is turkish 'İ' +// \u0131 is turkish 'ı' + +TEST(CompareFilenames, shouldBeCaseInsensitiveAndLocaleInvariant) { + EXPECT_EQ(0, CompareFilenames("i", "I")); + EXPECT_EQ(-1, CompareFilenames("i", u8"\u0130")); + EXPECT_EQ(-1, CompareFilenames("i", u8"\u0131")); + EXPECT_EQ(-1, CompareFilenames("I", u8"\u0130")); + EXPECT_EQ(-1, CompareFilenames("I", u8"\u0131")); + EXPECT_EQ(-1, CompareFilenames(u8"\u0130", u8"\u0131")); + EXPECT_EQ(1, CompareFilenames(u8"\u03f1", u8"\u03a1")); + EXPECT_EQ(1, CompareFilenames(u8"\u03f1", u8"\u03c1")); + EXPECT_EQ(0, CompareFilenames(u8"\u03a1", u8"\u03c1")); + + // Set locale to Turkish. + std::locale::global(boost::locale::generator().generate("tr_TR.UTF-8")); + + EXPECT_EQ(0, CompareFilenames("i", "I")); + EXPECT_EQ(-1, CompareFilenames("i", u8"\u0130")); + EXPECT_EQ(-1, CompareFilenames("i", u8"\u0131")); + EXPECT_EQ(-1, CompareFilenames("I", u8"\u0130")); + EXPECT_EQ(-1, CompareFilenames("I", u8"\u0131")); + EXPECT_EQ(-1, CompareFilenames(u8"\u0130", u8"\u0131")); + EXPECT_EQ(1, CompareFilenames(u8"\u03f1", u8"\u03a1")); + EXPECT_EQ(1, CompareFilenames(u8"\u03f1", u8"\u03c1")); + EXPECT_EQ(0, CompareFilenames(u8"\u03a1", u8"\u03c1")); + + // Set locale to Greek. + std::locale::global(boost::locale::generator().generate("el_GR.UTF-8")); + + EXPECT_EQ(0, CompareFilenames("i", "I")); + EXPECT_EQ(-1, CompareFilenames("i", u8"\u0130")); + EXPECT_EQ(-1, CompareFilenames("i", u8"\u0131")); + EXPECT_EQ(-1, CompareFilenames("I", u8"\u0130")); + EXPECT_EQ(-1, CompareFilenames("I", u8"\u0131")); + EXPECT_EQ(-1, CompareFilenames(u8"\u0130", u8"\u0131")); + EXPECT_EQ(1, CompareFilenames(u8"\u03f1", u8"\u03a1")); + EXPECT_EQ(1, CompareFilenames(u8"\u03f1", u8"\u03c1")); + EXPECT_EQ(0, CompareFilenames(u8"\u03a1", u8"\u03c1")); + + // Reset locale. + std::locale::global(boost::locale::generator().generate("")); +} + +TEST(NormalizeFilename, shouldUppercaseStringsAndBeLocaleInvariant) { + EXPECT_EQ("I", NormalizeFilename("i")); + EXPECT_EQ("I", NormalizeFilename("I")); + EXPECT_EQ(u8"\u0130", NormalizeFilename(u8"\u0130")); + EXPECT_EQ(u8"\u0131", NormalizeFilename(u8"\u0131")); + EXPECT_EQ(u8"\u03f1", NormalizeFilename(u8"\u03f1")); + EXPECT_EQ(u8"\u03a1", NormalizeFilename(u8"\u03a1")); + EXPECT_EQ(u8"\u03a1", NormalizeFilename(u8"\u03c1")); + + // Set locale to Turkish. + std::locale::global(boost::locale::generator().generate("tr_TR.UTF-8")); + + EXPECT_EQ("I", NormalizeFilename("i")); + EXPECT_EQ("I", NormalizeFilename("I")); + EXPECT_EQ(u8"\u0130", NormalizeFilename(u8"\u0130")); + EXPECT_EQ(u8"\u0131", NormalizeFilename(u8"\u0131")); + EXPECT_EQ(u8"\u03f1", NormalizeFilename(u8"\u03f1")); + EXPECT_EQ(u8"\u03a1", NormalizeFilename(u8"\u03a1")); + EXPECT_EQ(u8"\u03a1", NormalizeFilename(u8"\u03c1")); + + // Set locale to Greek. + std::locale::global(boost::locale::generator().generate("el_GR.UTF-8")); + + EXPECT_EQ("I", NormalizeFilename("i")); + EXPECT_EQ("I", NormalizeFilename("I")); + EXPECT_EQ(u8"\u0130", NormalizeFilename(u8"\u0130")); + EXPECT_EQ(u8"\u0131", NormalizeFilename(u8"\u0131")); + EXPECT_EQ(u8"\u03f1", NormalizeFilename(u8"\u03f1")); + EXPECT_EQ(u8"\u03a1", NormalizeFilename(u8"\u03a1")); + EXPECT_EQ(u8"\u03a1", NormalizeFilename(u8"\u03c1")); + + // Reset locale. + std::locale::global(boost::locale::generator().generate("")); +} +#endif } }