From 021007e0ef13787f55f42bd11aa898388bb64746 Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 20 Aug 2026 12:23:17 +0900 Subject: [PATCH 1/9] fix: report path failures through the return type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit path::expand() called the throwing overload of std::filesystem::canonical without catching it, so a ~ path whose target did not exist terminated the process rather than returning the std::unexpected its signature promises. Wrap that call, leaving the rest of the function as it was: a path that already resolves still comes back in canonical form. path::mkdir() read a false return from create_directories() as failure, but that overload also returns false when the directory is already there, and leaves the error_code untouched in that case — the filesystem_error built from it printed as "failed: Success". Judge the outcome by the error_code instead, which makes an already-present directory a success and leaves a real failure carrying its own diagnostic code. --- src/platform/path.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/platform/path.cpp b/src/platform/path.cpp index d94b8eb..72e6e2a 100644 --- a/src/platform/path.cpp +++ b/src/platform/path.cpp @@ -14,13 +14,10 @@ std::expected path::mkdir(const std::st std::expected path::mkdir(const std::filesystem::path& absolute_path) { - try { - std::error_code err; - if (! std::filesystem::create_directories(absolute_path, err)) { - return std::unexpected(std::filesystem::filesystem_error("failed", absolute_path, err)); - } - } catch (const std::filesystem::filesystem_error& e) { - return std::unexpected(e); + std::error_code err; + std::filesystem::create_directories(absolute_path, err); + if (err) { + return std::unexpected(std::filesystem::filesystem_error("failed", absolute_path, err)); } return path{absolute_path}; @@ -97,7 +94,11 @@ std::expected path::expand() const return std::make_optional(p.append(_path.string().replace(0, 1, "")).string()); }); if (expanded) { - return path { std::filesystem::canonical(std::filesystem::path{expanded.value()}) }; + try { + return path { std::filesystem::canonical(std::filesystem::path{expanded.value()}) }; + } catch (const std::filesystem::filesystem_error& e) { + return std::unexpected(e); + } } else { return std::unexpected(std::filesystem::filesystem_error("fail to expand tilde", _path, std::make_error_code(std::errc::no_such_file_or_directory))); } From 97fd1add3ea57ff2c596e255b1fb0e64472062ba Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 20 Aug 2026 12:23:25 +0900 Subject: [PATCH 2/9] test: cover path, xdg and error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These three classes had no test file. The suite reached the type system, environment and TOML, so the classes whose documented behaviour turned out not to match the code were exactly the untested ones. Cover what the fixes now guarantee — mkdir() on an existing directory, on a path whose parent is a regular file, and on an empty string; expand() on a missing target, on a target reached through a symlink, and on a path with no leading tilde — and pin what was left alone: exists() and the default constructor still call the throwing filesystem functions. Each test restores the environment variables it sets, including restoring an unset variable as unset, and works in a directory named after the process rather than a fixed path. --- test/CMakeLists.txt | 3 + test/platform/test_path.cpp | 281 ++++++++++++++++++++++++++++++++++++ test/platform/test_xdg.cpp | 163 +++++++++++++++++++++ test/type/test_error.cpp | 102 +++++++++++++ 4 files changed, 549 insertions(+) create mode 100644 test/platform/test_path.cpp create mode 100644 test/platform/test_xdg.cpp create mode 100644 test/type/test_error.cpp diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 900d8f2..8d2707e 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -19,10 +19,13 @@ add_executable(${TEST_NAME}) target_sources(${TEST_NAME} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/platform/test_environment.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/platform/test_path.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/platform/test_xdg.cpp ${CMAKE_CURRENT_SOURCE_DIR}/type/test_array.cpp ${CMAKE_CURRENT_SOURCE_DIR}/type/test_boolean.cpp ${CMAKE_CURRENT_SOURCE_DIR}/type/test_data.cpp ${CMAKE_CURRENT_SOURCE_DIR}/type/test_dictionary.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/type/test_error.cpp ${CMAKE_CURRENT_SOURCE_DIR}/type/test_number.cpp ${CMAKE_CURRENT_SOURCE_DIR}/type/test_string.cpp ${CMAKE_CURRENT_SOURCE_DIR}/type/test_timestamp.cpp diff --git a/test/platform/test_path.cpp b/test/platform/test_path.cpp new file mode 100644 index 0000000..61702ed --- /dev/null +++ b/test/platform/test_path.cpp @@ -0,0 +1,281 @@ +#include + +#include "dross/platform/path.h" + +#include +#include +#include +#include +#include +#include + +namespace { + +// Process-unique scratch directory (CWE-377: never a predictable fixed +// name). Removes itself and everything created inside it on destruction, +// including when a test exits early via an ASSERT_* failure: ASSERT_* +// returns from the test function rather than throwing, so the stack still +// unwinds normally and the destructor still runs. +class scoped_temp_dir { +public: + scoped_temp_dir() + : _path(std::filesystem::temp_directory_path() + / ("dross_path_test_" + std::to_string(::getpid()) + "_" + std::to_string(_next_id++))) + { + std::filesystem::create_directories(_path); + } + + ~scoped_temp_dir() + { + std::error_code ignored; + std::filesystem::remove_all(_path, ignored); + } + + scoped_temp_dir(const scoped_temp_dir&) = delete; + scoped_temp_dir& operator=(const scoped_temp_dir&) = delete; + + const std::filesystem::path& path() const { return _path; } + +private: + std::filesystem::path _path; + static inline unsigned _next_id = 0; +}; + +// Saves and restores a single environment variable across a test, even +// when the test exits early via an ASSERT_* failure. Restores "unset" as +// unset rather than as an empty string. +class scoped_env_var { +public: + explicit scoped_env_var(std::string name) + : _name(std::move(name)) + { + if (const char* v = std::getenv(_name.c_str())) { + _original = std::string(v); + } + } + + ~scoped_env_var() + { + if (_original) { + setenv(_name.c_str(), _original->c_str(), 1); + } else { + unsetenv(_name.c_str()); + } + } + + scoped_env_var(const scoped_env_var&) = delete; + scoped_env_var& operator=(const scoped_env_var&) = delete; + + void set(const std::string& value) const + { + setenv(_name.c_str(), value.c_str(), 1); + } + +private: + std::string _name; + std::optional _original; +}; + +void write_regular_file(const std::filesystem::path& p) +{ + std::ofstream out(p); + out << "content"; +} + +} + +// --- path::mkdir ------------------------------------------------------- + +TEST(path_test, mkdir_creates_a_new_directory) +{ + const scoped_temp_dir base; + const auto target = base.path() / "created"; + + const auto result = dross::path::mkdir(target.string()); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result->string(), target.string()); + EXPECT_TRUE(std::filesystem::is_directory(target)); +} + +TEST(path_test, mkdir_creates_nested_parent_directories) +{ + const scoped_temp_dir base; + const auto target = base.path() / "a" / "b" / "c"; + + const auto result = dross::path::mkdir(target.string()); + + ASSERT_TRUE(result.has_value()); + EXPECT_TRUE(std::filesystem::is_directory(target)); +} + +TEST(path_test, mkdir_on_an_existing_directory_succeeds) +{ + const scoped_temp_dir base; + + const auto result = dross::path::mkdir(base.path().string()); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result->string(), base.path().string()); +} + +TEST(path_test, mkdir_called_twice_on_the_same_directory_succeeds_both_times) +{ + const scoped_temp_dir base; + const auto target = base.path() / "repeat"; + + const auto first = dross::path::mkdir(target.string()); + const auto second = dross::path::mkdir(target.string()); + + ASSERT_TRUE(first.has_value()); + ASSERT_TRUE(second.has_value()); + EXPECT_EQ(second->string(), target.string()); +} + +TEST(path_test, mkdir_fails_when_a_path_component_is_a_regular_file) +{ + // The true-failure case is exercised through ENOTDIR (a parent path + // component that is a regular file) rather than through a permission + // failure, so the assertion still holds when the suite runs as root, + // where permission checks are bypassed. + const scoped_temp_dir base; + const auto blocking_file = base.path() / "not_a_directory"; + write_regular_file(blocking_file); + const auto target = blocking_file / "child"; + + const auto result = dross::path::mkdir(target.string()); + + ASSERT_FALSE(result.has_value()); + EXPECT_NE(result.error().code().value(), 0); +} + +TEST(path_test, mkdir_target_is_an_existing_regular_file) +{ + // Boundary case: the target itself already exists but is not a + // directory. This must not be folded into the "existing directory" + // success case. + const scoped_temp_dir base; + const auto target = base.path() / "already_a_file"; + write_regular_file(target); + + const auto result = dross::path::mkdir(target.string()); + + ASSERT_FALSE(result.has_value()); + EXPECT_NE(result.error().code().value(), 0); +} + +TEST(path_test, mkdir_with_an_empty_path) +{ + // Boundary case: an empty path must not be silently accepted as an + // "already exists" success. + const auto result = dross::path::mkdir(std::string{}); + + ASSERT_FALSE(result.has_value()); + EXPECT_NE(result.error().code().value(), 0); +} + +// --- path::expand -------------------------------------------------------- + +TEST(path_test, expand_of_a_tilde_path_matches_a_freshly_computed_canonical_path) +{ + const dross::path p{std::string{"~"}}; + + const auto result = p.expand(); + + ASSERT_TRUE(result.has_value()); + const auto home = dross::path::home(); + ASSERT_TRUE(home.has_value()); + const auto expected = std::filesystem::canonical(std::filesystem::path{home->string()}); + EXPECT_EQ(result->string(), expected.string()); +} + +TEST(path_test, expand_of_a_tilde_path_resolves_through_a_symlinked_home) +{ + const scoped_temp_dir base; + const auto real_home = base.path() / "real_home"; + std::filesystem::create_directories(real_home); + const auto home_link = base.path() / "home_link"; + std::filesystem::create_directory_symlink(real_home, home_link); + + scoped_env_var home("HOME"); + home.set(home_link.string()); + + const dross::path p{std::string{"~"}}; + const auto result = p.expand(); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result->string(), std::filesystem::canonical(real_home).string()); +} + +TEST(path_test, expand_of_a_tilde_path_to_a_nonexistent_target_does_not_terminate) +{ + // Regression test: std::filesystem::canonical() throws on a missing + // path. expand() must report that as std::unexpected instead of + // letting the exception escape, which previously terminated the + // process. + const std::string tilde_path = "~/dross_test_nonexistent_" + std::to_string(::getpid()); + const dross::path p{tilde_path}; + + const auto result = p.expand(); + + EXPECT_FALSE(result.has_value()); +} + +TEST(path_test, expand_of_a_non_tilde_path_is_returned_unchanged) +{ + const std::string original = "relative/does/not/exist"; + const dross::path p{original}; + + const auto result = p.expand(); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result->string(), original); +} + +// --- path::resolve --------------------------------------------------------- + +TEST(path_test, resolve_of_an_existing_path_succeeds_in_canonical_form) +{ + const scoped_temp_dir base; + const dross::path p{base.path()}; + + const auto result = p.resolve(); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result->string(), std::filesystem::canonical(base.path()).string()); +} + +TEST(path_test, resolve_of_a_missing_path_returns_unexpected) +{ + const scoped_temp_dir base; + const dross::path p{base.path() / "does_not_exist"}; + + const auto result = p.resolve(); + + EXPECT_FALSE(result.has_value()); +} + +// --- path::exists / default constructor (pinning current, unmodified behavior) --- + +TEST(path_test, exists_is_false_for_a_missing_path) +{ + const scoped_temp_dir base; + const dross::path p{base.path() / "does_not_exist"}; + + EXPECT_FALSE(p.exists()); +} + +TEST(path_test, exists_is_true_for_a_present_path) +{ + const scoped_temp_dir base; + const dross::path p{base.path()}; + + EXPECT_TRUE(p.exists()); +} + +TEST(path_test, default_constructor_refers_to_the_current_working_directory) +{ + const dross::path p; + + EXPECT_TRUE(std::filesystem::equivalent(std::filesystem::path(p), std::filesystem::current_path())); +} diff --git a/test/platform/test_xdg.cpp b/test/platform/test_xdg.cpp new file mode 100644 index 0000000..b6f2c85 --- /dev/null +++ b/test/platform/test_xdg.cpp @@ -0,0 +1,163 @@ +#include + +#include "dross/platform/xdg.h" + +#include +#include +#include + +namespace { + +// Saves and restores a single environment variable across a test, even +// when the test exits early via an ASSERT_* failure. Restores "unset" as +// unset rather than as an empty string. +class scoped_env_var { +public: + explicit scoped_env_var(std::string name) + : _name(std::move(name)) + { + if (const char* v = std::getenv(_name.c_str())) { + _original = std::string(v); + } + } + + ~scoped_env_var() + { + if (_original) { + setenv(_name.c_str(), _original->c_str(), 1); + } else { + unsetenv(_name.c_str()); + } + } + + scoped_env_var(const scoped_env_var&) = delete; + scoped_env_var& operator=(const scoped_env_var&) = delete; + + void set(const std::string& value) const + { + setenv(_name.c_str(), value.c_str(), 1); + } + + void unset() const + { + unsetenv(_name.c_str()); + } + +private: + std::string _name; + std::optional _original; +}; + +} + +// --- config_home ----------------------------------------------------------- + +TEST(xdg_test, config_home_uses_xdg_config_home_when_set) +{ + scoped_env_var xdg_config_home("XDG_CONFIG_HOME"); + xdg_config_home.set("/tmp/dross_xdg_test/xdg_config_home"); + + const dross::xdg app{"myapp"}; + const auto result = app.config_home(); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, "/tmp/dross_xdg_test/xdg_config_home/myapp"); +} + +TEST(xdg_test, config_home_falls_back_to_home_dot_config_when_unset) +{ + scoped_env_var xdg_config_home("XDG_CONFIG_HOME"); + xdg_config_home.unset(); + scoped_env_var home("HOME"); + home.set("/tmp/dross_xdg_test/home"); + + const dross::xdg app{"myapp"}; + const auto result = app.config_home(); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, "/tmp/dross_xdg_test/home/.config/myapp"); +} + +// --- data_home --------------------------------------------------------- + +TEST(xdg_test, data_home_uses_xdg_data_home_when_set) +{ + scoped_env_var xdg_data_home("XDG_DATA_HOME"); + xdg_data_home.set("/tmp/dross_xdg_test/xdg_data_home"); + + const dross::xdg app{"myapp"}; + const auto result = app.data_home(); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, "/tmp/dross_xdg_test/xdg_data_home/myapp"); +} + +TEST(xdg_test, data_home_falls_back_to_home_dot_local_share_when_unset) +{ + scoped_env_var xdg_data_home("XDG_DATA_HOME"); + xdg_data_home.unset(); + scoped_env_var home("HOME"); + home.set("/tmp/dross_xdg_test/home"); + + const dross::xdg app{"myapp"}; + const auto result = app.data_home(); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, "/tmp/dross_xdg_test/home/.local/share/myapp"); +} + +// --- cache_home -------------------------------------------------------- + +TEST(xdg_test, cache_home_uses_xdg_cache_home_when_set) +{ + scoped_env_var xdg_cache_home("XDG_CACHE_HOME"); + xdg_cache_home.set("/tmp/dross_xdg_test/xdg_cache_home"); + + const dross::xdg app{"myapp"}; + const auto result = app.cache_home(); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, "/tmp/dross_xdg_test/xdg_cache_home/myapp"); +} + +TEST(xdg_test, cache_home_falls_back_to_home_dot_cache_when_unset) +{ + scoped_env_var xdg_cache_home("XDG_CACHE_HOME"); + xdg_cache_home.unset(); + scoped_env_var home("HOME"); + home.set("/tmp/dross_xdg_test/home"); + + const dross::xdg app{"myapp"}; + const auto result = app.cache_home(); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, "/tmp/dross_xdg_test/home/.cache/myapp"); +} + +// --- state_home -------------------------------------------------------- + +TEST(xdg_test, state_home_uses_xdg_state_home_when_set) +{ + scoped_env_var xdg_state_home("XDG_STATE_HOME"); + xdg_state_home.set("/tmp/dross_xdg_test/xdg_state_home"); + + const dross::xdg app{"myapp"}; + const auto result = app.state_home(); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, "/tmp/dross_xdg_test/xdg_state_home/myapp"); +} + +TEST(xdg_test, state_home_falls_back_to_home_dot_local_state_when_unset) +{ + scoped_env_var xdg_state_home("XDG_STATE_HOME"); + xdg_state_home.unset(); + scoped_env_var home("HOME"); + home.set("/tmp/dross_xdg_test/home"); + + const dross::xdg app{"myapp"}; + const auto result = app.state_home(); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, "/tmp/dross_xdg_test/home/.local/state/myapp"); +} diff --git a/test/type/test_error.cpp b/test/type/test_error.cpp new file mode 100644 index 0000000..352a048 --- /dev/null +++ b/test/type/test_error.cpp @@ -0,0 +1,102 @@ +#include + +#include "dross/type/error.h" + +#include +#include +#include +#include +#include + +TEST(error_test, default_constructed_error_is_falsy) +{ + const dross::error e; + + EXPECT_FALSE(static_cast(e)); +} + +TEST(error_test, constructed_from_an_error_code_enum_is_truthy) +{ + // io_errc is a genuine std::is_error_code_enum type (unlike std::errc, + // which is an error_condition enum), so it exercises the templated + // constructor the way it is actually meant to be used. + const dross::error e(std::io_errc::stream); + + EXPECT_TRUE(static_cast(e)); +} + +TEST(error_test, code_and_message_reflect_the_underlying_error_code) +{ + const auto ec = std::make_error_code(std::io_errc::stream); + const dross::error e(std::io_errc::stream); + + EXPECT_EQ(e.code(), ec.value()); + EXPECT_EQ(e.message(), ec.message()); + EXPECT_EQ(e.domain(), ec.category().name()); +} + +TEST(error_test, category_and_condition_match_the_error_code_enum) +{ + const auto ec = std::make_error_code(std::io_errc::stream); + const dross::error e(std::io_errc::stream); + + EXPECT_EQ(e.category(), std::iostream_category()); + EXPECT_EQ(e.condition(), ec.default_error_condition()); +} + +TEST(error_test, equality_operator_compares_against_the_error_code_enum) +{ + const dross::error e(std::io_errc::stream); + + EXPECT_TRUE(e == std::io_errc::stream); + EXPECT_FALSE(e != std::io_errc::stream); +} + +TEST(error_test, equality_operator_compares_against_the_error_category) +{ + const dross::error e(std::io_errc::stream); + + EXPECT_TRUE(e == std::iostream_category()); + EXPECT_FALSE(e == std::generic_category()); + EXPECT_TRUE(e != std::generic_category()); +} + +TEST(error_test, copy_constructor_preserves_the_underlying_code) +{ + const dross::error original(1, std::generic_category()); + const dross::error copy(original); + + EXPECT_EQ(copy.code(), original.code()); + EXPECT_EQ(copy.domain(), original.domain()); +} + +TEST(error_test, three_way_comparison_orders_by_the_underlying_error_code) +{ + // dross::error has no operator==(const error&) of its own (only the + // category/enum overloads), so equality here is checked through <=>. + const dross::error smaller(0, std::generic_category()); + const dross::error larger(1, std::generic_category()); + + EXPECT_TRUE(smaller < larger); + EXPECT_TRUE(larger > smaller); + EXPECT_TRUE((smaller <=> smaller) == std::strong_ordering::equal); +} + +TEST(error_test, stream_insertion_writes_domain_and_code) +{ + const dross::error e(1, std::generic_category()); + + std::ostringstream out; + out << e; + + const std::string expected = e.domain() + ":" + std::to_string(e.code()); + EXPECT_EQ(out.str(), expected); +} + +TEST(error_test, value_and_category_constructor) +{ + const dross::error e(5, std::generic_category()); + + EXPECT_EQ(e.code(), 5); + EXPECT_EQ(e.category(), std::generic_category()); +} From 1bfc758b9ad9a17e433118e7e8b2c4888c767a70 Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 20 Aug 2026 12:33:36 +0900 Subject: [PATCH 3/9] docs: describe what path now does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pages and the header comments were brought in line with the previous behaviour, so they now describe something that is no longer true: that mkdir() reports an already-present directory as a failure carrying a zero code(), and that expand() lets a filesystem_error escape. Rewrite both claims where they appear, and drop the caller-side idiom they required — an else branch guarded on error().code() being nonzero is no longer needed when an existing directory is a success. Note the two things that follow from the new mkdir(): it is closer to "ensure this directory exists" than to a strict create, and it accepts a directory or symbolic link another party left in place, so a caller who cares about ownership has to check for itself. The sentence in each header that grouped expand() with exists() and the default constructor keeps the other two: both still call the throwing std::filesystem functions. --- docs/sphinx/source/api/index.rst | 4 -- docs/sphinx/source/api/platform.rst | 57 +++++++++++++------------ docs/sphinx/source/changelog.rst | 6 +++ docs/sphinx/source/user-guide/index.rst | 7 ++- include/dross/platform.h | 6 +-- include/dross/platform/path.h | 23 ++++++---- include/dross/platform/xdg.h | 3 +- 7 files changed, 57 insertions(+), 49 deletions(-) diff --git a/docs/sphinx/source/api/index.rst b/docs/sphinx/source/api/index.rst index 7da6b11..4fb3686 100644 --- a/docs/sphinx/source/api/index.rst +++ b/docs/sphinx/source/api/index.rst @@ -53,10 +53,6 @@ Some operations are exceptions to that rule: ``array::operator[]`` and ``array::value_at()`` — throw ``std::out_of_range`` when the key or index is not present. Ask ``dictionary::contains()`` or ``array::length()`` before indexing. -- ``path::expand()``, despite returning ``std::expected``, lets a - ``std::filesystem::filesystem_error`` escape for any canonicalisation - failure on a ``~`` path. ``path::resolve()`` catches those and returns - them. - ``path``'s ``exists()`` calls the throwing form of ``std::filesystem::exists``, so an error while querying the path — as opposed to the path simply being absent — escapes as a diff --git a/docs/sphinx/source/api/platform.rst b/docs/sphinx/source/api/platform.rst index 67bf0bf..fddb94c 100644 --- a/docs/sphinx/source/api/platform.rst +++ b/docs/sphinx/source/api/platform.rst @@ -58,12 +58,12 @@ The ``path`` class provides filesystem path operations: dross::path config_path = home->append(".config").append("app"); std::cout << "Config path: " << config_path.string() << std::endl; - // Create the directory, including any missing parents. mkdir() - // reports failure when the directory is already there, so tell that - // case apart by its code(): zero means it was already present. + // Create the directory, including any missing parents. mkdir() is + // idempotent: it succeeds whether it creates the directory or + // finds it already there. if (auto created = dross::path::mkdir(config_path.string())) { std::cout << "Created: " << created->string() << std::endl; - } else if (created.error().code()) { + } else { std::cerr << "mkdir: " << created.error().what() << std::endl; } } @@ -85,12 +85,15 @@ The ``path`` class provides filesystem path operations: } // Expand a leading ~ to the home directory. For a ~ path, expand() - // canonicalises without catching, so any canonicalisation failure -- - // a missing target, a permission problem, a symlink loop -- escapes as - // a filesystem_error instead of being returned. resolve() catches it. + // canonicalises and turns any failure -- a missing target, a + // permission problem, a symlink loop -- into the returned + // std::expected instead of letting it escape. A path that does not + // begin with ~ is returned unchanged. dross::path user_config{std::string{"~/.config/app"}}; if (auto expanded = user_config.expand()) { std::cout << "Expanded: " << expanded->string() << std::endl; + } else { + std::cerr << "Expand failed: " << expanded.error().what() << std::endl; } Path Operations @@ -120,20 +123,20 @@ Operations that consult the filesystem: Some caveats apply to the current implementation: -- ``mkdir()`` succeeds only when it actually creates the directory. If the - path already exists it returns an error, but one whose ``code()`` is zero, - so a caller can tell it apart from a real filesystem failure, which - carries a nonzero code. Testing with ``exists()`` beforehand is not a - better answer — see below, and it races with other processes anyway. -- ``expand()`` does not route every failure through its return type. For a - path beginning with ``~`` it canonicalises without catching, so *any* - canonicalisation failure — a missing target, a permission problem, a - symlink loop, an invalid component — escapes as a - ``std::filesystem::filesystem_error`` instead of being returned, which - terminates a program that is not catching it. A path that does not begin - with ``~`` is returned unchanged and never throws. ``resolve()`` catches - the same failures and returns them, so prefer it when the target may not - be reachable. +- ``mkdir()`` is idempotent: it succeeds whether it creates the directory + or finds it already there. It fails only when + ``std::filesystem::create_directories`` reports an actual error, such as + a path component that exists and is not a directory. Because an + already-present directory is accepted without inspection, a directory or + symbolic link left there by another party is accepted too — verify + ownership or the link target first if that matters to your use. +- ``expand()`` routes canonicalisation failures through its return type. + For a path beginning with ``~`` it canonicalises and converts any + failure — a missing target, a permission problem, a symlink loop, an + invalid component — into the returned ``std::expected`` rather than + letting it escape. A path that does not begin with ``~`` is returned + unchanged. Home directory resolution failing (``path::home()`` returning + ``std::nullopt``) is reported the same way. - ``exists()`` calls the throwing form of ``std::filesystem::exists``. An absent path is simply ``false``, but an error while querying it — an over-long name, or a directory the process may not traverse — escapes as a @@ -197,9 +200,8 @@ the application name passed to the constructor: Every accessor returns ``std::optional`` and yields ``std::nullopt`` when the home directory cannot be determined. The directory -itself is not created for you — pass the result to ``path::mkdir()``, keeping -in mind that ``mkdir()`` reports an already-present directory as an error, -recognisable by its zero ``code()``. +itself is not created for you — pass the result to ``path::mkdir()``, which +accepts an already-present directory as success. Example Usage ~~~~~~~~~~~~~ @@ -216,12 +218,11 @@ Creating application directories: dross::xdg app{"myapp"}; // Create the config directory, then name a file inside it. mkdir() - // reports an already-present directory as a failure too, but with a - // zero code(), so only a nonzero one is a real problem. + // accepts an already-present directory as success, so any failure + // here is a real problem. if (auto config_home = app.config_home()) { const dross::path config_dir{*config_home}; - if (auto created = dross::path::mkdir(*config_home); - !created && created.error().code()) { + if (auto created = dross::path::mkdir(*config_home); !created) { std::cerr << "mkdir: " << created.error().what() << std::endl; } dross::path config_file = config_dir.append("settings.toml"); diff --git a/docs/sphinx/source/changelog.rst b/docs/sphinx/source/changelog.rst index 43e9f10..27605ef 100644 --- a/docs/sphinx/source/changelog.rst +++ b/docs/sphinx/source/changelog.rst @@ -29,6 +29,12 @@ Changed - Enhanced error handling: ``timezone::from_string()`` returns ``std::optional`` - Simplified API: removed redundant timezone methods (``is_local()``, ``has_offset()``) - Updated documentation to reflect timestamp and timezone APIs +- ``path::mkdir()`` now succeeds when the target directory already exists + instead of reporting it as a failure with a zero ``code()``; it fails + only when the underlying filesystem operation reports an actual error +- ``path::expand()`` now reports canonicalisation failures on a ``~`` path + through its ``std::expected`` return value instead of letting a + ``std::filesystem::filesystem_error`` escape v0.1.0 - 2024-01-20 ------------------- diff --git a/docs/sphinx/source/user-guide/index.rst b/docs/sphinx/source/user-guide/index.rst index dfcdbe9..7fca7c2 100644 --- a/docs/sphinx/source/user-guide/index.rst +++ b/docs/sphinx/source/user-guide/index.rst @@ -155,9 +155,8 @@ Platform Utilities xdg app{"myapp"}; const std::string app_data = app.data_home().value_or(config_path.string()); - // Create the directory. mkdir() succeeds only when it actually creates - // one, so an already-present directory comes back as an error too -- - // but with a zero code(), unlike a real failure. - if (auto result = path::mkdir(app_data); !result && result.error().code()) { + // Create the directory. mkdir() is idempotent: it succeeds whether it + // creates the directory or finds it already there. + if (auto result = path::mkdir(app_data); !result) { std::cerr << "mkdir: " << result.error().what() << std::endl; } diff --git a/include/dross/platform.h b/include/dross/platform.h index 2758168..5908415 100644 --- a/include/dross/platform.h +++ b/include/dross/platform.h @@ -26,7 +26,7 @@ * // Filesystem operations * path config_dir = path::home().value_or(path{"/tmp"}) / "myapp"; * if (auto result = path::mkdir(config_dir.string()); result) { - * // Directory created successfully + * // Directory created, or already there * } * * // XDG directories @@ -40,8 +40,8 @@ * - Uses std::optional for operations that may not return a value * - Uses std::expected for operations that may fail with detailed error info * - The platform layer throws nothing of its own, but std::filesystem - * exceptions do propagate: see path::exists(), path::expand() and the - * default path constructor + * exceptions do propagate: see path::exists() and the default path + * constructor * * Platform support: * - Unix-like systems (Linux, macOS, BSD) diff --git a/include/dross/platform/path.h b/include/dross/platform/path.h index 94deb4a..d5337b1 100644 --- a/include/dross/platform/path.h +++ b/include/dross/platform/path.h @@ -26,8 +26,8 @@ namespace dross { * - Uses std::expected for fallible operations * - Uses std::optional for operations that may not return a value * - No exceptions thrown directly, but std::filesystem ones propagate: - * exists(), expand() on a ~ path, and the default constructor all call - * throwing std::filesystem functions + * exists() and the default constructor call throwing std::filesystem + * functions * * Performance characteristics: * - Thin wrapper over std::filesystem with minimal overhead @@ -71,10 +71,10 @@ class path { * @return Expected containing the created path on success, or filesystem_error on failure * * Creates the specified directory and any necessary parent directories. - * Succeeds only when a directory is actually created: if dir_path is - * already present the call reports failure. That case is still - * recognisable — the reported error's code() is zero, whereas a real - * filesystem failure carries a nonzero code. + * Succeeds both when it creates the directory and when dir_path is + * already present — the call is idempotent. It fails only when the + * underlying std::filesystem::create_directories call reports an + * actual error. * * @code * if (auto result = path::mkdir("/tmp/myapp/data")) { @@ -92,9 +92,14 @@ class path { * @return Expected containing the created path on success, or filesystem_error on failure * * Creates the specified directory and any necessary parent directories. - * Succeeds only when a directory is actually created: if dir_path is - * already present the call reports failure, with an error whose code() - * is zero; a real filesystem failure carries a nonzero code. This + * Succeeds both when it creates the directory and when dir_path already + * exists — the call is idempotent, closer to "ensure this directory + * exists" than a strict create. It fails only when + * std::filesystem::create_directories reports an actual error, for + * example when a path component exists and is not a directory. Because + * an already-present directory is accepted without inspection, a + * directory or symbolic link left there by another party is accepted + * too; verify ownership or the link target first if that matters. This * overload holds the logic; the std::string one forwards to it. */ static std::expected mkdir(const std::filesystem::path& dir_path); diff --git a/include/dross/platform/xdg.h b/include/dross/platform/xdg.h index eaa9a01..f7f9921 100644 --- a/include/dross/platform/xdg.h +++ b/include/dross/platform/xdg.h @@ -99,7 +99,8 @@ class xdg { * // Typically returns something like "/home/user/.config/myapp" * path config_path{*config_dir}; * if (auto result = path::mkdir(config_path.string())) { - * // Directory created and ready for config files + * // Directory ready for config files, whether just created or + * // already there * } * } * @endcode From 39efe36d5a9fc4e0b4c07a2542b86642b697babb Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 20 Aug 2026 12:56:09 +0900 Subject: [PATCH 4/9] docs: narrow the claims about mkdir and expand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wording that landed with the fixes said more than what was measured. mkdir() was described as succeeding when the target "is already present", but a regular file already sitting at that path is a failure, and a test pins that. Say "already a directory". The note about accepting what another party left in place said "a directory or symbolic link"; a dangling symbolic link is refused with EEXIST. Say "a symbolic link that resolves to one". The same note told the caller to verify ownership or the link target first, which recommends a check that cannot close the gap it warns about — the check and the use are separate operations — and which this library gives no way to perform. Replace the advice with the reason the gap stays open. expand() was said to convert "any failure", but only what the standard library reports as a filesystem_error is caught. expand()'s own comment never said what a failure is, though it is the call whose failure behaviour changed: canonicalisation needs the target to exist, so a path that has not been created yet comes back as unexpected. Say so, and let the example show the branch. Also note in mkdir() why the return value of create_directories() is discarded, add the POSIX header the environment helpers rely on, and make the non-terminating expand() test assert that home() resolved, so it cannot pass through the other branch without reaching the call it guards. --- docs/sphinx/source/api/platform.rst | 23 +++++++++++++-------- include/dross/platform/path.h | 32 +++++++++++++++++++---------- src/platform/path.cpp | 3 +++ test/platform/test_path.cpp | 13 +++++++++++- test/platform/test_xdg.cpp | 1 + 5 files changed, 51 insertions(+), 21 deletions(-) diff --git a/docs/sphinx/source/api/platform.rst b/docs/sphinx/source/api/platform.rst index fddb94c..fb61a08 100644 --- a/docs/sphinx/source/api/platform.rst +++ b/docs/sphinx/source/api/platform.rst @@ -85,7 +85,8 @@ The ``path`` class provides filesystem path operations: } // Expand a leading ~ to the home directory. For a ~ path, expand() - // canonicalises and turns any failure -- a missing target, a + // canonicalises and turns any failure the standard library reports as + // a std::filesystem::filesystem_error -- a missing target, a // permission problem, a symlink loop -- into the returned // std::expected instead of letting it escape. A path that does not // begin with ~ is returned unchanged. @@ -127,15 +128,19 @@ Some caveats apply to the current implementation: or finds it already there. It fails only when ``std::filesystem::create_directories`` reports an actual error, such as a path component that exists and is not a directory. Because an - already-present directory is accepted without inspection, a directory or - symbolic link left there by another party is accepted too — verify - ownership or the link target first if that matters to your use. + already-present directory is accepted without inspection, a directory, + or a symbolic link that resolves to one, left there by another party is + accepted too. Checking beforehand does not close that gap — the check + and the use are separate operations, and the entry can be replaced in + between; create the directory under a parent only you can write to + instead. - ``expand()`` routes canonicalisation failures through its return type. - For a path beginning with ``~`` it canonicalises and converts any - failure — a missing target, a permission problem, a symlink loop, an - invalid component — into the returned ``std::expected`` rather than - letting it escape. A path that does not begin with ``~`` is returned - unchanged. Home directory resolution failing (``path::home()`` returning + For a path beginning with ``~`` it canonicalises and converts any error + the standard library reports as a ``std::filesystem::filesystem_error`` + — a missing target, a permission problem, a symlink loop, an invalid + component — into the returned ``std::expected`` rather than letting it + escape. A path that does not begin with ``~`` is returned unchanged. + Home directory resolution failing (``path::home()`` returning ``std::nullopt``) is reported the same way. - ``exists()`` calls the throwing form of ``std::filesystem::exists``. An absent path is simply ``false``, but an error while querying it — an diff --git a/include/dross/platform/path.h b/include/dross/platform/path.h index d5337b1..4da493b 100644 --- a/include/dross/platform/path.h +++ b/include/dross/platform/path.h @@ -72,7 +72,7 @@ class path { * * Creates the specified directory and any necessary parent directories. * Succeeds both when it creates the directory and when dir_path is - * already present — the call is idempotent. It fails only when the + * already a directory — the call is idempotent. It fails only when the * underlying std::filesystem::create_directories call reports an * actual error. * @@ -92,15 +92,18 @@ class path { * @return Expected containing the created path on success, or filesystem_error on failure * * Creates the specified directory and any necessary parent directories. - * Succeeds both when it creates the directory and when dir_path already - * exists — the call is idempotent, closer to "ensure this directory - * exists" than a strict create. It fails only when + * Succeeds both when it creates the directory and when dir_path is + * already a directory — the call is idempotent, closer to "ensure this + * directory exists" than a strict create. It fails only when * std::filesystem::create_directories reports an actual error, for * example when a path component exists and is not a directory. Because * an already-present directory is accepted without inspection, a - * directory or symbolic link left there by another party is accepted - * too; verify ownership or the link target first if that matters. This - * overload holds the logic; the std::string one forwards to it. + * directory, or a symbolic link that resolves to one, left there by + * another party is accepted too. Checking beforehand does not close + * that gap — the check and the use are separate operations, and the + * entry can be replaced in between; create the directory under a + * parent only you can write to instead. This overload holds the + * logic; the std::string one forwards to it. */ static std::expected mkdir(const std::filesystem::path& dir_path); @@ -205,14 +208,21 @@ class path { /** * @brief Expand user home directory (~) in the path. * @return Expected containing the expanded path on success, or filesystem_error on failure - * - * Expands tilde (~) notation to the actual home directory path. - * Only processes paths that start with "~" or "~/". - * + * + * Expands tilde (~) notation to the actual home directory path, then + * canonicalises the result — the returned path has symbolic links + * resolved. Because canonicalisation requires the target to exist, + * expand() returns unexpected when the expanded path does not (yet) + * exist. A path that does not start with "~" is returned unchanged + * and always succeeds. + * * @code * path user_config{"~/.config/myapp"}; * if (auto expanded = user_config.expand()) { * // expanded contains something like "/home/user/.config/myapp" + * } else { + * // The expanded path does not exist yet: create it first, e.g. + * // with path::mkdir(), before calling expand() again. * } * @endcode */ diff --git a/src/platform/path.cpp b/src/platform/path.cpp index 72e6e2a..39ab4e4 100644 --- a/src/platform/path.cpp +++ b/src/platform/path.cpp @@ -15,6 +15,9 @@ std::expected path::mkdir(const std::st std::expected path::mkdir(const std::filesystem::path& absolute_path) { std::error_code err; + // create_directories() returns false both when the directory already + // existed and when it failed to create one, so the return value alone + // cannot tell success from failure; check err instead. std::filesystem::create_directories(absolute_path, err); if (err) { return std::unexpected(std::filesystem::filesystem_error("failed", absolute_path, err)); diff --git a/test/platform/test_path.cpp b/test/platform/test_path.cpp index 61702ed..2ac1dec 100644 --- a/test/platform/test_path.cpp +++ b/test/platform/test_path.cpp @@ -6,7 +6,9 @@ #include #include #include +#include #include +#include #include namespace { @@ -71,6 +73,11 @@ class scoped_env_var { setenv(_name.c_str(), value.c_str(), 1); } + void unset() const + { + unsetenv(_name.c_str()); + } + private: std::string _name; std::optional _original; @@ -212,7 +219,11 @@ TEST(path_test, expand_of_a_tilde_path_to_a_nonexistent_target_does_not_terminat // Regression test: std::filesystem::canonical() throws on a missing // path. expand() must report that as std::unexpected instead of // letting the exception escape, which previously terminated the - // process. + // process. Guard that the regression path is actually exercised: if + // home() were nullopt, expand() would fail through the other branch + // and this test would pass without ever reaching canonical(). + ASSERT_TRUE(dross::path::home().has_value()); + const std::string tilde_path = "~/dross_test_nonexistent_" + std::to_string(::getpid()); const dross::path p{tilde_path}; diff --git a/test/platform/test_xdg.cpp b/test/platform/test_xdg.cpp index b6f2c85..f9038f8 100644 --- a/test/platform/test_xdg.cpp +++ b/test/platform/test_xdg.cpp @@ -4,6 +4,7 @@ #include #include +#include #include namespace { From ee0567c1f58f1e1fdd315226f31dc91c93d6f62c Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 20 Aug 2026 13:18:18 +0900 Subject: [PATCH 5/9] docs: say what makes the mkdir gap close, and what does not The note added with the previous change told the caller to create the directory under a parent only they can write to. Read against the sentence before it, that reads as sufficient, and it is not: a writable ancestor can be renamed or replaced, taking the parent with it. Put the condition on the whole chain of ancestors instead. Two more places said more than they should. mkdir() does not roll back the directories it managed to create before a failure, which the caller has to clean up; the standard does not promise either way, so say they may remain rather than that they are left behind. And the example under expand() named a missing target as the reason its else branch runs, when a permission problem, a symlink loop or an unresolvable home directory reach it too. --- docs/sphinx/source/api/platform.rst | 19 +++++++++------- include/dross/platform/path.h | 35 ++++++++++++++++------------- 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/docs/sphinx/source/api/platform.rst b/docs/sphinx/source/api/platform.rst index fb61a08..919bad9 100644 --- a/docs/sphinx/source/api/platform.rst +++ b/docs/sphinx/source/api/platform.rst @@ -124,16 +124,19 @@ Operations that consult the filesystem: Some caveats apply to the current implementation: -- ``mkdir()`` is idempotent: it succeeds whether it creates the directory - or finds it already there. It fails only when +- ``mkdir()`` is idempotent: it succeeds whether it creates + the directory or finds it already there. It fails only when ``std::filesystem::create_directories`` reports an actual error, such as - a path component that exists and is not a directory. Because an + a path component that exists and is not a directory. The operation is + not atomic — directories created before the failure may remain. Some + failures are rejected before anything is created at all. Because an already-present directory is accepted without inspection, a directory, - or a symbolic link that resolves to one, left there by another party is - accepted too. Checking beforehand does not close that gap — the check - and the use are separate operations, and the entry can be replaced in - between; create the directory under a parent only you can write to - instead. + or a symbolic link that resolves to one, left there by another party + is accepted too. Checking beforehand does not close that gap — the + check and the use are separate operations, and the entry can be replaced + in between. What closes it is a location where no other party can write + to any ancestor of the directory: a writable ancestor can be renamed or + replaced, so securing only the immediate parent is not enough. - ``expand()`` routes canonicalisation failures through its return type. For a path beginning with ``~`` it canonicalises and converts any error the standard library reports as a ``std::filesystem::filesystem_error`` diff --git a/include/dross/platform/path.h b/include/dross/platform/path.h index 4da493b..3cbd2c2 100644 --- a/include/dross/platform/path.h +++ b/include/dross/platform/path.h @@ -91,19 +91,24 @@ class path { * @param dir_path The directory path to create as a filesystem::path * @return Expected containing the created path on success, or filesystem_error on failure * - * Creates the specified directory and any necessary parent directories. - * Succeeds both when it creates the directory and when dir_path is - * already a directory — the call is idempotent, closer to "ensure this - * directory exists" than a strict create. It fails only when - * std::filesystem::create_directories reports an actual error, for - * example when a path component exists and is not a directory. Because - * an already-present directory is accepted without inspection, a - * directory, or a symbolic link that resolves to one, left there by - * another party is accepted too. Checking beforehand does not close - * that gap — the check and the use are separate operations, and the - * entry can be replaced in between; create the directory under a - * parent only you can write to instead. This overload holds the - * logic; the std::string one forwards to it. + * Creates the specified directory and any necessary parent + * directories. Succeeds both when it creates the directory and + * when dir_path is already a directory — the call is idempotent, + * closer to "ensure this directory exists" than a strict create. It + * fails only when std::filesystem::create_directories reports an + * actual error, for example when a path component exists and is not + * a directory. The operation is not atomic — directories created + * before the failure may remain. Some failures are rejected before + * anything is created at all. Because an already-present directory + * is accepted without inspection, a directory, or a symbolic link + * that resolves to one, left there by another party is accepted + * too. Checking beforehand does not close that gap — the check and + * the use are separate operations, and the entry can be replaced in + * between. What closes it is a location where no other party can + * write to any ancestor of the directory: a writable ancestor can + * be renamed or replaced, so securing only the immediate parent is + * not enough. This overload holds the logic; the std::string one + * forwards to it. */ static std::expected mkdir(const std::filesystem::path& dir_path); @@ -221,8 +226,8 @@ class path { * if (auto expanded = user_config.expand()) { * // expanded contains something like "/home/user/.config/myapp" * } else { - * // The expanded path does not exist yet: create it first, e.g. - * // with path::mkdir(), before calling expand() again. + * // The expanded path typically does not exist yet: create it + * // first, e.g. with path::mkdir(), before calling expand() again. * } * @endcode */ From 46c9d84e068345309d70c84bc5a0e0946ba836d3 Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 20 Aug 2026 13:36:10 +0900 Subject: [PATCH 6/9] docs: separate the race from the trust, and make the example compile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The note said what closes the gap, and named one thing: a location whose ancestors no other party can write to. That closes the race — nothing can be slipped in while the call runs — but it says nothing about a directory that was already sitting there when the condition came to hold. Its owner, its mode, the links under it: none of that follows from securing the chain afterwards. Say which half each measure covers, and that the call asks nothing about the other. expand()'s example initialised a path from a bare string literal, which is ambiguous between the two converting constructors and does not compile — the same trap the pages document, and which the copy of this example over there already avoids. Name the type, as the other copy does. The string overload sent readers nowhere: every mkdir() call in the pages and headers goes through it, while the failure and safety notes live on the filesystem::path one. Point at it. Cover the symbolic link the note now claims mkdir() accepts. --- docs/sphinx/source/api/platform.rst | 10 +++++++--- include/dross/platform/path.h | 19 ++++++++++++------- test/platform/test_path.cpp | 16 ++++++++++++++++ 3 files changed, 35 insertions(+), 10 deletions(-) diff --git a/docs/sphinx/source/api/platform.rst b/docs/sphinx/source/api/platform.rst index 919bad9..273c4f6 100644 --- a/docs/sphinx/source/api/platform.rst +++ b/docs/sphinx/source/api/platform.rst @@ -134,9 +134,13 @@ Some caveats apply to the current implementation: or a symbolic link that resolves to one, left there by another party is accepted too. Checking beforehand does not close that gap — the check and the use are separate operations, and the entry can be replaced - in between. What closes it is a location where no other party can write - to any ancestor of the directory: a writable ancestor can be renamed or - replaced, so securing only the immediate parent is not enough. + in between. Placing the directory where no other party can write to any + ancestor removes the opportunity to insert or replace an entry along the + way; a writable ancestor can be renamed or replaced, so securing only + the immediate parent is not enough. That does not vouch for a directory + that is already there: whether its owner, its permissions and any links + beneath it can be trusted is a separate question, and this call does + not ask it. - ``expand()`` routes canonicalisation failures through its return type. For a path beginning with ``~`` it canonicalises and converts any error the standard library reports as a ``std::filesystem::filesystem_error`` diff --git a/include/dross/platform/path.h b/include/dross/platform/path.h index 3cbd2c2..6f93736 100644 --- a/include/dross/platform/path.h +++ b/include/dross/platform/path.h @@ -74,7 +74,8 @@ class path { * Succeeds both when it creates the directory and when dir_path is * already a directory — the call is idempotent. It fails only when the * underlying std::filesystem::create_directories call reports an - * actual error. + * actual error. See the std::filesystem::path overload for the + * failure and safety notes. * * @code * if (auto result = path::mkdir("/tmp/myapp/data")) { @@ -104,11 +105,15 @@ class path { * that resolves to one, left there by another party is accepted * too. Checking beforehand does not close that gap — the check and * the use are separate operations, and the entry can be replaced in - * between. What closes it is a location where no other party can - * write to any ancestor of the directory: a writable ancestor can - * be renamed or replaced, so securing only the immediate parent is - * not enough. This overload holds the logic; the std::string one - * forwards to it. + * between. Placing the directory where no other party can write to + * any ancestor removes the opportunity to insert or replace an + * entry along the way; a writable ancestor can be renamed or + * replaced, so securing only the immediate parent is not enough. + * That does not vouch for a directory that is already there: + * whether its owner, its permissions and any links beneath it can + * be trusted is a separate question, and this call does not ask + * it. This overload holds the logic; the std::string one forwards + * to it. */ static std::expected mkdir(const std::filesystem::path& dir_path); @@ -222,7 +227,7 @@ class path { * and always succeeds. * * @code - * path user_config{"~/.config/myapp"}; + * path user_config{std::string{"~/.config/myapp"}}; * if (auto expanded = user_config.expand()) { * // expanded contains something like "/home/user/.config/myapp" * } else { diff --git a/test/platform/test_path.cpp b/test/platform/test_path.cpp index 2ac1dec..787dec1 100644 --- a/test/platform/test_path.cpp +++ b/test/platform/test_path.cpp @@ -181,6 +181,22 @@ TEST(path_test, mkdir_with_an_empty_path) EXPECT_NE(result.error().code().value(), 0); } +TEST(path_test, mkdir_on_a_symlink_that_resolves_to_a_directory_succeeds) +{ + // Documented behavior: an already-present directory is accepted + // without inspection, and that includes a symbolic link that + // resolves to one. + const scoped_temp_dir base; + const auto real_dir = base.path() / "real_dir"; + std::filesystem::create_directories(real_dir); + const auto link = base.path() / "link_to_dir"; + std::filesystem::create_directory_symlink(real_dir, link); + + const auto result = dross::path::mkdir(link.string()); + + ASSERT_TRUE(result.has_value()); +} + // --- path::expand -------------------------------------------------------- TEST(path_test, expand_of_a_tilde_path_matches_a_freshly_computed_canonical_path) From 1030331a9f0cd93a910956232e2ebb02b68105c4 Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 20 Aug 2026 13:53:03 +0900 Subject: [PATCH 7/9] docs: say what mkdir does not do, and stop advising MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four attempts at this paragraph each named a way to be safe, and each was wrong in a different place: check the owner first (the check races), use a parent only you can write to (a writable ancestor takes the parent with it), secure the whole chain of ancestors (it says nothing about a directory that was already there), that removes the opportunity to insert an entry (the directories this call creates itself get the platform's default mode, which can be world-writable). The advice was never what the note was for. Drop it and state what the call does not do — it does not look at who owns the directories on the path, at their permissions, or at where the links under them go, and it does not set the mode of the ones it creates. A caller who needs any of that is on their own, which was the point all along. --- docs/sphinx/source/api/platform.rst | 19 +++++++++---------- include/dross/platform/path.h | 26 ++++++++++++-------------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/docs/sphinx/source/api/platform.rst b/docs/sphinx/source/api/platform.rst index 273c4f6..dc85488 100644 --- a/docs/sphinx/source/api/platform.rst +++ b/docs/sphinx/source/api/platform.rst @@ -131,16 +131,15 @@ Some caveats apply to the current implementation: not atomic — directories created before the failure may remain. Some failures are rejected before anything is created at all. Because an already-present directory is accepted without inspection, a directory, - or a symbolic link that resolves to one, left there by another party - is accepted too. Checking beforehand does not close that gap — the - check and the use are separate operations, and the entry can be replaced - in between. Placing the directory where no other party can write to any - ancestor removes the opportunity to insert or replace an entry along the - way; a writable ancestor can be renamed or replaced, so securing only - the immediate parent is not enough. That does not vouch for a directory - that is already there: whether its owner, its permissions and any links - beneath it can be trusted is a separate question, and this call does - not ask it. + or a symbolic link that resolves to one, left there by another party is + accepted too. Checking beforehand does not close that gap — the check + and the use are separate operations, and the entry can be replaced in + between. This call does not check who owns the directories along the + path, what their permissions are, or where any links beneath them point, + and it does not set the permissions of the directories it creates: those + are left to the platform's default for new directories, which can be + group- or world-writable. A caller who needs any of that has to arrange + it separately. - ``expand()`` routes canonicalisation failures through its return type. For a path beginning with ``~`` it canonicalises and converts any error the standard library reports as a ``std::filesystem::filesystem_error`` diff --git a/include/dross/platform/path.h b/include/dross/platform/path.h index 6f93736..42313aa 100644 --- a/include/dross/platform/path.h +++ b/include/dross/platform/path.h @@ -100,20 +100,18 @@ class path { * actual error, for example when a path component exists and is not * a directory. The operation is not atomic — directories created * before the failure may remain. Some failures are rejected before - * anything is created at all. Because an already-present directory - * is accepted without inspection, a directory, or a symbolic link - * that resolves to one, left there by another party is accepted - * too. Checking beforehand does not close that gap — the check and - * the use are separate operations, and the entry can be replaced in - * between. Placing the directory where no other party can write to - * any ancestor removes the opportunity to insert or replace an - * entry along the way; a writable ancestor can be renamed or - * replaced, so securing only the immediate parent is not enough. - * That does not vouch for a directory that is already there: - * whether its owner, its permissions and any links beneath it can - * be trusted is a separate question, and this call does not ask - * it. This overload holds the logic; the std::string one forwards - * to it. + * anything is created at all. Because an already-present directory is + * accepted without inspection, a directory, or a symbolic link that + * resolves to one, left there by another party is accepted too. + * Checking beforehand does not close that gap — the check and the use + * are separate operations, and the entry can be replaced in between. + * This call does not check who owns the directories along the path, + * what their permissions are, or where any links beneath them point, + * and it does not set the permissions of the directories it creates: + * those are left to the platform's default for new directories, which + * can be group- or world-writable. A caller who needs any of that has + * to arrange it separately. This overload holds the logic; the + * std::string one forwards to it. */ static std::expected mkdir(const std::filesystem::path& dir_path); From 7d32d124d90c5a6c81d5826569fd440aaec69d8d Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 20 Aug 2026 14:52:08 +0900 Subject: [PATCH 8/9] docs: stop telling the caller to mkdir a path that still has a tilde in it The else branch of expand()'s example said the target does not exist yet, so create it with mkdir() and call expand() again. That cannot be done in that order. mkdir() does not expand a tilde, so passing the same string to it makes a directory literally named "~" under the working directory, and expand() will not hand back a path to create until the path already exists. Point at the route that works: build it from home(). --- include/dross/platform/path.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/dross/platform/path.h b/include/dross/platform/path.h index 42313aa..2e4452b 100644 --- a/include/dross/platform/path.h +++ b/include/dross/platform/path.h @@ -229,8 +229,8 @@ class path { * if (auto expanded = user_config.expand()) { * // expanded contains something like "/home/user/.config/myapp" * } else { - * // The expanded path typically does not exist yet: create it - * // first, e.g. with path::mkdir(), before calling expand() again. + * // The target does not exist yet. mkdir() does not expand ~, + * // so build the path from path::home() before creating it. * } * @endcode */ From b86c28adccbca5e18f7f3a82fe148a914f5ebf96 Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 20 Aug 2026 14:58:34 +0900 Subject: [PATCH 9/9] docs: put back the hedge on why the else branch runs Rewriting these lines dropped the qualifier they had carried since the example gained an else branch. A missing target is the case worth showing, but a permission problem, a symlink loop or an unresolvable home directory reach the same branch, and the caveat on the page lists all of them. Frame it as the case being supposed rather than the one that holds. --- include/dross/platform/path.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/dross/platform/path.h b/include/dross/platform/path.h index 2e4452b..20e3429 100644 --- a/include/dross/platform/path.h +++ b/include/dross/platform/path.h @@ -229,8 +229,9 @@ class path { * if (auto expanded = user_config.expand()) { * // expanded contains something like "/home/user/.config/myapp" * } else { - * // The target does not exist yet. mkdir() does not expand ~, - * // so build the path from path::home() before creating it. + * // Suppose the target does not exist yet. mkdir() does not + * // expand ~, so build the path from path::home() before + * // creating it. * } * @endcode */