From dc98c5223203365ecc7cd68b3393a8b9e1c7246a Mon Sep 17 00:00:00 2001 From: "minion[bot]" Date: Wed, 18 Mar 2026 10:29:01 +0000 Subject: [PATCH] Install git hooks using chaining instead of overwriting existing hooks Automated by partio-io/minions (task: git-hook-chaining) Co-Authored-By: Claude --- internal/git/hooks/hooks.go | 34 +++++++++----- internal/git/hooks/hooks_test.go | 78 +++++++++++++++++++++----------- internal/git/hooks/install.go | 41 +++++++++++------ internal/git/hooks/uninstall.go | 31 +++++++++---- 4 files changed, 122 insertions(+), 62 deletions(-) diff --git a/internal/git/hooks/hooks.go b/internal/git/hooks/hooks.go index c38ecf4..d519a62 100644 --- a/internal/git/hooks/hooks.go +++ b/internal/git/hooks/hooks.go @@ -2,29 +2,39 @@ package hooks import ( "fmt" + "regexp" "strings" ) -const partioMarker = "# Installed by partio" +const ( + beginSentinel = "# BEGIN partio" + endSentinel = "# END partio" +) var hookNames = []string{"pre-commit", "post-commit", "pre-push"} -// hookScript returns the bash shim for a given hook name. -func hookScript(name string) string { - return fmt.Sprintf(`#!/bin/bash -%s +var partioBlockRe = regexp.MustCompile(`\n?` + beginSentinel + `\n[\s\S]*?` + endSentinel + `\n?`) + +// partioBlock returns the partio invocation block for the given hook name. +func partioBlock(name string) string { + return fmt.Sprintf(`%s if command -v partio &> /dev/null; then partio _hook %s "$@" exit_code=$? [ $exit_code -ne 0 ] && exit $exit_code fi -# Chain to original hook if backed up -hooks_dir="$(git rev-parse --git-common-dir)/hooks" -[ -f "$hooks_dir/%s.partio-backup" ] && exec "$hooks_dir/%s.partio-backup" "$@" -exit 0 -`, partioMarker, name, name, name) +%s`, beginSentinel, name, endSentinel) +} + +// newHookScript returns a complete hook script for a new hook file. +func newHookScript(name string) string { + return "#!/bin/bash\n" + partioBlock(name) + "\n" +} + +func hasPartioBlock(content string) bool { + return strings.Contains(content, beginSentinel) } -func isPartioHook(content string) bool { - return strings.Contains(content, partioMarker) +func removePartioBlock(content string) string { + return partioBlockRe.ReplaceAllString(content, "") } diff --git a/internal/git/hooks/hooks_test.go b/internal/git/hooks/hooks_test.go index 246cb3c..221029d 100644 --- a/internal/git/hooks/hooks_test.go +++ b/internal/git/hooks/hooks_test.go @@ -4,6 +4,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" ) @@ -36,8 +37,8 @@ func TestInstallAndUninstall(t *testing.T) { } content := string(data) - if !isPartioHook(content) { - t.Errorf("hook %s missing partio marker", name) + if !hasPartioBlock(content) { + t.Errorf("hook %s missing partio block", name) } // Check executable permission @@ -52,7 +53,7 @@ func TestInstallAndUninstall(t *testing.T) { t.Fatalf("Uninstall error: %v", err) } - // Verify hooks are removed + // Verify hooks are removed (no pre-existing hook, so file should be gone) for _, name := range hookNames { path := filepath.Join(hooksDir, name) if _, err := os.Stat(path); !os.IsNotExist(err) { @@ -61,7 +62,7 @@ func TestInstallAndUninstall(t *testing.T) { } } -func TestInstallBackupChaining(t *testing.T) { +func TestInstallChaining(t *testing.T) { dir := initGitRepo(t) hooksDir := filepath.Join(dir, ".git", "hooks") @@ -77,36 +78,40 @@ func TestInstallBackupChaining(t *testing.T) { t.Fatalf("Install error: %v", err) } - // Original should be backed up + // No backup should be created backupPath := hookPath + ".partio-backup" - data, err := os.ReadFile(backupPath) - if err != nil { - t.Fatalf("backup not found: %v", err) - } - if string(data) != existingHook { - t.Errorf("backup content mismatch: %q", string(data)) + if _, err := os.Stat(backupPath); err == nil { + t.Error("backup file should not be created with chaining approach") } - // New hook should be ours - data, err = os.ReadFile(hookPath) + // Hook should contain both original content and partio block + data, err := os.ReadFile(hookPath) if err != nil { t.Fatalf("reading hook: %v", err) } - if !isPartioHook(string(data)) { - t.Error("installed hook missing partio marker") + content := string(data) + if !strings.Contains(content, "echo 'existing hook'") { + t.Error("hook missing original content") + } + if !hasPartioBlock(content) { + t.Error("hook missing partio block") } - // Uninstall should restore original + // Uninstall should remove only the partio block, leaving original content if err := Uninstall(dir); err != nil { t.Fatalf("Uninstall error: %v", err) } data, err = os.ReadFile(hookPath) if err != nil { - t.Fatalf("hook should exist after uninstall (restored): %v", err) + t.Fatalf("hook should exist after uninstall (original preserved): %v", err) + } + restored := string(data) + if hasPartioBlock(restored) { + t.Error("partio block should be removed after uninstall") } - if string(data) != existingHook { - t.Errorf("original hook not restored: %q", string(data)) + if !strings.Contains(restored, "echo 'existing hook'") { + t.Error("original hook content should be preserved after uninstall") } } @@ -152,26 +157,47 @@ func TestInstallWorktree(t *testing.T) { t.Errorf("hook %s not found in worktree: %v", name, err) continue } - if !isPartioHook(string(data)) { - t.Errorf("hook %s missing partio marker in worktree", name) + if !hasPartioBlock(string(data)) { + t.Errorf("hook %s missing partio block in worktree", name) } } } -func TestIsPartioHook(t *testing.T) { +func TestHasPartioBlock(t *testing.T) { tests := []struct { content string expected bool }{ - {"#!/bin/bash\n# Installed by partio\npartio _hook pre-commit", true}, + {"#!/bin/bash\n" + beginSentinel + "\npartio _hook pre-commit\n" + endSentinel, true}, {"#!/bin/bash\necho hello", false}, {"", false}, - {partioMarker, true}, + {beginSentinel, true}, } for _, tt := range tests { - if got := isPartioHook(tt.content); got != tt.expected { - t.Errorf("isPartioHook(%q) = %v, want %v", tt.content, got, tt.expected) + if got := hasPartioBlock(tt.content); got != tt.expected { + t.Errorf("hasPartioBlock(%q) = %v, want %v", tt.content, got, tt.expected) } } } + +func TestRemovePartioBlock(t *testing.T) { + block := partioBlock("pre-commit") + + // Test removal from a file that only has the partio block + onlyPartio := "#!/bin/bash\n" + block + "\n" + result := removePartioBlock(onlyPartio) + if strings.Contains(result, beginSentinel) { + t.Errorf("partio block not removed: %q", result) + } + + // Test removal from a file that has existing content + partio block + withExisting := "#!/bin/bash\necho 'existing'\n\n" + block + "\n" + result = removePartioBlock(withExisting) + if strings.Contains(result, beginSentinel) { + t.Errorf("partio block not removed: %q", result) + } + if !strings.Contains(result, "echo 'existing'") { + t.Errorf("existing content removed: %q", result) + } +} diff --git a/internal/git/hooks/install.go b/internal/git/hooks/install.go index 206d342..7b509fb 100644 --- a/internal/git/hooks/install.go +++ b/internal/git/hooks/install.go @@ -8,7 +8,7 @@ import ( "github.com/partio-io/cli/internal/git" ) -// Install installs partio git hooks into the repository, backing up existing hooks. +// Install installs partio git hooks into the repository, appending to existing hooks. func Install(repoRoot string) error { hooksDir, err := git.HooksDir(repoRoot) if err != nil { @@ -21,22 +21,35 @@ func Install(repoRoot string) error { for _, name := range hookNames { hookPath := filepath.Join(hooksDir, name) - backupPath := hookPath + ".partio-backup" - - // If existing hook is not ours, back it up - if data, err := os.ReadFile(hookPath); err == nil { - content := string(data) - if !isPartioHook(content) { - if err := os.Rename(hookPath, backupPath); err != nil { - return fmt.Errorf("backing up %s hook: %w", name, err) - } + + data, err := os.ReadFile(hookPath) + if err != nil { + // No existing hook — create a new one + script := newHookScript(name) + if err := os.WriteFile(hookPath, []byte(script), 0o755); err != nil { + return fmt.Errorf("writing %s hook: %w", name, err) } + continue + } + + if hasPartioBlock(string(data)) { + // Already installed, skip + continue } - // Write our hook - script := hookScript(name) - if err := os.WriteFile(hookPath, []byte(script), 0o755); err != nil { - return fmt.Errorf("writing %s hook: %w", name, err) + // Append partio block to existing hook + block := "\n" + partioBlock(name) + "\n" + f, err := os.OpenFile(hookPath, os.O_APPEND|os.O_WRONLY, 0o755) + if err != nil { + return fmt.Errorf("opening %s hook for append: %w", name, err) + } + _, writeErr := f.WriteString(block) + closeErr := f.Close() + if writeErr != nil { + return fmt.Errorf("appending to %s hook: %w", name, writeErr) + } + if closeErr != nil { + return fmt.Errorf("closing %s hook: %w", name, closeErr) } } diff --git a/internal/git/hooks/uninstall.go b/internal/git/hooks/uninstall.go index 720372d..6dbd97f 100644 --- a/internal/git/hooks/uninstall.go +++ b/internal/git/hooks/uninstall.go @@ -1,13 +1,15 @@ package hooks import ( + "fmt" "os" "path/filepath" + "strings" "github.com/partio-io/cli/internal/git" ) -// Uninstall removes partio git hooks, restoring backups if present. +// Uninstall removes the partio sentinel block from git hooks, leaving other content intact. func Uninstall(repoRoot string) error { hooksDir, err := git.HooksDir(repoRoot) if err != nil { @@ -16,18 +18,27 @@ func Uninstall(repoRoot string) error { for _, name := range hookNames { hookPath := filepath.Join(hooksDir, name) - backupPath := hookPath + ".partio-backup" - // Only remove if it's our hook - if data, err := os.ReadFile(hookPath); err == nil { - if isPartioHook(string(data)) { - _ = os.Remove(hookPath) - } + data, err := os.ReadFile(hookPath) + if err != nil { + continue // Hook doesn't exist, skip + } + + content := string(data) + if !hasPartioBlock(content) { + continue // No partio block, skip } - // Restore backup if present - if _, err := os.Stat(backupPath); err == nil { - _ = os.Rename(backupPath, hookPath) + stripped := strings.TrimSpace(removePartioBlock(content)) + + if stripped == "" || stripped == "#!/bin/bash" { + // Only partio content remained — remove the file + _ = os.Remove(hookPath) + } else { + // Write back without the partio block + if err := os.WriteFile(hookPath, []byte(stripped+"\n"), 0o755); err != nil { + return fmt.Errorf("writing %s hook after removal: %w", name, err) + } } }