git-codereview: make hooks command report conflicting hooks
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>
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 @@
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 @@
"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 @@
}
}
- // 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 @@
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 @@
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 @@
}
}
+// 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 @@
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 @@
hookArgs = append(hookArgs, arg)
}
}
- installHook(hookArgs)
+ installHook(hookArgs, true)
}
cmd(args)
