test(ls): regression test for -aRv recursing into ./.. (#13501)#13515
Open
Gooh456 wants to merge 1 commit into
Open
test(ls): regression test for -aRv recursing into ./.. (#13501)#13515Gooh456 wants to merge 1 commit into
Gooh456 wants to merge 1 commit into
Conversation
ls -aRv used to recurse into the listed "." and ".." entries themselves, walking all the way up past the filesystem root, when combined with -a (show dot entries) and version-sort. Root cause: the old recursion code pushed "."/".." at a fixed prefix of the entry vector and then skipped that fixed prefix count after sorting - which only worked because the default name sort happens to keep them first. Version sort (-v) doesn't, so the skip landed on the wrong entries and "."/".." got treated as regular subdirectories to recurse into. This was already fixed on main as a side effect of the unrelated "enter_directory" traversal refactor in uutils#9851, which replaced the position-based skip with an explicit is_dot_dir flag on each entry that's checked at recursion time regardless of sort order (landed 2026-04-17, 12 days after the 0.8.0 release the issue was filed against - the reporter just hadn't picked up a newer build yet). Verified directly: reproduced the walk-up-to-root behavior with a real 0.8.0 build, confirmed it's gone on current main, and confirmed via git blame + testing the actual pre-refactor code that the position- based skip is the real cause. No behavior change here, just adds coverage so this can't silently regress: the case is not covered by any existing test.
Gooh456
force-pushed
the
test-ls-recursive-version-sort-13501
branch
from
July 23, 2026 11:02
2f727ec to
8fa9d17
Compare
Merging this PR will improve performance by 3.55%
Performance Changes
Tip Curious why this is faster? Comment Comparing Footnotes
|
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.
What this is
Test-only PR, no behavior change. Adds coverage for the bug in #13501 (
ls -aRvrecurses into the listed./..entries and walks all the way up to the filesystem root).What I found
I went in expecting to write an actual fix, but the bug is already gone on
main- just never closed out. Details:0.8.0build (the version the reporter was on):ls -aRvon a smalla/b/ctree walked straight past the fixture dir and started listing my actual home directory tree, exactly matching the "recurses up to filesystem root" behavior described in the issue.0.8.0:./..get pushed into the entries buffer first, then the whole buffer gets sorted, then recursion does.skip(trim)assuming./..are still the firsttrim(2) entries after sorting. That assumption only holds for the default name sort (where./..happen to sort first anyway) - version sort (-v) doesn't keep them there, so the skip lands on the wrong entries and./..get treated as regular subdirectories to recurse into.main- doesn't happen anymore. Traced it viagit blameto refactoruu_lsso that crate users can call thelswithout having to print everything to stdout #9851 (April 2026, an unrelated refactor ofls's directory traversal into an explicit stack for library-embedding purposes). That refactor replaced the position-based skip with an actualis_dot_dirflag checked per-entry at recursion time, which is correct regardless of sort order. Landed 12 days after the0.8.0release the issue was filed against, so the reporter's build predates the fix.So there's nothing to fix in
ls.rsitself. What was missing is test coverage for this exact case - I checked and no existing test exercises-a+-R+-vtogether, so this could regress again silently. This PR adds that one test.Testing
lsfrom a real0.8.0checkout (viagit worktree) and reproduced the bug for real (recursed all the way into my Windows home directory).main.lstest module (cargo test --release --test tests -p coreutils --features ls test_ls::) before and after: same 28 pre-existing failures both times (allA required privilege is not held by the client- my Windows account can't create symlinks without dev mode/admin, unrelated to this change), no new failures, my new test passes.rustfmt --checkon the touched file: no diff introduced by my change (the file has some pre-existing formatting drift elsewhere from a rustfmt version mismatch, not touched here).Disclosure: I used an AI coding assistant (Claude) to help investigate and write this, but did the actual verification (building 0.8.0 from source, reproducing the crash for real, bisecting to #9851, running the test suite) myself/directly, not just theorizing from reading the code.