fix: git hook chaining now works correctly (GH#816)
Two fixes: 1. `bd init` chaining was broken - the code referenced `.old` hooks but never actually renamed the existing hooks. Added the missing os.Rename(). 2. `bd hooks install` now supports --chain flag to chain with existing hooks (e.g., pre-commit framework). When used, existing hooks are renamed to .old and bd hooks run will call them before the bd logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
committed by
Steve Yegge
parent
310d374264
commit
f77b290d43
@@ -182,6 +182,9 @@ By default, hooks are installed to .git/hooks/ in the current repository.
|
|||||||
Use --shared to install to a versioned directory (.beads-hooks/) that can be
|
Use --shared to install to a versioned directory (.beads-hooks/) that can be
|
||||||
committed to git and shared with team members.
|
committed to git and shared with team members.
|
||||||
|
|
||||||
|
Use --chain to preserve existing hooks and run them before bd hooks. This is
|
||||||
|
useful if you have pre-commit framework hooks or other custom hooks.
|
||||||
|
|
||||||
Installed hooks:
|
Installed hooks:
|
||||||
- pre-commit: Flush changes to JSONL before commit
|
- pre-commit: Flush changes to JSONL before commit
|
||||||
- post-merge: Import JSONL after pull/merge
|
- post-merge: Import JSONL after pull/merge
|
||||||
@@ -191,6 +194,7 @@ Installed hooks:
|
|||||||
Run: func(cmd *cobra.Command, args []string) {
|
Run: func(cmd *cobra.Command, args []string) {
|
||||||
force, _ := cmd.Flags().GetBool("force")
|
force, _ := cmd.Flags().GetBool("force")
|
||||||
shared, _ := cmd.Flags().GetBool("shared")
|
shared, _ := cmd.Flags().GetBool("shared")
|
||||||
|
chain, _ := cmd.Flags().GetBool("chain")
|
||||||
|
|
||||||
embeddedHooks, err := getEmbeddedHooks()
|
embeddedHooks, err := getEmbeddedHooks()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -206,7 +210,7 @@ Installed hooks:
|
|||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := installHooks(embeddedHooks, force, shared); err != nil {
|
if err := installHooks(embeddedHooks, force, shared, chain); err != nil {
|
||||||
if jsonOutput {
|
if jsonOutput {
|
||||||
output := map[string]interface{}{
|
output := map[string]interface{}{
|
||||||
"error": err.Error(),
|
"error": err.Error(),
|
||||||
@@ -224,12 +228,17 @@ Installed hooks:
|
|||||||
"success": true,
|
"success": true,
|
||||||
"message": "Git hooks installed successfully",
|
"message": "Git hooks installed successfully",
|
||||||
"shared": shared,
|
"shared": shared,
|
||||||
|
"chained": chain,
|
||||||
}
|
}
|
||||||
jsonBytes, _ := json.MarshalIndent(output, "", " ")
|
jsonBytes, _ := json.MarshalIndent(output, "", " ")
|
||||||
fmt.Println(string(jsonBytes))
|
fmt.Println(string(jsonBytes))
|
||||||
} else {
|
} else {
|
||||||
fmt.Println("✓ Git hooks installed successfully")
|
fmt.Println("✓ Git hooks installed successfully")
|
||||||
fmt.Println()
|
fmt.Println()
|
||||||
|
if chain {
|
||||||
|
fmt.Println("Mode: chained (existing hooks renamed to .old and will run first)")
|
||||||
|
fmt.Println()
|
||||||
|
}
|
||||||
if shared {
|
if shared {
|
||||||
fmt.Println("Hooks installed to: .beads-hooks/")
|
fmt.Println("Hooks installed to: .beads-hooks/")
|
||||||
fmt.Println("Git config set: core.hooksPath=.beads-hooks")
|
fmt.Println("Git config set: core.hooksPath=.beads-hooks")
|
||||||
@@ -307,7 +316,7 @@ var hooksListCmd = &cobra.Command{
|
|||||||
},
|
},
|
||||||
}
|
}
|
||||||
|
|
||||||
func installHooks(embeddedHooks map[string]string, force bool, shared bool) error {
|
func installHooks(embeddedHooks map[string]string, force bool, shared bool, chain bool) error {
|
||||||
// Get actual git directory (handles worktrees where .git is a file)
|
// Get actual git directory (handles worktrees where .git is a file)
|
||||||
gitDir, err := git.GetGitDir()
|
gitDir, err := git.GetGitDir()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -334,13 +343,20 @@ func installHooks(embeddedHooks map[string]string, force bool, shared bool) erro
|
|||||||
|
|
||||||
// Check if hook already exists
|
// Check if hook already exists
|
||||||
if _, err := os.Stat(hookPath); err == nil {
|
if _, err := os.Stat(hookPath); err == nil {
|
||||||
// Hook exists - back it up unless force is set
|
if chain {
|
||||||
if !force {
|
// Chain mode - rename to .old so bd hooks run can call it
|
||||||
|
oldPath := hookPath + ".old"
|
||||||
|
if err := os.Rename(hookPath, oldPath); err != nil {
|
||||||
|
return fmt.Errorf("failed to rename %s to .old for chaining: %w", hookName, err)
|
||||||
|
}
|
||||||
|
} else if !force {
|
||||||
|
// Default mode - back it up
|
||||||
backupPath := hookPath + ".backup"
|
backupPath := hookPath + ".backup"
|
||||||
if err := os.Rename(hookPath, backupPath); err != nil {
|
if err := os.Rename(hookPath, backupPath); err != nil {
|
||||||
return fmt.Errorf("failed to backup %s: %w", hookName, err)
|
return fmt.Errorf("failed to backup %s: %w", hookName, err)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// If force is set and not chaining, we just overwrite
|
||||||
}
|
}
|
||||||
|
|
||||||
// Write hook file
|
// Write hook file
|
||||||
@@ -408,11 +424,55 @@ func uninstallHooks() error {
|
|||||||
// Hook Implementation Functions (called by thin shims via 'bd hooks run')
|
// Hook Implementation Functions (called by thin shims via 'bd hooks run')
|
||||||
// =============================================================================
|
// =============================================================================
|
||||||
|
|
||||||
|
// runChainedHook runs a .old hook if it exists. Returns the exit code.
|
||||||
|
// If the hook doesn't exist, returns 0 (success).
|
||||||
|
func runChainedHook(hookName string, args []string) int {
|
||||||
|
// Get the hooks directory
|
||||||
|
gitDir, err := git.GetGitDir()
|
||||||
|
if err != nil {
|
||||||
|
return 0 // Not a git repo, nothing to chain
|
||||||
|
}
|
||||||
|
|
||||||
|
oldHookPath := filepath.Join(gitDir, "hooks", hookName+".old")
|
||||||
|
|
||||||
|
// Check if the .old hook exists and is executable
|
||||||
|
info, err := os.Stat(oldHookPath)
|
||||||
|
if err != nil {
|
||||||
|
return 0 // No chained hook
|
||||||
|
}
|
||||||
|
if info.Mode().Perm()&0111 == 0 {
|
||||||
|
return 0 // Not executable
|
||||||
|
}
|
||||||
|
|
||||||
|
// Run the chained hook
|
||||||
|
// #nosec G204 -- hookName is from controlled list, path is from git directory
|
||||||
|
cmd := exec.Command(oldHookPath, args...)
|
||||||
|
cmd.Stdout = os.Stdout
|
||||||
|
cmd.Stderr = os.Stderr
|
||||||
|
cmd.Stdin = os.Stdin
|
||||||
|
|
||||||
|
if err := cmd.Run(); err != nil {
|
||||||
|
if exitErr, ok := err.(*exec.ExitError); ok {
|
||||||
|
return exitErr.ExitCode()
|
||||||
|
}
|
||||||
|
// Other error - treat as failure
|
||||||
|
fmt.Fprintf(os.Stderr, "Warning: chained hook %s failed: %v\n", hookName, err)
|
||||||
|
return 1
|
||||||
|
}
|
||||||
|
|
||||||
|
return 0
|
||||||
|
}
|
||||||
|
|
||||||
// runPreCommitHook flushes pending changes to JSONL before commit.
|
// runPreCommitHook flushes pending changes to JSONL before commit.
|
||||||
// Returns 0 on success (or if not applicable), non-zero on error.
|
// Returns 0 on success (or if not applicable), non-zero on error.
|
||||||
//
|
//
|
||||||
//nolint:unparam // Always returns 0 by design - warnings don't block commits
|
//nolint:unparam // Always returns 0 by design - warnings don't block commits
|
||||||
func runPreCommitHook() int {
|
func runPreCommitHook() int {
|
||||||
|
// Run chained hook first (if exists)
|
||||||
|
if exitCode := runChainedHook("pre-commit", nil); exitCode != 0 {
|
||||||
|
return exitCode
|
||||||
|
}
|
||||||
|
|
||||||
// Check if we're in a bd workspace
|
// Check if we're in a bd workspace
|
||||||
if _, err := os.Stat(".beads"); os.IsNotExist(err) {
|
if _, err := os.Stat(".beads"); os.IsNotExist(err) {
|
||||||
return 0 // Not a bd workspace, nothing to do
|
return 0 // Not a bd workspace, nothing to do
|
||||||
@@ -449,6 +509,11 @@ func runPreCommitHook() int {
|
|||||||
//
|
//
|
||||||
//nolint:unparam // Always returns 0 by design - warnings don't block merges
|
//nolint:unparam // Always returns 0 by design - warnings don't block merges
|
||||||
func runPostMergeHook() int {
|
func runPostMergeHook() int {
|
||||||
|
// Run chained hook first (if exists)
|
||||||
|
if exitCode := runChainedHook("post-merge", nil); exitCode != 0 {
|
||||||
|
return exitCode
|
||||||
|
}
|
||||||
|
|
||||||
// Skip during rebase
|
// Skip during rebase
|
||||||
if isRebaseInProgress() {
|
if isRebaseInProgress() {
|
||||||
return 0
|
return 0
|
||||||
@@ -485,6 +550,11 @@ func runPostMergeHook() int {
|
|||||||
// runPrePushHook prevents pushing stale JSONL.
|
// runPrePushHook prevents pushing stale JSONL.
|
||||||
// Returns 0 to allow push, non-zero to block.
|
// Returns 0 to allow push, non-zero to block.
|
||||||
func runPrePushHook() int {
|
func runPrePushHook() int {
|
||||||
|
// Run chained hook first (if exists)
|
||||||
|
if exitCode := runChainedHook("pre-push", nil); exitCode != 0 {
|
||||||
|
return exitCode
|
||||||
|
}
|
||||||
|
|
||||||
// Check if we're in a bd workspace
|
// Check if we're in a bd workspace
|
||||||
if _, err := os.Stat(".beads"); os.IsNotExist(err) {
|
if _, err := os.Stat(".beads"); os.IsNotExist(err) {
|
||||||
return 0
|
return 0
|
||||||
@@ -552,6 +622,11 @@ func runPrePushHook() int {
|
|||||||
//
|
//
|
||||||
//nolint:unparam // Always returns 0 by design - warnings don't block checkouts
|
//nolint:unparam // Always returns 0 by design - warnings don't block checkouts
|
||||||
func runPostCheckoutHook(args []string) int {
|
func runPostCheckoutHook(args []string) int {
|
||||||
|
// Run chained hook first (if exists)
|
||||||
|
if exitCode := runChainedHook("post-checkout", args); exitCode != 0 {
|
||||||
|
return exitCode
|
||||||
|
}
|
||||||
|
|
||||||
// Only run on branch checkouts (flag=1)
|
// Only run on branch checkouts (flag=1)
|
||||||
if len(args) >= 3 && args[2] != "1" {
|
if len(args) >= 3 && args[2] != "1" {
|
||||||
return 0
|
return 0
|
||||||
@@ -616,6 +691,11 @@ func runPostCheckoutHook(args []string) int {
|
|||||||
//
|
//
|
||||||
//nolint:unparam // Always returns 0 by design - we don't block commits
|
//nolint:unparam // Always returns 0 by design - we don't block commits
|
||||||
func runPrepareCommitMsgHook(args []string) int {
|
func runPrepareCommitMsgHook(args []string) int {
|
||||||
|
// Run chained hook first (if exists)
|
||||||
|
if exitCode := runChainedHook("prepare-commit-msg", args); exitCode != 0 {
|
||||||
|
return exitCode
|
||||||
|
}
|
||||||
|
|
||||||
if len(args) < 1 {
|
if len(args) < 1 {
|
||||||
return 0 // No message file provided
|
return 0 // No message file provided
|
||||||
}
|
}
|
||||||
@@ -967,6 +1047,7 @@ installed bd version - upgrading bd automatically updates hook behavior.`,
|
|||||||
func init() {
|
func init() {
|
||||||
hooksInstallCmd.Flags().Bool("force", false, "Overwrite existing hooks without backup")
|
hooksInstallCmd.Flags().Bool("force", false, "Overwrite existing hooks without backup")
|
||||||
hooksInstallCmd.Flags().Bool("shared", false, "Install hooks to .beads-hooks/ (versioned) instead of .git/hooks/")
|
hooksInstallCmd.Flags().Bool("shared", false, "Install hooks to .beads-hooks/ (versioned) instead of .git/hooks/")
|
||||||
|
hooksInstallCmd.Flags().Bool("chain", false, "Chain with existing hooks (run them before bd hooks)")
|
||||||
|
|
||||||
hooksCmd.AddCommand(hooksInstallCmd)
|
hooksCmd.AddCommand(hooksInstallCmd)
|
||||||
hooksCmd.AddCommand(hooksUninstallCmd)
|
hooksCmd.AddCommand(hooksUninstallCmd)
|
||||||
|
|||||||
@@ -51,7 +51,7 @@ func TestInstallHooks(t *testing.T) {
|
|||||||
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := installHooks(hooks, false, false); err != nil {
|
if err := installHooks(hooks, false, false, false); err != nil {
|
||||||
t.Fatalf("installHooks() failed: %v", err)
|
t.Fatalf("installHooks() failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -103,7 +103,7 @@ func TestInstallHooksBackup(t *testing.T) {
|
|||||||
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := installHooks(hooks, false, false); err != nil {
|
if err := installHooks(hooks, false, false, false); err != nil {
|
||||||
t.Fatalf("installHooks() failed: %v", err)
|
t.Fatalf("installHooks() failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -148,7 +148,7 @@ func TestInstallHooksForce(t *testing.T) {
|
|||||||
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := installHooks(hooks, true, false); err != nil {
|
if err := installHooks(hooks, true, false, false); err != nil {
|
||||||
t.Fatalf("installHooks() failed: %v", err)
|
t.Fatalf("installHooks() failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -176,7 +176,7 @@ func TestUninstallHooks(t *testing.T) {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
||||||
}
|
}
|
||||||
if err := installHooks(hooks, false, false); err != nil {
|
if err := installHooks(hooks, false, false, false); err != nil {
|
||||||
t.Fatalf("installHooks() failed: %v", err)
|
t.Fatalf("installHooks() failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -211,7 +211,7 @@ func TestHooksCheckGitHooks(t *testing.T) {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
||||||
}
|
}
|
||||||
if err := installHooks(hooks, false, false); err != nil {
|
if err := installHooks(hooks, false, false, false); err != nil {
|
||||||
t.Fatalf("installHooks() failed: %v", err)
|
t.Fatalf("installHooks() failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -245,7 +245,7 @@ func TestInstallHooksShared(t *testing.T) {
|
|||||||
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := installHooks(hooks, false, true); err != nil {
|
if err := installHooks(hooks, false, true, false); err != nil {
|
||||||
t.Fatalf("installHooks() with shared=true failed: %v", err)
|
t.Fatalf("installHooks() with shared=true failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -283,6 +283,66 @@ func TestInstallHooksShared(t *testing.T) {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestInstallHooksChaining(t *testing.T) {
|
||||||
|
tmpDir := t.TempDir()
|
||||||
|
runInDir(t, tmpDir, func() {
|
||||||
|
if err := exec.Command("git", "init").Run(); err != nil {
|
||||||
|
t.Skipf("Skipping test: git init failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
gitDirPath, err := git.GetGitDir()
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("git.GetGitDir() failed: %v", err)
|
||||||
|
}
|
||||||
|
gitDir := filepath.Join(gitDirPath, "hooks")
|
||||||
|
if err := os.MkdirAll(gitDir, 0750); err != nil {
|
||||||
|
t.Fatalf("Failed to create hooks directory: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Create an existing hook
|
||||||
|
existingHook := filepath.Join(gitDir, "pre-commit")
|
||||||
|
existingContent := "#!/bin/sh\necho old hook\n"
|
||||||
|
if err := os.WriteFile(existingHook, []byte(existingContent), 0755); err != nil {
|
||||||
|
t.Fatalf("Failed to create existing hook: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
hooks, err := getEmbeddedHooks()
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("getEmbeddedHooks() failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Install with chain=true
|
||||||
|
if err := installHooks(hooks, false, false, true); err != nil {
|
||||||
|
t.Fatalf("installHooks() with chain=true failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify the original hook was renamed to .old
|
||||||
|
oldPath := existingHook + ".old"
|
||||||
|
if _, err := os.Stat(oldPath); os.IsNotExist(err) {
|
||||||
|
t.Errorf("Existing hook was not renamed to .old for chaining")
|
||||||
|
}
|
||||||
|
|
||||||
|
oldContent, err := os.ReadFile(oldPath)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("Failed to read .old hook: %v", err)
|
||||||
|
}
|
||||||
|
if string(oldContent) != existingContent {
|
||||||
|
t.Errorf(".old hook content mismatch: got %q, want %q", string(oldContent), existingContent)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify new hook was installed
|
||||||
|
if _, err := os.Stat(existingHook); os.IsNotExist(err) {
|
||||||
|
t.Errorf("New pre-commit hook was not installed")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify .backup was NOT created (chain mode uses .old, not .backup)
|
||||||
|
backupPath := existingHook + ".backup"
|
||||||
|
if _, err := os.Stat(backupPath); !os.IsNotExist(err) {
|
||||||
|
t.Errorf("Backup was created but should not be in chain mode")
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
func TestFormatHookWarnings(t *testing.T) {
|
func TestFormatHookWarnings(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
|
|||||||
@@ -160,6 +160,16 @@ func installGitHooks() error {
|
|||||||
switch choice {
|
switch choice {
|
||||||
case "1", "":
|
case "1", "":
|
||||||
chainHooks = true
|
chainHooks = true
|
||||||
|
// Chain mode - rename existing hooks to .old so they can be called
|
||||||
|
for _, hook := range existingHooks {
|
||||||
|
if hook.exists && !hook.isBdHook {
|
||||||
|
oldPath := hook.path + ".old"
|
||||||
|
if err := os.Rename(hook.path, oldPath); err != nil {
|
||||||
|
return fmt.Errorf("failed to rename %s to .old: %w", hook.name, err)
|
||||||
|
}
|
||||||
|
fmt.Printf(" Renamed %s to %s\n", hook.name, filepath.Base(oldPath))
|
||||||
|
}
|
||||||
|
}
|
||||||
case "2":
|
case "2":
|
||||||
// Overwrite mode - backup existing hooks
|
// Overwrite mode - backup existing hooks
|
||||||
for _, hook := range existingHooks {
|
for _, hook := range existingHooks {
|
||||||
@@ -211,12 +221,11 @@ func installGitHooks() error {
|
|||||||
// buildPreCommitHook generates the pre-commit hook content
|
// buildPreCommitHook generates the pre-commit hook content
|
||||||
func buildPreCommitHook(chainHooks bool, existingHooks []hookInfo) string {
|
func buildPreCommitHook(chainHooks bool, existingHooks []hookInfo) string {
|
||||||
if chainHooks {
|
if chainHooks {
|
||||||
// Find existing pre-commit hook
|
// Find existing pre-commit hook (already renamed to .old by caller)
|
||||||
var existingPreCommit string
|
var existingPreCommit string
|
||||||
for _, hook := range existingHooks {
|
for _, hook := range existingHooks {
|
||||||
if hook.name == "pre-commit" && hook.exists && !hook.isBdHook {
|
if hook.name == "pre-commit" && hook.exists && !hook.isBdHook {
|
||||||
existingPreCommit = hook.path + ".old"
|
existingPreCommit = hook.path + ".old"
|
||||||
// Note: caller handles the rename
|
|
||||||
break
|
break
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -309,7 +318,7 @@ exit 0
|
|||||||
// buildPostMergeHook generates the post-merge hook content
|
// buildPostMergeHook generates the post-merge hook content
|
||||||
func buildPostMergeHook(chainHooks bool, existingHooks []hookInfo) string {
|
func buildPostMergeHook(chainHooks bool, existingHooks []hookInfo) string {
|
||||||
if chainHooks {
|
if chainHooks {
|
||||||
// Find existing post-merge hook
|
// Find existing post-merge hook (already renamed to .old by caller)
|
||||||
var existingPostMerge string
|
var existingPostMerge string
|
||||||
for _, hook := range existingHooks {
|
for _, hook := range existingHooks {
|
||||||
if hook.name == "post-merge" && hook.exists && !hook.isBdHook {
|
if hook.name == "post-merge" && hook.exists && !hook.isBdHook {
|
||||||
|
|||||||
Reference in New Issue
Block a user