git-review: add gofmt pre-commit hook
Also change getLines to not strip spaces, making it more generally
useful, and the use it more generally.
Change-Id: I0969a2bee5ec5f331fba82e5902ca9879b988fe8
Reviewed-on: https://go-review.googlesource.com/1610
Reviewed-by: Russ Cox <rsc@golang.org>
diff --git a/git-review/branch.go b/git-review/branch.go
index 8c31d8a..c4a44c5 100644
--- a/git-review/branch.go
+++ b/git-review/branch.go
@@ -104,8 +104,7 @@
var stagedRE = regexp.MustCompile(`^[ACDMR] `)
func HasStagedChanges() bool {
- // NOTE: Cannot use getLines, because it throws away leading spaces.
- for _, s := range strings.Split(getOutput("git", "status", "-b", "--porcelain"), "\n") {
+ for _, s := range getLines("git", "status", "-b", "--porcelain") {
if stagedRE.MatchString(s) {
return true
}
@@ -116,8 +115,7 @@
var unstagedRE = regexp.MustCompile(`^.[ACDMR]`)
func HasUnstagedChanges() bool {
- // NOTE: Cannot use getLines, because it throws away leading spaces.
- for _, s := range strings.Split(getOutput("git", "status", "-b", "--porcelain"), "\n") {
+ for _, s := range getLines("git", "status", "-b", "--porcelain") {
if unstagedRE.MatchString(s) {
return true
}
@@ -128,7 +126,8 @@
func LocalBranches() []*Branch {
var branches []*Branch
for _, s := range getLines("git", "branch", "-q") {
- branches = append(branches, &Branch{Name: strings.TrimPrefix(s, "* ")})
+ s = strings.TrimPrefix(strings.TrimSpace(s), "* ")
+ branches = append(branches, &Branch{Name: s})
}
return branches
}
@@ -136,6 +135,7 @@
func OriginBranches() []string {
var branches []string
for _, line := range getLines("git", "branch", "-a", "-q") {
+ line = strings.TrimSpace(line)
if i := strings.Index(line, " -> "); i >= 0 {
line = line[:i]
}
diff --git a/git-review/hook.go b/git-review/hook.go
index 7eed33f..f79d78d 100644
--- a/git-review/hook.go
+++ b/git-review/hook.go
@@ -14,11 +14,13 @@
"path/filepath"
"regexp"
"runtime"
+ "strings"
)
var hookPath = ".git/hooks/"
var hookFiles = []string{
"commit-msg",
+ "pre-commit",
}
func installHook() {
@@ -93,6 +95,8 @@
switch args[0] {
case "commit-msg":
hookCommitMsg(args[1:])
+ case "pre-commit":
+ hookPreCommit(args[1:])
}
}
@@ -135,6 +139,55 @@
return regexp.MustCompile(`(?m)^#.*\n`).ReplaceAll(in, nil)
}
+// hookPreCommit is installed as the git pre-commit hook.
+// It prevents commits to the master branch.
+// It checks that the Go files added, copied, or modified by
+// the change are gofmt'd, and if not it prints gofmt instructions
+// and exits with nonzero status.
+func hookPreCommit(args []string) {
+ // Prevent commits to master branches.
+ b := CurrentBranch()
+ if !b.IsLocalOnly() {
+ dief("cannot commit on %s branch", b.Name)
+ }
+
+ root := repoRoot()
+
+ // Run the gofmt check over the pending commit, if any.
+ rev := "HEAD"
+ if b.HasPendingCommit() {
+ rev = "HEAD^" // branch point, hopefully
+ }
+ // This command prints file names relative to the repo root.
+ files := getLines("git", "diff", "--cached", "--name-only", "--diff-filter=ACM", rev)
+ var goFiles []string // absolute paths of files that need gofmting
+ for _, file := range files {
+ if gofmtRequired(file) {
+ goFiles = append(goFiles, filepath.Join(root, file))
+ }
+ }
+ if len(goFiles) == 0 {
+ return
+ }
+
+ // Check gofmt.
+ unfmted := getLines("gofmt", append([]string{"-l"}, goFiles...)...)
+ if len(unfmted) == 0 {
+ return
+ }
+
+ dief("gofmt needs to format these files (run 'git gofmt'):\n\t%s",
+ strings.Join(unfmted, "\n\t"))
+}
+
+// gofmtRequired reports whether the specified file should be checked
+// for gofmt'dness by the pre-commit hook.
+// The file name is relative to the repo root.
+func gofmtRequired(file string) bool {
+ return strings.HasSuffix(file, ".go") &&
+ !(strings.HasPrefix(file, "test/") && !strings.HasPrefix(file, "test/bench/"))
+}
+
// This is NOT USED ANYMORE.
// It is here only for comparing against old commit-hook files.
var oldCommitMsgHook = `#!/bin/sh
diff --git a/git-review/hook_test.go b/git-review/hook_test.go
index eaa6016..2ccc7c4 100644
--- a/git-review/hook_test.go
+++ b/git-review/hook_test.go
@@ -47,6 +47,38 @@
}
}
+func TestHookPreCommit(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ gt.removeStubHooks()
+ testMain(t, "hooks") // install hooks
+
+ // Write out a non-Go file.
+ testMain(t, "change", "mybranch")
+ write(t, gt.client+"/msg.txt", "A test message.")
+ trun(t, gt.client, "git", "add", "msg.txt")
+ testMain(t, "hook-invoke", "pre-commit") // should be no-op
+
+ // Write out a badly formatted Go files.
+ if err := os.MkdirAll(gt.client+"/test/bench", 0755); err != nil {
+ t.Fatal(err)
+ }
+ const badGo = "package main\nfunc main(){}"
+ const goodGo = "package main\n\nfunc main() {\n}\n"
+ write(t, gt.client+"/bad.go", badGo)
+ write(t, gt.client+"/good.go", goodGo)
+ write(t, gt.client+"/test/bad.go", badGo)
+ write(t, gt.client+"/test/good.go", goodGo)
+ write(t, gt.client+"/test/bench/bad.go", badGo)
+ write(t, gt.client+"/test/bench/good.go", goodGo)
+ trun(t, gt.client, "git", "add", ".")
+
+ testMainDied(t, "hook-invoke", "pre-commit")
+ testPrintedStderr(t, "gofmt needs to format these files (run 'git gofmt'):",
+ "bad.go", "!good.go", "!test/bad", "test/bench/bad.go")
+}
+
func TestHooks(t *testing.T) {
gt := newGitTest(t)
defer gt.done()
@@ -92,6 +124,7 @@
gt.removeStubHooks()
testMain(t, "hooks") // install hooks
+ testMain(t, "change", "mybranch")
write(t, gt.client+"/file", "more data")
trun(t, gt.client, "git", "add", "file")
trun(t, gt.client, "git", "commit", "-m", "mymsg")
diff --git a/git-review/review.go b/git-review/review.go
index 054c510..8f5843c 100644
--- a/git-review/review.go
+++ b/git-review/review.go
@@ -204,7 +204,9 @@
func getLines(command string, args ...string) []string {
var s []string
for _, l := range strings.Split(getOutput(command, args...), "\n") {
- s = append(s, strings.TrimSpace(l))
+ if len(strings.TrimSpace(l)) > 0 {
+ s = append(s, l)
+ }
}
return s
}
