fix: capture files when git HEAD is unborn - #1016
Conversation
No changeset foundThis PR does not add a changeset, so it will not appear in the changelog or trigger a release. If the change is user-facing, add one: pnpm changesetPick a bump (patch / minor / major) and write the changelog entry in our usual voice ("Added X... Thanks to @you. Closes #123."), then commit the generated If this PR is docs-only or a chore that needs no release note, you can ignore this - or run |
will-lamerton
left a comment
There was a problem hiding this comment.
Thanks for this - the diagnosis is right and I reproduced it: with an unborn HEAD git diff --name-only HEAD exits 128 and takes the whole try block down, while git ls-files --others --exclude-standard returns exactly the files your test asserts.
Two things before this can land.
1. The branch is on a stale base and will conflict. The merge base is b3eb0349, but main has since moved this code (e2770f18, 1d0cf103). getModifiedFiles() is now a one-line delegate and the body you patched lives in getModifiedFilesResult(). Please rebase and re-apply the fix there.
This matters beyond the conflict: that function now returns an available flag, and source/acp/acp-timeline.ts skips timeline checkpointing entirely when it is false. Today an unborn repo reads as "git unavailable"; after the fix it should be available: true with the untracked files listed.
2. Drop the probe, catch the diff instead. getModifiedFiles runs on every checkpoint and twice per ACP tool call, so git rev-parse --verify HEAD adds a spawn to every call on the common path where HEAD exists. Same result, one spawn:
let modifiedOutput = '';
try {
modifiedOutput = execSync('git diff --name-only HEAD', {
cwd: this.workspaceRoot,
encoding: 'utf-8',
stdio: ['pipe', 'pipe', 'pipe'],
}).trim();
} catch {
// Unborn HEAD: no tracked files to diff, untracked scan below still works.
}The available: false contract survives, since a non-git directory still throws at the git ls-files call right below.
Smaller points:
- Point the test at
getModifiedFilesResult()after the rebase and assertavailable: true/truncated: falsealongside the file list. That is the assertion that catches a regression breaking ACP checkpoints in fresh repos. - The exact
t.deepEqualis sensitive to a contributor's globalcore.excludesFileorinit.templateDir, which do apply to the nested repo.git -c core.excludesFile=/dev/null initplusGIT_CONFIG_GLOBAL=/dev/nullin the env makes it deterministic. Also worth flagging that this is the first git-invoking test in the file. files.sort()mutates the array the service returned;[...files].sort()is safer.- The file already imports
execFileSyncand uses the arg-array form forgit show. No injection risk here, but the new call drifts from where the rest of the repo is going. - Please add a changeset so this shows up in the release notes.
|
Hi @ethanhawkes-gif, thanks for this PR! It looks like a codeowner has left feedback Whenever you get a chance, could you take a look at the open comments? |
Closes #1010.
Summary
HEADexists before asking Git for tracked modificationsgit ls-files --others --exclude-standardwhenHEADis unbornWhy this works
An unborn
HEADmeans there are no tracked files to diff, but it does not preventgit ls-files --others --exclude-standardfrom listing untracked files. The existing ignore filtering and checkpoint file limit remain unchanged. In repositories with a validHEAD, the existinggit diff --name-only HEADpath still runs.Tests
pnpm run test:ava source/services/file-snapshot.spec.ts— 28 passed, includingFileSnapshotService finds untracked files when git HEAD is unbornpnpm run test:types— passedpnpm exec biome check source/services/file-snapshot.ts source/services/file-snapshot.spec.ts— passedpnpm run build— passed