Return directory entries in name order (fixes #179) - #181
Merged
Conversation
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
PR metricsPerformance (gcc example, Linux)
Deterministic signals: instructions (cachegrind-simulated instruction reads — exact across runs, no PMU needed), page faults, peak RSS, and the cachegrind D1/LL miss rates. CPU time is user+sys from time(1). Internal statistics (gcc example, up-to-date dry run)
Counters from Binary size (Linux)
Test coverage (lines)
103 files · 14995/17322 lines covered Deltas vs main@1738aa715. Updated for 389f021 |
…m 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
This was referenced Jul 27, 2026
typeless
added a commit
that referenced
this pull request
Jul 27, 2026
Three findings from an adversarial review of the enumeration-order PRs (#174, #176, #181, #186). compose_nested_project_subtree binary-searched `available` while push_back-ing into it, so after the first append the range was unsorted and lower_bound's precondition was violated: a directory already present could be reported absent and appended again, and the final sort had no unique to remove it. A duplicate there means sort_dirs_by_depth parses that Tupfile twice in one run, registering its rules twice -- which now dies at the duplicate-key rejection with an error naming a project that is perfectly valid. Which projects hit it was decided by interning-order accidents, i.e. exactly what this PR series was removing. The search now covers only the sorted prefix, which is sufficient because the rel_ids within one call are distinct, and the sort dedups so the invariant no longer rests on that being true. PR #186 deleted <, >, <= and >= on StringId so that every comparison has to state whether it means handle order or name order. It left two routes open, both verified by compilation against the real header: auto x = (a <=> b) < 0; // compiled struct Node { StringId name; int x; // compiled: member-wise auto operator<=>(Node const&) const = default; }; // built-in enum <=> The second is the dangerous one: a defaulted spaceship on any struct holding a StringId sorts by handle without a single character at the call site saying so. Deleting operator<=> rejects both; equality is untouched. Neither route is used anywhere in the tree today, so this closes the class before it grows a member. prepare_job_launch emitted a job's exported variables in interning-handle order. create_env_block (process-win32.cpp:102) writes that sequence into the environment block verbatim, and Win32 expects it sorted alphabetically. Now emitted by name. Not claimed: the review reported observing handle order in a child's `env` output. That evidence is confounded -- commands run under sh, which re-exports its own environment in its own order, so the block order putup passes is not observable that way. The Win32 block requirement stands on its own and is why this changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
readdirorder is where filesystem nondeterminism enters putup. Every consumer was responsible for canonicalizing it — a conventional invariant — and two of the five consumers did not: glob expansion (#171) anddiscover_variants(#175). The first put readdir order into%fand therefore into command identity.read_directorynow sorts entries by name on both platforms, andwalk_directoryinherits it because both implementations enumerate throughread_directory. Enumeration order becomes a function of directory content at the one site that produces it, instead of 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
This branch carries neither downstream fix, so the variant hint shows the boundary alone doing the work. Six build dirs enumerating (
ls -U) asyankee mike charlie bravo alpha zeta:What this does not replace
Boundary sorting gives determinism, not canonical path order, for recursive walks: DFS pre-order over per-directory sorted entries is not lexicographic —
sub.txtsorts beforesub/a.txtas a string, while the walk emitssub/a.txtfirst because it descendssubon the way past.glob_expand's own sort (#174) is still what makes%fpath-ordered, and #176's sort still states the intent locally rather than depending on a remote invariant.Tests
Two scenarios:
read_directoryreturns name order, andwalk_directoryenumerates every level in name order depth-first. The RED artifact depends on the filesystem — the tests create entries in reverse-lexicographic order, so they fail on a creation-ordered filesystem and a hash-ordered one alike. Observed here as{ alpha, zeta, charlie, mike, yankee, bravo }.The win32 half is compile-checked by CI only; it is the same one-line sort in the same position.
make testgreen (142313 assertions, 618 cases, 32 e2e shards);make tidyandmake iwyuclean; bootstrap scripts unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA