feat: hook ownership model with external manager detection#12980
feat: hook ownership model with external manager detection#12980ichoosetoaccept wants to merge 1 commit intogitbutlerapp:masterfrom
Conversation
|
Someone is attempting to deploy a commit to the GitButler Team on Vercel. A member of the Team first needs to authorize it. |
bf4324d to
519dbd7
Compare
|
Thanks a lot for resubmitting the PR, I will take it on! I appreciate the 'relatively' concise PR description with notes to help organising the review. Regarding Please also put all integration tests, i.e. tests for the public API, into Please address the above before I go into a deeper review. PS: Please ping me in a message when you think everything is addressed. |
|
You're very welcome! Thank you for the detailed feedback. The PR is still a draft so I appreciate your pointers. I'll start iterating until this gets closer to being properly reviewable (within the confines of being unreviewably large 🙂). |
1acaf9c to
d5eae1f
Compare
80d3b86 to
f212d37
Compare
d2b97d1 to
d9c0b97
Compare
|
My hope is that this PR is at least half-tests. Do you have data on that? |
d9c0b97 to
c72cf71
Compare
Hey @Byron . Indeed I do. In fact, most of the code is tests! The result of running this script from root of the project... #!/usr/bin/env python3
# /// script
# requires-python = ">=3.10"
# ///
"""Break down PR diff stats by category: tests, implementation, other."""
import subprocess
import sys
CATEGORIES = [
("Tests", ["*/tests/*"]),
("Implementation", [":!*/tests/*", "*.rs"]),
("Other", [":!*.rs"]),
]
def numstat(base: str, pathspecs: list[str]) -> tuple[int, int]:
result = subprocess.run(
["git", "diff", "--numstat", f"{base}..HEAD", "--", *pathspecs],
capture_output=True,
text=True,
)
added = deleted = 0
for line in result.stdout.strip().splitlines():
parts = line.split("\t")
if parts[0] != "-": # skip binary files
added += int(parts[0])
deleted += int(parts[1])
return added, deleted
def main() -> None:
if len(sys.argv) > 1:
base = sys.argv[1]
else:
# Auto-detect merge base with master
result = subprocess.run(
["git", "merge-base", "HEAD", "master"],
capture_output=True,
text=True,
)
base = result.stdout.strip()
if not base:
print("Could not determine merge base. Pass a base ref explicitly.", file=sys.stderr)
sys.exit(1)
print(f"Base: {base[:10]}\n")
total_add = total_del = 0
for label, specs in CATEGORIES:
added, deleted = numstat(base, specs)
total_add += added
total_del += deleted
print(f" {label:<20} +{added:<6} -{deleted:<6} (net {added - deleted:+d})")
print(f" {'─' * 48}")
print(f" {'Total':<20} +{total_add:<6} -{total_del:<6} (net {total_add - total_del:+d})")
if __name__ == "__main__":
main()is Testing is the reason I've been taking so long. I discovered some small issues when testing myself and am just about to push fixes, but in general, I tried to cover everything that is meaningfully testable by test code with tests. |
|
BTW in an effort to do the most I can to make your review a "nicer" experience, I sync my fork's origin/master with upstream/master multiple times a day and rebase onto the latest changes. I don't remember whether I ever had a merge conflict. I'm inclined to say that's good news 🙂. EDIT: Of course I ran into conflicts JUST after posting this. Handling it. |
c72cf71 to
1f2e43e
Compare
|
Update: discovered a few issues when testing this manually, myself, for real. Implementing multiple usability and clarity fixes. "Draft" status is still appropriate. |
dadc0f5 to
0bdc9f6
Compare
Implement two-mode hook ownership: GitButler-managed (installs hooks directly) vs externally-managed (defers to prek/lefthook/husky). - Add hook_manager.rs: detect external managers via config + binary gate - Refactor managed_hooks.rs: ensure_managed_hooks() shared entry point - Add pre-push guard hook (blocks pushing gitbutler/workspace) - Add --no-hooks / --force-hooks escape hatches to setup/teardown - Hook lifecycle: staleness detection, auto-update, config persistence - Desktop app (integration.rs) uses same ensure_managed_hooks() path Addresses gitbutlerapp#12748 refactor: extract but-hooks crate from gitbutler-repo feat: add but hook CLI subcommands for hook manager integration Expose GitButler workspace guard and cleanup as standalone CLI commands that external hook managers can invoke from their configuration: - but hook pre-commit: blocks git commit on gitbutler/workspace - but hook post-checkout: info message when leaving workspace branch - but hook pre-push: blocks git push on gitbutler/workspace - but hook status: diagnostic view of hook ownership and integration state Follows up on hook-ownership-core.
0bdc9f6 to
c8f7865
Compare
|
Hey @Byron I have lots of ideas for followups and improvements but had to draw the line somewhere. I apologize this got as large as it did but I hope you'll appreciate that more than half of the contribution is test code. Consider yourself pinged and do let me know if anything substantial needs to change or if you'd prefer a different direction altogether. Cheers! |
|
Thanks so much for staying on top of this, but as I am currently putting much less hours in that I have to and other priorities came up, it will be a while until this PR can get a proper review. I will, however, see if quick glimpses can already provide some meaningful feedback. |
Closes #12748
Summary
Adds a hook ownership model so GitButler can coexist with external hook managers (now: prek, later: pre-commit, lefthook, husky, etc.) instead of unconditionally overwriting their hooks.
This PR ships the plumbing +
butCLI milestone described in #12748 (comment). It does not includebut doctor, Desktop UI integration, or onboarding changes — those are separate follow-ups.What changed
New
but-hookscrateAll hook detection and management logic moved into a dedicated
but-hookscrate, following thegitbutler-→but-migration. Existing consumers ingitbutler-repocontinue working via re-exports.owns_hook,has_config,is_available, andintegration_instructions.--force-hooksis used, in which case the original is backed up to<hook>-user..venv/bin/andnode_modules/.bin/in addition to system PATH, so project-local hook manager installations are detected.but hooksubcommandsComposable CLI entry points for external hook managers to invoke:
but hook pre-commit— blocksgit commitongitbutler/workspacewith a helpful error directing the user tobut commit.but hook post-checkout— prints an informational message when leaving the workspace branch.but hook pre-push— blocksgit pushongitbutler/workspacewith a helpful error directing the user tobut push.but hook status— diagnostic command showing hook mode (managed/external/disabled/unconfigured), per-hook ownership, orphaned hook detection, and context-aware recommendations. Supports human, shell, and JSON output.Updated lifecycle commands
but setup— detects external hook managers before installing. If detected: persists opt-out config, cleans up any previously-installed GitButler hooks, and prints integration instructions.--no-hooks/--force-hooksflags provide manual overrides.but teardown— respects hook ownership during cleanup. Externally-managed hooks are left untouched.Desktop backend (gitbutler-branch-actions)
Updated the existing
update_workspace_commitcall site to useensure_managed_hooks()instead of the oldinstall_managed_hooks_gix(). This is a backend-only change — no UI, no Tauri, no onboarding — ensuring the Desktop app's workspace migration path also respects external hook managers rather than overwriting them. Added a test confirming hooks are not installed when the config opt-out is active.Automated tests
115 non-ignored tests across
but-hooks,but, andgitbutler-branch-actions, organized in five layers:.venv/bin/andnode_modules/.bin/), configuration persistence, backup status formattinggit commit, post-checkout cleanup fires when leaving workspace, user hook delegation works on non-workspace branchessnapbox-based output validation forbut hook status,but setup, andbut teardownacross all flag combinations (--no-hooks,--force-hooks,--json) and hook ownership modes (managed, external, disabled, unconfigured)#[ignore]-gated) — realprekbinary scenarios; requireprekon PATH, run withcargo test -p but -- --ignored hook_prekTesting patterns: temp dir isolation for every test, environment variable mocking for binary availability (
test-overridesfeature flag),#[cfg(unix)]guards for permission-specific tests.Line count breakdown (from
uv run prstats.py):Manual testing
In addition to the automated suite above, I manually tested each user-facing scenario end-to-end. I built a release binary (
cargo build --release -p but) and ran each scenario against a clean disposable git repo with a local bare remote. I captured screenshots by piping command output throughcleen— a tool that renders ANSI terminal output to a PNG — using a pseudo-TTY wrapper (script -q /dev/null) so the binary emits colors through the pipe. The capture script is a local test artifact and is not included in the PR. All on macOS. Twelve scenarios covered:Happy path
1.
but setupon a clean repo — hooks installedI ran
but setupon a fresh repo. It switches togitbutler/workspaceand confirms each hook is in place — one line per hook. Verified that the installed hooks blockgit commiton the workspace branch with aGITBUTLER_ERRORas expected (pre-existing behavior, not screenshotted).2.
but hook status— verify hooks are in placeAfter setup, I ran
but hook status. It shows all three hooks listed as GitButler-managed with green checkmarks. Running with--jsonemits structured per-hook status including ownership and any warnings (not shown here).3.
but teardown— clean exitI ran
but teardown. It reports each removed hook individually before checking out the target branch. Only GitButler-managed hooks are removed — any hooks not owned by GitButler are left untouched. The managed post-checkout script also auto-removes hooks on a plaingit checkout <branch>away from the workspace.External manager coexistence
4. prek detected — setup defers to external manager
With
prek.tomlpresent andprek installalready run, I ranbut setup. It detects prek, leaves the hooks untouched, persistsgitbutler.installHooks=false, and prints ready-to-paste integration instructions.5.
but hook status— external ownership shownI then ran
but hook status. It shows external manager mode with each hook owned by prek and actionable recommendations for hooks not yet wired up.6.
but teardown— prek hooks left untouchedI ran
but teardownin the prek-managed repo. It left prek's hooks untouched and printed guidance about removingbut hookentries from the prek config if you want to fully disable the workspace guards.7. prek detected — config+binary fallback (before
prek install)With only
prek.tomlpresent and the binary on PATH (no hook files yet), I ranbut setup. It still detects prek via the config+binary fallback and shows the same integration instructions.Override behaviors and re-run
8.
--no-hooksflag — persistent opt-outI ran
but setup --no-hooks. It skips hook installation and persistsgitbutler.installHooks=false.9. Re-run detection — config opt-out preserved
After opting out (scenario 8), I ran
but setupagain. It shows the config-preserved opt-out without re-scanning and hints at--force-hooksto re-enable.10.
--force-hooks— override external manager detectionIn a repo where prek was previously detected (
installHooks=false), I ranbut setup --force-hooks. It skips detection entirely, backs prek's hooks up to<hook>-user, installs GitButler's managed hooks in their place, and reports each one with✓ Installed pre-commitetc.11.
but teardownwith user backup — hooks restoredContinuing from scenario 10, I ran
but teardown. It reports each hook removal and showsRestored pre-commit (your original hook is back)for the hook that was backed up topre-commit-user.Edge case
12. Pre-existing plain user hook — partial install with actionable warning
I placed a plain
pre-commithook (not owned by any known manager) and ranbut setup. It installspost-checkoutandpre-push(each with✓ Installed …) and skipspre-commitwith a yellow warning including the--force-hooksoverride hint.Architecture
Notes
gix::discover_with_environment_overrides()in hook subcommandsByron flagged in his initial review that bare
gix::discover()is not the right call for hook subcommands — Git setsGIT_DIR/GIT_WORK_TREEbefore invoking hooks andgitoxidehasgix::open_with_environment_overrides()for exactly this. We usegix::discover_with_environment_overrides()which satisfies both requirements: it honours the git-provided environment variables when present (sogit commitinvocations work correctly), and falls back to a directory walk when invoked directly by an external manager like prek withoutGIT_DIRset.agents.mdsays not to re-discover repositories inside commands; these subcommands are a documented exception since they run as standalone hook subprocesses with noContext.Skill files updated
crates/but/skill/references/reference.mdwas updated with the newbut hooksubcommands and the--no-hooks/--force-hooksflags forbut setup, per theagents.mdconvention.Scope note
This PR is scoped to plumbing + CLI only, per the agreed plan in #12748:
but-hookscratebut doctor/but-diagnosebut hooksubcommandsbut setup/but teardown