Restore commit hooks in the CLI - #15222
Conversation
`but commit` ran the commit hooks until 45a8387 replaced it with commit2, which carried none of them across. A repository whose quality gates live in `pre-commit` has had them silently skipped since, and the CLI is the surface the bundled agent skill directs agents to. Restores `pre-commit` and `post-commit` along with `-n`/`--no-hooks`, keeping the flag declaration and failure wording of the code commit2 replaced. `commit-msg` needs the message resolved before the commit is created, which commit2 inverted, so it is left for its own change. Separately, `join_output` tested `stderr.is_ascii()` where it meant `is_empty()`, discarding every ASCII-only hook message and reporting "hook produced no output" instead.
Every diff spec carries hunk headers, even when no ids are given, and `DiffSpec` drops headers that no longer match the worktree without saying so. A `pre-commit` hook that reformats a file therefore moved the headers computed before it ran, and the commit silently carried less than was asked for. With nothing singled out, the specs are rebuilt from the worktree as the hook left it, which is the result `git` gives a hook that stages its own edits. When files were singled out, the commit is refused rather than guessed at, and the worktree is left untouched.
Watching every path in the commit refused commits that were never at risk: additions and deletions carry no hunk headers, so the file is read whole while the commit is built, and the same is true of symlinks, binaries and files too large to diff. Only specs with hunk headers can go stale, which also keeps the largest files out of the snapshot. Rebuilding from the worktree also reached past what was being committed. A hook is free to write elsewhere, and files it created or merely dirtied were landing in the commit; `git` commits neither. The rebuild is now limited to the paths that were already going in. Adds the missing test for the refusal, which had none.
|
Thanks for working on this. Could you please also restore the |
|
I put together the It runs the hook after provided/editor message resolution but before the workspace transaction commits, so hook edits are retained and rejection rolls everything back. Merging that PR into this branch will update #15222 directly. |
libgit2 only honours GIT_CONFIG_NOSYSTEM and GIT_CONFIG_GLOBAL when the repository is opened FROM_ENV, which `but` never does, so the sandbox's config isolation does not reach any hook path. A developer with `core.hooksPath` set globally therefore had the hook these tests write skipped and their own hook run instead, and the tests failed. Pinning `core.hooksPath` per repository keeps the hook under test the one that runs. The wider leak - every libgit2-read config key escaping the sandbox - belongs in but-testsupport rather than here. The failure mode was pointed out by @sensei-woo.
The retired grammar spells the flag exactly as the modern one does, but it was listed among the forms with no modern equivalent, so a command carrying it was refused rather than translated. `but commit` runs hooks again, so it carries over as it stands. The unit test's refusal list and the CLI test both used this flag as their example of something untranslatable; they now use `--message-file`, which still is.
|
Took the Holding off on the rest. Both fixable. Worth sending as your own PR on top of this one. |
2 participants
🧠 Changes
but commitruns the repository'spre-commithook before writing the commit andpost-commitafter, with-n/--no-hooks(alias--no-verify) to bypass — restoring what45a8387f40("feat(but): replace old commit command with commit2") dropped. The flag declaration, the failure wording and the "post-commit failures don't undo the commit" rule are taken from the code commit2 replaced. The TUI's commit runs them too.A hook that rewrites files needs care.
DiffSpecdocuments that hunk headers failing to match the worktree are dropped, andpush_changes_from_uncommitted_area()expands every change into hunks — so evenbut commit -mwith no ids carries headers a hook can invalidate, and committing the specs computed beforehand would carry less than was asked for without saying so. So:retired_syntax.rslisted--no-hooksamong the retired forms with no modern equivalent, sobut commit -c -m "msg" --no-hookswas refused rather than translated. Both grammars spell it identically, so it now carries over.Separately,
join_outputtestedstderr.is_ascii()where it meantis_empty(), so every ASCII-only hook message was discarded and reported as "hook produced no output". That one also affects the desktop andbut push.☕️ Reasoning
Before commit2,
but commitran all three commit hooks. The rewrite carried none across, so a repository whose gates live inpre-commithas had them silently skipped since 24 July — and the CLI is the surface the bundled agent skill directs agents to in preference togit.PR #15139 documents the invariant a hook breaks: the changes handed to
create_commit"must describe the pre-commit state — in practice, run this before any other operation that could change what the worktree is based on". Apre-commithook is exactly such an operation, running between the specs being computed and the commit being made. Rebuilding restores that invariant; refusing is the answer when rebuilding would overrule a choice the user made.On parity with
git: there isn't any, and I would rather say so than imply it.gitcommits the index, so a hook's edits land only if the hook stages them. GitButler has no staging, and nothing here can tell a staging hook from one that merely writes, so the rule chosen is "the files already going into this commit, as the hook left them" — narrower than committing whatever the hook touched, wider than ignoring it. If you would rather it refuse in both cases, that is a smaller change than this one.commit-msgis not restored here. commit2 creates the commit and then rewords it, because the editor shows the commit's own diff, and that reword runs inside a transaction that owns theContextwhichhooks::commit_msgneeds. Restoring it means moving message resolution ahead of commit creation, and deciding what the hook is handed — a message read back from the commit has no trailing newline, so the commonecho ... >> "$1"sign-off hook welds itself onto the subject line. That belongs in its own change.Hooks default on, matching
but pushandgit. The desktop'srunCommitHooksdefaults to false, but it is frontend-onlylocalStoragewith no Rust counterpart, so the CLI cannot read it; worth knowing the two surfaces differ.A note on the test isolation fix
e71c886798pinscore.hooksPathper test repository. libgit2 only honoursGIT_CONFIG_NOSYSTEM/GIT_CONFIG_GLOBALwhen a repository is openedFROM_ENV, whichbutnever does, so the sandbox's config isolation does not reach any hook path. On a machine withcore.hooksPathset globally, the hook these tests write was skipped and the developer's own hook ran instead — I reproduced both halves. That is worth knowing beyond this PR: every libgit2-read config key escapes the sandbox today, and the general fix belongs inbut-testsupport. Happy to send that separately. The failure mode was pointed out by @sensei-woo.📌 Todos
Known and deliberately out of scope. None of these are introduced by this change - they are
pre-existing, and several are live in the desktop today. Listed so they are not mistaken for
oversights, and I am happy to take any of them on separately:
pre_commit_hook_diffspecsand then commits those same pre-hook changes, so a hook that reformats a file still drops the hunks it moved. This PR fixes the CLI only, which means the two surfaces now behave differently — arguably the more valuable follow-up, and it belongs inbut_corerather than here.butdeadlocks, becausegitsetsGIT_INDEX_FILE/GIT_EDITORandgit2-hooksdoes not, so the re-entry escape hatch never triggers; and abnormal termination during a hook leaves a staleindex.gitbutler-hook-backupthat blocks later commits until it is removed by hand.post-commitfailures only reachtracing. Both should go to stderr, asgitdoes.worktree_file_to_objectwould be filter-aware and constant-memory. Only files with hunk headers are read, so binaries and large files are already excluded, but it is the better primitive.but amend,squashand the other commit-creating commands still run no hooks.🎫 Affected issues
Addresses: #15209