diff --git a/cmd/bd/doctor/git.go b/cmd/bd/doctor/git.go index fbc875f6..edd711e8 100644 --- a/cmd/bd/doctor/git.go +++ b/cmd/bd/doctor/git.go @@ -24,6 +24,10 @@ const ( // bdShimMarker identifies bd shim hooks (GH#946) const bdShimMarker = "# bd-shim" +// bdInlineHookMarker identifies inline hooks created by bd init (GH#1120) +// These hooks have the logic embedded directly rather than calling bd hooks run +const bdInlineHookMarker = "# bd (beads)" + // bdHooksRunPattern matches hooks that call bd hooks run var bdHooksRunPattern = regexp.MustCompile(`\bbd\s+hooks\s+run\b`) @@ -150,10 +154,11 @@ func CheckGitHooks() DoctorCheck { } } -// areBdShimsInstalled checks if the installed hooks are bd shims or call bd hooks run. +// areBdShimsInstalled checks if the installed hooks are bd shims, call bd hooks run, +// or are inline bd hooks created by bd init. // This helps detect when bd hooks are installed directly but an external manager config exists. -// Returns (true, installedHooks) if bd shims are detected, (false, nil) otherwise. -// (GH#946) +// Returns (true, installedHooks) if bd hooks are detected, (false, nil) otherwise. +// (GH#946, GH#1120) func areBdShimsInstalled(hooksDir string) (bool, []string) { hooks := []string{"pre-commit", "post-merge", "pre-push"} var bdHooks []string @@ -165,8 +170,10 @@ func areBdShimsInstalled(hooksDir string) (bool, []string) { continue } contentStr := string(content) - // Check for bd-shim marker or bd hooks run call - if strings.Contains(contentStr, bdShimMarker) || bdHooksRunPattern.MatchString(contentStr) { + // Check for bd-shim marker, bd hooks run call, or inline bd hook marker (from bd init) + if strings.Contains(contentStr, bdShimMarker) || + strings.Contains(contentStr, bdInlineHookMarker) || + bdHooksRunPattern.MatchString(contentStr) { bdHooks = append(bdHooks, hookName) } } diff --git a/cmd/bd/hooks.go b/cmd/bd/hooks.go index 6b5f25c7..5cf54ff2 100644 --- a/cmd/bd/hooks.go +++ b/cmd/bd/hooks.go @@ -37,6 +37,10 @@ func getEmbeddedHooks() (map[string]string, error) { const hookVersionPrefix = "# bd-hooks-version: " const shimVersionPrefix = "# bd-shim " +// inlineHookMarker identifies inline hooks created by bd init (GH#1120) +// These hooks have the logic embedded directly rather than using shims +const inlineHookMarker = "# bd (beads)" + // HookStatus represents the status of a single git hook type HookStatus struct { Name string @@ -92,8 +96,9 @@ func CheckGitHooks() []HookStatus { // hookVersionInfo contains version information extracted from a hook file type hookVersionInfo struct { - Version string // bd version (for legacy hooks) or shim version - IsShim bool // true if this is a thin shim + Version string // bd version (for legacy hooks) or shim version + IsShim bool // true if this is a thin shim + IsBdHook bool // true if this is any type of bd hook (shim or inline) } // getHookVersion extracts the version from a hook file @@ -106,24 +111,33 @@ func getHookVersion(path string) (hookVersionInfo, error) { defer file.Close() scanner := bufio.NewScanner(file) - // Read first few lines looking for version marker + // Read first few lines looking for version marker or bd hook marker lineCount := 0 - for scanner.Scan() && lineCount < 10 { + var content strings.Builder + for scanner.Scan() && lineCount < 15 { line := scanner.Text() + content.WriteString(line) + content.WriteString("\n") // Check for thin shim marker first if strings.HasPrefix(line, shimVersionPrefix) { version := strings.TrimSpace(strings.TrimPrefix(line, shimVersionPrefix)) - return hookVersionInfo{Version: version, IsShim: true}, nil + return hookVersionInfo{Version: version, IsShim: true, IsBdHook: true}, nil } // Check for legacy version marker if strings.HasPrefix(line, hookVersionPrefix) { version := strings.TrimSpace(strings.TrimPrefix(line, hookVersionPrefix)) - return hookVersionInfo{Version: version, IsShim: false}, nil + return hookVersionInfo{Version: version, IsShim: false, IsBdHook: true}, nil } lineCount++ } - // No version found (old hook) + // Check if it's an inline bd hook (from bd init) - GH#1120 + // These don't have version markers but have "# bd (beads)" comment + if strings.Contains(content.String(), inlineHookMarker) { + return hookVersionInfo{IsBdHook: true}, nil + } + + // No version found and not a bd hook return hookVersionInfo{}, nil } @@ -363,17 +377,26 @@ func installHooksWithOptions(embeddedHooks map[string]string, force bool, shared if _, err := os.Stat(hookPath); err == nil { if chain { // Chain mode - rename to .old so bd hooks run can call it - // But skip if existing hook is already a bd shim - renaming it would - // cause infinite recursion. See: https://github.com/steveyegge/beads/issues/843 + // But skip if existing hook is already a bd hook (shim or inline) - renaming it would + // cause infinite recursion or destroy user's original hook. See: GH#843, GH#1120 versionInfo, verr := getHookVersion(hookPath) - if verr != nil || !versionInfo.IsShim { - // Not a bd shim - safe to rename for chaining + if verr != nil || !versionInfo.IsBdHook { + // Not a bd hook - safe to rename for chaining 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) + // Safety check: don't overwrite existing .old file (GH#1120) + // This prevents destroying user's original hook if bd hooks install --chain + // is run multiple times or after bd init already created .old + if _, oldErr := os.Stat(oldPath); oldErr == nil { + // .old already exists - the user's original hook is there + // Just overwrite the current hook without renaming + // (the existing .old will be chained by the new hook) + } else { + if err := os.Rename(hookPath, oldPath); err != nil { + return fmt.Errorf("failed to rename %s to .old for chaining: %w", hookName, err) + } } } - // If it IS a bd shim, just overwrite it (no rename needed) + // If it IS a bd hook, just overwrite it (no rename needed) } else if !force { // Default mode - back it up backupPath := hookPath + ".backup" @@ -492,12 +515,12 @@ func runChainedHook(hookName string, args []string) int { return 0 // Not executable } - // Check if .old is itself a bd shim - skip to prevent infinite recursion + // Check if .old is itself a bd hook (shim or inline) - skip to prevent infinite recursion // This can happen if user runs `bd hooks install --chain` multiple times, - // renaming an existing bd shim to .old. See: https://github.com/steveyegge/beads/issues/843 + // renaming an existing bd hook to .old. See: GH#843, GH#1120 versionInfo, err := getHookVersion(oldHookPath) - if err == nil && versionInfo.IsShim { - // Skip execution - .old is a bd shim which would call us again + if err == nil && versionInfo.IsBdHook { + // Skip execution - .old is a bd hook which would call us again return 0 } diff --git a/cmd/bd/hooks_test.go b/cmd/bd/hooks_test.go index a629622d..e66b9c7a 100644 --- a/cmd/bd/hooks_test.go +++ b/cmd/bd/hooks_test.go @@ -542,3 +542,228 @@ func TestRunChainedHookSkipsBdShim(t *testing.T) { } }) } + +// TestGetHookVersionRecognizesInlineHooks verifies that getHookVersion() +// correctly identifies inline bd hooks created by bd init. +// See: https://github.com/steveyegge/beads/issues/1120 +func TestGetHookVersionRecognizesInlineHooks(t *testing.T) { + tmpDir := t.TempDir() + + // Test inline hook from bd init + inlineHook := filepath.Join(tmpDir, "pre-commit") + inlineContent := `#!/bin/sh +# +# bd (beads) pre-commit hook (chained) +# +# This hook chains bd functionality with your existing pre-commit hook. + +if ! command -v bd >/dev/null 2>&1; then + echo "Warning: bd command not found" >&2 + exit 0 +fi + +bd sync --flush-only +` + if err := os.WriteFile(inlineHook, []byte(inlineContent), 0755); err != nil { + t.Fatalf("Failed to create inline hook: %v", err) + } + + info, err := getHookVersion(inlineHook) + if err != nil { + t.Fatalf("getHookVersion() failed: %v", err) + } + + if !info.IsBdHook { + t.Error("getHookVersion() IsBdHook = false, want true for inline bd hook") + } + if info.IsShim { + t.Error("getHookVersion() IsShim = true, want false for inline bd hook") + } + + // Test shim hook (should also be recognized as IsBdHook) + shimHook := filepath.Join(tmpDir, "pre-commit-shim") + shimContent := "#!/bin/sh\n# bd-shim v1\nexec bd hooks run pre-commit \"$@\"\n" + if err := os.WriteFile(shimHook, []byte(shimContent), 0755); err != nil { + t.Fatalf("Failed to create shim hook: %v", err) + } + + info, err = getHookVersion(shimHook) + if err != nil { + t.Fatalf("getHookVersion() failed: %v", err) + } + + if !info.IsBdHook { + t.Error("getHookVersion() IsBdHook = false, want true for shim") + } + if !info.IsShim { + t.Error("getHookVersion() IsShim = false, want true for shim") + } + + // Test non-bd hook + userHook := filepath.Join(tmpDir, "pre-commit-user") + userContent := "#!/bin/sh\necho 'User hook'\n" + if err := os.WriteFile(userHook, []byte(userContent), 0755); err != nil { + t.Fatalf("Failed to create user hook: %v", err) + } + + info, err = getHookVersion(userHook) + if err != nil { + t.Fatalf("getHookVersion() failed: %v", err) + } + + if info.IsBdHook { + t.Error("getHookVersion() IsBdHook = true, want false for user hook") + } +} + +// TestInstallHooksChainingSkipsInlineHook verifies that bd hooks install --chain +// does NOT rename existing inline bd hooks to .old (which would destroy user's original). +// See: https://github.com/steveyegge/beads/issues/1120 +func TestInstallHooksChainingSkipsInlineHook(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 that IS an inline bd hook (from bd init) + existingHook := filepath.Join(gitDir, "pre-commit") + inlineContent := `#!/bin/sh +# +# bd (beads) pre-commit hook (chained) +# +bd sync --flush-only +` + if err := os.WriteFile(existingHook, []byte(inlineContent), 0755); err != nil { + t.Fatalf("Failed to create existing inline 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 inline hook was NOT renamed to .old (would destroy user's original) + oldPath := existingHook + ".old" + if _, err := os.Stat(oldPath); !os.IsNotExist(err) { + t.Errorf("inline bd hook was renamed to .old - this would destroy user's original hook!") + } + + // Verify new hook was installed (overwrote the inline hook) + if _, err := os.Stat(existingHook); os.IsNotExist(err) { + t.Errorf("New pre-commit hook was not installed") + } + }) +} + +// TestInstallHooksChainingPreservesExistingOld verifies that bd hooks install --chain +// does NOT overwrite an existing .old file (which would destroy user's original hook). +// See: https://github.com/steveyegge/beads/issues/1120 +func TestInstallHooksChainingPreservesExistingOld(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 the user's original hook as .old (simulating bd init already ran) + originalHook := filepath.Join(gitDir, "pre-commit.old") + originalContent := "#!/bin/sh\necho 'User original hook'\n" + if err := os.WriteFile(originalHook, []byte(originalContent), 0755); err != nil { + t.Fatalf("Failed to create original .old hook: %v", err) + } + + // Create a current hook that is NOT a bd hook (e.g., user modified it) + currentHook := filepath.Join(gitDir, "pre-commit") + currentContent := "#!/bin/sh\necho 'Some other hook'\n" + if err := os.WriteFile(currentHook, []byte(currentContent), 0755); err != nil { + t.Fatalf("Failed to create current 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 .old was preserved (not overwritten) + preservedContent, err := os.ReadFile(originalHook) + if err != nil { + t.Fatalf("Failed to read preserved .old hook: %v", err) + } + if string(preservedContent) != originalContent { + t.Errorf(".old hook was overwritten! got %q, want %q", string(preservedContent), originalContent) + } + + // Verify new hook was installed + if _, err := os.Stat(currentHook); os.IsNotExist(err) { + t.Errorf("New pre-commit hook was not installed") + } + }) +} + +// TestRunChainedHookSkipsInlineHook verifies that runChainedHook() skips +// .old hooks that are inline bd hooks (to prevent recursion). +// See: https://github.com/steveyegge/beads/issues/1120 +func TestRunChainedHookSkipsInlineHook(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 a .old hook that IS an inline bd hook + oldHook := filepath.Join(gitDir, "pre-commit.old") + inlineContent := `#!/bin/sh +# +# bd (beads) pre-commit hook (chained) +# +bd sync --flush-only +` + if err := os.WriteFile(oldHook, []byte(inlineContent), 0755); err != nil { + t.Fatalf("Failed to create .old inline hook: %v", err) + } + + // runChainedHook should return 0 (skip the inline hook) instead of executing it + exitCode := runChainedHook("pre-commit", nil) + if exitCode != 0 { + t.Errorf("runChainedHook() = %d, want 0 (should skip inline bd hook)", exitCode) + } + }) +}