900504f985
* fix(#3776): decide nothing-to-commit from the staged diff, not from staging success
`cmdCommit`'s empty-diff guard tested `stagedPaths.length === 0`, but
`stagedPaths` records paths whose `git add` exited 0 — "did staging
succeed", not "is there anything to commit". Staging an already-committed,
unmodified file succeeds while contributing no diff, so the guard was
reachable only when every named path was missing from disk.
The ordinary empty-diff case therefore fell through to `git commit`, where
the only thing converting the failure back to `nothing_to_commit` was a
string match on git's output. Git runs the pre-commit hook before it
decides there is nothing to commit, so a rejecting hook pre-empted that
match and the caller was handed `commit_failed` carrying a gate message
about a commit that had nothing to gate.
Ask git whether the staged paths actually differ instead. Two conjuncts
are load-bearing: the `length === 0` short-circuit keeps the
all-missing-paths case exact (a pathspec-less `diff --cached` would test
the whole index, so unrelated staged work would suppress the guard), and
`!isMergeInProgress` keeps a merge from being abandoned — during a merge
git refuses a partial commit, so the pathspec describes nothing about what
would land.
Nine regression cases in tests/commands.test.cjs cover the brief's six
acceptance criteria plus the merge interaction. Against the pre-fix build
exactly one fails; the other eight pin behaviour that was already correct.
Residual sibling of #2608/#2693, which covered `git add` failing; this
covers `git add` succeeding and contributing nothing.
* fix(#3776): exempt a cherry-pick too, but never a revert
The empty-diff guard must not decide from a pathspec git will not honour.
That test was merge-only; git refuses a partial commit during a cherry-pick
for the same reason, so the guard would have fired there and reported a
silent `nothing_to_commit` where the pre-fix code surfaced git's refusal.
The three sequencer states do not agree, so this is driven rather than
reasoned by analogy (git 2.54):
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 excluded: enumerating it alongside the
other two — the obvious move — would suppress this fix during a revert and
reintroduce the very misreport it removes. Both new states are pinned by a
test, and the revert arm fails against the pre-fix build exactly as AC1 does.
`canScope` keeps its narrower merge-only test on purpose; widening it would
change pre-existing cherry-pick behaviour, which is outside this fix.
* fix(#3776): probe the working tree, not the index
`git commit -- <paths>` is a PARTIAL commit: it records the working-tree
content of those paths and ignores what is staged. The guard was probing
`git diff --cached` — the index — which answers a different question than
the commit asks.
Driven on the same path, in this order: `git add` an unmodified file, then
write to it, then probe.
git diff --cached --quiet -- p rc 0 ("nothing staged")
git diff --quiet HEAD -- p rc 1 ("the tree differs")
git commit -m m -- p committed the new content
So a working-tree write landing between the `git add` above and the probe —
another process in a shared checkout, which this project explicitly supports
— would let the guard report `nothing_to_commit` for a call that would have
recorded that content. Probing `HEAD` asks the question the commit answers.
Not reachable through this function single-threaded, because the staging
loop re-adds every named path immediately beforehand, so index and working
tree agree at the probe. The change is correctness by construction rather
than a fix for an observed miscommit.
An unborn HEAD makes `diff HEAD` fatal; that falls through to the commit as
any other probe error does, and is now pinned by a test — the first commit
in a repo must not be swallowed by an empty-diff guard.
Both added probes are now gated on the guard being able to fire at all, so
an unscoped commit and an `--amend` pay for neither.
Found by adversarial pre-filing review; the index/worktree distinction was
not something my own path-shape probes could have surfaced.
* test(#3776): pin the assume-unchanged boundary; correct a stale comment
`git update-index --assume-unchanged` makes `git add` stage nothing and makes
BOTH diff forms — `--cached` and `HEAD` — report no difference, so no
diff-based guard can see a change to such a path. `git commit -- <path>` is
the odd one out: it reads the working tree directly and records it.
So a modified assume-unchanged path now reports `nothing_to_commit` where it
previously committed. That is the answer consistent with this function's own
staging step, which honoured the flag one loop earlier — but it is a
behaviour change, and it belongs on the record as a decision rather than
surfacing later as a surprise.
Also corrects an AC3 comment still describing the `diff --cached` whole-index
form that the previous commit replaced.
* chore(#3776): set changeset fragment pr to 3859
The fragment carries the PR's own number, which is unknowable before the PR
exists. Backfilled post-create; the repo's changeset lint rejects the `pr: 0`
placeholder.
* fix(#3776): do not read an unanswered sequencer probe as "no merge"
`execGit` surfaces a spawn timeout as `exitCode: 1` (`_spawnResult`:
`result.status ?? 1`) — the same code `rev-parse --verify` returns for a ref
that does not exist. So the MERGE_HEAD and CHERRY_PICK_HEAD probes could not
tell "not in that state" from "never answered", and the empty-diff guard read
both as "not in that state". That is the one path in #3776 that did not fail
toward the previous behaviour: a timeout during a real merge decided
`nothing_to_commit` from a pathspec git will not honour and left the merge
unconcluded, where before it was a loud `commit_failed`.
Treat an unanswered probe as "assume the partial commit would be refused" —
which falls through to `git commit` and lets git speak for itself.
Routed into `partialCommitRefused` only, deliberately never into
`isMergeInProgress`. That flag also feeds the pre-existing `canScope`, and
widening it there is worse than the misreport it fixes: with `canScope` false
the commit runs bare, and a bare commit during a merge is PERMITTED — git
concludes the merge with the whole index under a message naming one file.
Driven: the whole-flag form reports `committed` where this form reports
`commit_failed`, and it drops the pathspec on the ordinary timeout, re-opening
the #2112 scope leak.
* fix(#3776): pin the empty-diff probe against diff-only configuration
`git diff` is porcelain and honours settings `git commit -- <paths>` does not,
so an unpinned probe let a caller's configuration decide whether the guard
fires. Driven against git 2.54, each with the paired `git commit -- <path>`
confirmed to record the change the unpinned 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 at
all — it is checked in, so it arrives with
a clone
diff=<driver> + textconv two different blobs converge to one text,
so the probe sees no change; 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,
which moves only when the submodule's HEAD does. Under `=none` a merely dirty
submodule work tree reports a difference the commit would not record, sending
an empty call back to `git commit` — the same misreport, re-entered from the
other side. `dirty` still overrides both `diff.ignoreSubmodules` and a
checked-in `.gitmodules` `ignore`, so the gitlink vectors stay closed.
`--no-ext-diff` is deliberately absent: `--quiet` short-circuits ahead of an
external diff driver, so `diff.<driver>.command` cannot invert the probe
(driven: rc 1 with and without the flag).
* docs(#3776): disclose the two outcome changes the changeset omitted
The body listed what stays unchanged and never named the arms whose
user-visible outcome moves, so neither would have reached the changelog:
- a modified path under `git update-index --assume-unchanged` now reports
`nothing_to_commit` where it was previously committed. `git add` already
honoured the flag one loop earlier; the guard reports what staging did.
Documented in a code comment and pinned by a test since the first round,
but absent from the fragment.
- naming a submodule whose work tree is dirty while its recorded commit has
not moved now reports `nothing_to_commit` rather than `commit_failed`,
because nothing would have landed. New in this round, from the
`--ignore-submodules=dirty` pin.
* test(#3776): use helpers.cleanup() for the submodule fixture teardown
`local/no-raw-rmsync-in-tests` rejects a bare `fs.rmSync` in a test: the
helper carries the Windows-EBUSY retry budget (`maxRetries`/`retryDelay`)
that a raw call does not, and a submodule work tree is exactly the shape
that holds handles open on Windows.
Caught by CI, not locally — the round ran the two affected suites but not
`npm run lint:ci`, so the repo's own rule never fired until the push. The
chain now exits 0 locally against this tree.
* fix(#3776): never drop a named assume-unchanged path
`--assume-unchanged` is the one state where `git diff` and
`git commit -- <paths>` genuinely disagree: `git add` stages nothing,
both diff forms report no difference, and `git commit -- <path>` still
reads the working tree and records it. The empty-diff guard therefore
reported `nothing_to_commit` about content the caller named in `--files`
and git would have written.
commit is made — so suppressing its misreport must not be paid for by a
silent drop. Same rule the timeout routing already follows: a fix for a
misreport may not cost content.
The guard now asks `git commit --dry-run --porcelain` whether the commit
would record anything, and stands aside on rc 0. That is the same
decision the real commit makes, so there is no second implementation of
it to drift. It does not run the `pre-commit` hook (driven: a rejecting
one neither fires nor writes a marker), which is what matters — a firing
`pre-commit` is the whole of #3776. It is NOT hook-free in general: git
2.54 fires `post-index-change` here, so a repo using that hook sees it
once for the probe and once for the commit. Stated rather than claimed
away.
Asking git rather than reconstructing its answer was reached by
measurement. Comparing `git hash-object` against `HEAD:<path>` was tried
and is wrong three ways, each a silent drop of named content: it misses a
mode-only change (`chmod +x` leaves the blob identical while the 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. Each has its own arm, and the
non-ASCII arm pins `core.quotePath` so it cannot go vacuous.
Falling through on the `ls-files` tag alone — without asking whether
anything would land — is also wrong: an UNMODIFIED assume-unchanged path
would reach `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 by its own arm.
The `ls-files` read is an optimisation, not a gate: it keeps the dry run
off the hot path when no assume-unchanged entry is present, and when it
cannot answer the dry run simply runs, because the dry run needs nothing
from it. Failing closed there would drop content and failing open would
re-enter #3776 — both are wrong answers to a question that can be asked
directly.
`--skip-worktree` is not a second instance. A present, modified one exits
1 from `git add` and fails closed as `staging_failed` above the guard; an
absent one is skipped before `git add` runs (#2014) and is answered by
the `stagedPaths.length === 0` arm, exactly as it was pre-fix. Both
shapes pinned, because the shorter claim ("never reaches the guard") is
too strong.
* test(#3776): register fixture teardown so a failed assertion cannot leak
`bumpedSubmodule()` creates its sub-repo as a SIBLING of `tmpDir`, and
the unborn-HEAD arm creates `fresh` outside it too, so the describe's
`afterEach(() => cleanup(tmpDir))` reaches neither. Both were cleaned by
a trailing statement in the test body, which any failing assertion above
it skips — leaking a git repo into the temp root.
`bumpedSubmodule()` now records the path and a describe-scoped
`afterEach` drains it, which covers all three of its callers at once;
the unborn-HEAD arm takes `t.after`, the form already used elsewhere in
this file.
Negative-controlled both ways with a deliberate assertion failure
injected into the dirty-submodule arm, under an overridden TMPDIR:
before, one `*-sub` repo survives the run; after, none.
* fix(#3776): never read an unanswered dry-run probe as "nothing to record"
The `git commit --dry-run --porcelain` probe that decides the assume-unchanged
boundary is the one probe in the guard whose rc 0 is the reassuring answer, so
it inverts the diff probe's safety: `execGit` collapses a spawn timeout (or any
spawn error) to `exitCode: 1`, byte-identical to git's own "nothing to record",
and the guard then reported `nothing_to_commit` about content named in
`--files` that git was never asked to write. Same conflation the sequencer
probes already defend against.
Only a CONFIRMED rc 1 with no spawn error closes the path now; a timeout, a
spawn error, or rc 128 falls toward the real commit, where git speaks for
itself. Five arms in tests/commit-files-pathspec.test.cjs pin it (posix +
windows timeout shapes, rc 128, the ls-files optimisation's own timeout, and a
negative control on an unmodified path); the injection helper gains an optional
`matchArg` so the dry run can be targeted without intercepting the real commit.
Also corrects the comment that claimed both sequencer probes are gated on
`guardApplies` — the MERGE_HEAD probe predates this fix and is unconditional.
* fix(#3776): probe with --no-verify so a hook-firing git cannot close the guard
Round 4, review finding 3 (Minor). The `git commit --dry-run --porcelain`
probe's safety rested on an empirical claim about one git version: that
`--dry-run` does not run `pre-commit`. git 2.54 satisfies it, but the failure a
differing version would produce is silent and lands in exactly #3776's own
configuration.
A `pre-commit` that fires and rejects exits 1 — the same code git returns for
"nothing to record" — so the closure would read it as a CONFIRMED empty answer,
drop the content the caller named in `--files`, and report `nothing_to_commit`.
That is #3776 re-entered through the probe the fix added.
`--no-verify` forecloses it structurally rather than documenting the version
dependency. Driven on git 2.54: rc-identical in both directions (rc 0
would-record, rc 1 nothing) with and without the flag, so it is behaviour-
neutral where the version already agrees.
Two claims deliberately NOT widened: `--no-verify` does not suppress
`post-index-change`, which still fires on this call with or without it (driven
both ways); and the real `git commit` is untouched — #3776 is a bug about a
hook's message reaching the caller wrongly, never a licence to skip hooks.
The new arm pins the FLAG rather than an outcome, because the outcome it
protects is unobservable on a git that already declines to run the hook. It is
a seam assertion over the argv the guard actually issued, not a source grep.
* test(#3776): pin all-missing --files during a merge or cherry-pick
Round 4, review finding 1 (Major) and finding 7 (Nit, its coverage half). The
review asks for the `stagedPaths.length === 0` disjunct to be gated on
`!partialCommitRefused`, or for the combination to be documented and tested.
Documented and tested — the gating is refused, with cause.
The premise is confirmed: the state is reachable exactly as described, and
during a merge the `nothing_to_commit` report does not tell the caller the merge
is still open. The prescription is not. 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 it.
Driven, git 2.54, through cmdCommit with the prescription applied:
cmdCommit(cwd, 'add the thing', ['.planning/never-produced.md'])
-> { "committed": true, "hash": "8e6bf45", "reason": "committed" }
MERGE_HEAD gone; HEAD is a 2-parent merge commit recording
.planning/shared.md with the caller's resolution content.
So the gating trades a report that writes nothing for one that silently writes
the whole index under a message naming a path that does not exist, and reports
success. That is the same trade the timeout routing already refuses one block
up, which is why the sequencer states gate the DIFF branch only.
The behaviour is also pre-existing and unchanged by this PR: at 86452da7 the
identical short-circuit sat ABOVE the MERGE_HEAD probe, so it never consulted
the sequencer either. The residual — a merge held open behind a
`nothing_to_commit` report — is offered as a separate issue alongside the three
already deferred, not folded into this fix.
These are behaviour pins, not regression tests: they pass at base and red on the
gated implementation (both arms, verified).
* docs(#3776): state the git-version provenance once, not at three claims
Round 4, review finding 6 (Nit). The guard carries ~159 comment lines around 32
lines of executable logic, and the review's specific complaint is that "driven
against git 2.54" is repeated near-verbatim in three places, which makes the
decision tree harder to scan.
Hoists the provenance to a single block header and reduces the three repeats to
the observation each actually carries. One claim keeps its version explicitly
and now says why: the `--no-verify` reasoning is version-SENSITIVE rather than
merely version-observed, so it is the one place the version is load-bearing
instead of incidental.
The behavioural matrix stays inline rather than moving to an ADR or a doc block.
Every claim in it is a constraint on the four flags immediately below it, and the
value of having it here is that the next reader who wants to "simplify" one of
those flags meets the driven counter-example in the same screen. Splitting the
constraint from the code it constrains is how the flags get dropped.
Comments only. No behaviour change; suite and lint:ci unchanged.
* docs(#3776): correct two driven figures in the new guard commentary
Both found by this round's own pre-push adversarial review, and both re-driven
before adopting.
1. The empty-paths rationale said a bare commit during a merge produces a
"three-parent commit". It produces a TWO-parent merge commit. Three was the
token count of `git rev-list --parents -n1 HEAD` (commit + two parents) read
as a parent count. `git cat-file -p HEAD | grep -c '^parent '` returns 2.
This round's commit message for the pins already said two, so the tree
contradicted itself.
2. The `post-index-change` disclosure said a repo using that hook "sees it once
for the probe and once for the commit". Driven with a counting hook: git
fires it TWICE per `git commit --dry-run`, and twice again for the real
commit — 2/2/2 across the flagged probe, the unflagged probe and the real
commit. The disclosure understated the cost by half in both halves.
Comments only. No behaviour change; suite 351/351 and lint:ci unchanged.
---------
Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
1.1 KiB
1.1 KiB
type, pr
| type | pr |
|---|---|
| Fixed | 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 -- <path> 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)