git-codereview: make commit-msg hook play nicely with other systems

Gerrit requires 'Change-Id:' lines to be included in each commit
message, but they are not the only kind of 'metadata line' that might
appear in a commit message. Metadata lines observed by other systems
include 'Bug:' and 'Signed-off-by:'. This change ensures that the
commit-msg hook adds its 'Change-Id:' line to the set of metadata lines,
separated only by a single linefeed, rather than creating a new set by
inserting two line feeds.

Change-Id: Ia3bdce6f52f663685eea1e648874ef81ddb2bd91
Reviewed-on: https://go-review.googlesource.com/c/review/+/169097
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

2 files changed, 33 insertions, 8 deletions

diff --git a/git-codereview/hook.go b/git-codereview/hook.go
index b80c567..09bed8b 100644
--- a/git-codereview/hook.go
+++ b/git-codereview/hook.go
@@ -189,15 +189,12 @@ func hookCommitMsg(args []string) {
 
 		// Add Change-Id to commit message if not present.
 		if nChangeId == 0 {
-			n := len(data)
-			for n > 0 && data[n-1] == '\n' {
-				n--
+			data = bytes.TrimRight(data, "\n")
+			sep := "\n\n"
+			if endsWithMetadataLine(data) {
+				sep = "\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[:])...)
+			data = append(data, fmt.Sprintf("%sChange-Id: I%x\n", sep, randomBytes())...)
 		}
 
 		// Add branch prefix to commit message if not present and on a
@@ -221,6 +218,24 @@ func hookCommitMsg(args []string) {
 	}
 }
 
+// randomBytes returns 20 random bytes suitable for use in a Change-Id line.
+func randomBytes() []byte {
+	var id [20]byte
+	if _, err := io.ReadFull(rand.Reader, id[:]); err != nil {
+		dief("generating Change-Id: %v", err)
+	}
+	return id[:]
+}
+
+var metadataLineRE = regexp.MustCompile(`^[a-zA-Z0-9-]+: `)
+
+// endsWithMetadataLine reports whether the given commit message ends with a
+// metadata line such as "Bug: #42" or "Signed-off-by: Al <al@example.com>".
+func endsWithMetadataLine(msg []byte) bool {
+	i := bytes.LastIndexByte(msg, '\n')
+	return i >= 0 && metadataLineRE.Match(msg[i+1:])
+}
+
 var (
 	fixupBang  = []byte("fixup!")
 	squashBang = []byte("squash!")
diff --git a/git-codereview/hook_test.go b/git-codereview/hook_test.go
index 8cb04f9..d1cc0c6 100644
--- a/git-codereview/hook_test.go
+++ b/git-codereview/hook_test.go
@@ -42,6 +42,16 @@ func TestHookCommitMsgGerrit(t *testing.T) {
 	if got := testStderr.String(); got != multiple {
 		t.Fatalf("unexpected output:\ngot: %q\nwant: %q", got, multiple)
 	}
+
+	// Check that hook doesn't add two line feeds before Change-Id
+	// if the exsting message ends with a metadata line.
+	write(t, gt.client+"/msg.txt", "Test message.\n\nBug: 1234\n")
+	testMain(t, "hook-invoke", "commit-msg", gt.client+"/msg.txt")
+	data = read(t, gt.client+"/msg.txt")
+	if !bytes.Contains(data, []byte("Bug: 1234\nChange-Id: ")) {
+		t.Fatalf("after hook-invoke commit-msg, missing Change-Id: directly after Bug line\n%s", data)
+	}
+
 }
 
 func TestHookCommitMsg(t *testing.T) {