Repository navigation
feat(hooks): install git hooks once per clone with just hooks - #603
wpfleger96 wants to merge 3 commits into
Conversation
Hooks live in the tracked .githooks/, so a clone-wide relative core.hooksPath still runs each worktree's own branch's hooks, while per-worktree install made every new worktree a manual step. Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
23496e9 to
78c1617
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
One P2 blocker; details and reproduction are inline. Preserve inherited custom hooks during migration and add the masked-global/include regression cases before merging.
Reviewed head 78c16178bb6f86b75c5399fca14bbccea824af3d against base a82ecbcc3953e03be13a7dbddf231c4dd789c16f (merge base c8e7abb0eeeb7c5231a2c767edc28bef86c2a4ef). Focused disposable-repository probes exercised the exact installer, linked-worktree installation, existing/future hook lookup, repeat installation, migration and refusal paths; the global-hook failure is absent with the base installer. Existing CI passed both JavaScript shards and all Node integration tests. Browser measurements were cancelled and browser journeys were incomplete at review time; no broad local suite was duplicated.
| ); | ||
| if (!existing) { | ||
| const hooks = git("rev-parse", "--git-path", "hooks"); | ||
| const shared = config("core.hooksPath", "--local"); |
There was a problem hiding this comment.
[P2] Check inherited hooks hidden by an existing worktree override
--local --get reads only the local config file and does not follow includes by default. Together with the effective read above, this misses custom hooks inherited from global/system configuration whenever an earlier installation left core.hooksPath=.githooks in the calling worktree.
Reproduction: worktree A has that legacy worktree-scoped setting, the clone has no local hooks path or custom default hooks, and a global core.hooksPath points to a custom hook directory. A sibling without an override runs those custom hooks. Installing from A sees existing=.githooks and an empty shared, succeeds, and writes the clone-wide .githooks, silently disabling the sibling’s custom validation hooks. With the exact head installer on Git 2.54.0, my rejecting sentinel hook returned 31 before installation and 0 afterward; the base installer preserved it. The same masked read misses a path in a local include; in my fixture that include continued to win after the write, so installation reported success without activating the advertised hooks there.
Before the clone-wide mutation, inspect the applicable non-worktree configuration with includes enabled, not just the direct local key, and refuse conflicting hooks. Add masked-global and local-include regressions that verify refusal leaves both configuration and sibling hook execution unchanged.
A global core.hooksPath should not prevent this clone from installing its tracked hooks. Continue refusing conflicting clone-local and per-worktree paths, while making the precedence explicit in the integration coverage and contributor docs. Signed-off-by: Alia <d32955ad69077062930cc46cfe2df30ca9aaf6f8e76422681265e9e9af704d78@buzz.block.builderlab.xyz>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
One P2 remains: the include-config portion of the previous finding is still unresolved; details inline. Overriding global/system hooks is now explicit scope, not a requested change.
Star Lord automated source review via Wes's account; head 61614bf99be4d0453d421027f3eb44716c81b43e, base 5aef2f03eead999167f89d3e7331caf41269a580. Source only: no installer, hooks, tests, or app executed. Hosted CI was still running; DCO passed and Windows validation was skipped. No concrete privacy finding in the changed public material; the description has no attached images.
| const hooks = git("rev-parse", "--git-path", "hooks"); | ||
| // Refuse different hooks paths in this clone or its worktree config. Global and | ||
| // system settings are intentionally ignored because this install replaces them. | ||
| const shared = config("core.hooksPath", "--local"); |
There was a problem hiding this comment.
[P2] Expand includes in the repository-scoped config reads
Scoped git config reads disable include/includeIf expansion by default. An explicit hooks path supplied through a clone-local include is invisible to shared; worktree includes are likewise missed by worktree. The installer passes the conflict guard, writes the clone setting, and reports success even when the included path remains effective, leaving the advertised repository hooks inactive. This is the remaining include-config part of the prior finding, not an objection to overriding global hooks.
Enable --includes on these scoped reads, including the worktree-config flag lookup, and add local/worktree-include conflict tests that verify refusal preserves both configuration and effective hook selection.
Follow clone and worktree include files when checking for conflicting hook paths, including the worktreeConfig extension. Refusal regressions verify both saved configuration and the effective hook path, while contributor docs state that command-scoped overrides remain outside the installer contract. Signed-off-by: Alia <d32955ad69077062930cc46cfe2df30ca9aaf6f8e76422681265e9e9af704d78@buzz.block.builderlab.xyz>
|
superseded by #641 |
2 participants
Install Buzz's tracked Git hooks once per clone while preserving explicit repository settings and custom hooks.
scripts/install-hooks.mjschecks clone-local and enabled per-worktreecore.hooksPathvalues with--includes, so savedinclude.pathfiles are covered. It also follows includedextensions.worktreeConfigsettings before reading the worktree config. Global and system values are ignored, allowing the clone's.githookspath to override them; conflicting saved clone/worktree paths still fail closed, and the existing custom-hook and nonstandard-worktree/bare checks remain unchanged..githookspath makes every existing and future worktree run its own checkout's tracked hooks. Existing per-worktree.githooksconfiguration remains compatible.tests/integration/hooks.test.mjscovers global-path override, direct clone-local conflict refusal, included clone config before and after[core], and an included worktree hooks path. Each included-config regression verifies installation fails, configuration files remain unchanged, and Git still resolves the custom path.docs/contributing.mddocuments included saved settings and the boundary that command-scoped overrides such asgit -c core.hooksPath=...andGIT_CONFIG_*can still take precedence.The change makes
buzz-appbehave likebuzz: repository hooks take precedence over global hooks without silently overwriting explicit repository or worktree configuration.