git-codereview: Properly detect Rietveld-style fix messages
Detection of Rietveld-style fix messages is already in place as a warning,
but was broken when we added feature to rewrite issue references in commit edbdf1a7 .
To fix this, we automatically reformat the fix message into the GitHub style.
We also forget to write the change to the fix message back,
if we do not also need to write Change-Id and origin branch.
This change also fixes this.
Fixes golang/go#11472
Change-Id: Ie77358867e38cf976a0688b6e2f80525dae3891e
Reviewed-on: https://go-review.googlesource.com/15754
Reviewed-by: Andrew Gerrand <adg@golang.org>
diff --git a/git-codereview/change.go b/git-codereview/change.go
index b5bcaa2..4c0e252 100644
--- a/git-codereview/change.go
+++ b/git-codereview/change.go
@@ -156,10 +156,7 @@
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]+)`)
-)
+var messageRE = regexp.MustCompile(`^(\[[a-zA-Z0-9.-]+\] )?[a-zA-Z0-9-/,. ]+: `)
func commitMessageOK() bool {
body := cmdOutput("git", "log", "--format=format:%B", "-n", "1")
@@ -168,10 +165,6 @@
fmt.Print(commitMessageWarning)
ok = false
}
- if m := oldFixesRE.FindStringSubmatch(body); m != nil {
- fmt.Printf(fixesIssueWarning, m[0], m[2])
- ok = false
- }
return ok
}
diff --git a/git-codereview/hook.go b/git-codereview/hook.go
index 18994c8..570001e 100644
--- a/git-codereview/hook.go
+++ b/git-codereview/hook.go
@@ -111,7 +111,10 @@
}
}
-var issueRefRE = regexp.MustCompile(`(?P<space>\s)(?P<ref>#\d+\w)`)
+var (
+ issueRefRE = regexp.MustCompile(`(?P<space>\s)(?P<ref>#\d+\w)`)
+ oldFixesRETemplate = `Fixes +(issue +(%s)?#?)?(?P<issueNum>[0-9]+)`
+)
// hookCommitMsg is installed as the git commit-msg hook.
// It adds a Change-Id line to the bottom of the commit message
@@ -129,10 +132,11 @@
}
file := args[0]
- data, err := ioutil.ReadFile(file)
+ oldData, err := ioutil.ReadFile(file)
if err != nil {
dief("%v", err)
}
+ data := append([]byte{}, oldData...)
data = stripComments(data)
// Empty message not allowed.
@@ -148,10 +152,14 @@
data[eol+1] = '\n'
}
+ issueRepo := config()["issuerepo"]
// Update issue references to point to issue repo, if set.
- if issueRepo := config()["issuerepo"]; issueRepo != "" {
+ if issueRepo != "" {
data = issueRefRE.ReplaceAll(data, []byte("${space}"+issueRepo+"${ref}"))
}
+ // TestHookCommitMsgIssueRepoRewrite makes sure the regex is valid
+ oldFixesRE := regexp.MustCompile(fmt.Sprintf(oldFixesRETemplate, regexp.QuoteMeta(issueRepo)))
+ data = oldFixesRE.ReplaceAll(data, []byte("Fixes "+issueRepo+"#${issueNum}"))
// Complain if two Change-Ids are present.
// This can happen during an interactive rebase;
@@ -162,9 +170,7 @@
}
// Add Change-Id to commit message if not present.
- edited := false
if nChangeId == 0 {
- edited = true
n := len(data)
for n > 0 && data[n-1] == '\n' {
n--
@@ -182,13 +188,12 @@
if branch != "master" {
prefix := "[" + branch + "] "
if !bytes.HasPrefix(data, []byte(prefix)) && !isFixup(data) {
- edited = true
data = []byte(prefix + string(data))
}
}
// Write back.
- if edited {
+ if !bytes.Equal(data, oldData) {
if err := ioutil.WriteFile(file, data, 0666); err != nil {
dief("%v", err)
}
diff --git a/git-codereview/hook_test.go b/git-codereview/hook_test.go
index b3686a4..fc17e1e 100644
--- a/git-codereview/hook_test.go
+++ b/git-codereview/hook_test.go
@@ -89,17 +89,24 @@
gt := newGitTest(t)
defer gt.done()
- // If there's no config, don't rewrite issue references.
- const msg = "math/big: catch all the rats\n\nFixes #99999, at least for now\n"
- write(t, gt.client+"/msg.txt", msg)
- testMain(t, "hook-invoke", "commit-msg", gt.client+"/msg.txt")
- got, err := ioutil.ReadFile(gt.client + "/msg.txt")
- if err != nil {
- t.Fatal(err)
+ msgs := []string{
+ // If there's no config, don't rewrite issue references.
+ "math/big: catch all the rats\n\nFixes #99999, at least for now\n",
+ // Fix the fix-message, even without config
+ "math/big: catch all the rats\n\nFixes issue #99999, at least for now\n",
+ "math/big: catch all the rats\n\nFixes issue 99999, at least for now\n",
+ // Don't forget to write back if Change-Id already exists
+ "math/big: catch all the rats\n\nFixes issue #99999, at least for now\n\nChange-Id: Ie77358867e38cf976a0688b6e2f80525dae3891e\n",
}
- got = got[:len(got)-lenChangeId]
- if string(got) != msg {
- t.Errorf("hook changed %s to %s", msg, got)
+ for _, msg := range msgs {
+ write(t, gt.client+"/msg.txt", msg)
+ testMain(t, "hook-invoke", "commit-msg", gt.client+"/msg.txt")
+ got := read(t, gt.client+"/msg.txt")
+ got = got[:len(got)-lenChangeId]
+ const want = "math/big: catch all the rats\n\nFixes #99999, at least for now\n"
+ if string(got) != want {
+ t.Errorf("issue rewrite failed: got\n\n%s\nwant\n\n%s\nlen %d and %d", got, want, len(got), len(want))
+ }
}
// Add issuerepo config.
@@ -113,16 +120,22 @@
cachedConfig = nil
// Check for the rewrite
- write(t, gt.client+"/msg.txt", msg)
- testMain(t, "hook-invoke", "commit-msg", gt.client+"/msg.txt")
- got, err = ioutil.ReadFile(gt.client + "/msg.txt")
- if err != nil {
- t.Fatal(err)
+ msgs = []string{
+ "math/big: catch all the rats\n\nFixes #99999, at least for now\n",
+ "math/big: catch all the rats\n\nFixes issue #99999, at least for now\n",
+ "math/big: catch all the rats\n\nFixes issue 99999, at least for now\n",
+ "math/big: catch all the rats\n\nFixes issue golang/go#99999, at least for now\n",
+ "math/big: catch all the rats\n\nFixes issue #99999, at least for now\n\nChange-Id: Ie77358867e38cf976a0688b6e2f80525dae3891e\n",
}
- got = got[:len(got)-lenChangeId]
- const want = "math/big: catch all the rats\n\nFixes golang/go#99999, at least for now\n"
- if string(got) != want {
- t.Errorf("issue rewrite failed: got\n\n%s\nwant\n\n%s", got, want)
+ for _, msg := range msgs {
+ write(t, gt.client+"/msg.txt", msg)
+ testMain(t, "hook-invoke", "commit-msg", gt.client+"/msg.txt")
+ got := read(t, gt.client+"/msg.txt")
+ got = got[:len(got)-lenChangeId]
+ const want = "math/big: catch all the rats\n\nFixes golang/go#99999, at least for now\n"
+ if string(got) != want {
+ t.Errorf("issue rewrite failed: got\n\n%s\nwant\n\n%s", got, want)
+ }
}
// Reset config state
