From 2c9ea2209bb2144d6b8a92f3ba5626859d68ba05 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sun, 20 Jan 2019 11:02:17 +0000 Subject: [PATCH] Workaround probable Windows bug in path equivalency checks In Windows, std::filesystem::equivalent throws std::system_error if a path contains characters that can't be represented in the system code page (e.g. Windows 1252). This is undocumented and seems unnecessary as Microsoft's own docs say Windows paths are stored natively in Unicode, so it sounds like a bug to me. To work around this, return true early if the path objects are equal, so std::filesystem::equivalent doesn't need to be called, and catch system_error if it does need to be called - this may result in false negatives, but for LOOT's purposes that is fine and is very unlikely anyway. --- src/api/plugin.cpp | 14 +++++ src/tests/api/internals/game/game_test.h | 11 ++++ src/tests/api/internals/main.cpp | 35 ++++++++----- src/tests/api/internals/plugin_test.h | 65 ++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 14 deletions(-) diff --git a/src/api/plugin.cpp b/src/api/plugin.cpp index e35c12e5..e22261b8 100644 --- a/src/api/plugin.cpp +++ b/src/api/plugin.cpp @@ -304,12 +304,26 @@ std::filesystem::path replaceExtension(std::filesystem::path path, const std::st } bool equivalent(const std::filesystem::path& path1, const std::filesystem::path& path2) { + // If the paths are identical, they've got to be equivalent, + // it doesn't matter if the paths exist or not. + if (path1 == path2) { + return true; + } + // If the paths are not identical, the filesystem might be case-insensitive + // so check with the filesystem. try { return std::filesystem::equivalent(path1, path2); } catch (std::filesystem::filesystem_error) { // One of the paths checked for equivalence doesn't exist, // so they can't be equivalent. return false; + } catch (std::system_error) { + // This can be thrown if one or both of the paths contains a character + // that can't be represented in Windows' multi-byte code page (e.g. + // Windows-1252), even though Unicode paths shouldn't be a problem, + // and throwing system_error is undocumented. Seems like a bug in MSVC's + // implementation. + return false; } } diff --git a/src/tests/api/internals/game/game_test.h b/src/tests/api/internals/game/game_test.h index 7931f9b2..dd1c071c 100644 --- a/src/tests/api/internals/game/game_test.h +++ b/src/tests/api/internals/game/game_test.h @@ -177,6 +177,17 @@ TEST_P(GameTest, EXPECT_EQ(1, game.GetCache()->GetArchivePaths().size()); } +TEST_P(GameTest, + loadPluginsShouldNotThrowIfAFilenameHasNonWindows1252EncodableCharacters) { + auto path = dataPath / std::filesystem::u8path(u8"\u2551\u00BB\u00C1\u2510\u2557\u00FE\u00C3\u00CE.txt"); + std::ofstream out(path); + out.close(); + + Game game = Game(GetParam(), dataPath.parent_path(), localPath); + + EXPECT_NO_THROW(loadInstalledPlugins(game, false)); +} + TEST_P(GameTest, shouldShowBlankEsmAsActiveIfItHasNotBeenLoaded) { Game game = Game(GetParam(), dataPath.parent_path(), localPath); game.LoadCurrentLoadOrderState(); diff --git a/src/tests/api/internals/main.cpp b/src/tests/api/internals/main.cpp index b0c472ad..df8e8a84 100644 --- a/src/tests/api/internals/main.cpp +++ b/src/tests/api/internals/main.cpp @@ -164,6 +164,20 @@ TEST(Filesystem, equivalentShouldRequireThatBothPathsExist) { EXPECT_THROW(std::filesystem::equivalent(lower, upper), std::filesystem::filesystem_error); } + +TEST(Filesystem, equivalentShouldBeCaseInsensitive) { + auto upper = std::filesystem::path("LICENSE"); + auto lower = std::filesystem::path("license"); + + EXPECT_TRUE(std::filesystem::equivalent(lower, upper)); +} + +TEST(Filesystem, equivalentCannotHandleCharactersThatAreUnrepresentableInTheSystemCodePage) { + auto path1 = std::filesystem::u8path(u8"\u2551\u00BB\u00C1\u2510\u2557\u00FE\u00C3\u00CE.txt"); + auto path2 = std::filesystem::u8path(u8"\u2551\u00BB\u00C1\u2510\u2557\u00FE\u00C3\u00CE.txt"); + + EXPECT_THROW(std::filesystem::equivalent(path1, path2), std::system_error); +} #else TEST(Filesystem, equivalentShouldNotRequireThatBothPathsExist) { auto upper = std::filesystem::path("LICENSE"); @@ -171,6 +185,13 @@ TEST(Filesystem, equivalentShouldNotRequireThatBothPathsExist) { EXPECT_FALSE(std::filesystem::equivalent(lower, upper)); } + +TEST(Filesystem, equivalentShouldBeCaseSensitive) { + auto upper = std::filesystem::path("LICENSE"); + auto lower = std::filesystem::path("license"); + + EXPECT_FALSE(std::filesystem::equivalent(lower, upper)); +} #endif TEST(Filesystem, canonicalShouldRequireThatThePathExists) { @@ -178,13 +199,6 @@ TEST(Filesystem, canonicalShouldRequireThatThePathExists) { } #ifdef _WIN32 -TEST(Filesystem, equivalentShouldBeCaseInsensitive) { - auto upper = std::filesystem::path("LICENSE"); - auto lower = std::filesystem::path("license"); - - EXPECT_TRUE(std::filesystem::equivalent(lower, upper)); -} - TEST(Filesystem, canonicalShouldFoldCase) { auto upper = std::filesystem::canonical("LICENSE"); auto lower = std::filesystem::canonical("license"); @@ -192,13 +206,6 @@ TEST(Filesystem, canonicalShouldFoldCase) { EXPECT_EQ(lower, upper); } #else -TEST(Filesystem, equivalentShouldBeCaseSensitive) { - auto upper = std::filesystem::path("LICENSE"); - auto lower = std::filesystem::path("license"); - - EXPECT_FALSE(std::filesystem::equivalent(lower, upper)); -} - TEST(Filesystem, canonicalShouldNotFoldCase) { std::ofstream out("license"); out.close(); diff --git a/src/tests/api/internals/plugin_test.h b/src/tests/api/internals/plugin_test.h index 7b3e29bb..9cd74e91 100644 --- a/src/tests/api/internals/plugin_test.h +++ b/src/tests/api/internals/plugin_test.h @@ -523,6 +523,71 @@ TEST_P( EXPECT_EQ(GetParam() == GameType::fo4 || GetParam() == GameType::tes5se, result); } + +TEST(equivalent, shouldReturnTrueIfGivenEqualPathsThatExist) { + auto path1 = std::filesystem::path("LICENSE"); + auto path2 = std::filesystem::path("LICENSE"); + + EXPECT_TRUE(loot::equivalent(path1, path2)); +} + +TEST(equivalent, shouldReturnTrueIfGivenEqualPathsThatDoNotExist) { + auto path1 = std::filesystem::path("LICENSE2"); + auto path2 = std::filesystem::path("LICENSE2"); + + EXPECT_TRUE(loot::equivalent(path1, path2)); +} + +TEST(equivalent, shouldReturnFalseIfPathsAreNotCaseInsensitivelyEqual) { + auto upper = std::filesystem::path("LICENSE"); + auto lower = std::filesystem::path("license2"); + + EXPECT_FALSE(loot::equivalent(lower, upper)); +} + +#ifdef _WIN32 +TEST(equivalent, shouldReturnTrueIfGivenCaseInsensitivelyEqualPathsThatExist) { + auto upper = std::filesystem::path("LICENSE"); + auto lower = std::filesystem::path("license"); + + EXPECT_TRUE(loot::equivalent(lower, upper)); +} + +TEST(equivalent, shouldReturnFalseIfGivenCaseInsensitivelyEqualPathsThatDoNotExist) { + auto upper = std::filesystem::path("LICENSE2"); + auto lower = std::filesystem::path("license2"); + + EXPECT_FALSE(loot::equivalent(lower, upper)); +} + +TEST(equivalent, shouldReturnTrueIfEqualPathsHaveCharactersThatAreUnrepresentableInTheSystemMultiByteCodePage) { + auto path1 = std::filesystem::u8path(u8"\u2551\u00BB\u00C1\u2510\u2557\u00FE\u00C3\u00CE.txt"); + auto path2 = std::filesystem::u8path(u8"\u2551\u00BB\u00C1\u2510\u2557\u00FE\u00C3\u00CE.txt"); + + EXPECT_TRUE(loot::equivalent(path1, path2)); +} + +TEST(equivalent, shouldReturnFalseIfCaseInsensitivelyEqualPathsHaveCharactersThatAreUnrepresentableInTheSystemMultiByteCodePage) { + auto path1 = std::filesystem::u8path(u8"\u2551\u00BB\u00C1\u2510\u2557\u00FE\u00E3\u00CE.txt"); + auto path2 = std::filesystem::u8path(u8"\u2551\u00BB\u00C1\u2510\u2557\u00FE\u00C3\u00CE.txt"); + + EXPECT_FALSE(loot::equivalent(path1, path2)); +} +#else +TEST(equivalent, shouldReturnFalseIfGivenCaseInsensitivelyEqualPathsThatExist) { + auto upper = std::filesystem::path("LICENSE"); + auto lower = std::filesystem::path("license"); + + EXPECT_FALSE(loot::equivalent(lower, upper)); +} + +TEST(equivalent, shouldReturnFalseIfGivenCaseInsensitivelyEqualPathsThatDoNotExist) { + auto upper = std::filesystem::path("LICENSE2"); + auto lower = std::filesystem::path("license2"); + + EXPECT_FALSE(loot::equivalent(lower, upper)); +} +#endif } }