From dd5f2ad755f1eacd85ddb91455ea8c3f694c8eff Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 6 Aug 2024 23:53:47 +0200 Subject: [PATCH] Return strings directly from read_file_as_utf8_string_if_exists (#17667) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Every single place that called `read_file_as_utf8_string_if_exists` would immediately do a `.value_or(std::string{})`. As such, the function now returns a string directly. * There was just one caller to `read_file_as_utf8_string` and it only cared about files that are non-empty. As such, the specialization got removed. Both of these make sense to me, as in practice there's seldom a difference between an empty file and a non-existent one. ## Validation Steps Performed * Compiles ✅ * Starts ✅ * Deleting the `settings.json` contents triggers a reload ✅ --- .../TerminalSettingsModel/ActionMap.cpp | 5 ++- .../ApplicationState.cpp | 10 +++--- .../TerminalSettingsModel/ApplicationState.h | 4 +-- .../CascadiaSettingsSerialization.cpp | 11 +++--- src/inc/til/io.h | 35 +++++++------------ 5 files changed, 29 insertions(+), 36 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 5090733342..5d298457c7 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -923,13 +923,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { // This returns an empty string if we fail to load the file. std::filesystem::path localSnippetsPath{ std::wstring_view{ currentWorkingDirectory + L"\\.wt.json" } }; - const auto localTasksFileContents = til::io::read_file_as_utf8_string_if_exists(localSnippetsPath); - if (!localTasksFileContents.has_value() || localTasksFileContents->empty()) + const auto data = til::io::read_file_as_utf8_string_if_exists(localSnippetsPath); + if (data.empty()) { return {}; } - const auto& data = *localTasksFileContents; Json::Value root; std::string errs; const std::unique_ptr reader{ Json::CharReaderBuilder{}.newCharReader() }; diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 4cd1219fb2..7b6040b141 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -146,7 +146,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::unique_ptr reader{ Json::CharReaderBuilder{}.newCharReader() }; // First get shared state out of `state.json`. - const auto sharedData = _readSharedContents().value_or(std::string{}); + const auto sharedData = _readSharedContents(); if (!sharedData.empty()) { Json::Value root; @@ -165,7 +165,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation FromJson(root, FileSource::Shared); // Then, try and get anything in elevated-state - if (const auto localData{ _readLocalContents().value_or(std::string{}) }; !localData.empty()) + if (const auto localData{ _readLocalContents() }; !localData.empty()) { Json::Value root; if (!reader->parse(localData.data(), localData.data() + localData.size(), &root, &errs)) @@ -216,7 +216,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // First load the contents of state.json into a json blob. This will // contain the Shared properties and the unelevated instance's Local // properties. - const auto sharedData = _readSharedContents().value_or(std::string{}); + const auto sharedData = _readSharedContents(); if (!sharedData.empty()) { if (!reader->parse(sharedData.data(), sharedData.data() + sharedData.size(), &root, &errs)) @@ -334,7 +334,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // - Read the contents of our "shared" state - state that should be shared // for elevated and unelevated instances. This is things like the list of // generated profiles, the command palette commandlines. - std::optional ApplicationState::_readSharedContents() const + std::string ApplicationState::_readSharedContents() const { return til::io::read_file_as_utf8_string_if_exists(_sharedPath); } @@ -346,7 +346,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // those don't matter when unelevated). // - When elevated, this will DELETE `elevated-state.json` if it has bad // permissions, so we don't potentially read malicious data. - std::optional ApplicationState::_readLocalContents() const + std::string ApplicationState::_readLocalContents() const { return ::Microsoft::Console::Utils::IsRunningElevated() ? til::io::read_file_as_utf8_string_if_exists(_elevatedPath, true) : diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index df7fd803e7..af43f0e999 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -95,9 +95,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value _toJsonWithBlob(Json::Value& root, FileSource parseSource) const noexcept; - std::optional _readSharedContents() const; + std::string _readSharedContents() const; void _writeSharedContents(const std::string_view content) const; - std::optional _readLocalContents() const; + std::string _readLocalContents() const; void _writeLocalContents(const std::string_view content) const; }; } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 237b687964..abb6caa0e1 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -237,8 +237,11 @@ void SettingsLoader::FindFragmentsAndMergeIntoUserSettings() { try { - const auto content = til::io::read_file_as_utf8_string(fragmentExt.path()); - _parseFragment(source, content, fragmentSettings); + const auto content = til::io::read_file_as_utf8_string_if_exists(fragmentExt.path()); + if (!content.empty()) + { + _parseFragment(source, content, fragmentSettings); + } } CATCH_LOG(); } @@ -934,7 +937,7 @@ Model::CascadiaSettings CascadiaSettings::LoadAll() try { FILETIME lastWriteTime{}; - auto settingsString = til::io::read_file_as_utf8_string_if_exists(_settingsPath(), false, &lastWriteTime).value_or(std::string{}); + auto settingsString = til::io::read_file_as_utf8_string_if_exists(_settingsPath(), false, &lastWriteTime); auto firstTimeSetup = settingsString.empty(); // If it's the firstTimeSetup and a preview build, then try to @@ -947,7 +950,7 @@ try { try { - settingsString = til::io::read_file_as_utf8_string_if_exists(_releaseSettingsPath()).value_or(std::string{}); + settingsString = til::io::read_file_as_utf8_string_if_exists(_releaseSettingsPath()); releaseSettingExists = settingsString.empty() ? false : true; } catch (...) diff --git a/src/inc/til/io.h b/src/inc/til/io.h index 693751105b..6f917f0d0b 100644 --- a/src/inc/til/io.h +++ b/src/inc/til/io.h @@ -53,8 +53,8 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" } // details // Tries to read a file somewhat atomically without locking it. - // Strips the UTF8 BOM if it exists. - _TIL_INLINEPREFIX std::string read_file_as_utf8_string(const std::filesystem::path& path, const bool elevatedOnly = false, FILETIME* lastWriteTime = nullptr) + // Returns an empty string if the file couldn't be opened. + _TIL_INLINEPREFIX std::string read_file_as_utf8_string_if_exists(const std::filesystem::path& path, const bool elevatedOnly = false, FILETIME* lastWriteTime = nullptr) { // From some casual observations we can determine that: // * ReadFile() always returns the requested amount of data (unless the file is smaller) @@ -69,7 +69,16 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr) }; - THROW_LAST_ERROR_IF(!file); + + if (!file) + { + const auto gle = GetLastError(); + if (gle == ERROR_FILE_NOT_FOUND) + { + return {}; + } + THROW_WIN32(gle); + } // Open the file _first_, then check if it has the right // permissions. This prevents a "Time-of-check to time-of-use" @@ -92,7 +101,7 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" LOG_LAST_ERROR_IF(!DeleteFile(path.c_str())); // Exit early, because obviously there's nothing to read from the deleted file. - return ""; + return {}; } } @@ -137,24 +146,6 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" THROW_WIN32_MSG(ERROR_READ_FAULT, "file size changed while reading"); } - // Same as read_file_as_utf8_string, but returns an empty optional, if the file couldn't be opened. - _TIL_INLINEPREFIX std::optional read_file_as_utf8_string_if_exists(const std::filesystem::path& path, const bool elevatedOnly = false, FILETIME* lastWriteTime = nullptr) - { - try - { - return { read_file_as_utf8_string(path, elevatedOnly, lastWriteTime) }; - } - catch (const wil::ResultException& exception) - { - if (exception.GetErrorCode() == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) - { - return {}; - } - - throw; - } - } - _TIL_INLINEPREFIX void write_utf8_string_to_file(const std::filesystem::path& path, const std::string_view& content, const bool elevatedOnly = false, FILETIME* lastWriteTime = nullptr) { SECURITY_ATTRIBUTES sa;