review/git-review: rewrite commit-msg hook in Go
We are surely going to need to change this in the future.
Make the commit-hook script as trivial as possible, just
delegating to the git-review command, so that we have
some hope of making changes when we need to.
This should also be a good pattern for the gofmt hook.
Change-Id: Ia9cab1320cf50c12c1766a91f7164696b68d8922
Reviewed-on: https://go-review.googlesource.com/1584
Reviewed-by: Andrew Gerrand <adg@golang.org>
diff --git a/git-review/hook.go b/git-review/hook.go
index 7e86457..bb5b024 100644
--- a/git-review/hook.go
+++ b/git-review/hook.go
@@ -5,28 +5,59 @@
package main
import (
+ "bytes"
+ "crypto/rand"
+ "fmt"
+ "io"
"io/ioutil"
"os"
"path/filepath"
"runtime"
)
-var hookFile = filepath.FromSlash(".git/hooks/commit-msg")
+var hookPath = ".git/hooks/"
+var hookFiles = []string{
+ "commit-msg",
+}
func installHook() {
- filename := filepath.Join(repoRoot(), hookFile)
- _, err := os.Stat(filename)
- if err == nil {
- return
- }
- if !os.IsNotExist(err) {
- dief("error checking for hook file: %v", err)
- }
- verbosef("Presubmit hook to add Change-Id to commit messages is missing.")
- verbosef("Automatically creating it at %v.", filename)
- hookContent := []byte(commitMsgHook)
- if err := ioutil.WriteFile(filename, hookContent, 0700); err != nil {
- dief("error writing hook file: %v", err)
+ for _, hookFile := range hookFiles {
+ filename := filepath.Join(repoRoot(), hookPath+hookFile)
+
+ // Special case: remove old commit-msg shell script
+ // in favor of invoking the git-review hook implementation,
+ // which will be easier to change in the future.
+ if hookFile == "commit-msg" {
+ data, err := ioutil.ReadFile(filename)
+ if err == nil && string(data) == oldCommitMsgHook {
+ verbosef("removing old commit-msg hook")
+ os.Remove(filename)
+ }
+ }
+
+ hookContent := fmt.Sprintf(hookScript, hookFile)
+
+ // If hook file exists, assume it is okay.
+ _, 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)
+ }
+ }
+ continue
+ }
+
+ if !os.IsNotExist(err) {
+ dief("checking hook: %v", err)
+ }
+ verbosef("installing %s hook", hookFile)
+ if err := ioutil.WriteFile(filename, []byte(hookContent), 0700); err != nil {
+ dief("writing hook: %v", err)
+ }
}
}
@@ -50,7 +81,53 @@
}
}
-var commitMsgHook = `#!/bin/sh
+var hookScript = `#!/bin/sh
+exec git-review hook-invoke %s "$@"
+`
+
+func hookInvoke(args []string) {
+ if len(args) == 0 {
+ dief("usage: git-review hook-invoke <hook-name> [args...]")
+ }
+ switch args[0] {
+ case "commit-msg":
+ hookCommitMsg(args[1:])
+ }
+}
+
+// hookCommitMsg is installed as the git commit-msg hook.
+// It adds a Change-Id line to the bottom of the commit message
+// if there is not one already.
+func hookCommitMsg(args []string) {
+ // Add Change-Id to commit message if needed.
+ if len(args) != 1 {
+ dief("usage: git-review hook-invoke commit-msg message.txt\n")
+ }
+ file := args[0]
+ data, err := ioutil.ReadFile(file)
+ if err != nil {
+ dief("%v", err)
+ }
+ if bytes.Contains(data, []byte("\nChange-Id: ")) {
+ return
+ }
+ n := len(data)
+ for n > 0 && data[n-1] == '\n' {
+ n--
+ }
+ var id [20]byte
+ if _, err := io.ReadFull(rand.Reader, id[:]); err != nil {
+ dief("generating Change-Id: %v", err)
+ }
+ data = append(data[:n], fmt.Sprintf("\n\nChange-Id: I%x\n", id[:])...)
+ if err := ioutil.WriteFile(file, data, 0666); err != nil {
+ dief("%v", err)
+ }
+}
+
+// This is NOT USED ANYMORE.
+// It is here only for comparing against old commit-hook files.
+var oldCommitMsgHook = `#!/bin/sh
# From Gerrit Code Review 2.2.1
#
# Part of Gerrit Code Review (http://code.google.com/p/gerrit/)
diff --git a/git-review/hook_test.go b/git-review/hook_test.go
new file mode 100644
index 0000000..7770636
--- /dev/null
+++ b/git-review/hook_test.go
@@ -0,0 +1,99 @@
+// Copyright 2014 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import (
+ "bytes"
+ "io/ioutil"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+)
+
+func TestHookCommitMsg(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ // Check that hook adds Change-Id.
+ write(t, gt.client+"/msg.txt", "Test message.\n")
+ testMain(t, "hook-invoke", "commit-msg", gt.client+"/msg.txt")
+ data, err := ioutil.ReadFile(gt.client + "/msg.txt")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !bytes.Contains(data, []byte("\n\nChange-Id: ")) {
+ t.Fatalf("after hook-invoke commit-msg, missing Change-Id:\n%s", data)
+ }
+
+ // Check that hook is no-op when Change-Id is already present.
+ testMain(t, "hook-invoke", "commit-msg", gt.client+"/msg.txt")
+ data1, err := ioutil.ReadFile(gt.client + "/msg.txt")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !bytes.Equal(data, data1) {
+ t.Fatalf("second hook-invoke commit-msg changed Change-Id:\nbefore:\n%s\n\nafter:\n%s", data, data1)
+ }
+}
+
+func TestHooks(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ gt.removeStubHooks()
+ testMain(t, "hooks") // install hooks
+
+ data, err := ioutil.ReadFile(gt.client + "/.git/hooks/commit-msg")
+ if err != nil {
+ t.Fatalf("hooks did not write commit-msg hook: %v", err)
+ }
+ if string(data) != "#!/bin/sh\nexec git-review hook-invoke commit-msg \"$@\"\n" {
+ t.Fatalf("invalid commit-msg hook:\n%s", string(data))
+ }
+}
+
+func TestHooksOverwriteOldCommitMsg(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ write(t, gt.client+"/.git/hooks/commit-msg", oldCommitMsgHook)
+ testMain(t, "hooks") // install hooks
+ data, err := ioutil.ReadFile(gt.client + "/.git/hooks/commit-msg")
+ if err != nil {
+ t.Fatalf("hooks did not write commit-msg hook: %v", err)
+ }
+ if string(data) == oldCommitMsgHook {
+ t.Fatalf("hooks left old commit-msg hook in place")
+ }
+ if string(data) != "#!/bin/sh\nexec git-review hook-invoke commit-msg \"$@\"\n" {
+ t.Fatalf("invalid commit-msg hook:\n%s", string(data))
+ }
+}
+
+func TestHookCommitMsgFromGit(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ trun(t, gt.pwd, "go", "build", "-o", gt.client+"/git-review")
+ path := os.Getenv("PATH")
+ defer os.Setenv("PATH", path)
+ os.Setenv("PATH", gt.client+string(filepath.ListSeparator)+path)
+
+ gt.removeStubHooks()
+ testMain(t, "hooks") // install hooks
+ write(t, gt.client+"/file", "more data")
+ trun(t, gt.client, "git", "add", "file")
+ trun(t, gt.client, "git", "commit", "-m", "mymsg")
+
+ log := trun(t, gt.client, "git", "log", "-n", "1")
+ if !strings.Contains(log, "mymsg") {
+ t.Fatalf("did not find mymsg in git log output:\n%s", log)
+ }
+ // The 4 spaces are because git indents the commit message proper.
+ if !strings.Contains(log, "\n \n Change-Id:") {
+ t.Fatalf("did not find Change-Id in git log output:\n%s", log)
+ }
+}
diff --git a/git-review/review.go b/git-review/review.go
index d06c928..054c510 100644
--- a/git-review/review.go
+++ b/git-review/review.go
@@ -117,6 +117,8 @@
change(args)
case "gofmt":
dief("gofmt not implemented")
+ case "hook-invoke":
+ hookInvoke(args)
case "hooks":
// done - installHook already ran
case "mail", "m":
diff --git a/git-review/util_test.go b/git-review/util_test.go
index 0c5c1e5..c2406dc 100644
--- a/git-review/util_test.go
+++ b/git-review/util_test.go
@@ -65,6 +65,17 @@
client := tmpdir + "/git-client"
mkdir(t, client)
trun(t, client, "git", "clone", server, ".")
+
+ // write stub hooks to keep installHook from installing its own.
+ // If it installs its own, git will look for git-review on the current path
+ // and may find an old git-review that does just about anything.
+ // In any event, we wouldn't be testing what we want to test.
+ // Tests that want to exercise hooks need to arrange for a git-review
+ // in the path and replace these with the real ones.
+ for _, h := range hookFiles {
+ write(t, client+"/.git/hooks/"+h, "#!/bin/bash\nexit 0\n")
+ }
+
trun(t, client, "git", "config", "core.editor", "false")
pwd, err := os.Getwd()
if err != nil {
@@ -85,6 +96,12 @@
return gt
}
+func (gt *gitTest) removeStubHooks() {
+ for _, h := range hookFiles {
+ os.RemoveAll(gt.client + "/.git/hooks/" + h)
+ }
+}
+
func mkdir(t *testing.T, dir string) {
if err := os.Mkdir(dir, 0777); err != nil {
t.Fatal(err)
