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>

3 files changed, 45 insertions, 34 deletions

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 @@ func checkoutOrCreate(target string) {
 	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 @@ func commitMessageOK() bool {
 		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 @@ func cmdHookInvoke(args []string) {
 	}
 }
 
-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 @@ func hookCommitMsg(args []string) {
 	}
 
 	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 @@ func hookCommitMsg(args []string) {
 		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 @@ func hookCommitMsg(args []string) {
 	}
 
 	// 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 @@ func hookCommitMsg(args []string) {
 	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 @@ func TestHookCommitMsgIssueRepoRewrite(t *testing.T) {
 	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 @@ func TestHookCommitMsgIssueRepoRewrite(t *testing.T) {
 	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