review: check form of commit messages
Fixes golang/go#9254
Change-Id: Iac4cc8474096209cd1b0f8ee812c0637babc2560
Reviewed-on: https://go-review.googlesource.com/1351
Reviewed-by: Russ Cox <rsc@golang.org>
diff --git a/git-review/change.go b/git-review/change.go
index d1a21c5..0990d58 100644
--- a/git-review/change.go
+++ b/git-review/change.go
@@ -7,6 +7,7 @@
import (
"fmt"
"os"
+ "regexp"
"strings"
)
@@ -57,17 +58,27 @@
if !HasStagedChanges() {
printf("no staged changes. Did you forget to 'git add'?")
}
- args := []string{"commit", "-q", "--allow-empty"}
- if amend {
- args = append(args, "--amend")
- if changeQuick {
- args = append(args, "--no-edit")
+ commit := func(amend bool) {
+ args := []string{"commit", "-q", "--allow-empty"}
+ if amend {
+ args = append(args, "--amend")
+ if changeQuick {
+ args = append(args, "--no-edit")
+ }
}
+ if testCommitMsg != "" {
+ args = append(args, "-m", testCommitMsg)
+ }
+ run("git", args...)
}
- if testCommitMsg != "" {
- args = append(args, "-m", testCommitMsg)
+ commit(amend)
+ for !commitMessageOK() {
+ fmt.Print("re-edit commit message (y/n)? ")
+ if !scanYes() {
+ break
+ }
+ commit(true)
}
- run("git", args...)
printf("change updated.")
}
@@ -111,3 +122,55 @@
run("git", "branch", "-q", "--set-upstream-to", origin)
printf("created branch %v tracking %s.", target, origin)
}
+
+var (
+ messageRE = regexp.MustCompile(`^(\[[a-zA-Z0-9.-]+\] )?[a-zA-Z0-9-/, ]+: `)
+ oldFixesRE = regexp.MustCompile(`Fixes +(issue +#?)?([0-9]+)`)
+)
+
+func commitMessageOK() bool {
+ body := getOutput("git", "log", "--format=format:%B", "-n", "1")
+ ok := true
+ if !messageRE.MatchString(body) {
+ fmt.Print(commitMessageWarning)
+ ok = false
+ }
+ if m := oldFixesRE.FindStringSubmatch(body); m != nil {
+ fmt.Printf(fixesIssueWarning, m[0], m[2])
+ ok = false
+ }
+ return ok
+}
+
+const commitMessageWarning = `
+Your CL description appears not to use the standard form.
+
+The first line of your change description is conventionally a one-line summary
+of the change, prefixed by the primary affected package, and is used as the
+subject for code review mail; the rest of the description elaborates.
+
+Examples:
+
+ encoding/rot13: new package
+
+ math: add IsInf, IsNaN
+
+ net: fix cname in LookupHost
+
+ unicode: update to Unicode 5.0.2
+
+`
+
+const fixesIssueWarning = `
+Your CL description contains the string %q, which is
+the old Google Code way of linking commits to issues.
+
+You should rewrite it to use the GitHub convention: "Fixes #%v".
+
+`
+
+func scanYes() bool {
+ var s string
+ fmt.Scan(&s)
+ return strings.HasPrefix(strings.ToLower(s), "y")
+}
diff --git a/git-review/change_test.go b/git-review/change_test.go
index f493d94..ad33e7a 100644
--- a/git-review/change_test.go
+++ b/git-review/change_test.go
@@ -14,12 +14,12 @@
testMain(t, "change", "master")
testRan(t, "git checkout -q master")
- testCommitMsg = "my commit msg"
+ testCommitMsg = "foo: my commit msg"
t.Logf("master -> work")
testMain(t, "change", "work")
testRan(t, "git checkout -q -b work",
"git branch -q --set-upstream-to origin/master",
- "git commit -q --allow-empty -m my commit msg")
+ "git commit -q --allow-empty -m foo: my commit msg")
t.Logf("work -> master")
testMain(t, "change", "master")
@@ -29,3 +29,22 @@
testMain(t, "change", "dev.branch")
testRan(t, "git checkout -q -t -b dev.branch origin/dev.branch")
}
+
+func TestMessageRE(t *testing.T) {
+ for _, c := range []struct {
+ in string
+ want bool
+ }{
+ {"blah", false},
+ {"[release-branch.go1.4] blah", false},
+ {"[release-branch.go1.4] math: fix cosine", true},
+ {"math: fix cosine", true},
+ {"math/rand: make randomer", true},
+ {"math/rand, crypto/rand: fix random sources", true},
+ } {
+ got := messageRE.MatchString(c.in)
+ if got != c.want {
+ t.Errorf("MatchString(%q) = %v, want %v", c.in, got, c.want)
+ }
+ }
+}
