git-codereview: make hooks command report conflicting hooksv1.8.0

Display warning message when a hook is already installed and is
different from the one installed by git-codereview.

Improves upon CL 184417.

Fixes golang/go#16777

Change-Id: I7579a3e86572e8b74f92317973e7cc7094b3942d
Reviewed-on: https://go-review.googlesource.com/c/review/+/377034
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>

4 files changed, 52 insertions, 11 deletions

diff --git a/git-codereview/doc.go b/git-codereview/doc.go
index 5f3e0b1..6e65e42 100644
--- a/git-codereview/doc.go
+++ b/git-codereview/doc.go
@@ -201,7 +201,6 @@ not present. It also checks that the message uses the convention established by
 the Go project that the first line has the form, pkg/path: summary.
 
 The hooks command will not overwrite an existing hook.
-If it is not installing hooks, use “git codereview hooks -v” for details.
 This hook installation is also done at startup by all other git codereview
 commands, except “git codereview help”.
 
diff --git a/git-codereview/hook.go b/git-codereview/hook.go
index 1ad0f09..0b4995f 100644
--- a/git-codereview/hook.go
+++ b/git-codereview/hook.go
@@ -21,9 +21,15 @@ var hookFiles = []string{
 	"pre-commit",
 }
 
-func installHook(args []string) {
+// installHook installs Git hooks to enforce code review conventions.
+//
+// auto is whether hooks are being installed automatically as part of
+// running another git-codereview command, rather than an explicit
+// invocation of the 'hooks' command itself.
+func installHook(args []string, auto bool) {
 	flags.Parse(args)
 	hooksDir := gitPath("hooks")
+	var existingHooks []string
 	for _, hookFile := range hookFiles {
 		filename := filepath.Join(hooksDir, hookFile)
 		hookContent := fmt.Sprintf(hookScript, hookFile)
@@ -45,15 +51,16 @@ func installHook(args []string) {
 			}
 		}
 
-		// If hook file exists, assume it is okay.
+		// If hook file exists but has different content, let the user know.
 		_, err := os.Stat(filename)
 		if err == nil {
-			if *verbose > 0 {
-				data, err := ioutil.ReadFile(filename)
-				if err != nil {
-					verbosef("reading hook: %v", err)
-				} else if string(data) != hookContent {
-					verbosef("unexpected hook content in %s", filename)
+			data, err := ioutil.ReadFile(filename)
+			if err != nil {
+				verbosef("reading hook: %v", err)
+			} else if string(data) != hookContent {
+				verbosef("unexpected hook content in %s", filename)
+				if !auto {
+					existingHooks = append(existingHooks, filename)
 				}
 			}
 			continue
@@ -74,6 +81,19 @@ func installHook(args []string) {
 			dief("writing hook: %v", err)
 		}
 	}
+
+	switch {
+	case len(existingHooks) == 1:
+		dief("Hooks file %s already exists."+
+			"\nTo install git-codereview hooks, delete that"+
+			" file and re-run 'git-codereview hooks'.",
+			existingHooks[0])
+	case len(existingHooks) > 1:
+		dief("Hooks files %s already exist."+
+			"\nTo install git-codereview hooks, delete these"+
+			" files and re-run 'git-codereview hooks'.",
+			strings.Join(existingHooks, ", "))
+	}
 }
 
 // repoRoot returns the root of the currently selected git repo, or
diff --git a/git-codereview/hook_test.go b/git-codereview/hook_test.go
index d4ffbc4..8ffe234 100644
--- a/git-codereview/hook_test.go
+++ b/git-codereview/hook_test.go
@@ -475,6 +475,8 @@ func TestHooksOverwriteOldCommitMsg(t *testing.T) {
 	gt := newGitTest(t)
 	defer gt.done()
 
+	gt.removeStubHooks()
+	mkdir(t, gt.client+"/.git/hooks")
 	write(t, gt.client+"/.git/hooks/commit-msg", oldCommitMsgHook, 0755)
 	testMain(t, "hooks") // install hooks
 	data, err := ioutil.ReadFile(gt.client + "/.git/hooks/commit-msg")
@@ -489,6 +491,26 @@ func TestHooksOverwriteOldCommitMsg(t *testing.T) {
 	}
 }
 
+// Test that 'git-codereview hooks' reports when it fails to write hooks.
+// See go.dev/issue/16777.
+func TestHooksReportConflictingContent(t *testing.T) {
+	gt := newGitTest(t)
+	defer gt.done()
+
+	const pretendUserHook = "#!/bin/sh\necho 'pretend to be a custom hook'\nexit 1\n"
+	for _, h := range hookFiles {
+		write(t, gt.client+"/.git/hooks/"+h, pretendUserHook, 0755)
+	}
+	testMainDied(t, "hooks") // install hooks
+	testPrintedStderr(t, "Hooks files", "already exist.", "To install git-codereview hooks, delete these files and re-run 'git-codereview hooks'.")
+	for _, h := range hookFiles {
+		data := read(t, gt.client+"/.git/hooks/"+h)
+		if string(data) != pretendUserHook {
+			t.Errorf("existing hook file %s was unexpectedly modified", h)
+		}
+	}
+}
+
 func testInstallHook(t *testing.T, gt *gitTest) (restore func()) {
 	trun(t, gt.pwd, "go", "build", "-o", gt.client+"/git-codereview")
 	path := os.Getenv("PATH")
diff --git a/git-codereview/review.go b/git-codereview/review.go
index 1e79114..fe5bf12 100644
--- a/git-codereview/review.go
+++ b/git-codereview/review.go
@@ -94,7 +94,7 @@ func main() {
 		fmt.Fprintf(stdout(), help, progName)
 		return // avoid installing hooks.
 	case "hooks": // in case hooks weren't installed.
-		installHook(args)
+		installHook(args, false)
 		return // avoid invoking installHook twice.
 
 	case "branchpoint":
@@ -135,7 +135,7 @@ func main() {
 				hookArgs = append(hookArgs, arg)
 			}
 		}
-		installHook(hookArgs)
+		installHook(hookArgs, true)
 	}
 
 	cmd(args)