From 94935462bb778c11d1d9995f713d30060b35fc6e Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:34:21 +0800 Subject: [PATCH 1/2] Read the platform's own separator and root forms when resolving a path A Tupfile could name an output outside the build root on Windows and have it accepted. pup::path split on '/' alone, so `..\victim.txt` was one opaque component that was never `..`, and the containment check saw nothing to refuse -- the #385 failure with the separator swapped. Three more spellings escaped the same way, unrecognised as rooted at all: the UNC `\\host\share\victim.txt`, whose target is another machine; the drive-relative `C:victim.txt`, which carries no separator to fix; and the root-relative `\victim.txt`. The module already knew half of this: root_length has read `C:\` as a drive root since it was written, while every splitter beside it read only '/'. The fix is to stop disagreeing -- on Windows the separator alphabet is both bytes and the root forms are the platform's, so the containment predicate refuses all four spellings without being touched. src/graph/builder.cpp is not in this diff. POSIX is unchanged, where a backslash is an ordinary character in a filename and accepting `..\victim.txt` is correct; both halves are pinned, because a rule pinned on one platform reads as a defect report against the other. The same disagreement recorded a nested output as one component while Windows created two on disk, so the record's key and the file it named were different paths. That is repaired here too, with its own pin and its own discharge. Teaching root_length the separator-less root `C:` then split the module against itself a second time, one function over: filename, parent and join locate the root by searching for a separator byte, which every other root contains and that one does not. filename returned the whole `C:a`, parent returned nothing where `/a` yields `/`, and join emitted `C:/a` -- silently promoting a drive-relative path to a drive-absolute one. The two splitters now clamp to root_length, and join shares append_root's predicate rather than restating it, so the two cannot drift apart. stem and extension inherit through filename, which a pin proves rather than assumes. Rejected: rejecting backslash paths in the lexer instead. That guards one door of a many-door room -- env-carried values and generated rule text reach the module without passing it -- and it does not repair the recorded-key defect at all. INDEX_VERSION 22 -> 23: recorded path spellings change meaning on Windows. Verified by cross-compiling the suite with clang-cl and running it under wine: every pin was observed failing against the pre-fix module and then passing, including the drive-relative promotion. That is a real Windows binary executing real Win32 path semantics, but it is wine rather than Windows, and it is the one place here where the evidence is a stand-in for the thing itself. The POSIX pins were shown to discriminate by implementing the naive fix and watching them fail. Linux 170755 assertions in 844 cases; Windows 170415 in 436 under the tag filter CI uses; format, tidy, iwyu and spec-check each at exit 0. A share-aware UNC root, which would stop `..` popping the host and share, was built first and refuted by the module's own laws test: `//a/.` makes "." the share and normalize emits a path that is not in normal form. The result stays rooted and therefore stays refused, so the residue is the spelling of an already-rejected path; it is filed as #411 with that input as the constraint. Written by the team's builder across two phases against an architect's ruling, with three cross-model reviews. The third found that the split/rejoin law added here does not catch the drive-relative regression on its own -- parent's empty result and join's empty-left arm cancel exactly -- and the added assertion that an absolute path's parent stays absolute is what closes it. Both halves were proven by probe. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014wx1bWwRf23eFT82y9D641 --- include/pup/index/format.hpp | 2 +- spec/requirements/output-paths.ears.md | 13 ++ src/core/path.cpp | 65 +++++++--- test/unit/test_builder.cpp | 81 ++++++++++++ test/unit/test_path.cpp | 169 ++++++++++++++++++++++++- 5 files changed, 310 insertions(+), 20 deletions(-) diff --git a/include/pup/index/format.hpp b/include/pup/index/format.hpp index e334c683..ee0c14d8 100644 --- a/include/pup/index/format.hpp +++ b/include/pup/index/format.hpp @@ -89,7 +89,7 @@ inline constexpr auto INDEX_MAGIC = std::array { 'P', 'U', 'P', 'I' }; /// `RawFileEntry::name_offset` is semantics-bearing on the same terms: `read_prior_paths` composes /// it into the paths `clean`/`distclean` delete and `reject_shadowed_sources` refuses a build over, /// so a name this reader cannot reproduce fails the record rather than reading as empty (#381). -inline constexpr auto INDEX_VERSION = std::uint32_t { 22 }; +inline constexpr auto INDEX_VERSION = std::uint32_t { 23 }; /// The oldest version whose `RawHeader` and `RawFileEntry` bytes mean what today's mean, so a /// record that old still says which paths it recorded as sources and which as generated even diff --git a/spec/requirements/output-paths.ears.md b/spec/requirements/output-paths.ears.md index 7ec9701f..d86d46c2 100644 --- a/spec/requirements/output-paths.ears.md +++ b/spec/requirements/output-paths.ears.md @@ -17,6 +17,15 @@ requirement. Upstream tup answers both in its parser, which is the enforcement p follows — establishing the rule once, where the path is read, rather than at each site that later acts on a recorded path. +"Resolves" is answered by the platform's own path law, which is the only sense the phrase ever had. +A backslash separates on Windows and is an ordinary character in a POSIX filename, so `..\victim.txt` +names a location above the build root on one and a single file on the other, and both answers are +this area's (issue #388). The same divergence already held for `C:\victim.txt`; what #388 added was +the spellings that carry no `/` at all — `..\`, the UNC `\\host\share\`, and the drive-relative +`C:victim.txt` — each of which reached the record unrecognised until `pup::path` was taught the +separator and root forms the platform's own file APIs already use. Both sides of the divergence are +pinned, because a rule pinned on one platform reads as a defect report against the other. + The second is which path is recorded. Every reader downstream — the deletion pass, the source-shadow guard, the record's own source/generated split — compares recorded paths against each other, and two spellings of one file defeat all of them. So an output is recorded by where it lands, not by how it @@ -38,6 +47,9 @@ How far out of the tree a rule may write. - discharge: test "GraphBuilder rejects an output above the build root under an unsatisfied guard" - discharge: test "GraphBuilder rejects an absolute output path" - discharge: test "Scenario: A rule writing above the build root fails the build instead of overwriting the file there" +- discharge: test "GraphBuilder rejects a backslash escape on Windows and names the file on POSIX" +- discharge: test "GraphBuilder rejects a UNC output on Windows and names the file on POSIX" +- discharge: test "GraphBuilder rejects a drive-relative output on Windows and names the file on POSIX" If a rule declares an output whose path does not resolve to a location inside the build root, then putup shall reject the Tupfile and name that path, whether or not the rule's guards are satisfied. @@ -52,6 +64,7 @@ Which of a path's spellings the record carries. - reference: upstream resolves each path element to a directory node rather than comparing strings, which has the same effect; not read closely enough to claim equivalence - discharge: test "GraphBuilder accepts a parent reference that stays inside the tree" - discharge: test "Scenario: A parent reference inside the tree is recorded canonically" +- discharge: test "GraphBuilder records a backslash output as two components on Windows and one on POSIX" While recording a rule's output, putup shall record the path the output resolves to rather than the path as the rule spelled it. diff --git a/src/core/path.cpp b/src/core/path.cpp index 9f6f4d38..8014db1e 100644 --- a/src/core/path.cpp +++ b/src/core/path.cpp @@ -17,30 +17,56 @@ namespace pup::path { namespace { -// The prefix `..` cannot escape and normalization reproduces verbatim: 1 for "/", 3 for "C:/". +// Windows spells a separator both ways; POSIX spells names with either byte, so `\` is a name byte. +#ifdef _WIN32 +constexpr auto separators = std::string_view { "/\\" }; +#else +constexpr auto separators = std::string_view { "/" }; +#endif + +auto is_separator(char c) -> bool +{ + return separators.find(c) != std::string_view::npos; +} + +// The prefix `..` cannot escape and normalization reproduces verbatim: 1 "/", 3 "C:/", 2 "//" or "C:". auto root_length(std::string_view p) -> std::size_t { if (p.empty()) { return 0; } - if (p[0] == '/') { - return 1; - } #ifdef _WIN32 - if (p.size() >= 3 && p[1] == ':' && (p[2] == '/' || p[2] == '\\')) { + if (p.size() >= 2 && is_separator(p[0]) && is_separator(p[1])) { + return 2; + } + if (p.size() >= 2 && p[1] == ':') { auto c = p[0]; if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z')) { - return 3; + return (p.size() >= 3 && is_separator(p[2])) ? 3 : 2; } } #endif + if (is_separator(p[0])) { + return 1; + } return 0; } +// A bare drive prefix names no directory to separate from: "C:x" is not the file "C:/x". +auto separates_from_child(std::string_view p) -> bool +{ + return !p.empty() && (is_separator(p.back()) || (p.back() == ':' && is_root(p))); +} + auto append_root(Buf& out, std::string_view p, std::size_t root_len) -> void { - out.append(p.substr(0, root_len - 1)); - out += '/'; + auto const root = p.substr(0, root_len); + for (auto c : root) { + out += is_separator(c) ? '/' : c; + } + if (!separates_from_child(root)) { + out += '/'; + } } // Used only by relative()'s precondition assertion, which a release build compiles out. @@ -64,7 +90,7 @@ auto join(std::string_view a, std::string_view b) -> StringId auto buf = Buf {}; buf.append(a); - if (a.back() != '/') { + if (!separates_from_child(a)) { buf += '/'; } buf.append(b); @@ -83,16 +109,16 @@ auto parent(std::string_view p) -> std::string_view } auto end = p.size(); - while (end > root_len && end > 1 && p[end - 1] == '/') { + while (end > root_len && end > 1 && is_separator(p[end - 1])) { --end; } if (end <= root_len) { return p.substr(0, root_len); } - auto pos = p.rfind('/', end - 1); + auto pos = p.find_last_of(separators, end - 1); if (pos == std::string_view::npos) { - return {}; + return p.substr(0, root_len); } if (pos < root_len) { return p.substr(0, root_len); @@ -105,9 +131,9 @@ auto filename(std::string_view p) -> std::string_view if (p.empty()) { return {}; } - auto pos = p.rfind('/'); + auto pos = p.find_last_of(separators); if (pos == std::string_view::npos) { - return p; + return p.substr(root_length(p)); } return p.substr(pos + 1); } @@ -155,6 +181,13 @@ auto is_normal(std::string_view p) -> bool return true; } +#ifdef _WIN32 + // normalize emits only '/', so the other separator spelling is by definition not normal. + if (p.find('\\') != std::string_view::npos) { + return false; + } +#endif + auto const root_len = root_length(p); auto const body = p.substr(root_len); if (!body.empty() && body.back() == '/') { @@ -192,7 +225,7 @@ auto normalize(std::string_view p) -> StringId auto absolute = root_len != 0; while (start < p.size()) { - auto end = p.find('/', start); + auto end = p.find_first_of(separators, start); if (end == std::string_view::npos) { end = p.size(); } @@ -248,7 +281,7 @@ auto relative(std::string_view target, std::string_view base) -> StringId auto parts = Vec {}; auto start = std::size_t { 0 }; while (start < p.size()) { - auto end = p.find('/', start); + auto end = p.find_first_of(separators, start); if (end == std::string_view::npos) { end = p.size(); } diff --git a/test/unit/test_builder.cpp b/test/unit/test_builder.cpp index d23c4289..d8b78181 100644 --- a/test/unit/test_builder.cpp +++ b/test/unit/test_builder.cpp @@ -2101,6 +2101,87 @@ TEST_CASE("GraphBuilder rejects an absolute output path", "[builder][hierarchy]" CHECK(rejected_inside.error().msg().find("hierarchy") != std::string_view::npos); } +/// One spelling of the escape per test, each answered by the platform's own path law (#388): +/// on Windows these name a location outside the build root, on POSIX they name one file whose +/// name contains a backslash. Pinning only the Windows half would read as a bug report on POSIX. +TEST_CASE("GraphBuilder rejects a backslash escape on Windows and names the file on POSIX", "[builder][hierarchy]") +{ + auto fixture = BuilderTestFixture {}; + + auto result = add_tupfile_from_source(fixture, ": |> echo x > %o |> ..\\victim.txt\n"); + +#ifdef _WIN32 + REQUIRE_FALSE(result.has_value()); + CHECK(result.error().msg().find("hierarchy") != std::string_view::npos); +#else + CHECK(result.has_value()); +#endif +} + +TEST_CASE("GraphBuilder rejects a UNC output on Windows and names the file on POSIX", "[builder][hierarchy]") +{ + auto fixture = BuilderTestFixture {}; + + auto result = add_tupfile_from_source(fixture, ": |> echo x > %o |> \\\\host\\share\\victim.txt\n"); + +#ifdef _WIN32 + REQUIRE_FALSE(result.has_value()); + CHECK(result.error().msg().find("hierarchy") != std::string_view::npos); +#else + CHECK(result.has_value()); +#endif +} + +TEST_CASE("GraphBuilder rejects a drive-relative output on Windows and names the file on POSIX", "[builder][hierarchy]") +{ + auto fixture = BuilderTestFixture {}; + + auto result = add_tupfile_from_source(fixture, ": |> echo x > %o |> C:victim.txt\n"); + +#ifdef _WIN32 + REQUIRE_FALSE(result.has_value()); + CHECK(result.error().msg().find("hierarchy") != std::string_view::npos); +#else + CHECK(result.has_value()); +#endif +} + +/// The record's key and the file the OS creates were two different paths for every backslash +/// spelling on Windows: one opaque component here, two components on disk (#388). +TEST_CASE("GraphBuilder records a backslash output as two components on Windows and one on POSIX", "[builder][hierarchy]") +{ + auto fixture = BuilderTestFixture {}; + + auto bs = make_build_graph(); + auto vars = VarDb {}; + auto ctx = EvalContext { .vars = &vars }; + + auto options = BuilderOptions { + .source_root = intern(fixture.root_str()), + .config_root = intern(fixture.root_str()), + .output_root = pup::StringId::Empty, + .config_path = pup::StringId::Empty, + .expand_globs = false, + .validate_inputs = false, + }; + auto builder_state = make_builder(options); + + auto parse_result = parse_tupfile(": |> echo x > %o |> sub\\gen.txt\n", fixture.tupfile_path("")); + REQUIRE(parse_result.success()); + + REQUIRE(add_tupfile(bs, parse_result.tupfile, ctx, builder_state).has_value()); + + auto generated = nodes_of_type(bs.graph, NodeType::Generated); + REQUIRE(generated.size() == 1); +#ifdef _WIN32 + CHECK(sv(get_full_path(bs.graph, generated[0])) == "sub/gen.txt"); + CHECK(sv(get(bs.graph, generated[0])) == "gen.txt"); +#else + CHECK(sv(get_full_path(bs.graph, generated[0])) == "sub\\gen.txt"); + CHECK(sv(get(bs.graph, generated[0])) == "sub\\gen.txt"); +#endif +} + TEST_CASE("GraphBuilder accepts a parent reference that stays inside the tree", "[builder][hierarchy]") { auto fixture = BuilderTestFixture {}; diff --git a/test/unit/test_path.cpp b/test/unit/test_path.cpp index 186f8921..423cc407 100644 --- a/test/unit/test_path.cpp +++ b/test/unit/test_path.cpp @@ -176,7 +176,9 @@ TEST_CASE("path::is_root", "[path]") REQUIRE(is_root("C:/")); REQUIRE(is_root("C:\\")); REQUIRE_FALSE(is_root("C:/a")); - REQUIRE_FALSE(is_root("C:")); + // "C:" is the whole prefix a drive-relative path is rooted on, so it is one (#388). + REQUIRE(is_root("C:")); + REQUIRE(is_root("//")); #endif } @@ -305,6 +307,69 @@ TEST_CASE("path::relative", "[path]") } } +/// A drive prefix is the one root that contains no separator byte, so the splitters cannot honour +/// it incidentally the way `find_last_of` honours "/", "C:/" and "//". Each function answers for +/// itself, and on POSIX "C:a" is an ordinary relative name with a colon in it (#388). +TEST_CASE("path::filename answers a separator-less root", "[path]") +{ +#ifdef _WIN32 + REQUIRE(filename("C:a") == "a"); + REQUIRE(filename("C:") == ""); +#else + REQUIRE(filename("C:a") == "C:a"); + REQUIRE(filename("C:") == "C:"); +#endif +} + +TEST_CASE("path::parent answers a separator-less root", "[path]") +{ +#ifdef _WIN32 + REQUIRE(parent("C:a") == "C:"); +#else + REQUIRE(parent("C:a") == ""); +#endif +} + +TEST_CASE("path::join answers a separator-less root", "[path]") +{ +#ifdef _WIN32 + REQUIRE(sv(join("C:", "a")) == "C:a"); +#else + REQUIRE(sv(join("C:", "a")) == "C:/a"); +#endif +} + +/// stem and extension read the name through filename, so the clamp reaches them without their +/// own edit -- pinned rather than argued. +TEST_CASE("path::stem and extension answer a separator-less root", "[path]") +{ +#ifdef _WIN32 + REQUIRE(stem("C:a.txt") == "a"); +#else + REQUIRE(stem("C:a.txt") == "C:a"); +#endif + REQUIRE(extension("C:a.txt") == ".txt"); +} + +#ifndef _WIN32 + +/// The other half of the asymmetry #388 settled: a backslash is an ordinary character in a POSIX +/// filename, so none of these spellings names anything but a file whose name contains one. +TEST_CASE("path::treats a backslash as an ordinary filename character", "[path][posix]") +{ + REQUIRE(sv(normalize("..\\victim.txt")) == "..\\victim.txt"); + REQUIRE(sv(normalize("a\\b")) == "a\\b"); + REQUIRE(filename("a\\b") == "a\\b"); + REQUIRE(parent("a\\b") == ""); + REQUIRE(is_normal("a\\b")); + REQUIRE_FALSE(is_absolute("\\victim.txt")); + REQUIRE_FALSE(is_absolute("\\\\host\\share\\x")); + REQUIRE_FALSE(is_absolute("C:a")); + REQUIRE_FALSE(is_absolute("C:\\a")); +} + +#endif + #ifdef _WIN32 TEST_CASE("path::normalize keeps the drive root", "[path][windows]") @@ -351,12 +416,87 @@ TEST_CASE("path::normalize keeps the drive root", "[path][windows]") } } +/// `C:` and `C:a` are rooted on a drive, not under the build root, so they answer this the same +/// way `C:/a` does; the earlier `REQUIRE_FALSE` pinned the hole #388 was filed for. TEST_CASE("path::is_absolute on drive-absolute paths", "[path][windows]") { REQUIRE(is_absolute("C:/a")); REQUIRE(is_absolute("C:\\a")); - REQUIRE_FALSE(is_absolute("C:")); - REQUIRE_FALSE(is_absolute("C:a")); + REQUIRE(is_absolute("C:")); + REQUIRE(is_absolute("C:a")); +} + +TEST_CASE("path::normalize reads a backslash as a separator", "[path][windows]") +{ + SECTION("between names") + { + REQUIRE(sv(normalize("a\\b")) == "a/b"); + REQUIRE(sv(normalize("a\\b/c")) == "a/b/c"); + } + + SECTION("a parent reference spelled with a backslash resolves") + { + REQUIRE(sv(normalize("a\\..\\b")) == "b"); + REQUIRE(sv(normalize("..\\victim.txt")) == "../victim.txt"); + REQUIRE(sv(normalize("a\\..\\..\\victim.txt")) == "../victim.txt"); + } + + SECTION("dot segments and repeated separators") + { + REQUIRE(sv(normalize("a\\.\\\\b")) == "a/b"); + REQUIRE(sv(normalize("a\\")) == "a"); + } +} + +TEST_CASE("path::normalize keeps a UNC path rooted", "[path][windows]") +{ + REQUIRE(sv(normalize("\\\\host\\share\\victim.txt")) == "//host/share/victim.txt"); + REQUIRE(sv(normalize("\\\\host\\share")) == "//host/share"); + + // Where the parent references resolve to does not matter: staying rooted is what refuses it. + REQUIRE(is_absolute(sv(normalize("\\\\host\\share\\..\\..\\victim.txt")))); +} + +TEST_CASE("path::normalize keeps a drive-relative prefix unseparated", "[path][windows]") +{ + // "C:a" names a's location on C's own current directory; "C:/a" is a different file. + REQUIRE(sv(normalize("C:a\\b")) == "C:a/b"); + REQUIRE(sv(normalize("C:")) == "C:"); +} + +TEST_CASE("path::is_absolute on the remaining Windows root spellings", "[path][windows]") +{ + REQUIRE(is_absolute("\\\\host\\share\\x")); + REQUIRE(is_absolute("\\victim.txt")); + REQUIRE_FALSE(is_absolute("a\\b")); + REQUIRE_FALSE(is_absolute("..\\victim.txt")); +} + +TEST_CASE("path::parent and filename read a backslash as a separator", "[path][windows]") +{ + REQUIRE(parent("a\\b\\c") == "a\\b"); + REQUIRE(filename("a\\b\\c") == "c"); + REQUIRE(stem("a\\b.txt") == "b"); + REQUIRE(extension("a\\b.txt") == ".txt"); +} + +TEST_CASE("path::is_normal rejects a backslash-separated spelling", "[path][windows]") +{ + REQUIRE_FALSE(is_normal("a\\b")); + REQUIRE_FALSE(is_normal("a\\")); + REQUIRE(is_normal("a/b")); +} + +TEST_CASE("path::join does not double a backslash separator", "[path][windows]") +{ + REQUIRE(sv(join("a\\", "b")) == "a\\b"); + REQUIRE(sv(join("x", "\\\\host\\share\\b")) == "\\\\host\\share\\b"); + REQUIRE(sv(join("x", "C:b")) == "C:b"); +} + +TEST_CASE("path::relative reads a backslash as a separator", "[path][windows]") +{ + REQUIRE(sv(relative("a/b/c", "a/b")) == "c"); } TEST_CASE("path::parent on drive-absolute paths", "[path][windows]") @@ -396,7 +536,18 @@ auto check_normalize_laws(std::string const& p, std::string_view root) -> void REQUIRE(once == twice); + // Splitting a normal path and rejoining it reproduces it. This is what reaches the three + // functions that honour a root only by finding a separator inside it (#388). + REQUIRE(sv(join(parent(once), filename(once))) == once); + auto const absolute = is_absolute(p); + + // Stated apart from the rejoin above, which cannot see this: join("", x) returns x, so an + // under-clamped parent and the whole name it leaves behind cancel exactly (#388). + if (absolute) { + REQUIRE(is_absolute(parent(once))); + } + if (absolute) { REQUIRE(is_absolute(once)); REQUIRE(std::string_view { once }.starts_with(root)); @@ -465,5 +616,17 @@ TEST_CASE("path::normalize obeys its laws", "[path]") { check_laws_under_root("C:/"); } + + SECTION("drive-relative paths") + { + check_laws_under_root("C:"); + } + + // The root a UNC path normalizes onto. The authority that follows it is ordinary components, + // which is why the laws are stated against "//" and not against "//host/share" (#388). + SECTION("UNC-rooted paths") + { + check_laws_under_root("//"); + } #endif } From 13ecfe3e7caed1f999855265ff179ca7a3ab341e Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:32:34 +0800 Subject: [PATCH 2/2] Stop asking Windows to create a directory that is already there Teaching pup::path that a backslash separates gave parent() a real answer for a backslash-spelled Windows path, where it had returned empty before. That woke three guards in file_io-win32.cpp that had never once run, and create_directories then walked to the drive root and asked the OS to create it -- which answers with neither success nor ERROR_ALREADY_EXISTS, failing every index write whose path came from a channel that does not convert separators. All seven test-windows failures were that write. The POSIX half of the pair has stat'd first and returned success for an existing directory since it was written; the Win32 half asked every time. Conforming it removes the dependency on what the OS answers for a root, rather than widening the error codes we tolerate, which would launder a real denial into success. The new pin is untagged so Windows CI runs it: every gate available here passes with or without this fix, because wine answers ERROR_ALREADY_EXISTS where Windows does not. --- src/platform/file_io-win32.cpp | 5 ++++ test/unit/test_platform_file_io.cpp | 43 +++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/src/platform/file_io-win32.cpp b/src/platform/file_io-win32.cpp index 9d8c07b3..cc7fd519 100644 --- a/src/platform/file_io-win32.cpp +++ b/src/platform/file_io-win32.cpp @@ -423,6 +423,10 @@ auto create_directories(std::string_view path) -> Result if (path.empty()) { return {}; } + // A drive root answers neither success nor ERROR_ALREADY_EXISTS, so never ask (#388). + if (is_directory(path)) { + return {}; + } auto par = pup::path::parent(path); if (!par.empty() && par != path) { auto r = create_directories(par); @@ -433,6 +437,7 @@ auto create_directories(std::string_view path) -> Result auto wpath = to_wide(path); if (!CreateDirectoryW(wpath.c_str(), nullptr)) { auto err = GetLastError(); + // A file in the way lands here too, so this succeeds where the POSIX half returns -ENOTDIR. if (err != ERROR_ALREADY_EXISTS) { return make_error(ErrorCode::IoError, make_err_msg("Failed to create directory: ", path, err)); } diff --git a/test/unit/test_platform_file_io.cpp b/test/unit/test_platform_file_io.cpp index 9e9d9521..88210dc4 100644 --- a/test/unit/test_platform_file_io.cpp +++ b/test/unit/test_platform_file_io.cpp @@ -632,3 +632,46 @@ SCENARIO("canonical resolves a symlinked prefix before cancelling what follows i } } #endif + +/// Untagged so Windows CI, which filters `~[e2e]~[shell]`, is where this runs: a root always +/// exists, and only there does asking the OS to create one fail the build (#388). +TEST_CASE("create_directories succeeds for what already exists", "[platform][file_io]") +{ + auto const cwd_id = pup::platform::current_directory(); + REQUIRE(cwd_id.has_value()); + auto const cwd = std::string { pup::global_pool().get(*cwd_id) }; + + SECTION("an existing directory") + { + CHECK(create_directories(cwd).has_value()); + } + + SECTION("the root the directory sits under") + { + auto root = std::string_view { cwd }; + while (!pup::path::is_root(root)) { + auto const par = pup::path::parent(root); + if (par.empty() || par == root) { + break; + } + root = par; + } + REQUIRE(pup::path::is_root(root)); + INFO("root: " << root); + CHECK(create_directories(root).has_value()); + } + +#ifdef _WIN32 + SECTION("a backslash-spelled directory whose parent walk reaches the root") + { + auto spelled = cwd; + for (auto& c : spelled) { + if (c == '/') { + c = '\\'; + } + } + INFO("spelled: " << spelled); + CHECK(create_directories(spelled).has_value()); + } +#endif +}