Read the platform's own separator and root forms when resolving a path

include/pup/index/format.hpp

@@ -89,7 +89,7 @@ inline constexpr auto INDEX_MAGIC = std::array<char, 4> { '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

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.

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<std::string_view> {};
         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();
             }

src/platform/file_io-win32.cpp

@@ -423,6 +423,10 @@ auto create_directories(std::string_view path) -> Result<void>
     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<void>
     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<void>(ErrorCode::IoError, make_err_msg("Failed to create directory: ", path, err));
         }

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<Name>(bs.graph, generated[0])) == "gen.txt");
+#else
+    CHECK(sv(get_full_path(bs.graph, generated[0])) == "sub\\gen.txt");
+    CHECK(sv(get<Name>(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 {};