diff --git a/cmd/roborev/hook_integration_test.go b/cmd/roborev/hook_integration_test.go index a4313ddf..7c8bd06d 100644 --- a/cmd/roborev/hook_integration_test.go +++ b/cmd/roborev/hook_integration_test.go @@ -4,10 +4,12 @@ package main import ( "os" + "path/filepath" "runtime" "strings" "testing" + "github.com/roborev-dev/roborev/internal/githook" "github.com/roborev-dev/roborev/internal/testutil" ) @@ -16,7 +18,6 @@ func TestInitCmdCreatesHooksDirectory(t *testing.T) { t.Skip("test uses shell script stub, skipping on Windows") } - // Override HOME to prevent reading real daemon.json tmpHome := t.TempDir() origHome := os.Getenv("HOME") os.Setenv("HOME", tmpHome) @@ -25,37 +26,29 @@ func TestInitCmdCreatesHooksDirectory(t *testing.T) { repo := testutil.NewTestRepo(t) repo.RemoveHooksDir() - // Verify hooks directory doesn't exist if _, err := os.Stat(repo.HooksDir); !os.IsNotExist(err) { t.Fatal("hooks directory should not exist before test") } - // Create a fake roborev binary in PATH defer testutil.MockBinaryInPath(t, "roborev", "#!/bin/sh\nexit 0\n")() - defer repo.Chdir()() - // Build init command initCommand := initCmd() initCommand.SetArgs([]string{"--agent", "test"}) err := initCommand.Execute() - // Should succeed (not fail with "no such file or directory") if err != nil { t.Fatalf("init command failed: %v", err) } - // Verify hooks directory was created if _, err := os.Stat(repo.HooksDir); os.IsNotExist(err) { t.Error("hooks directory was not created") } - // Verify hook file was created if _, err := os.Stat(repo.HookPath); os.IsNotExist(err) { t.Error("post-commit hook was not created") } - // Verify hook is executable info, err := os.Stat(repo.HookPath) if err != nil { t.Fatal(err) @@ -77,8 +70,15 @@ func TestInitCmdUpgradesOutdatedHook(t *testing.T) { repo := testutil.NewTestRepo(t) - // Write a realistic old-style hook (no version marker, has &, includes if/fi block) - oldHook := "#!/bin/sh\n# roborev post-commit hook - auto-reviews every commit\nROBOREV=\"/usr/local/bin/roborev\"\nif [ ! -x \"$ROBOREV\" ]; then\n ROBOREV=$(command -v roborev 2>/dev/null)\n [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ] && exit 0\nfi\n\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" + // Write a realistic old-style hook (v2 format) + oldHook := "#!/bin/sh\n" + + "# roborev post-commit hook v2 - auto-reviews every commit\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "if [ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=$(command -v roborev 2>/dev/null)\n" + + " [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ] && exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" repo.WriteHook(oldHook) defer testutil.MockBinaryInPath(t, "roborev", "#!/bin/sh\nexit 0\n")() @@ -97,18 +97,14 @@ func TestInitCmdUpgradesOutdatedHook(t *testing.T) { } contentStr := string(content) - if !strings.Contains(contentStr, "hook v2") { - t.Error("upgraded hook should contain version marker") - } - if strings.Contains(contentStr, "2>/dev/null &") { - t.Error("upgraded hook should not have backgrounded enqueue") + if !strings.Contains(contentStr, githook.PostCommitVersionMarker) { + t.Errorf("upgraded hook should contain v3 marker, got:\n%s", contentStr) } - if !strings.Contains(contentStr, "2>/dev/null\n") { - t.Error("upgraded hook should still have enqueue line (without &)") + if strings.Contains(contentStr, "hook v2") { + t.Error("upgraded hook should not contain old v2 marker") } - // Verify the if/fi block is preserved intact (no stray fi) - if !strings.Contains(contentStr, "if [ ! -x") { - t.Error("upgraded hook should preserve the if block") + if !strings.Contains(contentStr, "enqueue --quiet") { + t.Error("upgraded hook should have enqueue line") } } @@ -124,7 +120,16 @@ func TestInitCmdPreservesOtherHooksOnUpgrade(t *testing.T) { repo := testutil.NewTestRepo(t) - oldHook := "#!/bin/sh\necho 'my custom hook'\n# roborev post-commit hook - auto-reviews every commit\nROBOREV=\"/usr/local/bin/roborev\"\nif [ ! -x \"$ROBOREV\" ]; then\n ROBOREV=$(command -v roborev 2>/dev/null)\n [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ] && exit 0\nfi\n\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" + // Mixed hook: user content + old v2 roborev snippet + oldHook := "#!/bin/sh\n" + + "echo 'my custom hook'\n" + + "# roborev post-commit hook v2 - auto-reviews every commit\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "if [ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=$(command -v roborev 2>/dev/null)\n" + + " [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ] && exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" repo.WriteHook(oldHook) defer testutil.MockBinaryInPath(t, "roborev", "#!/bin/sh\nexit 0\n")() @@ -146,10 +151,79 @@ func TestInitCmdPreservesOtherHooksOnUpgrade(t *testing.T) { if !strings.Contains(contentStr, "echo 'my custom hook'") { t.Error("upgrade should preserve non-roborev lines") } - if !strings.Contains(contentStr, "hook v2") { - t.Error("upgrade should contain version marker") + if !strings.Contains(contentStr, githook.PostCommitVersionMarker) { + t.Errorf("upgrade should contain v3 marker, got:\n%s", contentStr) + } + if strings.Contains(contentStr, "hook v2") { + t.Error("upgrade should remove old v2 marker") + } +} + +func TestInitCmdEarlyExitHookStillRunsRoborev(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test uses shell script stub, skipping on Windows") + } + + tmpHome := t.TempDir() + origHome := os.Getenv("HOME") + os.Setenv("HOME", tmpHome) + defer os.Setenv("HOME", origHome) + + repo := testutil.NewTestRepo(t) + + // Husky-style hook with exit 0 at the end + huskyHook := "#!/bin/sh\n" + + ". \"$(dirname \"$0\")/_/husky.sh\"\n" + + "npx lint-staged\n" + + "exit 0\n" + repo.WriteHook(huskyHook) + + defer testutil.MockBinaryInPath(t, "roborev", "#!/bin/sh\nexit 0\n")() + defer repo.Chdir()() + + initCommand := initCmd() + initCommand.SetArgs([]string{"--agent", "test"}) + err := initCommand.Execute() + if err != nil { + t.Fatalf("init command failed: %v", err) + } + + content, err := os.ReadFile(repo.HookPath) + if err != nil { + t.Fatalf("Failed to read hook: %v", err) + } + + contentStr := string(content) + + // Roborev snippet should appear before exit 0 + snippetIdx := strings.Index(contentStr, "_roborev_hook") + exitIdx := strings.Index(contentStr, "exit 0") + if snippetIdx < 0 { + t.Fatal("roborev snippet should be present") + } + if exitIdx < 0 { + t.Fatal("exit 0 should be preserved") + } + if snippetIdx > exitIdx { + t.Error("roborev snippet should appear before exit 0") + } + + // All original content should be preserved + if !strings.Contains(contentStr, "husky.sh") { + t.Error("husky.sh reference should be preserved") + } + if !strings.Contains(contentStr, "npx lint-staged") { + t.Error("lint-staged command should be preserved") + } + + // Post-rewrite hook should also be installed + prContent, err := os.ReadFile( + filepath.Join(repo.HooksDir, "post-rewrite"), + ) + if err != nil { + t.Fatalf("post-rewrite hook should be created: %v", err) } - if strings.Contains(contentStr, "2>/dev/null &") { - t.Error("upgrade should remove trailing &") + if !strings.Contains(string(prContent), githook.PostRewriteVersionMarker) { + t.Error("post-rewrite hook should have version marker") } } diff --git a/cmd/roborev/hook_test.go b/cmd/roborev/hook_test.go index 2731c68d..c9a26ddd 100644 --- a/cmd/roborev/hook_test.go +++ b/cmd/roborev/hook_test.go @@ -3,7 +3,6 @@ package main import ( "errors" "fmt" - "io/fs" "net" "net/http" "net/http/httptest" @@ -14,6 +13,7 @@ import ( "strings" "testing" + "github.com/roborev-dev/roborev/internal/githook" "github.com/roborev-dev/roborev/internal/testutil" ) @@ -28,7 +28,6 @@ func TestUninstallHookCmd(t *testing.T) { t.Fatalf("uninstall-hook failed: %v", err) } - // Hook should still not exist if _, err := os.Stat(repo.HookPath); !os.IsNotExist(err) { t.Error("Hook file should not exist") } @@ -46,7 +45,6 @@ func TestUninstallHookCmd(t *testing.T) { t.Fatalf("uninstall-hook failed: %v", err) } - // Hook should be unchanged content, err := os.ReadFile(repo.HookPath) if err != nil { t.Fatalf("Failed to read hook: %v", err) @@ -58,7 +56,7 @@ func TestUninstallHookCmd(t *testing.T) { t.Run("hook with roborev only - removes file", func(t *testing.T) { repo := testutil.NewTestRepo(t) - repo.WriteHook(generateHookContent()) + repo.WriteHook(githook.GeneratePostCommit()) defer repo.Chdir()() cmd := uninstallHookCmd() @@ -67,7 +65,6 @@ func TestUninstallHookCmd(t *testing.T) { t.Fatalf("uninstall-hook failed: %v", err) } - // Hook should be removed entirely if _, err := os.Stat(repo.HookPath); !os.IsNotExist(err) { t.Error("Hook file should have been removed") } @@ -76,7 +73,7 @@ func TestUninstallHookCmd(t *testing.T) { t.Run("hook with roborev and other commands - preserves others", func(t *testing.T) { repo := testutil.NewTestRepo(t) mixed := "#!/bin/sh\necho 'before'\necho 'after'\n" + - generateHookContent() + githook.GeneratePostCommit() repo.WriteHook(mixed) defer repo.Chdir()() @@ -86,7 +83,6 @@ func TestUninstallHookCmd(t *testing.T) { t.Fatalf("uninstall-hook failed: %v", err) } - // Hook should exist with roborev snippet removed content, err := os.ReadFile(repo.HookPath) if err != nil { t.Fatalf("Failed to read hook: %v", err) @@ -106,12 +102,11 @@ func TestUninstallHookCmd(t *testing.T) { t.Run("also removes post-rewrite hook", func(t *testing.T) { repo := testutil.NewTestRepo(t) - repo.WriteHook(generateHookContent()) - // Also install post-rewrite hook + repo.WriteHook(githook.GeneratePostCommit()) prPath := filepath.Join(repo.HooksDir, "post-rewrite") os.WriteFile( prPath, - []byte(generatePostRewriteHookContent()), + []byte(githook.GeneratePostRewrite()), 0755, ) defer repo.Chdir()() @@ -132,12 +127,11 @@ func TestUninstallHookCmd(t *testing.T) { t.Run("removes post-rewrite even without post-commit", func(t *testing.T) { repo := testutil.NewTestRepo(t) - // Only install post-rewrite, no post-commit prPath := filepath.Join(repo.HooksDir, "post-rewrite") os.MkdirAll(repo.HooksDir, 0755) os.WriteFile( prPath, - []byte(generatePostRewriteHookContent()), + []byte(githook.GeneratePostRewrite()), 0755, ) defer repo.Chdir()() @@ -162,14 +156,12 @@ func TestInstallHookCmdCreatesHooksDirectory(t *testing.T) { repo := testutil.NewTestRepo(t) repo.RemoveHooksDir() - // Verify hooks directory doesn't exist if _, err := os.Stat(repo.HooksDir); !os.IsNotExist(err) { t.Fatal("hooks directory should not exist before test") } defer repo.Chdir()() - // Run install-hook command installCmd := installHookCmd() installCmd.SetArgs([]string{}) err := installCmd.Execute() @@ -178,198 +170,42 @@ func TestInstallHookCmdCreatesHooksDirectory(t *testing.T) { t.Fatalf("install-hook command failed: %v", err) } - // Verify hooks directory was created if _, err := os.Stat(repo.HooksDir); os.IsNotExist(err) { t.Error("hooks directory was not created") } - // Verify hook file was created if _, err := os.Stat(repo.HookPath); os.IsNotExist(err) { t.Error("post-commit hook was not created") } } -func TestGenerateHookContent(t *testing.T) { - content := generateHookContent() - lines := strings.Split(content, "\n") - - t.Run("has shebang", func(t *testing.T) { - if !strings.HasPrefix(content, "#!/bin/sh\n") { - t.Error("hook should start with #!/bin/sh") - } - }) - - t.Run("has roborev comment", func(t *testing.T) { - if !strings.Contains(content, "# roborev") { - t.Error("hook should contain roborev comment for detection") - } - }) - - t.Run("baked path comes first", func(t *testing.T) { - // Security: baked path should be set before any PATH lookup - bakedIdx := -1 - pathIdx := -1 - for i, line := range lines { - if strings.HasPrefix(line, "ROBOREV=") && !strings.Contains(line, "command -v") { - bakedIdx = i - } - if strings.Contains(line, "command -v roborev") { - pathIdx = i - } - } - if bakedIdx == -1 { - t.Error("hook should have baked ROBOREV= assignment") - } - if pathIdx == -1 { - t.Error("hook should have PATH fallback via command -v") - } - if bakedIdx > pathIdx { - t.Error("baked path should come before PATH lookup for security") - } - }) - - t.Run("enqueue line has quiet and stderr redirect without background", func(t *testing.T) { - found := false - for _, line := range lines { - if strings.Contains(line, "enqueue --quiet") && - strings.Contains(line, "2>/dev/null") && - !strings.HasSuffix(strings.TrimSpace(line), "&") { - found = true - break - } - } - if !found { - t.Error("hook should have enqueue line with --quiet and 2>/dev/null but no trailing &") - } - }) - - t.Run("has version marker", func(t *testing.T) { - if !strings.Contains(content, "hook v2") { - t.Error("hook should contain version marker 'hook v2'") - } - }) - - t.Run("baked path is quoted", func(t *testing.T) { - // The baked path should be properly quoted to handle spaces - for _, line := range lines { - if strings.HasPrefix(line, "ROBOREV=") && !strings.Contains(line, "command -v") { - // Should be ROBOREV="/path/to/roborev" or ROBOREV="roborev" - if !strings.Contains(line, `ROBOREV="`) { - t.Errorf("baked path should be quoted, got: %s", line) - } - break - } - } - }) - - t.Run("baked path is absolute", func(t *testing.T) { - // os.Executable() returns absolute path; verify it's baked correctly - for _, line := range lines { - if strings.HasPrefix(line, "ROBOREV=") && !strings.Contains(line, "command -v") { - // Extract the path from ROBOREV="/path/here" - start := strings.Index(line, `"`) - end := strings.LastIndex(line, `"`) - if start != -1 && end > start { - path := line[start+1 : end] - if !filepath.IsAbs(path) { - t.Errorf("baked path should be absolute, got: %s", path) - } - } - break - } - } - }) -} - -func TestHookNeedsUpgrade(t *testing.T) { - t.Run("outdated hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\n# roborev post-commit hook\nroborev enqueue\n") - if !hookNeedsUpgrade(repo.Root, "post-commit", hookVersionMarker) { - t.Error("should detect outdated hook") - } - }) - - t.Run("current hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\n# roborev post-commit hook v2 - auto-reviews every commit\nroborev enqueue\n") - if hookNeedsUpgrade(repo.Root, "post-commit", hookVersionMarker) { - t.Error("should not flag current hook as outdated") - } - }) - - t.Run("no hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if hookNeedsUpgrade(repo.Root, "post-commit", hookVersionMarker) { - t.Error("should not flag missing hook as outdated") - } - }) - - t.Run("non-roborev hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\necho hello\n") - if hookNeedsUpgrade(repo.Root, "post-commit", hookVersionMarker) { - t.Error("should not flag non-roborev hook as outdated") - } - }) - - t.Run("post-rewrite outdated", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - hooksDir := filepath.Join(repo.Root, ".git", "hooks") - os.MkdirAll(hooksDir, 0755) - os.WriteFile(filepath.Join(hooksDir, "post-rewrite"), - []byte("#!/bin/sh\n# roborev hook\nroborev remap\n"), 0755) - if !hookNeedsUpgrade(repo.Root, "post-rewrite", postRewriteHookVersionMarker) { - t.Error("should detect outdated post-rewrite hook") - } - }) - - t.Run("post-rewrite current", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - hooksDir := filepath.Join(repo.Root, ".git", "hooks") - os.MkdirAll(hooksDir, 0755) - os.WriteFile(filepath.Join(hooksDir, "post-rewrite"), - []byte("#!/bin/sh\n# roborev post-rewrite hook v1\nroborev remap\n"), 0755) - if hookNeedsUpgrade(repo.Root, "post-rewrite", postRewriteHookVersionMarker) { - t.Error("should not flag current post-rewrite hook") - } - }) -} +func TestInstallHookCmdCreatesPostRewriteHook(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test checks Unix exec bits, skipping on Windows") + } -func TestHookMissing(t *testing.T) { - t.Run("missing post-rewrite with roborev post-commit", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\n# roborev post-commit hook v2\nroborev enqueue\n") - if !hookMissing(repo.Root, "post-rewrite") { - t.Error("should detect missing post-rewrite when post-commit has roborev") - } - }) + repo := testutil.NewTestRepo(t) + repo.RemoveHooksDir() + defer repo.Chdir()() - t.Run("no post-commit hook at all", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if hookMissing(repo.Root, "post-rewrite") { - t.Error("should not warn when post-commit is not installed") - } - }) + installCmd := installHookCmd() + installCmd.SetArgs([]string{}) + if err := installCmd.Execute(); err != nil { + t.Fatalf("install-hook failed: %v", err) + } - t.Run("post-rewrite exists with roborev", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\n# roborev post-commit hook v2\nroborev enqueue\n") - hooksDir := filepath.Join(repo.Root, ".git", "hooks") - os.WriteFile(filepath.Join(hooksDir, "post-rewrite"), - []byte("#!/bin/sh\n# roborev post-rewrite hook v1\nroborev remap\n"), 0755) - if hookMissing(repo.Root, "post-rewrite") { - t.Error("should not warn when post-rewrite has roborev content") - } - }) + prHookPath := filepath.Join(repo.HooksDir, "post-rewrite") + content, err := os.ReadFile(prHookPath) + if err != nil { + t.Fatalf("post-rewrite hook not created: %v", err) + } - t.Run("non-roborev post-commit", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\necho hello\n") - if hookMissing(repo.Root, "post-rewrite") { - t.Error("should not warn when post-commit is not roborev") - } - }) + if !strings.Contains(string(content), "remap --quiet") { + t.Error("post-rewrite hook should contain 'remap --quiet'") + } + if !strings.Contains(string(content), githook.PostRewriteVersionMarker) { + t.Error("post-rewrite hook should contain version marker") + } } func TestIsTransportError(t *testing.T) { @@ -383,7 +219,6 @@ func TestIsTransportError(t *testing.T) { }) t.Run("url.Error without OpError is not transport error", func(t *testing.T) { - // e.g. malformed URL, TLS config error err := &url.Error{Op: "Post", URL: "http://127.0.0.1:7373", Err: errors.New("some non-transport error")} if isTransportError(err) { t.Error("expected url.Error without net.OpError to NOT be transport error") @@ -421,7 +256,6 @@ func TestRegisterRepoError(t *testing.T) { t.Errorf("unexpected error message: %s", err.Error()) } - // Verify it satisfies the error interface and is distinguishable var regErr *registerRepoError if !errors.As(err, ®Err) { t.Error("expected errors.As to match registerRepoError") @@ -433,7 +267,6 @@ func TestRegisterRepoError(t *testing.T) { // initNoDaemonSetup prepares the environment for init --no-daemon tests: // isolated HOME, fake roborev binary, and chdir to a test repo. -// Returns the repo path and a cleanup function. func initNoDaemonSetup(t *testing.T) string { t.Helper() @@ -455,9 +288,8 @@ func TestInitNoDaemon_ConnectionError(t *testing.T) { } initNoDaemonSetup(t) - // Point to a port that's definitely not listening oldAddr := serverAddr - serverAddr = "http://127.0.0.1:1" // port 1 won't have a daemon + serverAddr = "http://127.0.0.1:1" defer func() { serverAddr = oldAddr }() output := captureStdout(t, func() { @@ -483,7 +315,6 @@ func TestInitNoDaemon_ServerError(t *testing.T) { } initNoDaemonSetup(t) - // Spin up a test server that returns 500 ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(500) _, _ = w.Write([]byte("database locked")) @@ -520,7 +351,6 @@ func TestInitNoDaemon_Success(t *testing.T) { } initNoDaemonSetup(t) - // Spin up a test server that returns 200 ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) })) @@ -547,783 +377,97 @@ func TestInitNoDaemon_Success(t *testing.T) { } } -func TestInstallHookCmdCreatesPostRewriteHook(t *testing.T) { +func TestInitInstallsPostRewriteHookOnUpgrade(t *testing.T) { if runtime.GOOS == "windows" { - t.Skip("test checks Unix exec bits, skipping on Windows") + t.Skip("test uses shell script stub, skipping on Windows") } + root := initNoDaemonSetup(t) - repo := testutil.NewTestRepo(t) - repo.RemoveHooksDir() - defer repo.Chdir()() - - installCmd := installHookCmd() - installCmd.SetArgs([]string{}) - if err := installCmd.Execute(); err != nil { - t.Fatalf("install-hook failed: %v", err) + // Pre-install a current post-commit hook so init sees it + hooksDir := filepath.Join(root, ".git", "hooks") + if err := os.MkdirAll(hooksDir, 0755); err != nil { + t.Fatal(err) + } + hookContent := githook.GeneratePostCommit() + if err := os.WriteFile(filepath.Join(hooksDir, "post-commit"), []byte(hookContent), 0755); err != nil { + t.Fatal(err) } - prHookPath := filepath.Join(repo.HooksDir, "post-rewrite") + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + })) + defer ts.Close() + + oldAddr := serverAddr + serverAddr = ts.URL + defer func() { serverAddr = oldAddr }() + + captureStdout(t, func() { + cmd := initCmd() + cmd.SetArgs([]string{"--no-daemon"}) + _ = cmd.Execute() + }) + + prHookPath := filepath.Join(hooksDir, "post-rewrite") content, err := os.ReadFile(prHookPath) if err != nil { - t.Fatalf("post-rewrite hook not created: %v", err) + t.Fatalf("post-rewrite hook should be installed: %v", err) } - if !strings.Contains(string(content), "remap --quiet") { t.Error("post-rewrite hook should contain 'remap --quiet'") } - if !strings.Contains(string(content), postRewriteHookVersionMarker) { - t.Error("post-rewrite hook should contain version marker") - } } -func TestInstallOrUpgradeHook(t *testing.T) { - t.Run("appends to existing non-roborev hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - existing := "#!/bin/sh\necho 'custom logic'\n" - if err := os.WriteFile(hookPath, []byte(existing), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err != nil { - t.Fatalf("installOrUpgradeHook: %v", err) - } - - content, _ := os.ReadFile(hookPath) - contentStr := string(content) - if !strings.Contains(contentStr, "echo 'custom logic'") { - t.Error("original content should be preserved") - } - if !strings.Contains(contentStr, hookVersionMarker) { - t.Error("roborev snippet should be appended") - } - }) - - t.Run("skips current version", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - current := generatePostRewriteHookContent() - if err := os.WriteFile(hookPath, []byte(current), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-rewrite", - postRewriteHookVersionMarker, - generatePostRewriteHookContent, false, - ) - if err != nil { - t.Fatalf("installOrUpgradeHook: %v", err) - } - - content, _ := os.ReadFile(hookPath) - if string(content) != current { - t.Error("current hook should not be modified") - } - }) - - t.Run("upgrades outdated roborev hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - // Simulates an older generated hook (has marker but no version) - outdated := "#!/bin/sh\n" + - "# roborev post-commit hook\n" + - "ROBOREV=\"/usr/local/bin/roborev\"\n" + - "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" - if err := os.WriteFile(hookPath, []byte(outdated), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err != nil { - t.Fatalf("installOrUpgradeHook: %v", err) - } - - content, _ := os.ReadFile(hookPath) - contentStr := string(content) - if !strings.Contains(contentStr, hookVersionMarker) { - t.Error("should have new version marker") - } - // Old marker should be gone - if strings.Contains(contentStr, "# roborev post-commit hook\n") { - t.Error("old marker should be removed") - } - }) - - t.Run("upgrades mixed hook preserving user content", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - mixed := "#!/bin/sh\necho 'user code'\n# roborev post-rewrite hook\nROBOREV=\"/usr/bin/roborev\"\n\"$ROBOREV\" remap --quiet 2>/dev/null\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-rewrite", - postRewriteHookVersionMarker, - generatePostRewriteHookContent, false, - ) - if err != nil { - t.Fatalf("installOrUpgradeHook: %v", err) - } - - content, _ := os.ReadFile(hookPath) - contentStr := string(content) - if !strings.Contains(contentStr, "echo 'user code'") { - t.Error("user content should be preserved") - } - if !strings.Contains(contentStr, postRewriteHookVersionMarker) { - t.Error("should have new version marker") - } - }) - - t.Run("appends to hooks with various shell shebangs", func(t *testing.T) { - shebangs := []string{ - "#!/bin/sh", "#!/usr/bin/env sh", - "#!/bin/bash", "#!/usr/bin/env bash", - "#!/bin/zsh", "#!/usr/bin/env zsh", - "#!/bin/ksh", "#!/usr/bin/env ksh", - "#!/bin/dash", "#!/usr/bin/env dash", - } - for _, shebang := range shebangs { - t.Run(shebang, func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - existing := shebang + "\necho 'custom'\n" - if err := os.WriteFile(hookPath, []byte(existing), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err != nil { - t.Fatalf("should append to %s hook: %v", shebang, err) - } - - content, _ := os.ReadFile(hookPath) - if !strings.Contains(string(content), "echo 'custom'") { - t.Error("original content should be preserved") - } - if !strings.Contains(string(content), hookVersionMarker) { - t.Error("roborev content should be appended") - } - }) - } - }) - - t.Run("refuses to append to non-shell hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - pythonHook := "#!/usr/bin/env python3\nprint('hello')\n" - if err := os.WriteFile(hookPath, []byte(pythonHook), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err == nil { - t.Fatal("expected error for non-shell hook") - } - if !strings.Contains(err.Error(), "non-shell interpreter") { - t.Errorf("unexpected error: %v", err) - } - - // Verify hook was not modified - content, _ := os.ReadFile(hookPath) - if string(content) != pythonHook { - t.Errorf("hook should be unchanged, got:\n%s", content) - } - }) - - t.Run("upgrade returns error on re-read failure", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - // Outdated hook triggers the upgrade path. - outdated := "#!/bin/sh\n" + - "# roborev post-commit hook\n" + - "ROBOREV=\"/usr/local/bin/roborev\"\n" + - "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" - if err := os.WriteFile( - hookPath, []byte(outdated), 0755, - ); err != nil { - t.Fatal(err) - } - - // Inject a non-ENOENT error on re-read after cleanup. - origReadFile := hookReadFile - hookReadFile = func(string) ([]byte, error) { - return nil, fs.ErrPermission - } - t.Cleanup(func() { hookReadFile = origReadFile }) - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err == nil { - t.Fatal("expected error from re-read failure") - } - if !strings.Contains(err.Error(), "re-read") { - t.Errorf("error should mention re-read, got: %v", err) - } - if !errors.Is(err, fs.ErrPermission) { - t.Errorf( - "error should wrap ErrPermission, got: %v", err, - ) - } - }) - - t.Run("refuses upgrade of non-shell hook mentioning roborev", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - // Non-shell hook that mentions roborev but has no version marker. - pythonHook := "#!/usr/bin/env python3\n# reviewed by roborev\nprint('hello')\n" - if err := os.WriteFile(hookPath, []byte(pythonHook), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err == nil { - t.Fatal("expected error for non-shell hook in upgrade path") - } - if !strings.Contains(err.Error(), "non-shell interpreter") { - t.Errorf("unexpected error: %v", err) - } - - // Hook must not be modified. - content, _ := os.ReadFile(hookPath) - if string(content) != pythonHook { - t.Errorf("hook should be unchanged, got:\n%s", content) - } - }) - - t.Run("force overwrites existing hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - existing := "#!/bin/sh\necho 'custom'\n" - if err := os.WriteFile(hookPath, []byte(existing), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, true, - ) - if err != nil { - t.Fatalf("installOrUpgradeHook: %v", err) - } - - content, _ := os.ReadFile(hookPath) - contentStr := string(content) - if strings.Contains(contentStr, "echo 'custom'") { - t.Error("force should overwrite, not append") - } - if !strings.Contains(contentStr, hookVersionMarker) { - t.Error("should have roborev content") - } - }) -} - -func TestRemoveRoborevFromHook(t *testing.T) { - t.Run("generated hook is deleted entirely", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - hookContent := generatePostRewriteHookContent() - if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - if _, err := os.Stat(hookPath); !os.IsNotExist(err) { - t.Error("generated hook should have been deleted entirely") - } - }) - - t.Run("mixed hook preserves non-roborev content", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - mixed := "#!/bin/sh\necho 'custom logic'\n" + generatePostRewriteHookContent() - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if strings.Contains(strings.ToLower(contentStr), "roborev") { - t.Errorf("roborev content should be removed, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "echo 'custom logic'") { - t.Error("custom content should be preserved") - } - // Verify no orphaned fi - if strings.Contains(contentStr, "\nfi\n") || strings.HasSuffix(strings.TrimSpace(contentStr), "fi") { - t.Errorf("should not have orphaned fi, got:\n%s", contentStr) - } - }) - - t.Run("custom line mentioning roborev before snippet", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // Custom line mentions roborev but isn't the generated marker - mixed := "#!/bin/sh\necho 'notify roborev team'\n" + - generatePostRewriteHookContent() - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - // The custom line should be preserved - if !strings.Contains(contentStr, "notify roborev team") { - t.Error("custom line mentioning roborev should be preserved") - } - // The generated snippet should be removed - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("custom if-block after snippet is preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // Snippet followed by user's own if-block - mixed := "#!/bin/sh\n" + - generatePostRewriteHookContent() + - "if [ -f .notify ]; then\n" + - " echo 'send notification'\n" + - "fi\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - // User's if-block must survive - if !strings.Contains(contentStr, "send notification") { - t.Errorf("user if-block should be preserved, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "if [ -f .notify ]") { - t.Errorf("user if-statement should be preserved, got:\n%s", contentStr) - } - // No orphaned fi - lines := strings.Split(contentStr, "\n") - ifCount, fiCount := 0, 0 - for _, line := range lines { - trimmed := strings.TrimSpace(line) - if strings.HasPrefix(trimmed, "if ") { - ifCount++ - } - if trimmed == "fi" { - fiCount++ - } - } - if ifCount != fiCount { - t.Errorf("if/fi mismatch: %d if vs %d fi in:\n%s", - ifCount, fiCount, contentStr) - } - // Generated snippet should be gone - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("custom comment starting with # roborev is preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // User comment starts with "# roborev" but isn't a generated marker - mixed := "#!/bin/sh\n# roborev notes: this hook was customized\necho 'custom'\n" + - generatePostRewriteHookContent() - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if !strings.Contains(contentStr, "roborev notes") { - t.Error("custom comment should be preserved") - } - if !strings.Contains(contentStr, "echo 'custom'") { - t.Error("custom echo should be preserved") - } - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("post-snippet user line mentioning roborev is preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // Snippet followed by user logic that mentions roborev - mixed := "#!/bin/sh\n" + - generatePostRewriteHookContent() + - "echo 'roborev hook finished'\n" + - "logger 'roborev done'\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if !strings.Contains(contentStr, "roborev hook finished") { - t.Errorf("user line mentioning roborev should be preserved, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "roborev done") { - t.Errorf("user logger line should be preserved, got:\n%s", contentStr) - } - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("user $ROBOREV line after snippet is preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // User has their own "$ROBOREV" line (e.g. version check) after snippet - mixed := "#!/bin/sh\n" + - generatePostRewriteHookContent() + - "\"$ROBOREV\" --version\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if !strings.Contains(contentStr, "\"$ROBOREV\" --version") { - t.Errorf("user $ROBOREV line should be preserved, got:\n%s", contentStr) - } - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("user $ROBOREV enqueue/remap lines after snippet are preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // User added their own enqueue/remap invocations with different flags - mixed := "#!/bin/sh\n" + - generatePostRewriteHookContent() + - "\"$ROBOREV\" enqueue --dry-run\n" + - "\"$ROBOREV\" remap --verbose\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if !strings.Contains(contentStr, "enqueue --dry-run") { - t.Errorf("user enqueue line should be preserved, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "remap --verbose") { - t.Errorf("user remap line should be preserved, got:\n%s", contentStr) - } - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("user --quietly/--quiet-mode lines are preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // User lines that start with --quiet but aren't the exact generated form - mixed := "#!/bin/sh\n" + - generatePostRewriteHookContent() + - "\"$ROBOREV\" enqueue --quietly\n" + - "\"$ROBOREV\" remap --quiet-mode\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if !strings.Contains(contentStr, "enqueue --quietly") { - t.Errorf("user --quietly line should be preserved, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "remap --quiet-mode") { - t.Errorf("user --quiet-mode line should be preserved, got:\n%s", contentStr) - } - }) - - t.Run("v0 hook (plain roborev invocation) is removed", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - v0Hook := "#!/bin/sh\n" + - "# RoboRev post-commit hook - auto-reviews every commit\n" + - "roborev enqueue --sha HEAD 2>/dev/null &\n" - if err := os.WriteFile(hookPath, []byte(v0Hook), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - if _, err := os.Stat(hookPath); !os.IsNotExist(err) { - content, _ := os.ReadFile(hookPath) - t.Errorf("v0 hook should be deleted entirely, got:\n%s", content) - } - }) - - t.Run("v0.5 hook (early variable format) is removed", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - v05Hook := "#!/bin/sh\n" + - "# RoboRev post-commit hook - auto-reviews every commit\n" + - "ROBOREV=\"/usr/local/bin/roborev\"\n" + - "if [ ! -x \"$ROBOREV\" ]; then\n" + - " ROBOREV=$(command -v roborev) || exit 0\n" + - "fi\n" + - "\"$ROBOREV\" enqueue --quiet &\n" - if err := os.WriteFile(hookPath, []byte(v05Hook), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - if _, err := os.Stat(hookPath); !os.IsNotExist(err) { - content, _ := os.ReadFile(hookPath) - t.Errorf("v0.5 hook should be deleted entirely, got:\n%s", content) - } - }) - - t.Run("v1 hook (PATH-first format) is removed", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - v1Hook := "#!/bin/sh\n" + - "# RoboRev post-commit hook - auto-reviews every commit\n" + - "ROBOREV=$(command -v roborev 2>/dev/null)\n" + - "if [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ]; then\n" + - " ROBOREV=\"/usr/local/bin/roborev\"\n" + - " [ ! -x \"$ROBOREV\" ] && exit 0\n" + - "fi\n" + - "\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" - if err := os.WriteFile(hookPath, []byte(v1Hook), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } +func TestInitCmdWarnsOnNonShellHook(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test uses shell script stub, skipping on Windows") + } + root := initNoDaemonSetup(t) - if _, err := os.Stat(hookPath); !os.IsNotExist(err) { - content, _ := os.ReadFile(hookPath) - t.Errorf("v1 hook should be deleted entirely, got:\n%s", content) - } - }) + // Install a non-shell post-commit hook + hooksDir := filepath.Join(root, ".git", "hooks") + os.MkdirAll(hooksDir, 0755) + os.WriteFile( + filepath.Join(hooksDir, "post-commit"), + []byte("#!/usr/bin/env python3\nprint('hello')\n"), + 0755, + ) - t.Run("v1 mixed hook removes only roborev block", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - v1Block := "# RoboRev post-commit hook - auto-reviews every commit\n" + - "ROBOREV=$(command -v roborev 2>/dev/null)\n" + - "if [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ]; then\n" + - " ROBOREV=\"/usr/local/bin/roborev\"\n" + - " [ ! -x \"$ROBOREV\" ] && exit 0\n" + - "fi\n" + - "\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" - mixed := "#!/bin/sh\necho 'custom'\n" + v1Block - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + })) + defer ts.Close() - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } + oldAddr := serverAddr + serverAddr = ts.URL + defer func() { serverAddr = oldAddr }() - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if strings.Contains(strings.ToLower(contentStr), "roborev") { - t.Errorf("roborev content should be removed, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "echo 'custom'") { - t.Error("custom content should be preserved") - } + output := captureStdout(t, func() { + cmd := initCmd() + cmd.SetArgs([]string{"--no-daemon"}) + _ = cmd.Execute() }) - t.Run("no-op if hook has no roborev content", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - hookContent := "#!/bin/sh\necho 'unrelated'\n" - if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - if string(content) != hookContent { - t.Errorf("hook should be unchanged, got:\n%s", content) - } - }) + // Should warn about non-shell hook but still succeed + if !strings.Contains(output, "non-shell interpreter") { + t.Errorf("expected non-shell warning, got:\n%s", output) + } + if !strings.Contains(output, "Ready!") { + t.Errorf("init should still succeed with Ready!, got:\n%s", output) + } } -func TestInitInstallsPostRewriteHookOnUpgrade(t *testing.T) { +func TestInitCmdFailsOnHookWriteError(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("test uses shell script stub, skipping on Windows") } root := initNoDaemonSetup(t) - // Pre-install a current post-commit hook so init takes the - // "already installed" goto path + // Make hooks directory unwritable to trigger a real error hooksDir := filepath.Join(root, ".git", "hooks") - if err := os.MkdirAll(hooksDir, 0755); err != nil { - t.Fatal(err) - } - hookContent := generateHookContent() - if err := os.WriteFile(filepath.Join(hooksDir, "post-commit"), []byte(hookContent), 0755); err != nil { - t.Fatal(err) - } + os.MkdirAll(hooksDir, 0755) + os.Chmod(hooksDir, 0555) + t.Cleanup(func() { os.Chmod(hooksDir, 0755) }) ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) @@ -1334,35 +478,59 @@ func TestInitInstallsPostRewriteHookOnUpgrade(t *testing.T) { serverAddr = ts.URL defer func() { serverAddr = oldAddr }() - captureStdout(t, func() { - cmd := initCmd() - cmd.SetArgs([]string{"--no-daemon"}) - _ = cmd.Execute() - }) + cmd := initCmd() + cmd.SetArgs([]string{"--no-daemon"}) + err := cmd.Execute() - // Verify post-rewrite hook was installed despite post-commit - // taking the "already installed" path - prHookPath := filepath.Join(hooksDir, "post-rewrite") - content, err := os.ReadFile(prHookPath) - if err != nil { - t.Fatalf("post-rewrite hook should be installed even when "+ - "post-commit is already current: %v", err) + // Should fail on permission error, not warn-and-continue + if err == nil { + t.Fatal("expected error for unwritable hooks directory") } - if !strings.Contains(string(content), "remap --quiet") { - t.Error("post-rewrite hook should contain 'remap --quiet'") + if !strings.Contains(err.Error(), "install hooks") { + t.Errorf("error should mention install hooks: %v", err) } } -func TestGeneratePostRewriteHookContent(t *testing.T) { - content := generatePostRewriteHookContent() - - if !strings.HasPrefix(content, "#!/bin/sh\n") { - t.Error("hook should start with #!/bin/sh") +func TestInitCmdFailsOnMixedHookErrors(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test uses shell script stub, skipping on Windows") } - if !strings.Contains(content, postRewriteHookVersionMarker) { - t.Error("hook should contain version marker") + root := initNoDaemonSetup(t) + + // Non-shell post-commit (produces ErrNonShellHook) + + // unwritable post-rewrite (produces real write error). + // InstallAll joins both; init must treat as fatal. + hooksDir := filepath.Join(root, ".git", "hooks") + os.MkdirAll(hooksDir, 0755) + os.WriteFile( + filepath.Join(hooksDir, "post-commit"), + []byte("#!/usr/bin/env python3\nprint('hello')\n"), + 0755, + ) + // Create post-rewrite as a directory so writing it fails. + os.MkdirAll( + filepath.Join(hooksDir, "post-rewrite"), 0755, + ) + + ts := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + }), + ) + defer ts.Close() + + oldAddr := serverAddr + serverAddr = ts.URL + defer func() { serverAddr = oldAddr }() + + cmd := initCmd() + cmd.SetArgs([]string{"--no-daemon"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected error when mixed errors from InstallAll") } - if !strings.Contains(content, "remap --quiet") { - t.Error("hook should call remap --quiet") + if !strings.Contains(err.Error(), "install hooks") { + t.Errorf("error should mention install hooks: %v", err) } } diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 198f0d26..4b6b68eb 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -28,6 +28,7 @@ import ( "github.com/roborev-dev/roborev/internal/config" "github.com/roborev-dev/roborev/internal/daemon" "github.com/roborev-dev/roborev/internal/git" + "github.com/roborev-dev/roborev/internal/githook" "github.com/roborev-dev/roborev/internal/prompt" "github.com/roborev-dev/roborev/internal/skills" "github.com/roborev-dev/roborev/internal/storage" @@ -451,64 +452,21 @@ func initCmd() *cobra.Command { } } - // 4. Install post-commit hook + // 4. Install hooks (post-commit + post-rewrite) hooksDir, err := git.GetHooksPath(root) if err != nil { return fmt.Errorf("get hooks path: %w", err) } - hookPath := filepath.Join(hooksDir, "post-commit") - hookContent := generateHookContent() - - // Ensure hooks directory exists if err := os.MkdirAll(hooksDir, 0755); err != nil { return fmt.Errorf("create hooks directory: %w", err) } - - // Check for existing hook - if existing, err := os.ReadFile(hookPath); err == nil { - existingStr := string(existing) - if !strings.Contains(strings.ToLower(existingStr), "roborev") { - if !isShellHook(existingStr) { - fmt.Printf(" Warning: %s uses a non-shell interpreter, skipping roborev hook\n", hookPath) - goto startDaemon - } - // Append to existing hook - hookContent = existingStr + "\n" + hookContent - } else if strings.Contains(existingStr, hookVersionMarker) { - fmt.Println(" Hook already installed") - goto startDaemon - } else { - // Upgrade: try patching in place first (preserves hook structure) - // v1 → v2: remove trailing & from enqueue line, add version marker - upgraded := existingStr - upgraded = strings.Replace(upgraded, "2>/dev/null &", "2>/dev/null", 1) - upgraded = strings.Replace(upgraded, "post-commit hook -", "post-commit hook v2 -", 1) - if !strings.Contains(upgraded, hookVersionMarker) { - // Comment was edited — just append a marker so we don't re-upgrade - if !strings.HasSuffix(upgraded, "\n") { - upgraded += "\n" - } - upgraded += "# " + hookVersionMarker + "\n" - } - if err := os.WriteFile(hookPath, []byte(upgraded), 0755); err != nil { - return fmt.Errorf("upgrade hook: %w", err) - } - fmt.Println(" Upgraded post-commit hook") - goto startDaemon + if err := githook.InstallAll(hooksDir, false); err != nil { + if githook.HasRealErrors(err) { + return fmt.Errorf("install hooks: %w", err) } + fmt.Printf(" Warning: %v\n", err) } - if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil { - return fmt.Errorf("install hook: %w", err) - } - fmt.Printf(" Installed post-commit hook\n") - - startDaemon: - // 4b. Install post-rewrite hook (for rebase review preservation) - // Runs on all paths (fresh install, upgrade, already-installed) - // so existing users get the new hook on next `roborev init`. - installPostRewriteHook(hooksDir) - // 5. Start daemon (or just register if --no-daemon) var initIncomplete bool if noDaemon { @@ -851,13 +809,6 @@ Examples: return nil // Intentional skip, exit 0 } - // Ensure daemon is running (skip for --local mode) - if !local { - if err := ensureDaemon(); err != nil { - return err // Return error (quiet mode silences output, not exit code) - } - } - // Validate mutually exclusive options if branch != "" && dirty { return fmt.Errorf("cannot use --branch with --dirty") @@ -880,6 +831,21 @@ Examples: return fmt.Errorf("invalid --type %q (valid: security, design)", reviewType) } + // Auto-install/upgrade hooks when running from CLI + // (not when called from a hook via --quiet). + // Runs after validation so invalid args don't + // cause side effects. + if !quiet { + autoInstallHooks(root) + } + + // Ensure daemon is running (skip for --local mode) + if !local { + if err := ensureDaemon(); err != nil { + return err // Return error (quiet mode silences output, not exit code) + } + } + var gitRef string var diffContent string @@ -1577,12 +1543,12 @@ func statusCmd() *cobra.Command { // Check for outdated hooks in current repo if root, err := git.GetRepoRoot("."); err == nil { - if hookNeedsUpgrade(root, "post-commit", hookVersionMarker) { + if githook.NeedsUpgrade(root, "post-commit", githook.PostCommitVersionMarker) { fmt.Println() fmt.Println("Warning: post-commit hook is outdated -- run 'roborev init' to upgrade") } - if hookNeedsUpgrade(root, "post-rewrite", postRewriteHookVersionMarker) || - hookMissing(root, "post-rewrite") { + if githook.NeedsUpgrade(root, "post-rewrite", githook.PostRewriteVersionMarker) || + githook.Missing(root, "post-rewrite") { fmt.Println() fmt.Println("Warning: post-rewrite hook is missing or outdated -- run 'roborev init' to install") } @@ -2355,22 +2321,7 @@ func installHookCmd() *cobra.Command { return fmt.Errorf("create hooks directory: %w", err) } - if err := installOrUpgradeHook( - hooksDir, "post-commit", - hookVersionMarker, generateHookContent, force, - ); err != nil { - return err - } - - if err := installOrUpgradeHook( - hooksDir, "post-rewrite", - postRewriteHookVersionMarker, - generatePostRewriteHookContent, force, - ); err != nil { - return err - } - - return nil + return githook.InstallAll(hooksDir, force) }, } @@ -2379,73 +2330,6 @@ func installHookCmd() *cobra.Command { return cmd } -// installOrUpgradeHook handles the append/upgrade/skip logic for a -// single hook file, following the design doc's per-path behavior: -// - No existing hook: write fresh -// - Existing without roborev: append -// - Existing with current version: skip -// - Existing with old version: upgrade (remove old, append new) -// - --force: overwrite unconditionally -// -// hookReadFile is used to re-read the hook file after cleanup during -// upgrade. Replaceable in tests to simulate read failures. -var hookReadFile = os.ReadFile - -func installOrUpgradeHook( - hooksDir, hookName, versionMarker string, - generate func() string, force bool, -) error { - hookPath := filepath.Join(hooksDir, hookName) - hookContent := generate() - - existing, err := os.ReadFile(hookPath) - if err == nil && !force { - existingStr := string(existing) - if !strings.Contains(strings.ToLower(existingStr), "roborev") { - if !isShellHook(existingStr) { - return fmt.Errorf( - "%s hook uses a non-shell interpreter; "+ - "add the roborev snippet manually or use --force to overwrite", - hookName) - } - // No roborev content — append - hookContent = existingStr + "\n" + hookContent - } else if strings.Contains(existingStr, versionMarker) { - fmt.Printf("%s hook already installed (current)\n", hookName) - return nil - } else { - // Upgrade: remove old snippet, append new one - if !isShellHook(existingStr) { - return fmt.Errorf( - "%s hook uses a non-shell interpreter; "+ - "add the roborev snippet manually "+ - "or use --force to overwrite", - hookName) - } - if rmErr := removeRoborevFromHook(hookPath); rmErr != nil { - return fmt.Errorf("upgrade %s: %w", hookName, rmErr) - } - updated, readErr := hookReadFile(hookPath) - if readErr != nil && !os.IsNotExist(readErr) { - return fmt.Errorf("re-read %s after cleanup: %w", hookName, readErr) - } - if readErr == nil { - remaining := string(updated) - if remaining != "" && !strings.HasSuffix(remaining, "\n") { - remaining += "\n" - } - hookContent = remaining + hookContent - } - } - } - - if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil { - return fmt.Errorf("write %s hook: %w", hookName, err) - } - fmt.Printf("Installed %s hook at %s\n", hookName, hookPath) - return nil -} - func uninstallHookCmd() *cobra.Command { return &cobra.Command{ Use: "uninstall-hook", @@ -2464,7 +2348,7 @@ func uninstallHookCmd() *cobra.Command { for _, hookName := range []string{ "post-commit", "post-rewrite", } { - if err := removeRoborevFromHook( + if err := githook.Uninstall( filepath.Join(hooksDir, hookName), ); err != nil { return err @@ -2476,169 +2360,6 @@ func uninstallHookCmd() *cobra.Command { } } -// removeRoborevFromHook removes the roborev block from a hook file, -// or deletes it entirely if nothing else remains. Uses block-based -// removal: drops all lines from the first roborev comment marker -// through the end of that contiguous block (since roborev appends -// its snippet as a self-contained block at the end). -// isShellHook returns true if the hook content starts with a -// POSIX-compatible shell shebang (sh, bash, zsh, ksh, dash). -// Used to avoid appending shell snippets to non-shell hooks. -func isShellHook(content string) bool { - first, _, _ := strings.Cut(content, "\n") - first = strings.TrimSpace(first) - for _, sh := range []string{"sh", "bash", "zsh", "ksh", "dash"} { - if strings.HasPrefix(first, "#!/bin/"+sh) || - strings.HasPrefix(first, "#!/usr/bin/env "+sh) { - return true - } - } - return false -} - -// isRoborevMarker returns true if the line is a generated roborev hook -// marker comment. Only matches the known generated forms: -// -// # roborev post-commit hook ... -// # roborev post-rewrite hook ... -func isRoborevMarker(line string) bool { - trimmed := strings.TrimSpace(strings.ToLower(line)) - return strings.HasPrefix(trimmed, "# roborev post-commit hook") || - strings.HasPrefix(trimmed, "# roborev post-rewrite hook") -} - -// hasCommandPrefix checks if line starts with prefix and the prefix -// is followed by end-of-string, whitespace, or a shell operator -// (e.g. redirection). This prevents "enqueue --quiet" from matching -// "enqueue --quietly". -func hasCommandPrefix(line, prefix string) bool { - if !strings.HasPrefix(line, prefix) { - return false - } - if len(line) == len(prefix) { - return true - } - next := line[len(prefix)] - return next == ' ' || next == '\t' || next == '>' || - next == '|' || next == '&' || next == ';' -} - -// isRoborevSnippetLine returns true if the line is part of a -// generated roborev hook snippet (current or legacy versions). -func isRoborevSnippetLine(line string) bool { - trimmed := strings.TrimSpace(line) - if trimmed == "" { - return false - } - return strings.HasPrefix(trimmed, "ROBOREV=") || - strings.HasPrefix(trimmed, "ROBOREV=$(") || - hasCommandPrefix(trimmed, "\"$ROBOREV\" enqueue --quiet") || - hasCommandPrefix(trimmed, "\"$ROBOREV\" remap --quiet") || - hasCommandPrefix(trimmed, "roborev enqueue") || - hasCommandPrefix(trimmed, "roborev remap") || - strings.HasPrefix(trimmed, "if [ ! -x \"$ROBOREV\"") || - strings.HasPrefix(trimmed, "if [ -z \"$ROBOREV\"") || - strings.HasPrefix(trimmed, "[ -z \"$ROBOREV\"") || - strings.HasPrefix(trimmed, "[ ! -x \"$ROBOREV\"") -} - -func removeRoborevFromHook(hookPath string) error { - content, err := os.ReadFile(hookPath) - if os.IsNotExist(err) { - return nil - } - if err != nil { - return fmt.Errorf("read %s: %w", filepath.Base(hookPath), err) - } - - hookStr := string(content) - if !strings.Contains(strings.ToLower(hookStr), "roborev") { - return nil - } - - lines := strings.Split(hookStr, "\n") - - // Find the start: anchor on the generated marker comment line - // (e.g. "# roborev post-commit hook v2 - ..."), not just any - // line that mentions roborev. - blockStart := -1 - for i, line := range lines { - if isRoborevMarker(line) { - blockStart = i - break - } - } - if blockStart < 0 { - return nil - } - - // Find the end: scan forward from the marker, consuming only - // lines that are part of the generated snippet. Stop at the - // first line that isn't a snippet line AND isn't "fi" closing - // a snippet's if-block. - blockEnd := blockStart - inIfBlock := false - for i := blockStart + 1; i < len(lines); i++ { - trimmed := strings.TrimSpace(lines[i]) - if trimmed == "" { - // Blank lines between snippet lines are consumed; a - // blank line after the snippet ends the block. - if i+1 < len(lines) && isRoborevSnippetLine(lines[i+1]) { - blockEnd = i - continue - } - break - } - if isRoborevSnippetLine(trimmed) { - blockEnd = i - if strings.HasPrefix(trimmed, "if ") { - inIfBlock = true - } - continue - } - // "fi" only belongs to the block if we saw an "if" inside it - if trimmed == "fi" && inIfBlock { - blockEnd = i - inIfBlock = false - continue - } - break - } - - // Keep everything before and after the block - remaining := make([]string, 0, len(lines)) - remaining = append(remaining, lines[:blockStart]...) - remaining = append(remaining, lines[blockEnd+1:]...) - - // Check if anything meaningful remains - hasContent := false - for _, line := range remaining { - trimmed := strings.TrimSpace(line) - if trimmed != "" && !strings.HasPrefix(trimmed, "#!") { - hasContent = true - break - } - } - - hookName := filepath.Base(hookPath) - if hasContent { - newContent := strings.Join(remaining, "\n") - if !strings.HasSuffix(newContent, "\n") { - newContent += "\n" - } - if err := os.WriteFile(hookPath, []byte(newContent), 0755); err != nil { - return fmt.Errorf("write %s: %w", hookName, err) - } - fmt.Printf("Removed roborev from %s\n", hookName) - } else { - if err := os.Remove(hookPath); err != nil { - return fmt.Errorf("remove %s: %w", hookName, err) - } - fmt.Printf("Removed %s hook\n", hookName) - } - return nil -} - func skillsCmd() *cobra.Command { cmd := &cobra.Command{ Use: "skills", @@ -3404,151 +3125,28 @@ func resolveReasoningWithFast(reasoning string, fast bool, reasoningExplicitlySe return reasoning } -// hookVersionMarker is the string that identifies the current hook version. -// Bump this when the hook template changes to trigger upgrade warnings. -const hookVersionMarker = "post-commit hook v2" - -const postRewriteHookVersionMarker = "post-rewrite hook v1" - -// hookNeedsUpgrade checks whether a repo's named hook contains roborev -// but is outdated (missing the given version marker). -func hookNeedsUpgrade(repoPath, hookName, versionMarker string) bool { - hooksDir, err := git.GetHooksPath(repoPath) - if err != nil { - return false - } - content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) - if err != nil { - return false - } - s := string(content) - return strings.Contains(strings.ToLower(s), "roborev") && - !strings.Contains(s, versionMarker) -} - -// hookMissing checks whether a repo has roborev installed (post-commit -// hook present) but is missing the named hook entirely. -func hookMissing(repoPath, hookName string) bool { +// autoInstallHooks upgrades outdated hooks and installs +// companion hooks (e.g. post-rewrite when post-commit +// exists). It does NOT install hooks from scratch so that +// explicit uninstall-hook is respected. +func autoInstallHooks(repoPath string) { hooksDir, err := git.GetHooksPath(repoPath) if err != nil { - return false - } - // Only warn if roborev is installed (post-commit hook exists) - pcContent, err := os.ReadFile(filepath.Join(hooksDir, "post-commit")) - if err != nil { - return false - } - if !strings.Contains(strings.ToLower(string(pcContent)), "roborev") { - return false - } - // Check if the target hook is missing or has no roborev content - content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) - if err != nil { - return true // hook file doesn't exist - } - return !strings.Contains(strings.ToLower(string(content)), "roborev") -} - -func generateHookContent() string { - // Get path to the currently running binary (not just first in PATH) - roborevPath, err := os.Executable() - if err == nil { - // Resolve symlinks to get the real path - if resolved, err := filepath.EvalSymlinks(roborevPath); err == nil { - roborevPath = resolved - } - } else { - // Fallback to PATH lookup if os.Executable fails (shouldn't happen) - roborevPath, _ = exec.LookPath("roborev") - if roborevPath == "" { - roborevPath = "roborev" - } - } - - // Prefer baked path (security), fall back to PATH only if baked is missing - return fmt.Sprintf(`#!/bin/sh -# roborev post-commit hook v2 - auto-reviews every commit -ROBOREV=%q -if [ ! -x "$ROBOREV" ]; then - ROBOREV=$(command -v roborev 2>/dev/null) - [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && exit 0 -fi -"$ROBOREV" enqueue --quiet 2>/dev/null -`, roborevPath) -} - -func installPostRewriteHook(hooksDir string) { - hookPath := filepath.Join(hooksDir, "post-rewrite") - hookContent := generatePostRewriteHookContent() - - if existing, err := os.ReadFile(hookPath); err == nil { - existingStr := string(existing) - if !strings.Contains(strings.ToLower(existingStr), "roborev") { - if !isShellHook(existingStr) { - fmt.Printf(" Warning: %s uses a non-shell interpreter, skipping\n", hookPath) - return - } - // No roborev content — append to existing hook - hookContent = existingStr + "\n" + hookContent - } else if strings.Contains(existingStr, postRewriteHookVersionMarker) { - fmt.Println(" Post-rewrite hook already installed") - return - } else { - // Upgrade: remove old roborev snippet, append new one. - // This preserves user content around the snippet. - if !isShellHook(existingStr) { - fmt.Printf(" Warning: %s uses a non-shell interpreter, skipping\n", hookPath) - return - } - if rmErr := removeRoborevFromHook(hookPath); rmErr != nil { - fmt.Printf(" Warning: %v\n", rmErr) - return - } - updated, readErr := os.ReadFile(hookPath) - if readErr != nil && !os.IsNotExist(readErr) { - fmt.Printf(" Warning: re-read %s after cleanup: %v\n", - hookPath, readErr) - return - } - if readErr == nil { - remaining := string(updated) - if remaining != "" && !strings.HasSuffix(remaining, "\n") { - remaining += "\n" - } - hookContent = remaining + hookContent - } - // If the file was deleted (snippet-only), hookContent - // is already the fresh generated content. - } - } - - if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil { - fmt.Printf(" Warning: could not install post-rewrite hook: %v\n", err) return } - fmt.Printf(" Installed post-rewrite hook\n") -} - -func generatePostRewriteHookContent() string { - roborevPath, err := os.Executable() - if err == nil { - if resolved, err := filepath.EvalSymlinks(roborevPath); err == nil { - roborevPath = resolved - } - } else { - roborevPath, _ = exec.LookPath("roborev") - if roborevPath == "" { - roborevPath = "roborev" + for _, name := range []string{"post-commit", "post-rewrite"} { + marker := githook.VersionMarker(name) + if githook.NeedsUpgrade(repoPath, name, marker) || + githook.Missing(repoPath, name) { + if err := githook.Install(hooksDir, name, false); err != nil { + // Non-shell hooks are a persistent condition; + // don't warn on every invocation. + if !errors.Is(err, githook.ErrNonShellHook) { + fmt.Fprintf(os.Stderr, + "Warning: auto-install %s hook: %v\n", + name, err) + } + } } } - - return fmt.Sprintf(`#!/bin/sh -# roborev post-rewrite hook v1 - remaps reviews after rebase/amend -ROBOREV=%q -if [ ! -x "$ROBOREV" ]; then - ROBOREV=$(command -v roborev 2>/dev/null) - [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && exit 0 -fi -"$ROBOREV" remap --quiet 2>/dev/null -`, roborevPath) } diff --git a/cmd/roborev/review_test.go b/cmd/roborev/review_test.go index e7120f70..ccc4b84c 100644 --- a/cmd/roborev/review_test.go +++ b/cmd/roborev/review_test.go @@ -6,6 +6,8 @@ import ( "bytes" "encoding/json" "net/http" + "os" + "path/filepath" "strings" "testing" "time" @@ -687,3 +689,36 @@ func TestReviewFastFlag(t *testing.T) { } }) } + +func TestReviewInvalidArgsNoSideEffects(t *testing.T) { + _, cleanup := setupMockDaemon(t, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("daemon should not be contacted on invalid args") + w.WriteHeader(http.StatusOK) + }), + ) + defer cleanup() + + repo := newTestGitRepo(t) + hooksDir := filepath.Join(repo.Dir, ".git", "hooks") + + cmd := reviewCmd() + cmd.SetArgs([]string{ + "--repo", repo.Dir, "--branch", "--dirty", + }) + err := cmd.Execute() + if err == nil { + t.Fatal("expected error for --branch with --dirty") + } + + // Hooks directory should have no roborev-generated files. + for _, name := range []string{"post-commit", "post-rewrite"} { + if _, statErr := os.Stat( + filepath.Join(hooksDir, name), + ); statErr == nil { + t.Errorf( + "%s should not exist after invalid args", name, + ) + } + } +} diff --git a/internal/daemon/server.go b/internal/daemon/server.go index bfc3c271..bd3c91f4 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -8,8 +8,6 @@ import ( "fmt" "log" "net/http" - "os" - "path/filepath" "strings" "sync" "time" @@ -17,6 +15,7 @@ import ( "github.com/roborev-dev/roborev/internal/agent" "github.com/roborev-dev/roborev/internal/config" "github.com/roborev-dev/roborev/internal/git" + "github.com/roborev-dev/roborev/internal/githook" "github.com/roborev-dev/roborev/internal/storage" "github.com/roborev-dev/roborev/internal/version" ) @@ -143,11 +142,11 @@ func (s *Server) Start(ctx context.Context) error { if s.ciPoller == nil { if repos, err := s.db.ListRepos(); err == nil { for _, repo := range repos { - if hookNeedsUpgrade(repo.RootPath, "post-commit", hookVersionMarker) { + if githook.NeedsUpgrade(repo.RootPath, "post-commit", githook.PostCommitVersionMarker) { log.Printf("Warning: outdated post-commit hook in %s -- run 'roborev init' to upgrade", repo.RootPath) } - if hookNeedsUpgrade(repo.RootPath, "post-rewrite", postRewriteHookVersionMarker) || - hookMissing(repo.RootPath, "post-rewrite") { + if githook.NeedsUpgrade(repo.RootPath, "post-rewrite", githook.PostRewriteVersionMarker) || + githook.Missing(repo.RootPath, "post-rewrite") { log.Printf("Warning: missing or outdated post-rewrite hook in %s -- run 'roborev init' to install", repo.RootPath) } } @@ -164,47 +163,6 @@ func (s *Server) Start(ctx context.Context) error { return nil } -// hookVersionMarker identifies the current hook version. -const hookVersionMarker = "post-commit hook v2" -const postRewriteHookVersionMarker = "post-rewrite hook v1" - -// hookNeedsUpgrade checks whether a repo's named hook contains roborev -// but is outdated (missing the given version marker). -func hookNeedsUpgrade(repoPath, hookName, versionMarker string) bool { - hooksDir, err := git.GetHooksPath(repoPath) - if err != nil { - return false - } - content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) - if err != nil { - return false - } - s := string(content) - return strings.Contains(strings.ToLower(s), "roborev") && - !strings.Contains(s, versionMarker) -} - -// hookMissing checks whether a repo has roborev installed (post-commit -// hook present) but is missing the named hook entirely. -func hookMissing(repoPath, hookName string) bool { - hooksDir, err := git.GetHooksPath(repoPath) - if err != nil { - return false - } - pcContent, err := os.ReadFile(filepath.Join(hooksDir, "post-commit")) - if err != nil { - return false - } - if !strings.Contains(strings.ToLower(string(pcContent)), "roborev") { - return false - } - content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) - if err != nil { - return true - } - return !strings.Contains(strings.ToLower(string(content)), "roborev") -} - // Stop gracefully shuts down the server func (s *Server) Stop() error { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) diff --git a/internal/githook/githook.go b/internal/githook/githook.go new file mode 100644 index 00000000..bf4a6ab6 --- /dev/null +++ b/internal/githook/githook.go @@ -0,0 +1,506 @@ +// Package githook manages roborev's git hook installation, +// upgrade, and removal. It supports both standalone hooks +// (fresh installs) and embedded snippets that coexist with +// existing hook scripts. +package githook + +import ( + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + + "github.com/roborev-dev/roborev/internal/git" +) + +// ErrNonShellHook is returned when a hook uses a non-shell +// interpreter and cannot be safely modified. +var ErrNonShellHook = errors.New("non-shell interpreter") + +// HasRealErrors returns true if err contains any error that +// is not ErrNonShellHook. Use this instead of errors.Is when +// checking joined errors that may contain both non-shell +// warnings and real failures. +func HasRealErrors(err error) bool { + if err == nil { + return false + } + type unwrapper interface{ Unwrap() []error } + if joined, ok := err.(unwrapper); ok { + for _, e := range joined.Unwrap() { + if !errors.Is(e, ErrNonShellHook) { + return true + } + } + return false + } + return !errors.Is(err, ErrNonShellHook) +} + +// Version markers identify the current hook template version. +// Bump these when the hook template changes to trigger +// upgrade warnings and auto-upgrades. +const PostCommitVersionMarker = "post-commit hook v3" +const PostRewriteVersionMarker = "post-rewrite hook v2" + +// VersionMarker returns the current version marker for a hook. +func VersionMarker(hookName string) string { + switch hookName { + case "post-commit": + return PostCommitVersionMarker + case "post-rewrite": + return PostRewriteVersionMarker + default: + return "" + } +} + +// ReadFile is the function used to re-read a hook file after +// cleanup during upgrade. Replaceable in tests to simulate +// read failures. +var ReadFile = os.ReadFile + +// NeedsUpgrade checks whether a repo's named hook contains +// roborev but is outdated (missing the given version marker). +func NeedsUpgrade(repoPath, hookName, versionMarker string) bool { + hooksDir, err := git.GetHooksPath(repoPath) + if err != nil { + return false + } + content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) + if err != nil { + return false + } + s := string(content) + return strings.Contains(strings.ToLower(s), "roborev") && + !strings.Contains(s, versionMarker) +} + +// NotInstalled checks whether the named hook file is absent +// or does not contain any roborev content. +func NotInstalled(repoPath, hookName string) bool { + hooksDir, err := git.GetHooksPath(repoPath) + if err != nil { + return false + } + content, err := os.ReadFile( + filepath.Join(hooksDir, hookName), + ) + if err != nil { + return os.IsNotExist(err) + } + return !strings.Contains( + strings.ToLower(string(content)), "roborev", + ) +} + +// Missing checks whether a repo has roborev installed +// (post-commit hook present) but is missing the named hook. +func Missing(repoPath, hookName string) bool { + hooksDir, err := git.GetHooksPath(repoPath) + if err != nil { + return false + } + pcContent, err := os.ReadFile( + filepath.Join(hooksDir, "post-commit"), + ) + if err != nil { + return false + } + if !strings.Contains( + strings.ToLower(string(pcContent)), "roborev", + ) { + return false + } + content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) + if err != nil { + return os.IsNotExist(err) + } + return !strings.Contains( + strings.ToLower(string(content)), "roborev", + ) +} + +// resolveRoborevPath returns the absolute path to the running +// roborev binary, falling back to a PATH lookup. +func resolveRoborevPath() string { + roborevPath, err := os.Executable() + if err == nil { + if resolved, err := filepath.EvalSymlinks(roborevPath); err == nil { + roborevPath = resolved + } + return roborevPath + } + roborevPath, _ = exec.LookPath("roborev") + if roborevPath == "" { + roborevPath = "roborev" + } + return roborevPath +} + +// GeneratePostCommit returns a standalone post-commit hook +// (with shebang, suitable for fresh installs). +func GeneratePostCommit() string { + return fmt.Sprintf(`#!/bin/sh +# roborev %s - auto-reviews every commit +ROBOREV=%q +if [ ! -x "$ROBOREV" ]; then + ROBOREV=$(command -v roborev 2>/dev/null) + [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && exit 0 +fi +"$ROBOREV" enqueue --quiet 2>/dev/null +`, PostCommitVersionMarker, resolveRoborevPath()) +} + +// GeneratePostRewrite returns a standalone post-rewrite hook +// (with shebang, suitable for fresh installs). +func GeneratePostRewrite() string { + return fmt.Sprintf(`#!/bin/sh +# roborev %s - remaps reviews after rebase/amend +ROBOREV=%q +if [ ! -x "$ROBOREV" ]; then + ROBOREV=$(command -v roborev 2>/dev/null) + [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && exit 0 +fi +"$ROBOREV" remap --quiet 2>/dev/null +`, PostRewriteVersionMarker, resolveRoborevPath()) +} + +// generateEmbeddablePostCommit returns a function-wrapped +// snippet without shebang, for embedding in existing hooks. +// Uses return instead of exit so it doesn't terminate the +// parent script. +func generateEmbeddablePostCommit() string { + return fmt.Sprintf(`# roborev %s - auto-reviews every commit +_roborev_hook() { +ROBOREV=%q +if [ ! -x "$ROBOREV" ]; then + ROBOREV=$(command -v roborev 2>/dev/null) + [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && return 0 +fi +"$ROBOREV" enqueue --quiet 2>/dev/null +} +_roborev_hook +`, PostCommitVersionMarker, resolveRoborevPath()) +} + +// generateEmbeddablePostRewrite returns a function-wrapped +// snippet without shebang, for embedding in existing hooks. +func generateEmbeddablePostRewrite() string { + return fmt.Sprintf(`# roborev %s - remaps reviews after rebase/amend +_roborev_remap() { +ROBOREV=%q +if [ ! -x "$ROBOREV" ]; then + ROBOREV=$(command -v roborev 2>/dev/null) + [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && return 0 +fi +"$ROBOREV" remap --quiet 2>/dev/null +} +_roborev_remap +`, PostRewriteVersionMarker, resolveRoborevPath()) +} + +// generateContent returns the standalone hook content for the +// given hook name. +func generateContent(hookName string) string { + switch hookName { + case "post-commit": + return GeneratePostCommit() + case "post-rewrite": + return GeneratePostRewrite() + default: + return "" + } +} + +// generateEmbeddable returns the embeddable snippet for the +// given hook name. +func generateEmbeddable(hookName string) string { + switch hookName { + case "post-commit": + return generateEmbeddablePostCommit() + case "post-rewrite": + return generateEmbeddablePostRewrite() + default: + return "" + } +} + +// embedSnippet inserts snippet after the shebang line of +// existing, so it runs before any possible exit in the +// original script. If there is no shebang, the snippet is +// prepended. +func embedSnippet(existing, snippet string) string { + lines := strings.SplitAfter(existing, "\n") + if len(lines) > 0 && + strings.HasPrefix(strings.TrimSpace(lines[0]), "#!") { + shebang := lines[0] + if !strings.HasSuffix(shebang, "\n") { + shebang += "\n" + } + return shebang + snippet + strings.Join(lines[1:], "") + } + return snippet + existing +} + +// Install installs or upgrades a single hook. It handles: +// - No existing hook: write standalone content +// - Existing without roborev: embed snippet after shebang +// - Existing with current version: skip (no-op) +// - Existing with old version: remove old, embed new +// - force=true: overwrite unconditionally +func Install(hooksDir, hookName string, force bool) error { + hookPath := filepath.Join(hooksDir, hookName) + versionMarker := VersionMarker(hookName) + hookContent := generateContent(hookName) + + existing, err := os.ReadFile(hookPath) + if err == nil && !force { + existingStr := string(existing) + if !strings.Contains( + strings.ToLower(existingStr), "roborev", + ) { + if !isShellHook(existingStr) { + return fmt.Errorf( + "%s hook: %w; add the roborev snippet "+ + "manually or use --force to overwrite", + hookName, ErrNonShellHook) + } + hookContent = embedSnippet( + existingStr, + generateEmbeddable(hookName), + ) + } else if strings.Contains(existingStr, versionMarker) { + fmt.Printf( + "%s hook already installed (current)\n", + hookName, + ) + return nil + } else { + // Upgrade: remove old snippet, embed new one + if !isShellHook(existingStr) { + return fmt.Errorf( + "%s hook: %w; add the roborev snippet "+ + "manually or use --force to overwrite", + hookName, ErrNonShellHook) + } + if rmErr := Uninstall(hookPath); rmErr != nil { + return fmt.Errorf( + "upgrade %s: %w", hookName, rmErr, + ) + } + updated, readErr := ReadFile(hookPath) + if readErr != nil && !os.IsNotExist(readErr) { + return fmt.Errorf( + "re-read %s after cleanup: %w", + hookName, readErr, + ) + } + if readErr == nil { + remaining := string(updated) + hookContent = embedSnippet( + remaining, + generateEmbeddable(hookName), + ) + } + // If file was deleted (snippet-only), hookContent + // is the fresh standalone content. + } + } + + if err := os.WriteFile( + hookPath, []byte(hookContent), 0755, + ); err != nil { + return fmt.Errorf("write %s hook: %w", hookName, err) + } + fmt.Printf("Installed %s hook at %s\n", hookName, hookPath) + return nil +} + +// InstallAll installs both post-commit and post-rewrite hooks. +// It attempts all hooks and returns a joined error if any fail. +func InstallAll(hooksDir string, force bool) error { + var errs []error + for _, name := range []string{"post-commit", "post-rewrite"} { + if err := Install(hooksDir, name, force); err != nil { + errs = append(errs, err) + } + } + return errors.Join(errs...) +} + +// Uninstall removes the roborev block from a hook file, or +// deletes it entirely if nothing else remains. +func Uninstall(hookPath string) error { + content, err := os.ReadFile(hookPath) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return fmt.Errorf( + "read %s: %w", filepath.Base(hookPath), err, + ) + } + + hookStr := string(content) + if !strings.Contains(strings.ToLower(hookStr), "roborev") { + return nil + } + + lines := strings.Split(hookStr, "\n") + + blockStart := -1 + for i, line := range lines { + if isRoborevMarker(line) { + blockStart = i + break + } + } + if blockStart < 0 { + return nil + } + + blockEnd := blockStart + inIfBlock := false + inFuncBlock := false + for i := blockStart + 1; i < len(lines); i++ { + trimmed := strings.TrimSpace(lines[i]) + if trimmed == "" { + if i+1 < len(lines) && + isRoborevSnippetLine(lines[i+1]) { + blockEnd = i + continue + } + break + } + if isRoborevSnippetLine(trimmed) { + blockEnd = i + if strings.HasPrefix(trimmed, "if ") { + inIfBlock = true + } + if strings.HasSuffix(trimmed, "() {") { + inFuncBlock = true + } + continue + } + if trimmed == "fi" && inIfBlock { + blockEnd = i + inIfBlock = false + continue + } + if trimmed == "}" && inFuncBlock { + blockEnd = i + inFuncBlock = false + continue + } + break + } + + remaining := make([]string, 0, len(lines)) + remaining = append(remaining, lines[:blockStart]...) + remaining = append(remaining, lines[blockEnd+1:]...) + + hasContent := false + for _, line := range remaining { + trimmed := strings.TrimSpace(line) + if trimmed != "" && !strings.HasPrefix(trimmed, "#!") { + hasContent = true + break + } + } + + hookName := filepath.Base(hookPath) + if hasContent { + newContent := strings.Join(remaining, "\n") + if !strings.HasSuffix(newContent, "\n") { + newContent += "\n" + } + if err := os.WriteFile( + hookPath, []byte(newContent), 0755, + ); err != nil { + return fmt.Errorf("write %s: %w", hookName, err) + } + fmt.Printf("Removed roborev from %s\n", hookName) + } else { + if err := os.Remove(hookPath); err != nil { + return fmt.Errorf("remove %s: %w", hookName, err) + } + fmt.Printf("Removed %s hook\n", hookName) + } + return nil +} + +// isShellHook returns true if the hook content starts with a +// POSIX-compatible shell shebang. +func isShellHook(content string) bool { + first, _, _ := strings.Cut(content, "\n") + first = strings.TrimSpace(first) + for _, sh := range []string{ + "sh", "bash", "zsh", "ksh", "dash", + } { + if strings.HasPrefix(first, "#!/bin/"+sh) || + strings.HasPrefix(first, "#!/usr/bin/env "+sh) { + return true + } + } + return false +} + +// isRoborevMarker returns true if the line is a generated +// roborev hook marker comment. +func isRoborevMarker(line string) bool { + trimmed := strings.TrimSpace(strings.ToLower(line)) + return strings.HasPrefix( + trimmed, "# roborev post-commit hook", + ) || strings.HasPrefix( + trimmed, "# roborev post-rewrite hook", + ) +} + +// hasCommandPrefix checks if line starts with prefix and the +// prefix is followed by end-of-string, whitespace, or a shell +// operator. Prevents "enqueue --quiet" from matching +// "enqueue --quietly". +func hasCommandPrefix(line, prefix string) bool { + if !strings.HasPrefix(line, prefix) { + return false + } + if len(line) == len(prefix) { + return true + } + next := line[len(prefix)] + return next == ' ' || next == '\t' || next == '>' || + next == '|' || next == '&' || next == ';' +} + +// isRoborevSnippetLine returns true if the line is part of a +// generated roborev hook snippet (current or legacy versions). +func isRoborevSnippetLine(line string) bool { + trimmed := strings.TrimSpace(line) + if trimmed == "" { + return false + } + return strings.HasPrefix(trimmed, "ROBOREV=") || + strings.HasPrefix(trimmed, "ROBOREV=$(") || + hasCommandPrefix( + trimmed, "\"$ROBOREV\" enqueue --quiet", + ) || + hasCommandPrefix( + trimmed, "\"$ROBOREV\" remap --quiet", + ) || + hasCommandPrefix(trimmed, "roborev enqueue") || + hasCommandPrefix(trimmed, "roborev remap") || + strings.HasPrefix( + trimmed, "if [ ! -x \"$ROBOREV\"", + ) || + strings.HasPrefix( + trimmed, "if [ -z \"$ROBOREV\"", + ) || + strings.HasPrefix(trimmed, "[ -z \"$ROBOREV\"") || + strings.HasPrefix(trimmed, "[ ! -x \"$ROBOREV\"") || + trimmed == "return 0" || + strings.HasPrefix(trimmed, "_roborev_hook") || + strings.HasPrefix(trimmed, "_roborev_remap") +} diff --git a/internal/githook/githook_test.go b/internal/githook/githook_test.go new file mode 100644 index 00000000..de955cff --- /dev/null +++ b/internal/githook/githook_test.go @@ -0,0 +1,1390 @@ +package githook + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/roborev-dev/roborev/internal/testutil" +) + +func TestGeneratePostCommit(t *testing.T) { + content := GeneratePostCommit() + lines := strings.Split(content, "\n") + + t.Run("has shebang", func(t *testing.T) { + if !strings.HasPrefix(content, "#!/bin/sh\n") { + t.Error("hook should start with #!/bin/sh") + } + }) + + t.Run("has roborev comment", func(t *testing.T) { + if !strings.Contains(content, "# roborev") { + t.Error("hook should contain roborev comment") + } + }) + + t.Run("baked path comes first", func(t *testing.T) { + bakedIdx := -1 + pathIdx := -1 + for i, line := range lines { + if strings.HasPrefix(line, "ROBOREV=") && + !strings.Contains(line, "command -v") { + bakedIdx = i + } + if strings.Contains(line, "command -v roborev") { + pathIdx = i + } + } + if bakedIdx == -1 { + t.Error("hook should have baked ROBOREV= assignment") + } + if pathIdx == -1 { + t.Error("hook should have PATH fallback") + } + if bakedIdx > pathIdx { + t.Error("baked path should come before PATH lookup") + } + }) + + t.Run("enqueue line without background", func(t *testing.T) { + found := false + for _, line := range lines { + if strings.Contains(line, "enqueue --quiet") && + strings.Contains(line, "2>/dev/null") && + !strings.HasSuffix( + strings.TrimSpace(line), "&", + ) { + found = true + break + } + } + if !found { + t.Error("hook should have enqueue with --quiet " + + "and 2>/dev/null but no trailing &") + } + }) + + t.Run("has version marker", func(t *testing.T) { + if !strings.Contains(content, PostCommitVersionMarker) { + t.Errorf( + "hook should contain %q", + PostCommitVersionMarker, + ) + } + }) + + t.Run("baked path is quoted", func(t *testing.T) { + for _, line := range lines { + if strings.HasPrefix(line, "ROBOREV=") && + !strings.Contains(line, "command -v") { + if !strings.Contains(line, `ROBOREV="`) { + t.Errorf( + "baked path should be quoted: %s", + line, + ) + } + break + } + } + }) + + t.Run("baked path is absolute", func(t *testing.T) { + for _, line := range lines { + if strings.HasPrefix(line, "ROBOREV=") && + !strings.Contains(line, "command -v") { + start := strings.Index(line, `"`) + end := strings.LastIndex(line, `"`) + if start != -1 && end > start { + path := line[start+1 : end] + if !filepath.IsAbs(path) { + t.Errorf( + "baked path should be absolute: %s", + path, + ) + } + } + break + } + } + }) +} + +func TestGeneratePostRewrite(t *testing.T) { + content := GeneratePostRewrite() + + if !strings.HasPrefix(content, "#!/bin/sh\n") { + t.Error("hook should start with #!/bin/sh") + } + if !strings.Contains(content, PostRewriteVersionMarker) { + t.Error("hook should contain version marker") + } + if !strings.Contains(content, "remap --quiet") { + t.Error("hook should call remap --quiet") + } +} + +func TestGenerateEmbeddablePostCommit(t *testing.T) { + content := generateEmbeddablePostCommit() + + if strings.HasPrefix(content, "#!") { + t.Error("embeddable should not have shebang") + } + if !strings.Contains(content, "_roborev_hook() {") { + t.Error("embeddable should use function wrapper") + } + if !strings.Contains(content, "return 0") { + t.Error("embeddable should use return, not exit") + } + if strings.Contains(content, "exit 0") { + t.Error("embeddable must not use exit 0") + } + if !strings.Contains(content, PostCommitVersionMarker) { + t.Error("embeddable should contain version marker") + } + // Ends with function call + lines := strings.Split( + strings.TrimRight(content, "\n"), "\n", + ) + last := strings.TrimSpace(lines[len(lines)-1]) + if last != "_roborev_hook" { + t.Errorf( + "embeddable should end with function call, got: %s", + last, + ) + } +} + +func TestGenerateEmbeddablePostRewrite(t *testing.T) { + content := generateEmbeddablePostRewrite() + + if strings.HasPrefix(content, "#!") { + t.Error("embeddable should not have shebang") + } + if !strings.Contains(content, "_roborev_remap() {") { + t.Error("embeddable should use function wrapper") + } + if !strings.Contains(content, "return 0") { + t.Error("embeddable should use return, not exit") + } + if strings.Contains(content, "exit 0") { + t.Error("embeddable must not use exit 0") + } + if !strings.Contains(content, PostRewriteVersionMarker) { + t.Error("embeddable should contain version marker") + } +} + +func TestEmbedSnippet(t *testing.T) { + t.Run("inserts after shebang", func(t *testing.T) { + existing := "#!/bin/sh\necho 'user code'\nexit 0\n" + snippet := "# roborev snippet\n_roborev_hook\n" + result := embedSnippet(existing, snippet) + if !strings.HasPrefix(result, "#!/bin/sh\n") { + t.Error("should preserve shebang") + } + shebangEnd := strings.Index(result, "\n") + 1 + afterShebang := result[shebangEnd:] + if !strings.HasPrefix(afterShebang, "# roborev snippet") { + t.Errorf( + "snippet should come right after shebang, got:\n%s", + result, + ) + } + if !strings.Contains(result, "echo 'user code'") { + t.Error("user code should be preserved") + } + }) + + t.Run("snippet before exit 0", func(t *testing.T) { + existing := "#!/bin/sh\nexit 0\n" + snippet := "SNIPPET\n" + result := embedSnippet(existing, snippet) + snippetIdx := strings.Index(result, "SNIPPET") + exitIdx := strings.Index(result, "exit 0") + if snippetIdx > exitIdx { + t.Error("snippet should appear before exit 0") + } + }) + + t.Run("no shebang prepends", func(t *testing.T) { + existing := "echo 'no shebang'\n" + snippet := "SNIPPET\n" + result := embedSnippet(existing, snippet) + if !strings.HasPrefix(result, "SNIPPET\n") { + t.Errorf( + "snippet should be prepended, got:\n%s", + result, + ) + } + }) + + t.Run("shebang without trailing newline", func(t *testing.T) { + existing := "#!/bin/sh" + snippet := "SNIPPET\n" + result := embedSnippet(existing, snippet) + if !strings.HasPrefix(result, "#!/bin/sh\n") { + t.Errorf( + "shebang should get trailing newline, got:\n%q", + result, + ) + } + if !strings.Contains(result, "SNIPPET") { + t.Error("snippet should be present") + } + }) +} + +func TestNeedsUpgrade(t *testing.T) { + t.Run("outdated hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook( + "#!/bin/sh\n# roborev post-commit hook\n" + + "roborev enqueue\n", + ) + if !NeedsUpgrade( + repo.Root, "post-commit", PostCommitVersionMarker, + ) { + t.Error("should detect outdated hook") + } + }) + + t.Run("current hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook( + "#!/bin/sh\n# roborev " + + PostCommitVersionMarker + + "\nroborev enqueue\n", + ) + if NeedsUpgrade( + repo.Root, "post-commit", PostCommitVersionMarker, + ) { + t.Error("should not flag current hook") + } + }) + + t.Run("no hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if NeedsUpgrade( + repo.Root, "post-commit", PostCommitVersionMarker, + ) { + t.Error("should not flag missing hook") + } + }) + + t.Run("non-roborev hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook("#!/bin/sh\necho hello\n") + if NeedsUpgrade( + repo.Root, "post-commit", PostCommitVersionMarker, + ) { + t.Error("should not flag non-roborev hook") + } + }) + + t.Run("post-rewrite outdated", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + hooksDir := filepath.Join(repo.Root, ".git", "hooks") + os.MkdirAll(hooksDir, 0755) + os.WriteFile( + filepath.Join(hooksDir, "post-rewrite"), + []byte("#!/bin/sh\n# roborev hook\n"+ + "roborev remap\n"), + 0755, + ) + if !NeedsUpgrade( + repo.Root, "post-rewrite", + PostRewriteVersionMarker, + ) { + t.Error("should detect outdated post-rewrite hook") + } + }) + + t.Run("post-rewrite current", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + hooksDir := filepath.Join(repo.Root, ".git", "hooks") + os.MkdirAll(hooksDir, 0755) + os.WriteFile( + filepath.Join(hooksDir, "post-rewrite"), + []byte("#!/bin/sh\n# roborev "+ + PostRewriteVersionMarker+ + "\nroborev remap\n"), + 0755, + ) + if NeedsUpgrade( + repo.Root, "post-rewrite", + PostRewriteVersionMarker, + ) { + t.Error("should not flag current post-rewrite hook") + } + }) +} + +func TestNotInstalled(t *testing.T) { + t.Run("hook file absent", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if !NotInstalled(repo.Root, "post-commit") { + t.Error("absent hook should be not installed") + } + }) + + t.Run("hook without roborev", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook("#!/bin/sh\necho hello\n") + if !NotInstalled(repo.Root, "post-commit") { + t.Error("non-roborev hook should be not installed") + } + }) + + t.Run("hook with roborev", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook(GeneratePostCommit()) + if NotInstalled(repo.Root, "post-commit") { + t.Error("roborev hook should be installed") + } + }) + + t.Run("non-ENOENT read error returns false", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + // Create a directory where the hook file would be. + // Reading a directory is a non-ENOENT I/O error. + hookPath := filepath.Join( + repo.Root, ".git", "hooks", "post-commit", + ) + os.MkdirAll(hookPath, 0755) + if NotInstalled(repo.Root, "post-commit") { + t.Error( + "non-ENOENT error should not report " + + "as not installed", + ) + } + }, + ) +} + +func TestMissing(t *testing.T) { + t.Run("missing post-rewrite with roborev post-commit", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook( + "#!/bin/sh\n# roborev " + + PostCommitVersionMarker + "\n" + + "roborev enqueue\n", + ) + if !Missing(repo.Root, "post-rewrite") { + t.Error("should detect missing post-rewrite") + } + }, + ) + + t.Run("no post-commit hook at all", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if Missing(repo.Root, "post-rewrite") { + t.Error("should not warn without post-commit") + } + }) + + t.Run("post-rewrite exists with roborev", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook( + "#!/bin/sh\n# roborev " + + PostCommitVersionMarker + "\n" + + "roborev enqueue\n", + ) + hooksDir := filepath.Join(repo.Root, ".git", "hooks") + os.WriteFile( + filepath.Join(hooksDir, "post-rewrite"), + []byte("#!/bin/sh\n# roborev "+ + PostRewriteVersionMarker+ + "\nroborev remap\n"), + 0755, + ) + if Missing(repo.Root, "post-rewrite") { + t.Error("should not warn when present") + } + }) + + t.Run("non-roborev post-commit", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook("#!/bin/sh\necho hello\n") + if Missing(repo.Root, "post-rewrite") { + t.Error("should not warn for non-roborev") + } + }) + + t.Run("non-ENOENT read error returns false", + func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("permission test unreliable on Windows") + } + repo := testutil.NewTestRepo(t) + repo.WriteHook( + "#!/bin/sh\n# roborev " + + PostCommitVersionMarker + "\n" + + "roborev enqueue\n", + ) + // Create post-rewrite as a directory so ReadFile + // fails with a non-ENOENT error. + prPath := filepath.Join( + repo.Root, ".git", "hooks", "post-rewrite", + ) + os.MkdirAll(prPath, 0755) + if Missing(repo.Root, "post-rewrite") { + t.Error( + "non-ENOENT error should return false", + ) + } + }, + ) +} + +func TestInstall(t *testing.T) { + t.Run("fresh install creates standalone hook", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile( + filepath.Join(repo.HooksDir, "post-commit"), + ) + contentStr := string(content) + if !strings.HasPrefix(contentStr, "#!/bin/sh\n") { + t.Error("fresh install should have shebang") + } + if !strings.Contains( + contentStr, PostCommitVersionMarker, + ) { + t.Error("should have version marker") + } + }, + ) + + t.Run("embeds after shebang in existing hook", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + existing := "#!/bin/sh\necho 'custom'\n" + os.WriteFile(hookPath, []byte(existing), 0755) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if !strings.Contains( + contentStr, "echo 'custom'", + ) { + t.Error("original content should be preserved") + } + if !strings.Contains( + contentStr, PostCommitVersionMarker, + ) { + t.Error("should have roborev snippet") + } + }, + ) + + t.Run("function wrapper uses return not exit", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + existing := "#!/bin/sh\necho 'custom'\n" + os.WriteFile(hookPath, []byte(existing), 0755) + + Install(repo.HooksDir, "post-commit", false) + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if strings.Contains(contentStr, "exit 0") { + t.Error( + "embedded snippet must use return, not exit", + ) + } + if !strings.Contains(contentStr, "return 0") { + t.Error("embedded snippet should use return 0") + } + if !strings.Contains( + contentStr, "_roborev_hook() {", + ) { + t.Error("should use function wrapper") + } + }, + ) + + t.Run("early exit does not block snippet", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + // Husky-style hook with exit 0 at the end + husky := "#!/bin/sh\n" + + ". \"$(dirname \"$0\")/_/husky.sh\"\n" + + "npx lint-staged\n" + + "exit 0\n" + os.WriteFile(hookPath, []byte(husky), 0755) + + Install(repo.HooksDir, "post-commit", false) + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + snippetIdx := strings.Index( + contentStr, "_roborev_hook", + ) + exitIdx := strings.Index(contentStr, "exit 0") + if snippetIdx < 0 { + t.Fatal("snippet should be present") + } + if exitIdx < 0 { + t.Fatal("exit 0 should be preserved") + } + if snippetIdx > exitIdx { + t.Error( + "roborev snippet should appear before exit 0", + ) + } + }) + + t.Run("skips current version", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + current := GeneratePostRewrite() + os.WriteFile(hookPath, []byte(current), 0755) + + err := Install( + repo.HooksDir, "post-rewrite", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + if string(content) != current { + t.Error("current hook should not be modified") + } + }) + + t.Run("upgrades outdated hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + outdated := "#!/bin/sh\n" + + "# roborev post-commit hook\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" + os.WriteFile(hookPath, []byte(outdated), 0755) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if !strings.Contains( + contentStr, PostCommitVersionMarker, + ) { + t.Error("should have new version marker") + } + if strings.Contains( + contentStr, "# roborev post-commit hook\n", + ) { + t.Error("old marker should be removed") + } + }) + + t.Run("upgrade from v2 to v3", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v2Hook := "#!/bin/sh\n" + + "# roborev post-commit hook v2 - " + + "auto-reviews every commit\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "if [ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=$(command -v roborev " + + "2>/dev/null)\n" + + " [ -z \"$ROBOREV\" ] || " + + "[ ! -x \"$ROBOREV\" ] && exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" + os.WriteFile(hookPath, []byte(v2Hook), 0755) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if !strings.Contains( + contentStr, PostCommitVersionMarker, + ) { + t.Error("should have v3 marker") + } + if strings.Contains(contentStr, "hook v2") { + t.Error("v2 marker should be removed") + } + }) + + t.Run("upgrades mixed hook preserving user content", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + mixed := "#!/bin/sh\necho 'user code'\n" + + "# roborev post-rewrite hook\n" + + "ROBOREV=\"/usr/bin/roborev\"\n" + + "\"$ROBOREV\" remap --quiet 2>/dev/null\n" + os.WriteFile(hookPath, []byte(mixed), 0755) + + err := Install( + repo.HooksDir, "post-rewrite", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if !strings.Contains( + contentStr, "echo 'user code'", + ) { + t.Error("user content should be preserved") + } + if !strings.Contains( + contentStr, PostRewriteVersionMarker, + ) { + t.Error("should have new version marker") + } + }, + ) + + t.Run("appends to hooks with various shell shebangs", + func(t *testing.T) { + shebangs := []string{ + "#!/bin/sh", "#!/usr/bin/env sh", + "#!/bin/bash", "#!/usr/bin/env bash", + "#!/bin/zsh", "#!/usr/bin/env zsh", + "#!/bin/ksh", "#!/usr/bin/env ksh", + "#!/bin/dash", "#!/usr/bin/env dash", + } + for _, shebang := range shebangs { + t.Run(shebang, func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + existing := shebang + "\necho 'custom'\n" + os.WriteFile( + hookPath, + []byte(existing), + 0755, + ) + + err := Install( + repo.HooksDir, + "post-commit", false, + ) + if err != nil { + t.Fatalf( + "should append to %s: %v", + shebang, err, + ) + } + + content, _ := os.ReadFile(hookPath) + cs := string(content) + if !strings.Contains( + cs, "echo 'custom'", + ) { + t.Error("original should be preserved") + } + if !strings.Contains( + cs, PostCommitVersionMarker, + ) { + t.Error("roborev should be appended") + } + }) + } + }, + ) + + t.Run("refuses non-shell hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + python := "#!/usr/bin/env python3\nprint('hello')\n" + os.WriteFile(hookPath, []byte(python), 0755) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err == nil { + t.Fatal("expected error for non-shell hook") + } + if !errors.Is(err, ErrNonShellHook) { + t.Errorf("should wrap ErrNonShellHook: %v", err) + } + + content, _ := os.ReadFile(hookPath) + if string(content) != python { + t.Error("hook should be unchanged") + } + }) + + t.Run("upgrade returns error on re-read failure", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + outdated := "#!/bin/sh\n" + + "# roborev post-commit hook\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" + os.WriteFile(hookPath, []byte(outdated), 0755) + + origReadFile := ReadFile + ReadFile = func(string) ([]byte, error) { + return nil, fs.ErrPermission + } + t.Cleanup(func() { ReadFile = origReadFile }) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err == nil { + t.Fatal("expected error from re-read failure") + } + if !strings.Contains(err.Error(), "re-read") { + t.Errorf( + "error should mention re-read: %v", err, + ) + } + if !errors.Is(err, fs.ErrPermission) { + t.Errorf( + "should wrap ErrPermission: %v", err, + ) + } + }, + ) + + t.Run("refuses upgrade of non-shell hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + python := "#!/usr/bin/env python3\n" + + "# reviewed by roborev\nprint('hello')\n" + os.WriteFile(hookPath, []byte(python), 0755) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err == nil { + t.Fatal( + "expected error for non-shell upgrade", + ) + } + if !errors.Is(err, ErrNonShellHook) { + t.Errorf( + "should wrap ErrNonShellHook: %v", err, + ) + } + + content, _ := os.ReadFile(hookPath) + if string(content) != python { + t.Error("hook should be unchanged") + } + }) + + t.Run("force overwrites existing hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + existing := "#!/bin/sh\necho 'custom'\n" + os.WriteFile(hookPath, []byte(existing), 0755) + + err := Install( + repo.HooksDir, "post-commit", true, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if strings.Contains(contentStr, "echo 'custom'") { + t.Error("force should overwrite, not append") + } + if !strings.Contains( + contentStr, PostCommitVersionMarker, + ) { + t.Error("should have roborev content") + } + }) +} + +func TestInstallAll(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test checks Unix exec bits") + } + + repo := testutil.NewTestRepo(t) + repo.RemoveHooksDir() + + if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { + t.Fatal(err) + } + + if err := InstallAll(repo.HooksDir, false); err != nil { + t.Fatalf("InstallAll: %v", err) + } + + for _, name := range []string{"post-commit", "post-rewrite"} { + path := filepath.Join(repo.HooksDir, name) + content, err := os.ReadFile(path) + if err != nil { + t.Errorf("%s hook not created: %v", name, err) + continue + } + if !strings.Contains( + string(content), VersionMarker(name), + ) { + t.Errorf( + "%s should contain version marker", name, + ) + } + } +} + +func TestUninstall(t *testing.T) { + t.Run("generated hook is deleted entirely", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + os.WriteFile( + hookPath, + []byte(GeneratePostRewrite()), + 0755, + ) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + if _, err := os.Stat(hookPath); !os.IsNotExist(err) { + t.Error("should be deleted entirely") + } + }, + ) + + t.Run("mixed hook preserves non-roborev content", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + mixed := "#!/bin/sh\necho 'custom logic'\n" + + GeneratePostRewrite() + os.WriteFile(hookPath, []byte(mixed), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, err := os.ReadFile(hookPath) + if err != nil { + t.Fatalf("hook should still exist: %v", err) + } + cs := string(content) + if strings.Contains( + strings.ToLower(cs), "roborev", + ) { + t.Errorf( + "roborev content should be removed:\n%s", + cs, + ) + } + if !strings.Contains(cs, "echo 'custom logic'") { + t.Error("custom content should be preserved") + } + if strings.Contains(cs, "\nfi\n") || + strings.HasSuffix( + strings.TrimSpace(cs), "fi", + ) { + t.Errorf("should not have orphaned fi:\n%s", cs) + } + }, + ) + + t.Run("v3 function wrapper removed", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + // Simulate a v3 embedded hook + v3Content := "#!/bin/sh\n" + + generateEmbeddablePostCommit() + + "echo 'user code after'\n" + os.WriteFile(hookPath, []byte(v3Content), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, err := os.ReadFile(hookPath) + if err != nil { + t.Fatalf("hook should still exist: %v", err) + } + cs := string(content) + if strings.Contains(cs, "_roborev_hook") { + t.Errorf( + "function wrapper should be removed:\n%s", cs, + ) + } + if strings.Contains(cs, "return 0") { + t.Errorf( + "return 0 should be removed:\n%s", cs, + ) + } + if !strings.Contains(cs, "echo 'user code after'") { + t.Error("user content should be preserved") + } + }) + + t.Run("v3 mixed hook preserves user content", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v3Mixed := "#!/bin/sh\n" + + generateEmbeddablePostCommit() + + "echo 'before'\necho 'after'\n" + os.WriteFile(hookPath, []byte(v3Mixed), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, err := os.ReadFile(hookPath) + if err != nil { + t.Fatalf("hook should exist: %v", err) + } + cs := string(content) + if strings.Contains( + strings.ToLower(cs), "roborev", + ) { + t.Errorf("roborev removed:\n%s", cs) + } + if !strings.Contains(cs, "echo 'before'") { + t.Error("should preserve 'before'") + } + if !strings.Contains(cs, "echo 'after'") { + t.Error("should preserve 'after'") + } + }, + ) + + t.Run("v0 hook removed", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v0 := "#!/bin/sh\n" + + "# RoboRev post-commit hook - " + + "auto-reviews every commit\n" + + "roborev enqueue --sha HEAD 2>/dev/null &\n" + os.WriteFile(hookPath, []byte(v0), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + if _, err := os.Stat(hookPath); !os.IsNotExist(err) { + content, _ := os.ReadFile(hookPath) + t.Errorf( + "v0 hook should be deleted:\n%s", content, + ) + } + }) + + t.Run("v0.5 hook removed", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v05 := "#!/bin/sh\n" + + "# RoboRev post-commit hook - " + + "auto-reviews every commit\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "if [ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=$(command -v roborev) || exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet &\n" + os.WriteFile(hookPath, []byte(v05), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + if _, err := os.Stat(hookPath); !os.IsNotExist(err) { + content, _ := os.ReadFile(hookPath) + t.Errorf( + "v0.5 hook should be deleted:\n%s", content, + ) + } + }) + + t.Run("v1 hook removed", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v1 := "#!/bin/sh\n" + + "# RoboRev post-commit hook - " + + "auto-reviews every commit\n" + + "ROBOREV=$(command -v roborev 2>/dev/null)\n" + + "if [ -z \"$ROBOREV\" ] || " + + "[ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=\"/usr/local/bin/roborev\"\n" + + " [ ! -x \"$ROBOREV\" ] && exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" + os.WriteFile(hookPath, []byte(v1), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + if _, err := os.Stat(hookPath); !os.IsNotExist(err) { + content, _ := os.ReadFile(hookPath) + t.Errorf( + "v1 hook should be deleted:\n%s", content, + ) + } + }) + + t.Run("v1 mixed hook removes only roborev block", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v1Block := "# RoboRev post-commit hook - " + + "auto-reviews every commit\n" + + "ROBOREV=$(command -v roborev 2>/dev/null)\n" + + "if [ -z \"$ROBOREV\" ] || " + + "[ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=\"/usr/local/bin/roborev\"\n" + + " [ ! -x \"$ROBOREV\" ] && exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" + mixed := "#!/bin/sh\necho 'custom'\n" + v1Block + os.WriteFile(hookPath, []byte(mixed), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, err := os.ReadFile(hookPath) + if err != nil { + t.Fatalf("hook should exist: %v", err) + } + cs := string(content) + if strings.Contains( + strings.ToLower(cs), "roborev", + ) { + t.Errorf("roborev removed:\n%s", cs) + } + if !strings.Contains(cs, "echo 'custom'") { + t.Error("custom content should be preserved") + } + }, + ) + + t.Run("no-op for no roborev content", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + hookContent := "#!/bin/sh\necho 'unrelated'\n" + os.WriteFile(hookPath, []byte(hookContent), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, _ := os.ReadFile(hookPath) + if string(content) != hookContent { + t.Error("hook should be unchanged") + } + }) + + t.Run("custom if-block after snippet preserved", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + mixed := "#!/bin/sh\n" + + GeneratePostRewrite() + + "if [ -f .notify ]; then\n" + + " echo 'send notification'\n" + + "fi\n" + os.WriteFile(hookPath, []byte(mixed), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, err := os.ReadFile(hookPath) + if err != nil { + t.Fatalf("hook should exist: %v", err) + } + cs := string(content) + if !strings.Contains(cs, "send notification") { + t.Error("user if-block should be preserved") + } + if strings.Contains(cs, "remap --quiet") { + t.Errorf("snippet should be removed:\n%s", cs) + } + // Balanced if/fi + lines := strings.Split(cs, "\n") + ifCount, fiCount := 0, 0 + for _, l := range lines { + tr := strings.TrimSpace(l) + if strings.HasPrefix(tr, "if ") { + ifCount++ + } + if tr == "fi" { + fiCount++ + } + } + if ifCount != fiCount { + t.Errorf( + "if/fi mismatch: %d vs %d in:\n%s", + ifCount, fiCount, cs, + ) + } + }, + ) + + t.Run("missing file is no-op", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "nonexistent") + if err := Uninstall(path); err != nil { + t.Errorf("should be no-op: %v", err) + } + }) +} + +func TestVersionMarker(t *testing.T) { + if m := VersionMarker("post-commit"); m != PostCommitVersionMarker { + t.Errorf("got %q, want %q", m, PostCommitVersionMarker) + } + if m := VersionMarker("post-rewrite"); m != PostRewriteVersionMarker { + t.Errorf("got %q, want %q", m, PostRewriteVersionMarker) + } + if m := VersionMarker("unknown"); m != "" { + t.Errorf("unknown should return empty, got %q", m) + } +} + +func TestHasRealErrors(t *testing.T) { + realErr := errors.New("permission denied") + + t.Run("nil", func(t *testing.T) { + if HasRealErrors(nil) { + t.Error("nil should return false") + } + }) + + t.Run("only non-shell", func(t *testing.T) { + err := fmt.Errorf("hook: %w", ErrNonShellHook) + if HasRealErrors(err) { + t.Error("single ErrNonShellHook should return false") + } + }) + + t.Run("only real", func(t *testing.T) { + if !HasRealErrors(realErr) { + t.Error("real error should return true") + } + }) + + t.Run("joined all non-shell", func(t *testing.T) { + err := errors.Join( + fmt.Errorf("a: %w", ErrNonShellHook), + fmt.Errorf("b: %w", ErrNonShellHook), + ) + if HasRealErrors(err) { + t.Error("joined non-shell only should return false") + } + }) + + t.Run("joined mixed", func(t *testing.T) { + err := errors.Join( + fmt.Errorf("a: %w", ErrNonShellHook), + realErr, + ) + if !HasRealErrors(err) { + t.Error("joined with real error should return true") + } + }) + + t.Run("joined all real", func(t *testing.T) { + err := errors.Join(realErr, errors.New("disk full")) + if !HasRealErrors(err) { + t.Error("joined real errors should return true") + } + }) +}