From de078391cbd824939011c8fa276e3bab73fbc544 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:54:24 +0800 Subject: [PATCH 1/2] Return directory entries in name order (fixes #179) readdir order is where filesystem nondeterminism enters putup. Every consumer was responsible for canonicalizing it, which is a conventional invariant -- and two of the five consumers did not: glob expansion (#171) and discover_variants (#175), the first of which put readdir order into %f and therefore into command identity. read_directory now sorts its entries by name, on both platforms, and walk_directory inherits it because both implementations enumerate through read_directory. Enumeration order becomes a function of directory content at the one site that produces it, rather than at each consumer that remembers to ask. The guarantee is stated at the declaration, since that is what callers read. The same boundary in tup is already ordered: listings come from node_dir_index (dir, name). Demonstrated without either downstream fix in this branch -- six build dirs enumerating as yankee mike charlie bravo alpha zeta: Error: no build directory specified; found: build-alpha, build-bravo, build-charlie, build-mike, build-yankee, build-zeta This does not make the consumer-side sorts redundant. Boundary sorting gives determinism, not canonical path order, for recursive walks: DFS pre-order over per-directory sorted entries is not lexicographic, since sub.txt sorts before sub/a.txt as a string while the walk emits sub/a.txt first. glob_expand's own sort is still what makes %f path-ordered. The RED artifact depends on the filesystem: the test creates entries in reverse-lexicographic order, so it fails on a creation-ordered filesystem and on a hash-ordered one alike. Observed here as { alpha, zeta, charlie, mike, yankee, bravo }. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA --- include/pup/platform/file_io.hpp | 2 + src/platform/file_io-posix.cpp | 2 + src/platform/file_io-win32.cpp | 3 ++ test/unit/test_platform_file_io.cpp | 59 +++++++++++++++++++++++++++++ 4 files changed, 66 insertions(+) diff --git a/include/pup/platform/file_io.hpp b/include/pup/platform/file_io.hpp index 1ed57fa2..a994cf99 100644 --- a/include/pup/platform/file_io.hpp +++ b/include/pup/platform/file_io.hpp @@ -102,6 +102,8 @@ auto write_file(std::string_view path, std::string_view data) -> Result; // Directory traversal. // +// Entries come back in name order — readdir order is a filesystem artifact. +// // A DirEntry's `name` is a view into the `names` arena of the DirEntries that // produced it — valid until the next read_directory on that DirEntries, or its // destruction. In a walk, the `rel_path` handed to the visitor is likewise a diff --git a/src/platform/file_io-posix.cpp b/src/platform/file_io-posix.cpp index d052fab1..5509907a 100644 --- a/src/platform/file_io-posix.cpp +++ b/src/platform/file_io-posix.cpp @@ -9,6 +9,7 @@ #include "pup/platform/file_io.hpp" #include "pup/platform/sys.hpp" +#include #include #include #include @@ -630,6 +631,7 @@ auto read_directory(std::string_view path, DirEntries& out) -> Result for (auto i = std::size_t { 0 }; i < out.entries.size(); ++i) { out.entries[i].name = base.substr(offsets[i], offsets[i + 1U] - offsets[i]); } + std::ranges::sort(out.entries, {}, &DirEntry::name); return {}; } diff --git a/src/platform/file_io-win32.cpp b/src/platform/file_io-win32.cpp index 29cab68c..e9faf01c 100644 --- a/src/platform/file_io-win32.cpp +++ b/src/platform/file_io-win32.cpp @@ -8,6 +8,8 @@ #include "pup/core/string_pool.hpp" #include "pup/platform/file_io.hpp" +#include + #include namespace pup::platform { @@ -640,6 +642,7 @@ auto read_directory(std::string_view path, DirEntries& out) -> Result for (auto i = std::size_t { 0 }; i < out.entries.size(); ++i) { out.entries[i].name = base.substr(offsets[i], offsets[i + 1] - offsets[i]); } + std::ranges::sort(out.entries, {}, &DirEntry::name); return {}; } diff --git a/test/unit/test_platform_file_io.cpp b/test/unit/test_platform_file_io.cpp index ca71ab01..0fc4529c 100644 --- a/test/unit/test_platform_file_io.cpp +++ b/test/unit/test_platform_file_io.cpp @@ -7,12 +7,14 @@ #include "pup/core/string_pool.hpp" #include "pup/platform/file_io.hpp" +#include #include #include #include #include #include #include +#include #ifndef _WIN32 #include @@ -417,3 +419,60 @@ TEST_CASE("walk_directory descends deep trees without a per-frame stack blowup", REQUIRE(args.saw_leaf); } #endif + +SCENARIO("read_directory returns entries in name order", "[platform][file_io][walk]") +{ + GIVEN("a directory whose entries were created in reverse-lexicographic order") + { + auto const names = std::array { + "alpha", "bravo", "charlie", "mike", "yankee", "zeta" + }; + auto dir = TempDir { "order" }; + for (auto i = names.size(); i-- > 0;) { + dir.write(names[i], "x"); + } + + WHEN("the directory is read") + { + auto listing = pup::platform::DirEntries {}; + REQUIRE(pup::platform::read_directory(dir.path().string(), listing).has_value()); + + THEN("the entries are in name order") + { + auto seen = std::vector {}; + for (auto const& entry : listing.entries) { + seen.push_back(entry.name); + } + REQUIRE(seen == std::vector(names.begin(), names.end())); + } + } + } +} + +SCENARIO("walk_directory visits every level in name order", "[platform][file_io][walk]") +{ + GIVEN("a nested tree created in reverse-lexicographic order") + { + auto dir = TempDir { "walk_order" }; + for (auto name : std::array { "z.txt", "sub/y.txt", "sub/b.txt", "a.txt" }) { + dir.write(name, "x"); + } + + WHEN("the tree is walked") + { + auto seen = std::vector {}; + auto r = pup::platform::walk_directory( + dir.path().string(), + [&](pup::platform::DirEntry const& /*entry*/, std::string_view rel_path) -> bool { + seen.emplace_back(rel_path); + return true; + }); + + THEN("each directory is enumerated in name order, depth first") + { + REQUIRE(r.has_value()); + REQUIRE(seen == std::vector { "a.txt", "sub", "sub/b.txt", "sub/y.txt", "z.txt" }); + } + } + } +} From 389f02188e20db186d1580b64fe410aabae4bd1a Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 27 Jul 2026 15:47:30 +0800 Subject: [PATCH 2/2] Make the enumeration-order tests fail on a case-insensitive filesystem too The names were all lowercase with distinct initials. NTFS keys its directory index case-insensitively, so that set already arrives sorted there and the test would have passed with the production sort reverted -- on the one platform the win32 half of this change exists for. Mixed case makes byte order and NTFS's native order disagree: byte order gives Bravo, Zulu, alpha, mike; a case-insensitive enumeration gives alpha, Bravo, mike, Zulu. Same for the walk test, at both levels. Checked by reverting the sort in read_directory: the test fails, which it did not have to do before on every filesystem. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA --- test/unit/test_platform_file_io.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/unit/test_platform_file_io.cpp b/test/unit/test_platform_file_io.cpp index 0fc4529c..2ce2148e 100644 --- a/test/unit/test_platform_file_io.cpp +++ b/test/unit/test_platform_file_io.cpp @@ -424,8 +424,10 @@ SCENARIO("read_directory returns entries in name order", "[platform][file_io][wa { GIVEN("a directory whose entries were created in reverse-lexicographic order") { - auto const names = std::array { - "alpha", "bravo", "charlie", "mike", "yankee", "zeta" + // Mixed case on purpose: NTFS enumerates case-insensitively, so an + // all-lowercase set would already arrive sorted and pin nothing there. + auto const names = std::array { + "Bravo", "Zulu", "alpha", "mike" }; auto dir = TempDir { "order" }; for (auto i = names.size(); i-- > 0;) { @@ -454,7 +456,7 @@ SCENARIO("walk_directory visits every level in name order", "[platform][file_io] GIVEN("a nested tree created in reverse-lexicographic order") { auto dir = TempDir { "walk_order" }; - for (auto name : std::array { "z.txt", "sub/y.txt", "sub/b.txt", "a.txt" }) { + for (auto name : std::array { "alpha.txt", "sub/bravo.txt", "sub/Yankee.txt", "Zulu.txt" }) { dir.write(name, "x"); } @@ -471,7 +473,7 @@ SCENARIO("walk_directory visits every level in name order", "[platform][file_io] THEN("each directory is enumerated in name order, depth first") { REQUIRE(r.has_value()); - REQUIRE(seen == std::vector { "a.txt", "sub", "sub/b.txt", "sub/y.txt", "z.txt" }); + REQUIRE(seen == std::vector { "Zulu.txt", "alpha.txt", "sub", "sub/Yankee.txt", "sub/bravo.txt" }); } } }