fix(githooks): stop install and uninstall from destroying the hook they preserve - #35
Merged
Merged
Conversation
…ey preserve Three ways the chained-backup contract broke. Init chained any hook it did not recognise, including one of our own dispatchers left damaged by a partial write. That backup then named itself as its own predecessor, so the dispatcher would exec itself without bound. A file already pointing at the backup slot is now replaced instead of chained. Init also copied over an existing chained backup. Any tool that installed over the dispatcher after a chained install destroyed the original hook on the next init. The stored backup was there first, so it is kept; the newer file is replaced without one, and the report says so rather than claiming the existing hooks were preserved. Uninstall restored a self-referencing backup as if it were the user's hook, and skipped .git/hooks entirely once core.hooksPath moved elsewhere. Those dispatchers are dormant, not gone: unset the setting and they guard again, while `uninstall` had already printed success and exited 0. Uninstall now clears the repository hooks directory as well as the configured one, and drops a backup that points at itself instead of installing it. The global directory still belongs to `uninstall --global`. The refusal message for a worktree hooks path no longer promises Git will track a directory whose contents it already ignores, and the husky notes name the `.husky/_` path husky 9 actually sets, plus the exclude route that lets init manage it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three ways the chained-backup contract broke, all of them ending with the user's
own hook gone or the repository unable to commit.
A damaged dispatcher was chained onto its own backup
installHooksLockedchained anythingownedHookdid not recognise. A dispatcherleft damaged by a partial write fails that check, so it was copied into the
backup slot — the very path its own
CHAINED=line names. The backup thenpointed at itself, and the dispatcher would exec itself without bound.
The chain test still keys on
ownedHook, which keeps the deliberate decisionthat a marker-bearing file of an unknown format is foreign content worth
preserving. What is added is one guard: a file already carrying
CHAINED=<the backup path>is our own output, so it is replaced rather thanchained.
Init copied over an existing backup
The copy into the backup slot was unconditional. Any tool that installed over
the dispatcher after a chained install destroyed the preserved original on the
next
init— no damaged dispatcher required.The stored backup was there first, so it is the one kept. The newer file is
replaced without a backup of its own, which is a real trade-off and is now said
out loud in a warning naming both paths instead of happening in silence.
Aborting the install instead was considered and dropped:
unsafeaborts allfive hooks and turns into exit 20 with no guard installed and no route out from
the CLI. A warning that names what happened beats a repository that cannot be
guarded at all.
Uninstall restored a poisoned backup, and skipped the local directory
Two problems on the way out.
A backup naming itself as its own predecessor was installed as if it were the
user's original hook, so a successful uninstall left the repository unable to
commit — and following the tool's own advice to re-run
initchained it again.Such a backup is now removed along with the dispatcher.
Separately,
uninstallHooksreturned early whenevercore.hooksPathpointedsomewhere shared. The dispatchers in
.git/hooksstayed exactly where theywere. They are dormant, not gone: unset the setting and they guard again, while
uninstall --purge-statehas already deleted the state they need.uninstallprinted success and exited 0 over the top of that.
Uninstall now clears the repository hooks directory as well as the configured
one, deduped by canonical path.
remainingDispatchersreports against the sameset, so the check and the removal cannot disagree. The global directory is
excluded — it belongs to
uninstall --global— andownedByRepostill preventstouching another repository's dispatcher.
Message and documentation
The refusal for a worktree hooks path said Git "will track it on the next add".
That is false for a layout whose contents Git already ignores, which is the
common husky case. It now says Git can track it, and keeps the remedy.
The husky notes named
.huskyas the configured path; husky 9 sets.husky/_.They now name both, and add the third way out that was missing: excluding the
hooks path locally and re-running
init, which chains husky's shim and sharesnothing machine-specific with clones.
Tests
Four added: a damaged dispatcher is not chained and the original survives; a
foreign hook replacing the dispatcher does not overwrite the backup, and the
skip is reported; a self-referencing backup is dropped rather than restored; and
uninstall clears the dispatchers left behind after the hooks path moved.
Full suite green,
npm run checkgreen.