Repository navigation
Conversation
… tree `bun test --changed` read git's file list as lines made for a person, listed from the cwd. It lost a changed file when: - the file is untracked and outside the cwd (`git ls-files` lists only what is below the cwd), - `diff.relative` is set and the cwd is a subdirectory, - git reads an edit as a rename and the old name is still on disk, - the name holds a quote, a tab or a line break (git prints it in C quotes unless `-z` is given), - the name starts with a blank (each line was trimmed), - the repository path ends in a blank (the root was trimmed). The run then printed "no changed files, nothing to run" and exited 0. Every listing now passes `-z` and is split on NUL with no trim. The root is bun's own cwd joined with `git rev-parse --show-cdup`. `git ls-files` gets the root as its pathspec. `-c diff.relative=false` and `-c diff.renames=false` replace `-c core.quotePath=off`, which `-z` makes redundant. A listing that fails is now an error. Before, a failed `git ls-files`, or a failed `git diff` after `git diff HEAD` failed, read as an empty list. A path that does not fit the path buffer is skipped. Before, `bun test --changed` panicked with "range end index 4097 out of range for slice of length 4095". The `--changed --watch` restart list is NUL-terminated for the same reason: a path can hold a line break.
|
Status: open, waiting for CI and review. Reproduction on 1.4.3 and on main (each case fails the same way, this is the shortest one): mkdir -p r/packages/a r/packages/b && cd r
echo 'export const value = 1;' > packages/b/value.js
printf 'import { test, expect } from "bun:test";\nimport { value } from "../b/value";\ntest("a", () => { expect(value).toBe(1); });\n' > packages/a/a.test.ts
git init -q -b main . && git add -A && git -c user.name=t -c user.email=t@example.com commit -q -m init
echo 'export const value = 2;' > packages/b/value.ts # new untracked file, resolves before value.js
cd packages/a
bun test # 1 fail, exit 1
bun test --changed # "--changed: no changed files, nothing to run", exit 0With this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughWatch-trigger and Git changed-file lists now use NUL-delimited paths. Git paths are resolved relative to the worktree root. Tests cover unusual paths, listing failures, and watch-mode reruns. ChangesChanged-File Path Handling
Possibly related issues
Possibly related PRs
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes the main change: bun test --changed now reads NUL-terminated Git paths across the whole work tree. |
| Description check | |
The description explains the problem, fix, verification, test coverage, performance impact, and known limitations. It uses different headings from the template, but it provides the required informatio… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description explains the problem, fix, verification, test coverage, performance impact, and known limitations. It uses different headings from the template, but it provides the required information and is substantially complete.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ged file `git ls-files --others` lists a repository inside the work tree (a clone, or the target of `git worktree add`) as `dir/` and names no file in it. The entry matched no source. It made the run print "1 changed file, but no test files are affected" and scan the import graph on a clean tree, from the root before and from every directory now that the listing covers the whole work tree.
There was a problem hiding this comment.
I re-reviewed after a985be8 and found no new bugs; the follow-up commit closes the untracked-nested-repository case from the first round. Because this rewrites how --changed discovers the work tree and turns previously silent listing failures into exit 1, a maintainer should still weigh the design tradeoffs the description lists.
What was reviewed:
WorkTree::parseon each shape ofrev-parse --show-cdup --show-topleveloutput:../chain joined via the checked join, empty cdup at the root, and the duplicated-root halving when the cwd is outside the tree (both lines come from the sameget_git_work_tree()value, so the split is exact; a root with a line break stays intact).-c diff.relative=false/diff.renames=falseon git older than 2.28: unknown-ckeys are ignored, so the rename and relative cases degrade to the old behavior rather than failing.- The NUL terminator constant: exactly one writer (
flush_changed_paths_for_reload) and one reader (consume_watch_trigger), both non-Windows; no stale newline consumer remains, and every import inChangedFilesFilter.rsis still used. - The two fallback diffs and
ls-filesnow error throughgit_failed; the truncated-index and stand-in-git tests exercise each path and assert git's own message.
Extended reasoning...
The PR touches src/runtime/cli/test/ChangedFilesFilter.rs (git invocation and output parsing for bun test --changed), src/jsc/hot_reloader.rs (the watch trigger-file terminator) and test/cli/test/test-changed.test.ts (34 new cases). It spawns git with user-controlled cwd and ref arguments and parses its output, but adds no new injection surface beyond the pre-existing --changed=<ref> argument. The second commit addresses the nested-repository finding posted in the prior run; the remaining open thread is the whole-tree untracked listing cost, which the author documents as an intentional downside rather than fixing. Deferring rather than approving because the change restructures the git flow, hand-parses rev-parse output, and changes failure behavior from silent-empty to exit 1, which is a judgment call for a maintainer; no CODEOWNERS entry covers these files and no third-party objection is outstanding.
Problem
bun test --changedloses a changed file, prints--changed: no changed files, nothing to runand exits 0 wherebun testexits 1. Cases: a new file outside the cwd (cd packages/a && bun test --changed ../b),diff.relative, a rename, an odd name.append_paths(src/runtime/cli/test/ChangedFilesFilter.rs:630) trims lines that git shapes for the cwd and a person.panic: range end index 4097 out of range for slice of length 4095.Fix
-zand is split on NUL, untrimmed.git ls-filesgets the root as pathspec.-c diff.relative=false -c diff.renames=falsereplaces-c core.quotePath=off.git rev-parse --show-cdup. A failed listing is an error and an overlong path is skipped. The--watchrestart list is NUL-terminated.test/cli/test/test-changed.test.ts(36 new cases, 34 fail on main),test/cli/watch/.GIT_NO_LAZY_FETCH=1(git 2.39.5 fails with it), clearingGIT_*_PATHSPECS, new tracking issues.Background
-zgit ends each name with a NUL and quotes nothing. The cdup is the../path from cwd to root.git status --porcelain -z. It was built and dropped: it fixes no case and its gain is inside the noise.Downsides
Notes
Where the cases come from
No issue reports them. They come from a fuzz run over
bun test --changedagainst the rule "a test whose result can depend on the changed file must be selected". In each case plainbun testexits 1.cd packages/a && bun test --changed ../b, or an untrackedpackages/b/value.tsthat a test inpackages/aimportsno changed files, exit 0git config diff.relative true, cwdpackages/a, edit inpackages/bor in the cwdno changed files, exit 0git rm --cached config.ts, ignore it, add a copy asconfig.example.ts, editconfig.ts2 changed files, but no test files are affected, exit 0no changed files, exit 0no changed files, exit 0--changed --watch: a file with a line break in its name changes beside another filePATH_MAXSeven of these were run on 21 git versions, for
--changedand--changed=HEAD: the untracked file outside the cwd,diff.relative, the rename, a quote, a tab, a leading blank, and a root that ends in a blank. Each selects the test on git 1.7.2, 1.7.6, 1.7.9, 1.7.12.4, 1.8.1.6, 1.8.2, 1.8.3.1, 1.8.5.6, 1.9.1, 2.1.4, 2.7.4, 2.11.4, 2.14.5, 2.15.4, 2.17.6, 2.27.1, 2.28.0, 2.39.5, 2.43.0, 2.47.3 and 2.52.0.Commands that exit 0 on 1.4.3 and behave differently now
packages/a)no changed files, nothing to run1 changed file, but no test files are affected, exit 0. The import graph is scanned1 changed file, running 1/1 test file2 changed files, running 1/1 test filegit worktree addtarget,?? wt/ingit status)1 changed file, but no test files are affectedno changed files, nothing to run. The import graph is not scanned.git/indexis cut shortno changed files, nothing to run, exit 0. The edit is losterror: --changed: fatal: .git/index: index file smaller than expected, exit 1.gitignoreis not local.gitignore, asgit statusdoes. Exit 1 with git's message when the remote does not answerGIT_GLOB_PATHSPECS=1 GIT_NOGLOB_PATHSPECS=1error: --changed: fatal: global 'glob' and 'noglob' pathspec settings are incompatible, exit 1A failed listing is an error and not a run of every test: vitest (vitest-dev/vitest#11430), Jest and
bun lint --changedfail the run too.Measurements (Linux x64, git 2.47.3, release builds of main fec4a88 and of this branch unless stated)
--changed=<ref>3 to 3, a bad ref 2 to 2, outside a repository 1 to 1, no test files 0 to 0, plainbun test0 to 0 (a logginggitfirst inPATH).get_changed_files, 20 changed files: 86 to 100 (gdb count of the allocator entry points, debug builds). The 14 are 11 argv elements, 2 argv joins and the pathspec.catch syscall, debug builds). Oneaccessfor each listed name and 6 calls for the 3 spawns, as before. The spread isfutexandmmap. strace is not installed here.size).git ls-filesfromsrc/runtime/cli/testof this repository (24,710 files, no untracked file): 6.6 ms to 45.0 ms, median of 30 interleaved rounds. The same command twice: 6.6 ms and 6.2 ms. From the root: 45.9 ms to 46.2 ms.Why
git diffand not onegit statusThe design review picked one
git status --porcelain -z --untracked-files=allfor a bare--changed(2 git processes in place of 3). It was built with--no-optional-locksand a fallback for git older than 2.18. The self-review ruled it out:git statustook 76.4 ms against 34.9 ms + 45.0 ms forgit diff+git ls-files, with ranges that overlap.--changeda second listing and a second decoder, and a different list from--changed=HEADfor a file that is staged and then restored.The listing of
bun lint --changedsrc/lint/driver/git.rshas the same protocol (-z,--show-cdup,diff.relative=false,--no-renames, fatal on failure). This PR is a second copy of it and does not share code. The two differ where the commands differ:bun lintcompares with the default branch, ignores submodules and resolves the ref first. One difference is on purpose:git.rspasses--literal-pathspecs, andbun lint --changedthen exits 2 underGIT_GLOB_PATHSPECS=1(global 'literal' pathspec setting is incompatible with all other global pathspec settings). Here the pathspec is only../segments or., so it is passed as it is, and a test pinsGIT_GLOB_PATHSPECS=1andGIT_LITERAL_PATHSPECS=1.Not covered
sub), andfilter()matches a changed path to a source path byte for byte. A rule "every source below a listed directory is changed" would select its importers without another git process. It is not in this PR.nested/and no file in it. The entry is not counted, and a source below it is not selected.core.precomposeUnicodeon macOS, the case of the index undercore.ignoreCase. Not run on macOS.join_abs_string_bufcalls: Report a path that does not fit a path buffer instead of aborting #43067. This PR fixes the panic ofbun test --changedonly.Self-review
Three of the 39 points were not taken:
GIT_NO_LAZY_FETCH=1forgit ls-files, to keep a partial clone off the network. Git 2.39.5 (Debian 12) then fails withfatal: could not fetch <oid> from promisor remotewhere the plain call fetches one blob and works. The state is in the table above.GIT_*_PATHSPECSvariables from thegit ls-filescall. Only a pair that contradicts itself fails, and git fails on every pathspec then.Other open PRs on these files
filter()only. test --changed: follow bare specifiers that resolve to local files #44836 names "git exit errors that read as an empty change set" for its next PR: this PR does that part, andrunTestChangedhas the signature of that series.test --changed,--analyze,bun create <component>) #46281 add tests to the same file. The new cases here sit after--changed=<ref> includes untracked filesto stay clear of test --changed: follow HTML imports and every copy of a changed file #46262.flagparameter torun_gitandget_changed_files. It has to pass it on tolist_names_or_failandgit_failed.--changed. This PR changes none of what it says.What was run
Linux x64 only.
bun run rust:check-all x86_64-pc-windows-msvc aarch64-apple-darwinpasses. The cases with a quote, a tab or a line break in a name, the repository names that end in a blank, the stand-in for git and the restart list skip Windows.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/test/test-changed.test.ts