diff --git a/.changeset/quiet-hounds-report.md b/.changeset/quiet-hounds-report.md new file mode 100644 index 000000000..7d84ce077 --- /dev/null +++ b/.changeset/quiet-hounds-report.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3859 +--- +**A scoped `commit --files` call whose named files are already committed and unmodified now reports `nothing_to_commit` instead of a failed commit carrying your pre-commit hook's rejection message.** The empty-diff case used to reach `git commit`, where a rejecting hook fires before git can report "nothing to commit" — so callers were handed `commit_failed` and a gate message that was true about the repository and irrelevant to the call. Genuine rejections still report `commit_failed` with the hook's message, and `--amend`, missing named paths, and merges or cherry-picks in progress are unchanged. A modified path under `git update-index --assume-unchanged` is still committed exactly as before: `git commit -- ` reads the working tree directly, so the guard compares that content against `HEAD` and stands aside rather than dropping content you named. One further outcome does change: naming a submodule whose work tree is dirty but whose recorded commit has not moved now reports `nothing_to_commit` rather than `commit_failed`, because nothing would have landed. (#3776) diff --git a/src/commands.cts b/src/commands.cts index 897ce3c65..6b72d9335 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -1807,12 +1807,236 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u // During a merge, git refuses partial commits — fall back to a bare commit. // --amend is left without a pathspec: amending with -- is a different // operation that rewrites the tip with only those paths. - if (explicitFiles && stagedPaths.length === 0 && !amend) { + const mergeHeadProbe = execGit(['rev-parse', '-q', '--verify', 'MERGE_HEAD'], { cwd }); + const isMergeInProgress = mergeHeadProbe.exitCode === 0; + // PROVENANCE FOR THIS WHOLE BLOCK: every behavioural claim below was DRIVEN + // against git 2.54, not reasoned by analogy. Individual claims state what was + // observed and omit the version; where a claim is version-SENSITIVE rather + // than merely version-observed, it says so at the claim. + // + // #3776: git refuses a PARTIAL commit (`git commit -- `) while a merge + // or a cherry-pick is in progress, so in those states the pathspec describes + // nothing about what would actually land and the empty-diff decision below + // must not be made from it. The three sequencer states do NOT agree: + // MERGE_HEAD -> `fatal: cannot do a partial commit during a merge.` + // CHERRY_PICK_HEAD -> `fatal: cannot do a partial commit during a cherry-pick.` + // REVERT_HEAD -> permitted; behaves like an ordinary commit. + // REVERT_HEAD is therefore deliberately absent: including it would suppress + // this fix during a revert, reintroducing the very misreport it removes. + // `canScope` below keeps its narrower merge-only test on purpose — widening it + // would change pre-existing cherry-pick behaviour, which is outside this fix. + // Only the scoped, non-amend call can return through the guard below, so the + // cherry-pick probe and the guard's own probes are gated on that — an + // unscoped commit or an --amend would otherwise pay for git invocations whose + // answer it can never use. The MERGE_HEAD probe above predates this fix and + // stays unconditional: `canScope` needs it on every path. + // A non-zero exit from either sequencer probe means "not in that state" AND + // "the probe never answered" — `execGit` surfaces a spawn timeout as + // `exitCode: 1` (`_spawnResult`: `result.status ?? 1`), which is the exact + // code `rev-parse --verify` returns for a ref that does not exist. Conflating + // them is the one path in this fix that does NOT fail toward the old + // behaviour: a timeout during a real merge would leave `partialCommitRefused` + // false, the guard would decide `nothing_to_commit` from a pathspec git will + // not honour, and the merge would be silently abandoned where it previously + // reported a loud `commit_failed`. So an unanswered probe is treated as + // "assume the partial commit would be refused" — the conservative reading, + // which falls through to `git commit` and lets git speak for itself. + // + // This is deliberately routed into `partialCommitRefused` ONLY, never into + // `isMergeInProgress`: that flag also feeds the pre-existing `canScope` below, + // where a spurious timeout would convert a scoped commit into a bare one and + // record the whole index instead of the named paths. Suppressing a misreport + // must not be paid for by committing content the caller never named. + const guardApplies = explicitFiles && !amend; + const cherryPickProbe = guardApplies + ? execGit(['rev-parse', '-q', '--verify', 'CHERRY_PICK_HEAD'], { cwd }) + : null; + const partialCommitRefused = isMergeInProgress + || isSpawnTimeout(mergeHeadProbe) + || (cherryPickProbe !== null + && (cherryPickProbe.exitCode === 0 || isSpawnTimeout(cherryPickProbe))); + // `stagedPaths` records paths whose `git add` exited 0 — that is "did + // staging succeed", not "is there anything to commit". Staging an + // already-committed, unmodified file succeeds while contributing no diff, so + // `length === 0` is reachable only when EVERY named path was missing from + // disk. For the ordinary empty-diff case control fell through to `git commit`, + // and the only thing converting that back to `nothing_to_commit` was the + // string match on git's output below — which a rejecting pre-commit hook + // pre-empts, because git runs the hook before it decides there is nothing to + // commit. The caller was then handed `commit_failed` carrying a gate message + // about a commit that had nothing to gate. Ask git whether the named paths + // actually differ instead. Three things about that probe are load-bearing: + // - it compares the WORKING TREE to HEAD (`diff HEAD`), not the index + // (`diff --cached`). `git commit -- ` is a partial commit: it takes + // the working-tree content of those paths and ignores what is staged. A + // probe against the index therefore answers a different question than the + // commit asks, and a working-tree write landing between the `git add` + // above and this line — another process in a shared checkout — would make + // the index say "empty" while the commit would still have recorded the new + // content. Driven: `diff --cached` rc 0 and `diff HEAD` rc 1 on the same + // path, with `git commit -- ` then committing it. + // - the `length === 0` short-circuit keeps the all-missing-paths case exact. + // Spreading an empty array yields a pathspec-less `diff`, which tests the + // WHOLE tree — unrelated work elsewhere would then suppress the guard and + // regress the skip-missing contract (#2014). + // It is deliberately NOT gated on `partialCommitRefused`, and gating it + // would be a REGRESSION rather than a hardening. With every named path + // missing, `stagedPaths` is empty, so `canScope` is false and the + // fall-through reaches a BARE `git commit` — which git PERMITS during a + // merge, and which then CONCLUDES that merge: rc 0, a two-parent merge + // commit recording the entire index, under a message naming a path that + // does not exist, reported to the caller as `committed: true` (driven). + // Today's answer writes nothing at all. That is the same trade the timeout + // routing above already refuses — a misreport must not be paid for by + // committing content the caller never named — which is why the sequencer + // states gate the DIFF branch only. The behaviour is also PRE-EXISTING and + // unchanged by this fix: before it the identical short-circuit ran ABOVE + // the MERGE_HEAD probe, so it never consulted the sequencer either. The + // residual it leaves — a merge held open behind a `nothing_to_commit` + // report — is offered as a separate issue with the other three, not folded + // in here. Both sequencer shapes are pinned in + // tests/commit-files-pathspec.test.cjs. + // - `!partialCommitRefused`: see above — deciding "nothing to commit" from a + // pathspec git will not honour would abandon an in-progress merge, so those + // states keep their pre-existing behaviour untouched. + // - the probe is pinned against user configuration that would make `git diff` + // answer a DIFFERENT question than `git commit -- ` asks. `git diff` + // is porcelain and honours settings the commit does not, so without these + // flags a caller's config decides whether the guard fires. Each vector + // below was driven with the paired `git commit -- ` confirmed to + // record the change the probe reported as absent: + // `diff.ignoreSubmodules=all` -> a gitlink bump is invisible to the probe + // `.gitmodules` `ignore = all` -> the same, and it needs NO local config: + // it is checked in, so it arrives with a + // clone + // `diff=` + `textconv` -> two different blobs converge to one + // text, so the probe sees no change at + // all; no submodule involved + // `--ignore-submodules=dirty` rather than `=none`, because `dirty` is what + // a partial commit of a submodule path actually means: it records the + // GITLINK, and the gitlink moves only when the submodule's HEAD does. Under + // `=none` a merely dirty submodule WORKTREE reports a difference the commit + // would not record, sending an empty call back to `git commit` — the #3776 + // misreport, re-entered from the other side. `dirty` still overrides both + // `diff.ignoreSubmodules` and a checked-in `.gitmodules` `ignore`, so the + // gitlink vectors above stay closed (driven: rc 1 under every one of them). + // `--no-ext-diff` is deliberately absent: `--quiet` short-circuits ahead of + // an external diff driver, so an external `diff..command` cannot + // invert the probe (driven: rc 1 with and without the flag). + // Any other non-zero exit from the probe (a genuine git error, or an unborn + // HEAD) leaves the guard shut and falls through to the commit — failing toward + // today's path rather than manufacturing a no-op. + // THE ONE STATE WHERE `git diff` AND `git commit -- ` GENUINELY DISAGREE. + // `--assume-unchanged` tells git to skip the worktree stat for a path, so + // `git add` stages nothing and BOTH diff forms report no difference — while + // `git commit -- ` reads the working tree directly and records it + // (driven: probe rc 0, commit rc 0, new content in the tree). Left + // to the diff probe alone the guard reports `nothing_to_commit` about content + // the caller explicitly named in `--files` and git would have written. #3776 + // is a purely diagnostic bug — nothing is corrupted and no wrong commit is + // made — so suppressing its misreport must not be paid for by dropping named + // content. The same rule the timeout routing already follows one block up. + // + // `git ls-files -v` is the discriminator for the STATE: it tags an + // assume-unchanged path with a LOWERCASE letter (`h`), where + // `--skip-worktree` is an uppercase `S` and never reaches THIS branch: + // `git add` exits 1 under it, so a present-but-modified skip-worktree path + // fails closed as `staging_failed` above the guard. (An ABSENT one is skipped + // before `git add` runs at all per #2014, and is answered by the + // `stagedPaths.length === 0` arm above — correctly, and exactly as it was + // pre-fix. Both shapes are pinned.) + // + // Then ASK GIT, rather than reconstructing its answer. `git commit --dry-run` + // is the same decision the real commit makes, and `--no-verify` is what keeps + // it a DECISION rather than an execution. git 2.54 already declines to run + // `pre-commit` on a dry run (driven: a rejecting one neither fires nor writes + // its marker), which is the property that matters here, because a firing + // `pre-commit` is the whole of #3776 — but that is an observed behaviour of + // one version, and the failure it would produce on a version that differs is + // SILENT. A `pre-commit` that fires and rejects exits 1, the same code git + // returns for `nothing to record`, so the closure below would read it as a + // CONFIRMED empty answer, drop the content the caller named, and report + // `nothing_to_commit` — #3776's exact shape, in #3776's exact configuration. + // `--no-verify` forecloses that structurally instead of resting on the + // version, and is behaviour-neutral where the version already agrees (driven: + // rc 0 would-record / rc 1 nothing, identical with and without it). This is + // VERSION-SENSITIVE reasoning, hence stated at the claim per the provenance + // note above. + // + // It is still NOT hook-free in general, and `--no-verify` does not widen that + // claim: `post-index-change` fires on this call with or without the flag + // (driven both ways), so a repo using that hook sees TWO extra invocations + // for the probe — git fires it twice per `commit --dry-run`, and twice again + // for the real commit (driven: 2/2/2 across flagged probe, unflagged probe + // and real commit). Stated rather than claimed away; the narrower + // claim is the true one. `--porcelain` keeps the output to a couple + // of machine-readable lines instead of a full status listing — the rc is + // identical either way (driven: 0 would-record / 1 nothing), but the plain + // form prints every untracked path, which on a large tree is output this + // probe has no use for and `execGit` would have to buffer. rc 0 means the + // commit would record something, so the guard must stand aside. + // + // Reconstructing it was tried and is WRONG in three measured ways, all of + // them silent drops of named content. Comparing `git hash-object` against + // `HEAD:` misses a mode-only change (`chmod +x` leaves the blob + // identical while `git commit -- ` records `100755`); it cannot hash a + // submodule path at all (`fatal: Unable to hash sub`, while the commit + // advances the gitlink); and the path it needs must be parsed out of + // `ls-files` output, which `core.quotePath` renders as `"caf\303\251.md"` + // by default, so the probe reads a filename that does not exist. Asking git + // needs no path parsed and no case enumerated. + // + // Scoped to this branch on purpose. The diff probe above answers the ordinary + // case cheaply and is pinned against the configuration vectors below; the + // dry run is the heavier, exact answer, and it runs only when an + // assume-unchanged path is actually present. + // + // The `ls-files` read is an OPTIMISATION, never a gate — so an unreadable one + // must not decide anything. It exists only to keep the dry run off the hot + // path when no assume-unchanged entry is present; when it cannot answer, the + // dry run simply runs, because the dry run needs nothing from it. Both + // failing-closed (drop the content) and failing-open (re-enter #3776) are + // wrong answers to a question we can just ask directly. + const assumeUnchangedWouldRecord = (): boolean => { + const listed = execGit(['ls-files', '-v', '--', ...stagedPaths], { cwd }); + // Only the TAG is read; the path is deliberately never parsed out — see the + // `core.quotePath` note above, and the dry run below needs no path anyway. + if (listed.exitCode === 0 + && !listed.stdout.split('\n').some((line) => /^[a-z] /.test(line))) return false; + const dryRun = execGit( + ['commit', '--dry-run', '--porcelain', '--no-verify', '-m', sanitizedMessage as string, '--', ...stagedPaths], + { cwd }, + ); + // Only a CONFIRMED "nothing to record" closes the path: rc 1 from a git + // that actually answered. This is the one probe in the guard whose rc 0 + // is the REASSURING answer, so it inverts the diff probe's safety: there + // a timeout can only yield non-zero and reads as "not clean"; here + // `execGit` collapses a spawn timeout (or any spawn error) to + // `exitCode: 1` (`_spawnResult`: `result.status ?? 1`), byte-identical to + // git's own "nothing to record" — and the guard then reports + // `nothing_to_commit` about content it never asked git to write. Same + // conflation the sequencer probes above defend against, same remedy: an + // unanswered probe falls toward the commit, where git speaks for itself + // (and a genuine error there is reported loudly, as it always was). rc 128 + // is likewise not an answer. Timeout kill of a dry run CAN leave a stale + // `index.lock` behind (it refreshes the index); the real commit then + // fails on it, loudly — never silently. + if (isSpawnTimeout(dryRun) || dryRun.error !== null) return true; + return dryRun.exitCode !== 1; + }; + const nothingToCommit = guardApplies + && (stagedPaths.length === 0 + || (!partialCommitRefused + && execGit( + ['diff', '--quiet', '--ignore-submodules=dirty', '--no-textconv', 'HEAD', '--', ...stagedPaths], + { cwd }, + ).exitCode === 0 + && !assumeUnchangedWouldRecord())); + if (nothingToCommit) { const result = { committed: false, hash: null, reason: 'nothing_to_commit' }; output(result, raw, 'nothing'); return; } - const isMergeInProgress = execGit(['rev-parse', '-q', '--verify', 'MERGE_HEAD'], { cwd }).exitCode === 0; const canScope = explicitFiles && stagedPaths.length > 0 && !amend && !isMergeInProgress; const commitArgs = amend diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index 98eda61f7..942242683 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -5189,6 +5189,557 @@ describe('query commit --files scoping (#2269)', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3776: `query commit --files` must report an empty diff as `nothing_to_commit` +// even when a pre-commit hook would reject. +// +// `stagedPaths` records paths whose `git add` exited 0 — "did staging succeed", +// not "is there anything to commit". Staging an already-committed, unmodified +// file succeeds and contributes nothing, so the old `stagedPaths.length === 0` +// guard was reachable only when EVERY named path was missing from disk. The +// ordinary empty-diff case fell through to `git commit`, where the sole rescue +// was a string match on git's "nothing to commit" output — and git runs the +// pre-commit hook BEFORE deciding there is nothing to commit, so a rejecting +// hook pre-empted the match and the caller was handed `commit_failed` carrying +// a gate message about a commit that had nothing to gate. +// +// The residual sibling of #2608/#2693, which covered `git add` FAILING; this +// covers `git add` succeeding and contributing nothing. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3776: query commit --files reports an empty diff as nothing_to_commit', () => { + const { createTempGitProject } = require('./helpers.cjs'); + // runGit (never gitOrThrow) for the conflicting merge below — that merge is + // MEANT to exit non-zero, and the throwing wrapper would fail the fixture. + const { runGit } = require('./helpers/process-seam.cjs'); + let tmpDir; + + const REJECTING_HOOK = '#!/bin/sh\necho "gate: BACKLOG.md is stale" >&2\nexit 1\n'; + const PASSING_HOOK = '#!/bin/sh\nexit 0\n'; + + // Writes .git/hooks/pre-commit. Every arm below drives the real hook, not a + // stub of it: the defect lives in git's own hook-before-empty-diff ordering, + // so a faked rejection would not exercise the mechanism under test. + function installHook(body) { + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + fs.writeFileSync(hookPath, body); + fs.chmodSync(hookPath, 0o755); + } + + // A tracked, committed, unmodified file — `git add` on it succeeds and + // contributes no diff. This is the exact shape the guard used to miss. + function commitFixtureFile(name = 'doc.md', body = 'hello\n') { + const rel = path.posix.join('.planning', name); + fs.writeFileSync(path.join(tmpDir, '.planning', name), body); + gitOrThrow(['add', '--', rel], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'fixture: ' + name], { cwd: tmpDir }); + return rel; + } + + // The command emits its JSON payload on either stream depending on outcome; + // read whichever carries it rather than assuming success. + function commitFiles(rel, extra = '') { + const result = runGsdTools('commit "m"' + extra + ' --files ' + rel, tmpDir); + const payload = (result.output && result.output.trim()) ? result.output : result.error; + return JSON.parse(payload); + } + + beforeEach(() => { + tmpDir = createTempGitProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // AC1 — the defect. Pre-fix this returned commit_failed + the hook's message. + test('AC1: empty diff + rejecting pre-commit hook reports nothing_to_commit, not the hook rejection', () => { + const rel = commitFixtureFile(); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit', + 'an empty-diff --files call must not be reported as a failed commit'); + assert.ok(!output.error, + 'no hook message may be surfaced for a call that had nothing to gate'); + }); + + // AC2 — the two controls that isolate the hook as the only variable. + test('AC2: empty diff + no hook still reports nothing_to_commit', () => { + const rel = commitFixtureFile(); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit'); + }); + + test('AC2: empty diff + passing hook still reports nothing_to_commit', () => { + const rel = commitFixtureFile(); + installHook(PASSING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit'); + }); + + // AC3 — the all-missing short-circuit must not regress. + test('AC3: every named path missing from disk still reports nothing_to_commit', () => { + const rel = commitFixtureFile(); + fs.unlinkSync(path.join(tmpDir, rel)); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit'); + }); + + // AC3, sharp edge: the `stagedPaths.length === 0` short-circuit is + // load-bearing, not defensive noise. Without it an all-missing call spreads + // an empty array into the pathspec, and a pathspec-less `git diff HEAD` + // tests the WHOLE tree — so unrelated work would suppress the guard and turn + // this arm into a commit of somebody else's changes. + test('AC3: all named paths missing does not consult unrelated staged work', () => { + const rel = commitFixtureFile(); + fs.unlinkSync(path.join(tmpDir, rel)); + const unrelated = path.posix.join('.planning', 'unrelated.md'); + fs.writeFileSync(path.join(tmpDir, unrelated), 'staged by the caller\n'); + gitOrThrow(['add', '--', unrelated], { cwd: tmpDir }); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit'); + + const staged = gitOrThrow(['diff', '--cached', '--name-only'], { cwd: tmpDir }); + assert.match(staged, /unrelated\.md/, + "the caller's own staged work must be left in the index, not swept into a commit"); + }); + + // AC4 — a genuine rejection must still be reported. The goal is to stop + // reporting a rejection for a call that never had anything to gate, not to + // stop reporting rejections. + test('AC4: a real diff rejected by the hook still reports commit_failed with the hook message', () => { + const rel = commitFixtureFile(); + fs.writeFileSync(path.join(tmpDir, rel), 'hello\nmodified\n'); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'commit_failed'); + assert.match(String(output.error), /BACKLOG\.md is stale/, + "the hook's own message must still reach the caller"); + }); + + test('AC4: a real diff with no hook still commits', () => { + const rel = commitFixtureFile(); + fs.writeFileSync(path.join(tmpDir, rel), 'hello\nmodified\n'); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, true); + assert.strictEqual(output.reason, 'committed'); + assert.ok(output.hash, 'a successful commit must carry its hash'); + }); + + // AC5 — amending has a different empty-diff meaning; the guard stays exempt. + test('AC5: --amend remains exempt from the empty-diff guard', () => { + const rel = commitFixtureFile(); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel, ' --amend'); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'commit_failed', + '--amend must still reach git, where the hook governs the rewrite'); + }); + + // Beyond the brief's ACs: during a merge git refuses a partial commit, so the + // commit runs WITHOUT the pathspec and the named paths describe nothing about + // what would land. Deciding "nothing to commit" from them would abandon the + // merge — which is why the empty-diff probe is gated on !isMergeInProgress. + // Sets up a conflicted history and leaves the caller mid-sequence. `rel` (the + // file the commit call names) is never touched by the conflict, so it always + // contributes no diff of its own — which is what puts these arms on the + // empty-diff branch under test. + function conflictedSequence(kind) { + const shared = path.posix.join('.planning', 'shared.md'); + fs.writeFileSync(path.join(tmpDir, shared), 'base\n'); + gitOrThrow(['add', '--', shared], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'shared base'], { cwd: tmpDir }); + const trunk = gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir }).trim(); + + if (kind === 'revert') { + fs.writeFileSync(path.join(tmpDir, shared), 'second\n'); + gitOrThrow(['commit', '-am', 'second'], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'third\n'); + gitOrThrow(['commit', '-am', 'third'], { cwd: tmpDir }); + runGit(['revert', '--no-edit', 'HEAD~1'], { cwd: tmpDir }); + } else { + gitOrThrow(['checkout', '-b', 'side'], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'side\n'); + gitOrThrow(['commit', '-am', 'side edit'], { cwd: tmpDir }); + gitOrThrow(['checkout', trunk], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'trunk\n'); + gitOrThrow(['commit', '-am', 'trunk edit'], { cwd: tmpDir }); + runGit([kind === 'merge' ? 'merge' : 'cherry-pick', 'side'], { cwd: tmpDir }); + } + fs.writeFileSync(path.join(tmpDir, shared), 'resolved\n'); + gitOrThrow(['add', '--', shared], { cwd: tmpDir }); + } + + // The one state where `git diff` and `git commit -- ` genuinely + // disagree. `--assume-unchanged` makes `git add` stage nothing and BOTH diff + // forms (`--cached` and `HEAD`) report no difference, while + // `git commit -- ` reads the working tree directly and records it. The + // pre-#3776 build therefore COMMITTED this, and the guard must not turn a + // purely diagnostic fix into a silent drop of content the caller named in + // `--files`. Asking `git commit --dry-run` preserves the pre-fix outcome + // exactly, because it is the same decision the real commit makes. + test('a modified assume-unchanged path is still committed, not swallowed by the guard', () => { + const rel = commitFixtureFile(); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, rel), 'hello\nmodified under assume-unchanged\n'); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, true, + 'git commit -- reads the working tree and records it; the guard must not pre-empt that'); + assert.strictEqual( + gitOrThrow(['show', 'HEAD:' + rel], { cwd: tmpDir }), + 'hello\nmodified under assume-unchanged\n', + 'and the content it records must be the working-tree content'); + }); + + // THE OTHER DIRECTION, and the reason the check compares CONTENT rather than + // stopping at the `ls-files -v` tag. An unmodified assume-unchanged path has + // nothing to record; falling through on the tag alone would hand it to + // `git commit`, which — with any unrelated modified file present — prints + // `no changes added to commit`, a string the fallback does not match, and + // returns `commit_failed`. That is #3776 re-entered from the other side, the + // same shape `--ignore-submodules=none` would have re-entered it. Pinned so a + // later simplification to a tag-only test cannot pass. + test('an UNMODIFIED assume-unchanged path still reports nothing_to_commit, even with unrelated dirt', () => { + const rel = commitFixtureFile(); + const unrelated = path.posix.join('.planning', 'unrelated.md'); + fs.writeFileSync(path.join(tmpDir, unrelated), 'seed\n'); + gitOrThrow(['add', '--', unrelated], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'seed unrelated'], { cwd: tmpDir }); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir }); + // Unrelated modified work present — this is what turns git's answer from + // `nothing to commit` into `no changes added to commit`. + fs.writeFileSync(path.join(tmpDir, unrelated), 'unrelated edit\n'); + + assert.strictEqual(commitFiles(rel).reason, 'nothing_to_commit', + 'nothing would land for the named path, so the guard must still answer nothing_to_commit'); + }); + + // `--skip-worktree` is NOT a second instance of the above, and the PR body + // used to group them. `git add` exits 1 under it (the path reads as outside + // the sparse-checkout definition), so it fails closed as `staging_failed` + // ABOVE this guard and never reaches the empty-diff decision at all. + test('a modified skip-worktree path fails closed as staging_failed, never reaching the guard', () => { + const rel = commitFixtureFile(); + gitOrThrow(['update-index', '--skip-worktree', '--', rel], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, rel), 'hello\nmodified under skip-worktree\n'); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'staging_failed', + 'git add refuses the path, so the staging-failure block above the guard owns this case'); + }); + + // The same flag with the path ABSENT from disk — the canonical sparse shape — + // takes a DIFFERENT route, and the distinction is worth pinning because the + // obvious reading of the arm above ("skip-worktree never reaches the guard") + // is too strong. A missing path is skipped before `git add` runs at all + // (#2014), so `stagedPaths` is empty and the guard's own + // `stagedPaths.length === 0` arm answers it. `nothing_to_commit` is the + // correct answer there — the file does not exist, so a commit would record + // nothing — and it is the PRE-FIX answer too, unchanged by this PR. + test('a skip-worktree path absent from disk reports nothing_to_commit via the missing-path arm', () => { + const rel = commitFixtureFile(); + gitOrThrow(['update-index', '--skip-worktree', '--', rel], { cwd: tmpDir }); + fs.unlinkSync(path.join(tmpDir, rel)); + + assert.strictEqual(commitFiles(rel).reason, 'nothing_to_commit', + 'a missing path is skipped before git add, so the length === 0 arm owns this — not staging_failed'); + }); + + // THREE ARMS PINNING WHY THE PROBE ASKS GIT RATHER THAN RECONSTRUCTING ITS + // ANSWER. Each one reds if the dry run is replaced by a + // `hash-object` vs `HEAD:` blob comparison, and each is a silent drop + // of content the caller named — the exact class this whole guard is careful + // about. + + // A mode-only change leaves the blob identical, so a content comparison sees + // nothing — while `git commit -- ` records the new mode. + test('a mode-only change to an assume-unchanged path is still committed', (t) => { + const rel = commitFixtureFile('exec.md'); + // Windows, and any checkout with `core.filemode=false`, cannot represent + // the bit — `chmodSync` would then be a no-op and this arm would pass while + // pinning nothing. Assert the precondition and skip loudly instead. + gitOrThrow(['config', 'core.filemode', 'true'], { cwd: tmpDir }); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir }); + fs.chmodSync(path.join(tmpDir, rel), 0o755); + if (!/^100755 /.test(gitOrThrow(['ls-files', '-s', '--', rel], { cwd: tmpDir })) + && (fs.statSync(path.join(tmpDir, rel)).mode & 0o111) === 0) { + t.skip('filesystem cannot represent the executable bit — nothing to pin here'); + return; + } + + assert.strictEqual(commitFiles(rel).committed, true, + 'the mode moved and git would record it, so the guard must not report nothing_to_commit'); + assert.match( + gitOrThrow(['ls-tree', 'HEAD', '--', rel], { cwd: tmpDir }), /^100755 /, + 'and the recorded mode must actually be the executable one'); + }); + + // A non-ASCII path is rendered QUOTED by `git ls-files -v` under the default + // `core.quotePath` (`"caf\303\251.md"`), so any probe that parses the path + // out of that output reads a filename that does not exist and silently + // concludes there is nothing to commit. + test('a modified assume-unchanged path with a non-ASCII name is still committed', () => { + const rel = commitFixtureFile('caf\u00e9.md'); + // PIN the quoting explicitly. This arm's whole point is that a probe + // parsing the path out of `ls-files -v` reads `"caf\303\251.md"` and finds + // no such file; under an ambient `core.quotePath=false` the rejected + // implementation would pass here and the arm would be vacuous. + gitOrThrow(['config', 'core.quotePath', 'true'], { cwd: tmpDir }); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, rel), 'modified\n'); + + assert.strictEqual(commitFiles(rel).committed, true, + 'core.quotePath must not be able to hide a real change from the probe'); + assert.strictEqual(gitOrThrow(['show', 'HEAD:' + rel], { cwd: tmpDir }), 'modified\n'); + }); + + // The probe compares the WORKING TREE to HEAD, so an unborn HEAD makes it + // fatal (rc 128). That must fall through to the commit rather than be read as + // "nothing to commit" — there is plenty to commit in a repo with no commits. + test('an unborn HEAD falls through to the commit rather than reporting nothing_to_commit', (t) => { + const fresh = createTempDir(); + // REGISTERED teardown, not a trailing statement: `fresh` lives outside + // `tmpDir`, so afterEach does not reach it and any failing assertion below + // would leak a git repo into the temp root. + t.after(() => cleanup(fresh)); + fs.mkdirSync(path.join(fresh, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(fresh, '.planning', 'config.json'), '{}\n'); + fs.writeFileSync(path.join(fresh, '.planning', 'doc.md'), 'first content\n'); + gitOrThrow(['init', '-q', '.'], { cwd: fresh }); + gitOrThrow(['config', 'user.email', 't@t'], { cwd: fresh }); + gitOrThrow(['config', 'user.name', 't'], { cwd: fresh }); + + const result = runGsdTools('commit "m" --files .planning/doc.md', fresh); + const payload = (result.output && result.output.trim()) ? result.output : result.error; + const output = JSON.parse(payload); + assert.strictEqual(output.committed, true, + 'the very first commit in a repo must not be swallowed by the empty-diff guard'); + }); + + // git refuses a partial commit during a cherry-pick exactly as it does during + // a merge, so the guard must stay out of the way there too — this arm pins + // that the pre-fix outcome is preserved rather than turned into a silent + // no-op. Driven, not assumed: the three sequencer states disagree. + test('a cherry-pick in progress keeps its pre-existing outcome', () => { + const rel = commitFixtureFile(); + conflictedSequence('cherry-pick'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'CHERRY_PICK_HEAD')), + 'fixture must leave a cherry-pick in progress'); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'commit_failed', + 'git refuses the partial commit here; that must not become a silent nothing_to_commit'); + assert.match(String(output.error), /partial commit/, + "git's own refusal must reach the caller"); + }); + + // REVERT_HEAD is deliberately NOT in the refusal set: a revert permits partial + // commits, so the fix must still apply there. Including it would suppress the + // fix during a revert and reintroduce the misreport. + test('a revert in progress still reports nothing_to_commit, not the hook rejection', () => { + const rel = commitFixtureFile(); + conflictedSequence('revert'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'REVERT_HEAD')), + 'fixture must leave a revert in progress'); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit', + 'a revert permits partial commits, so the empty-diff guard must still apply'); + }); + + test('a merge in progress is still concluded when the named paths carry no diff', () => { + const shared = path.posix.join('.planning', 'shared.md'); + fs.writeFileSync(path.join(tmpDir, shared), 'base\n'); + gitOrThrow(['add', '--', shared], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'shared base'], { cwd: tmpDir }); + const rel = commitFixtureFile(); + + const trunk = gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir }).trim(); + gitOrThrow(['checkout', '-b', 'side'], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'side\n'); + gitOrThrow(['commit', '-am', 'side edit'], { cwd: tmpDir }); + gitOrThrow(['checkout', trunk], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'trunk\n'); + gitOrThrow(['commit', '-am', 'trunk edit'], { cwd: tmpDir }); + + // Conflicting merge, then resolve it so the index carries real content. + runGit(['merge', 'side'], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'resolved\n'); + gitOrThrow(['add', '--', shared], { cwd: tmpDir }); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'MERGE_HEAD')), + 'fixture must leave a merge in progress'); + + // `rel` is committed and unmodified — it contributes no diff of its own. + const output = commitFiles(rel); + assert.strictEqual(output.committed, true, + 'the merge must still be concluded, not reported as nothing to commit'); + assert.ok(!fs.existsSync(path.join(tmpDir, '.git', 'MERGE_HEAD')), + 'MERGE_HEAD must be gone once the merge commit lands'); + }); +}); + +// #3859: the empty-diff probe must answer the question `git commit -- ` +// asks. `git diff` is porcelain and honours user configuration the commit does +// not, so an unpinned probe lets a caller's config decide whether the guard +// fires — and every arm below was driven against git 2.54 by confirming that +// `git commit -- ` records exactly the change the unpinned probe reports +// as absent. +describe('#3859: the empty-diff probe is pinned against diff-only configuration', () => { + const { createTempGitProject } = require('./helpers.cjs'); + let tmpDir; + + beforeEach(() => { + tmpDir = createTempGitProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + function commitFiles(rel) { + const result = runGsdTools('commit "m" --files ' + rel, tmpDir); + const payload = (result.output && result.output.trim()) ? result.output : result.error; + return JSON.parse(payload); + } + + // Sub-repos created by `bumpedSubmodule()` are SIBLINGS of `tmpDir`, so the + // `afterEach` above does not reach them. Registering them here cleans every + // caller at once and — unlike a trailing `cleanup(subSrc)` in each test body + // — survives a failing assertion, which would otherwise leak a git repo into + // the temp root. + const strayRepos = []; + afterEach(() => { + while (strayRepos.length > 0) cleanup(strayRepos.pop()); + }); + + // A submodule whose recorded gitlink is AHEAD of what the superproject has + // committed — i.e. `git commit -- ` has something real to record. + function bumpedSubmodule() { + const subSrc = path.join(tmpDir, '..', path.basename(tmpDir) + '-sub'); + strayRepos.push(subSrc); + fs.mkdirSync(subSrc, { recursive: true }); + gitOrThrow(['init', '-q', '.'], { cwd: subSrc }); + gitOrThrow(['config', 'user.email', 't@t'], { cwd: subSrc }); + gitOrThrow(['config', 'user.name', 't'], { cwd: subSrc }); + fs.writeFileSync(path.join(subSrc, 'f.txt'), 'v1\n'); + gitOrThrow(['add', 'f.txt'], { cwd: subSrc }); + gitOrThrow(['commit', '-m', 'v1'], { cwd: subSrc }); + + gitOrThrow(['-c', 'protocol.file.allow=always', 'submodule', 'add', '-q', subSrc, 'sub'], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'add submodule'], { cwd: tmpDir }); + + fs.writeFileSync(path.join(subSrc, 'f.txt'), 'v2\n'); + gitOrThrow(['add', 'f.txt'], { cwd: subSrc }); + gitOrThrow(['commit', '-m', 'v2'], { cwd: subSrc }); + gitOrThrow(['-c', 'protocol.file.allow=always', 'submodule', 'update', '--remote', '--', 'sub'], { cwd: tmpDir }); + } + + // `diff.ignoreSubmodules=all` is local config; `.gitmodules` `ignore = all` is + // CHECKED IN and so arrives with a clone, needing no local setting at all — + // which makes it the stronger of the two vectors, and the one a reviewer + // reading only `diff.ignoreSubmodules` would not reach. + for (const vector of ['diff.ignoreSubmodules', '.gitmodules ignore']) { + test(`a submodule bump is not reported as nothing_to_commit under ${vector}=all`, () => { + bumpedSubmodule(); + if (vector === 'diff.ignoreSubmodules') { + gitOrThrow(['config', 'diff.ignoreSubmodules', 'all'], { cwd: tmpDir }); + } else { + gitOrThrow(['config', '-f', '.gitmodules', 'submodule.sub.ignore', 'all'], { cwd: tmpDir }); + gitOrThrow(['add', '--', '.gitmodules'], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'gitmodules ignore=all'], { cwd: tmpDir }); + } + + const before = gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(); + const output = commitFiles('sub'); + + assert.notStrictEqual(output.reason, 'nothing_to_commit', + 'the gitlink moved and `git commit -- sub` records it, so the probe must not say there is nothing'); + assert.strictEqual(output.committed, true); + assert.notStrictEqual( + gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(), before, + 'the recorded gitlink must actually advance'); + }); + } + + // A submodule path cannot be hashed at all (`fatal: Unable to hash sub`), + // while `git commit -- sub` advances the recorded gitlink. + test('an assume-unchanged submodule with an advanced gitlink is still committed', () => { + bumpedSubmodule(); + const before = gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(); + gitOrThrow(['update-index', '--assume-unchanged', '--', 'sub'], { cwd: tmpDir }); + + assert.notStrictEqual(commitFiles('sub').reason, 'nothing_to_commit', + 'the gitlink would advance, so the guard must stand aside'); + assert.notStrictEqual( + gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(), before, + 'and the recorded gitlink must actually advance'); + }); + + // The other direction, and the reason the pin is `=dirty` rather than `=none`. + // A partial commit of a submodule path records the GITLINK, which moves only + // when the submodule's HEAD does — so a merely dirty submodule WORKTREE would + // land nothing. Under `--ignore-submodules=none` the probe reports a + // difference there and sends an empty call back to `git commit`, which is the + // #3776 misreport re-entered from the other side. Pinned so a later widening + // to `=none` cannot pass. + test('a dirty submodule worktree with an unchanged gitlink still reports nothing_to_commit', () => { + bumpedSubmodule(); + gitOrThrow(['add', '--', 'sub'], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'bump sub'], { cwd: tmpDir }); + fs.appendFileSync(path.join(tmpDir, 'sub', 'f.txt'), 'dirty\n'); + + assert.strictEqual(commitFiles('sub').reason, 'nothing_to_commit', + 'nothing would land, so nothing_to_commit is the correct answer, not a misreport'); + }); + + // No submodule involved. A textconv driver maps two different blobs to the + // same text, so `git diff --quiet HEAD` reports no difference while + // `git commit -- ` records the new blob. + test('a change hidden by a textconv driver is not reported as nothing_to_commit', () => { + const rel = path.posix.join('.planning', 'binaryish.md'); + fs.writeFileSync(path.join(tmpDir, rel), 'A\n'); + fs.writeFileSync(path.join(tmpDir, '.gitattributes'), 'binaryish.md diff=flat\n'); + gitOrThrow(['add', '--', rel, '.gitattributes'], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'seed'], { cwd: tmpDir }); + // A textconv that collapses every input to one constant. `#` swallows the + // filename git appends, so the driver ignores its argument entirely. + gitOrThrow(['config', 'diff.flat.textconv', 'echo CONSTANT #'], { cwd: tmpDir }); + + fs.writeFileSync(path.join(tmpDir, rel), 'B\n'); + const output = commitFiles(rel); + + assert.notStrictEqual(output.reason, 'nothing_to_commit', + 'the blob changed and the commit would record it — textconv only changes how the DIFF renders'); + assert.strictEqual(output.committed, true); + assert.strictEqual( + gitOrThrow(['show', 'HEAD:' + rel], { cwd: tmpDir }), 'B\n', + 'the new content must actually be recorded'); + }); +}); + + describe('#2279: map-codebase date stamp instructions overwrite existing dates', () => { const REPO_ROOT = path.join(__dirname, '..'); diff --git a/tests/commit-files-pathspec.test.cjs b/tests/commit-files-pathspec.test.cjs index 1538b48cc..86868e874 100644 --- a/tests/commit-files-pathspec.test.cjs +++ b/tests/commit-files-pathspec.test.cjs @@ -317,7 +317,7 @@ const LIB = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib'); * returning the parsed JSON result and the git argv list that was actually * executed (so "git commit never ran" is asserted directly, not inferred). */ -function commitWithFailingAdd({ cwd, files, failFor = [], stderr = 'fatal: injected staging failure', timeout = false, amend = false, gitVerb = 'add' }) { +function commitWithFailingAdd({ cwd, files, failFor = [], stderr = 'fatal: injected staging failure', timeout = false, amend = false, gitVerb = 'add', matchArg = null }) { const callsOut = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2608-')), 'calls.json'); // `timeout` is `false` | `true` (alias for `'posix'`) | `'posix'` | `'windows'` — // #3050: the shared isSpawnTimeout predicate only requires `error.code === @@ -333,11 +333,12 @@ const failFor = ${JSON.stringify(failFor)}; const stderrText = ${JSON.stringify(stderr)}; const timeoutShape = ${JSON.stringify(timeoutShape)}; const gitVerb = ${JSON.stringify(gitVerb)}; +const matchArg = ${JSON.stringify(matchArg)}; const real = projection.execGit; const calls = []; projection.execGit = (args, opts) => { calls.push(args); - if (args[0] === gitVerb && failFor.includes(args[args.length - 1])) { + if (args[0] === gitVerb && (matchArg === null || args.includes(matchArg)) && failFor.includes(args[args.length - 1])) { if (timeoutShape === 'posix') { // The exact shape spawnSync produces on a POSIX timeout, which // shell-command-projection surfaces as signal + error.code. @@ -779,6 +780,386 @@ describe('#2608: commit --files fails closed when git add fails', () => { }); }); +describe('#3859: an unanswered sequencer probe must not open the empty-diff guard', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempGitProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // `execGit` reports a spawn timeout as `exitCode: 1` (`_spawnResult`: + // `result.status ?? 1`) — byte-identical to the code `rev-parse --verify` + // returns for a ref that does not exist. So "the probe says no merge" and + // "the probe never answered" are the same value, and the #3776 guard read + // both as "no merge". These arms pin the conservative reading. + // + // The seam is `commitWithFailingAdd`'s injected `execGit` with + // `gitVerb: 'rev-parse'`: `args[0]` is `rev-parse` and `args[args.length-1]` + // is the ref, so `failFor: ['MERGE_HEAD']` selects exactly that one probe and + // leaves every other git call real. + + // Leaves a conflicted merge or cherry-pick in progress with `bystander` + // committed and unmodified — the empty-diff shape that reaches the guard. + const BYSTANDER = path.posix.join('.planning', 'bystander.md'); + function conflictedSequence(kind) { + const shared = path.posix.join('.planning', 'shared.md'); + fs.writeFileSync(path.join(tmpDir, shared), 'base\n'); + fs.writeFileSync(path.join(tmpDir, BYSTANDER), 'bystander\n'); + gitOrThrow(['add', '--', shared, BYSTANDER], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'shared base'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + const trunk = gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }).trim(); + gitOrThrow(['checkout', '-b', 'side'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + fs.writeFileSync(path.join(tmpDir, shared), 'side\n'); + gitOrThrow(['commit', '-am', 'side edit'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['checkout', trunk], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + fs.writeFileSync(path.join(tmpDir, shared), 'trunk\n'); + gitOrThrow(['commit', '-am', 'trunk edit'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + // Deliberately conflicting — that is what leaves the sequencer ref behind. + spawnSync('git', [kind === 'merge' ? 'merge' : 'cherry-pick', 'side'], { + cwd: tmpDir, encoding: 'utf8', timeout: STAGING_GIT_TIMEOUT_MS, + }); + fs.writeFileSync(path.join(tmpDir, shared), 'resolved\n'); + gitOrThrow(['add', '--', shared], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + } + + for (const shape of ['posix', 'windows']) { + test(`a timed-out MERGE_HEAD probe (${shape}) reaches git instead of silently abandoning the merge`, () => { + conflictedSequence('merge'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'MERGE_HEAD')), + 'fixture must leave a merge in progress'); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [BYSTANDER], + failFor: ['MERGE_HEAD'], + gitVerb: 'rev-parse', + timeout: shape, + }); + + assert.notEqual(result.reason, 'nothing_to_commit', + 'an unanswered merge probe must not be read as "no merge in progress" — doing so decides ' + + 'nothing_to_commit from a pathspec git will not honour and leaves the merge unconcluded'); + assert.equal(result.reason, 'commit_failed', + 'git must be the one to refuse the partial commit, loudly, as it did before #3776'); + assert.ok(gitCalls.some((a) => a[0] === 'commit'), + 'the guard must fall through to git commit rather than returning early'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'MERGE_HEAD')), + 'the merge must still be in progress — silently abandoning it is the defect'); + }); + } + + test('a timed-out CHERRY_PICK_HEAD probe reaches git too', () => { + conflictedSequence('cherry-pick'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'CHERRY_PICK_HEAD')), + 'fixture must leave a cherry-pick in progress'); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [BYSTANDER], + failFor: ['CHERRY_PICK_HEAD'], + gitVerb: 'rev-parse', + timeout: true, + }); + + assert.notEqual(result.reason, 'nothing_to_commit', + 'the cherry-pick probe carries the identical conflation — #3776 added it, so it is in scope here'); + assert.equal(result.reason, 'commit_failed'); + assert.ok(gitCalls.some((a) => a[0] === 'commit')); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'CHERRY_PICK_HEAD')), + 'the cherry-pick must still be in progress'); + }); + + // THE ADAPTATION, PINNED. The obvious implementation — treat the timeout as + // `isMergeInProgress` — also flips `canScope`, which is PRE-EXISTING and + // gates the pathspec. A spurious timeout would then turn a scoped commit into + // a bare one and record whatever else happened to be staged, under a message + // describing only the named file: #2112, reintroduced by the fix for a + // misreport. The timeout must reach `partialCommitRefused` and nothing else. + test('a timed-out MERGE_HEAD probe outside a merge still commits ONLY the named paths', () => { + const named = path.posix.join('.planning', 'named.md'); + const unrelated = path.posix.join('.planning', 'unrelated.md'); + fs.writeFileSync(path.join(tmpDir, named), 'seed\n'); + fs.writeFileSync(path.join(tmpDir, unrelated), 'seed\n'); + gitOrThrow(['add', '--', named, unrelated], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + + // A real change to the named file, and an UNRELATED file sitting staged in + // the index — the #2112 shape a bare commit would sweep up. + fs.writeFileSync(path.join(tmpDir, named), 'named edit\n'); + fs.writeFileSync(path.join(tmpDir, unrelated), 'unrelated edit\n'); + gitOrThrow(['add', '--', unrelated], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [named], + failFor: ['MERGE_HEAD'], + gitVerb: 'rev-parse', + timeout: true, + }); + + assert.equal(result.committed, true, 'the commit must still happen — there is a real diff'); + const commitCall = gitCalls.find((a) => a[0] === 'commit'); + assert.ok(commitCall, 'git commit must have run'); + assert.ok(commitCall.includes('--') && commitCall.includes(named), + 'the pathspec must survive the timeout: routing it through isMergeInProgress would drop it'); + assert.deepEqual(committedFiles(tmpDir), [named], + 'only the named path may land — the staged unrelated file must not be swept in (#2112)'); + }); + + // NEGATIVE CONTROL for the conservative reading: outside a merge, treating an + // unanswered probe as "refused" must not manufacture a DIFFERENT answer. It + // falls through to git, git says there is nothing to commit, and the caller + // sees the same reason it would have seen anyway. + test('a timed-out MERGE_HEAD probe on a genuinely empty diff still reports nothing_to_commit', () => { + const rel = path.posix.join('.planning', 'quiet.md'); + fs.writeFileSync(path.join(tmpDir, rel), 'unchanged\n'); + gitOrThrow(['add', '--', rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'quiet'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: ['MERGE_HEAD'], + gitVerb: 'rev-parse', + timeout: true, + }); + + assert.equal(result.reason, 'nothing_to_commit', + 'the conservative reading defers to git, which reports the same thing the guard would have'); + assert.ok(gitCalls.some((a) => a[0] === 'commit'), + 'and it gets there by asking git, not by short-circuiting on an unanswered probe'); + }); + + // Round 4, review finding 1 + its coverage half. The `stagedPaths.length === 0` + // disjunct is NOT gated on `partialCommitRefused`, so an all-missing `--files` + // list during a merge or cherry-pick returns `nothing_to_commit` while the + // sequencer ref is still live and the resolved content sits staged. + // + // These arms pin that as the DELIBERATE answer, not an oversight. Gating the + // disjunct sends this case to a BARE `git commit` (stagedPaths is empty, so + // `canScope` is false), which git permits during a merge and which CONCLUDES + // it — recording the whole index under a message naming a path that does not + // exist, and reporting `committed: true`. Trading a report that writes nothing + // for one that silently writes everything is the trade the timeout routing + // above already refuses. + // + // The behaviour is also pre-existing: before this fix the identical + // short-circuit ran ABOVE the MERGE_HEAD probe, so it never consulted the + // sequencer either. Nothing here is a regression pin; these are behaviour + // pins, and they red on the gated implementation rather than at base. + for (const kind of ['merge', 'cherry-pick']) { + const ref = kind === 'merge' ? 'MERGE_HEAD' : 'CHERRY_PICK_HEAD'; + test(`all named --files paths missing during a ${kind} reports nothing_to_commit and leaves the ${kind} open`, () => { + conflictedSequence(kind); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', ref)), + `fixture must leave a ${kind} in progress`); + const before = headCount(tmpDir); + const missing = path.posix.join('.planning', 'never-produced.md'); + assert.ok(!fs.existsSync(path.join(tmpDir, missing)), + 'the named path must genuinely be absent from disk'); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [missing], + failFor: [], + }); + + assert.equal(result.reason, 'nothing_to_commit', + 'every named path was skipped before git add (#2014), so there is nothing declared to commit'); + assert.equal(result.committed, false); + assert.ok(!gitCalls.some((a) => a[0] === 'commit'), + 'git commit must NOT run — a bare commit here would conclude the sequencer with the whole index'); + assert.equal(headCount(tmpDir), before, + 'and no commit may be recorded'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', ref)), + `the ${kind} must still be in progress — the caller's own resolution is untouched`); + assert.equal( + gitOrThrow(['diff', '--cached', '--name-only'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }).trim(), + path.posix.join('.planning', 'shared.md'), + 'and the staged resolution is still staged, not swallowed'); + }); + } +}); + + +describe('#3859: an unanswered dry-run probe must not close the assume-unchanged path', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempGitProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // Round 3. The `git commit --dry-run --porcelain` probe is the ONE probe in + // the guard whose rc 0 is the reassuring answer ("git would record + // something, stand aside"). `execGit` reports a spawn timeout as + // `exitCode: 1` (`_spawnResult`: `result.status ?? 1`) — byte-identical to + // git's own "nothing to record" — so an unanswered probe read as a + // confirmed one, the guard returned `nothing_to_commit`, and content the + // caller named in `--files` was never committed. The diff probe is safe by + // construction (rc 0 is the DANGEROUS answer there, and a timeout can only + // produce non-zero); the two sequencer probes carry an explicit + // `isSpawnTimeout` disjunct. This probe now stands aside on anything but a + // CONFIRMED rc 1 with no spawn error — the same "only a confirmed answer + // decides" rule the diff probe already follows, from the other direction. + // + // Seam: `commitWithFailingAdd`'s injected `execGit` with `gitVerb: 'commit'` + // AND `matchArg: '--dry-run'` — the real `git commit … -- ` shares + // both `args[0]` and its last argument with the probe, so the verb alone + // would intercept the commit this arm exists to prove still happens. + function assumeUnchangedModified() { + const rel = path.posix.join('.planning', 'assumed.md'); + fs.writeFileSync(path.join(tmpDir, rel), 'seed\n'); + gitOrThrow(['add', '--', rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed assumed'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + fs.writeFileSync(path.join(tmpDir, rel), 'modified under assume-unchanged\n'); + return rel; + } + + for (const shape of ['posix', 'windows']) { + test(`a timed-out dry-run probe (${shape}) still commits the modified assume-unchanged path`, () => { + const rel = assumeUnchangedModified(); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: [rel], + gitVerb: 'commit', + matchArg: '--dry-run', + timeout: shape, + }); + + assert.notEqual(result.reason, 'nothing_to_commit', + 'an unanswered dry run must not be read as "nothing would be recorded" — that drops content ' + + 'the caller named in --files and reports there was nothing to write'); + assert.equal(result.committed, true, 'the commit must still happen — git would have recorded it'); + assert.ok(gitCalls.some((a) => a[0] === 'commit' && a.includes('--dry-run')), + 'the probe must have been the call that timed out'); + assert.ok(gitCalls.some((a) => a[0] === 'commit' && !a.includes('--dry-run')), + 'and the guard must fall through to the real git commit'); + assert.equal( + gitOrThrow(['show', 'HEAD:' + rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }), + 'modified under assume-unchanged\n', + 'and the content it records must be the working-tree content'); + }); + } + + // A genuine git error from the dry run (rc 128, no spawn error) is not a + // "nothing to record" answer either. It falls through to git, which fails + // the same way it would have — loudly — rather than manufacturing a no-op. + test('a dry-run probe that errors (rc 128) still reaches git instead of reporting nothing_to_commit', () => { + const rel = assumeUnchangedModified(); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: [rel], + gitVerb: 'commit', + matchArg: '--dry-run', + timeout: false, + }); + + assert.notEqual(result.reason, 'nothing_to_commit', + 'rc 128 is a failed probe, not a confirmed empty one'); + assert.equal(result.committed, true); + assert.ok(gitCalls.some((a) => a[0] === 'commit' && !a.includes('--dry-run'))); + }); + + // The `ls-files -v` read is an optimisation, never a gate: when it cannot + // answer, the dry run runs anyway. Pinned, because the reviewer named it + // as untested and a later "tidy-up" that returns false on a failed read + // would drop the content by a different door. + test('a timed-out ls-files probe falls through to the dry run, and the path is still committed', () => { + const rel = assumeUnchangedModified(); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: [rel], + gitVerb: 'ls-files', + timeout: true, + }); + + assert.equal(result.committed, true, + 'an unreadable tag list must not decide anything — the dry run answers instead'); + assert.ok(gitCalls.some((a) => a[0] === 'commit' && a.includes('--dry-run')), + 'the dry run must have run despite the unanswered ls-files read'); + assert.equal( + gitOrThrow(['show', 'HEAD:' + rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }), + 'modified under assume-unchanged\n'); + }); + + // NEGATIVE CONTROL for the conservative reading, mirroring the sequencer + // arms': an UNMODIFIED assume-unchanged path in a clean tree, dry run timed + // out. Standing aside must not manufacture a DIFFERENT answer — it falls + // through to git, git says there is nothing to commit, and the caller sees + // the same reason the guard would have given. + test('a timed-out dry-run probe on an unmodified assume-unchanged path still reports nothing_to_commit', () => { + const rel = path.posix.join('.planning', 'assumed.md'); + fs.writeFileSync(path.join(tmpDir, rel), 'seed\n'); + gitOrThrow(['add', '--', rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed assumed'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: [rel], + gitVerb: 'commit', + matchArg: '--dry-run', + timeout: true, + }); + + assert.equal(result.reason, 'nothing_to_commit', + 'the conservative reading defers to git, which reports the same thing the probe would have'); + assert.ok(gitCalls.some((a) => a[0] === 'commit' && !a.includes('--dry-run')), + 'and it gets there by asking git, not by short-circuiting on an unanswered probe'); + }); + + // Round 4, review finding 3. The probe's safety rests on `git commit + // --dry-run` not running `pre-commit`, which git 2.54 satisfies on its own — + // so this arm pins the FLAG, not an outcome, and that is deliberate: the + // outcome it protects is unobservable on a git that already declines to run + // the hook. On a git that DID run it, a rejecting hook exits 1, the closure + // reads that as a confirmed "nothing to record", and the caller's content is + // dropped under a `nothing_to_commit` report — #3776 re-entered through the + // probe. `--no-verify` removes the dependency on the version rather than + // documenting it. + // + // It is a seam assertion over the argv the guard actually issued, not a + // source grep: the flag is read off the executed call, so deleting it from + // the probe reds this arm. + test('the dry-run probe carries --no-verify so a hook-firing git cannot close the guard', () => { + const rel = assumeUnchangedModified(); + + const { gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: [], + }); + + const dryRuns = gitCalls.filter((a) => a[0] === 'commit' && a.includes('--dry-run')); + assert.equal(dryRuns.length, 1, + 'the assume-unchanged branch must have reached the dry-run probe exactly once'); + assert.ok(dryRuns[0].includes('--no-verify'), + 'the probe must not be able to execute a pre-commit hook, whatever the git version does by default'); + // And the REAL commit must not inherit it — #3776 is a bug about a hook + // whose message reached the caller wrongly, never a licence to skip hooks. + const realCommits = gitCalls.filter((a) => a[0] === 'commit' && !a.includes('--dry-run')); + assert.ok(realCommits.length > 0, 'the guard must have fallen through to a real commit'); + assert.ok(realCommits.every((a) => !a.includes('--no-verify')), + 'the real commit still runs the caller\'s hooks — only the probe is exempt'); + }); +}); + describe('workflow call sites declare --files (#2269)', () => { // WHAT COUNTS AS AN INVOCATION — the question this scan kept answering by // proxy, and kept getting wrong in both directions at once.