From a65ea19f6fc553ffba51f076e32e1ea55009dfe8 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 27 Jul 2026 11:59:30 +0800 Subject: [PATCH] Order glob matches by path, not by interning order (fixes #171) glob_expand collected matches into a Vec and finished with std::ranges::sort(results). StringId is an enum class over the interning handle, so that sorted by handle: the pool issues handles sequentially on first intern and the glob interns names in read_directory order, i.e. raw readdir order. The sort read as a canonicalization and was not one. That order reaches cmd->inputs unchanged, %f concatenates in it, and compute_command_identity hashes the expanded text. So the same commit produced different link command lines -- different binaries -- on two machines whose filesystems enumerate differently, and command identity, the cross-build join key for expand_implicit_deps, remove_stale_outputs, merge_out_of_scope_commands and reconcile_input_set, varied with readdir order for every glob rule. On one machine it is a fixpoint (the index re-interns in the previous run's order), which is why no local build-to-build churn exposed it. Sorting by the interned string makes %f a function of project content, which is what the identity hash already assumes, and matches tup: its glob query walks node_dir_index on (dir, name), so matches arrive in name order. Measured on a directory enumerating as bravo mike zeta yankee alpha charlie: before: d/bravo.txt d/mike.txt d/zeta.txt d/yankee.txt d/alpha.txt d/charlie.txt after: d/alpha.txt d/bravo.txt d/charlie.txt d/mike.txt d/yankee.txt d/zeta.txt INDEX_VERSION 15 -> 16: every glob-fed command's identity changes, so v15 identities no longer join. Rejecting them costs the rebuild those commands need anyway and avoids a scoped build merging the stale twin back in beside the new one. The test fixes interning order rather than trusting the filesystem to enumerate out of order: it interns the paths in reverse-lexicographic order before globbing, so handle order is deterministically the reverse of path order. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA --- include/pup/index/format.hpp | 6 ++- src/parser/glob.cpp | 3 +- test/unit/test_glob.cpp | 96 ++++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 2 deletions(-) diff --git a/include/pup/index/format.hpp b/include/pup/index/format.hpp index a18f03b3..e2f697a6 100644 --- a/include/pup/index/format.hpp +++ b/include/pup/index/format.hpp @@ -41,7 +41,11 @@ inline constexpr auto INDEX_MAGIC = std::array { 'P', 'U', 'P', 'I' }; /// resolve to one another through the identity join (issue #167). Also /// covers OrderOnly edges, persisted since issue #166 and load-bearing for /// removal routing; v14 indexes predate them and would lose one removal. -inline constexpr auto INDEX_VERSION = std::uint32_t { 15 }; +/// 16 - Glob matches are ordered by path rather than by interning (readdir) +/// order, so %f and the identity hashing it changed for every glob-fed +/// command (issue #171); v15 identities no longer join, and a scoped build +/// would merge the stale twin back in beside the new one. +inline constexpr auto INDEX_VERSION = std::uint32_t { 16 }; /// Index file header (56 bytes) - v9 struct alignas(8) RawHeader { diff --git a/src/parser/glob.cpp b/src/parser/glob.cpp index db58617b..da0525ba 100644 --- a/src/parser/glob.cpp +++ b/src/parser/glob.cpp @@ -254,7 +254,8 @@ auto glob_expand( } } - std::ranges::sort(results); + // StringId order is interning order, i.e. readdir order, not path order. + std::ranges::sort(results, {}, [&pool](StringId id) { return pool.get(id); }); return results; } diff --git a/test/unit/test_glob.cpp b/test/unit/test_glob.cpp index 823e4f34..4286239e 100644 --- a/test/unit/test_glob.cpp +++ b/test/unit/test_glob.cpp @@ -6,12 +6,58 @@ #include "pup/core/string_pool.hpp" #include "pup/parser/glob.hpp" +#include +#include +#include +#include +#include +#include + +namespace fs = std::filesystem; + using namespace pup::parser; using pup::StringId; using pup::global_pool; namespace { auto sv(StringId id) -> std::string_view { return global_pool().get(id); } + +/// RAII helper to create a temporary directory tree for testing +class TempDir { +public: + // Test shards run as concurrent processes, so the name must be unique across them. + TempDir() + { + auto rng = std::random_device {}; + auto dist = std::uniform_int_distribution { 0, 0xFFFFFFFF }; + for (;;) { + auto candidate = fs::temp_directory_path() / ("pup_glob_" + std::to_string(dist(rng))); + if (fs::create_directory(candidate)) { + path_ = candidate; + return; + } + } + } + + ~TempDir() + { + std::error_code ec; + fs::remove_all(path_, ec); + } + + TempDir(TempDir const&) = delete; + auto operator=(TempDir const&) -> TempDir& = delete; + + [[nodiscard]] auto path() const -> fs::path const& { return path_; } + + auto create_file(std::string_view rel) -> void + { + std::ofstream { path_ / rel }; + } + +private: + fs::path path_; +}; } // namespace TEST_CASE("Glob pattern matching", "[glob]") @@ -223,3 +269,53 @@ TEST_CASE("glob_match_extract", "[glob]") REQUIRE(sv(glob_match_extract("**.c", "foo.c")) == "foo"); } } + +SCENARIO("glob expansion orders matches by path, not by interning order", "[glob]") +{ + GIVEN("a directory tree whose paths were interned in reverse-lexicographic order") + { + // Names unique to this test, so the interning below decides their handles. + auto const paths = std::array { + "gord_alpha.txt", + "gord_bravo.txt", + "gord_sub/gord_charlie.txt", + "gord_sub/gord_mike.txt", + "gord_zeta.txt", + }; + + auto tmp = TempDir {}; + fs::create_directory(tmp.path() / "gord_sub"); + for (auto path : paths) { + tmp.create_file(path); + } + for (auto i = paths.size(); i-- > 0;) { + (void)global_pool().intern(paths[i]); + } + + auto expanded = [&](std::string_view pattern) { + auto matches = glob_expand(pattern, tmp.path().string()); + REQUIRE(matches.has_value()); + auto result = std::vector {}; + for (auto id : *matches) { + result.push_back(sv(id)); + } + return result; + }; + + WHEN("a plain pattern is expanded") + { + THEN("the matches are in lexicographic path order") + { + REQUIRE(expanded("*.txt") == std::vector { paths[0], paths[1], paths[4] }); + } + } + + WHEN("a recursive pattern is expanded") + { + THEN("the matches are in lexicographic path order") + { + REQUIRE(expanded("**/*.txt") == std::vector(paths.begin(), paths.end())); + } + } + } +}