cmd/gerritbot: account for a Change-Id in the PR description

If the Change-Id is not present in the footer section of the
git commit, then Gerrit will complain.

Fixes golang/go#23931

Change-Id: I0d6a435d489218c465fc90c3c79151f56254e915
Reviewed-on: https://go-review.googlesource.com/95636
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

cmd/gerritbot/gerritbot.go

@@ -513,10 +513,6 @@ func (b *bot) downloadRef(ctx context.Context, changeID string) (string, error)
 	return rev.Ref, nil
 }
 
-const gerritHostBase = "https://go.googlesource.com/"
-
-var gerritChangeRE = regexp.MustCompile(`https:\/\/go-review\.googlesource\.com\/#\/c\/\w+\/\+\/\d+`)
-
 func runCmd(c *exec.Cmd) error {
 	log.Printf("Executing %v", c.Args)
 	if b, err := c.CombinedOutput(); err != nil {
@@ -525,6 +521,11 @@ func runCmd(c *exec.Cmd) error {
 	return nil
 }
 
+const gerritHostBase = "https://go.googlesource.com/"
+
+// gerritChangeRE matches the URL to the Change within the git output when pushing to Gerrit.
+var gerritChangeRE = regexp.MustCompile(`https:\/\/go-review\.googlesource\.com\/#\/c\/\w+\/\+\/\d+`)
+
 // importGerritChangeFromPR mirrors the latest state of pr to cl. If cl is nil,
 // then a new Gerrit Change is created.
 func (b *bot) importGerritChangeFromPR(ctx context.Context, pr *github.PullRequest, cl *maintner.GerritCL) error {
@@ -590,22 +591,7 @@ func (b *bot) importGerritChangeFromPR(ctx context.Context, pr *github.PullReque
 	for _, c := range []*exec.Cmd{
 		exec.Command("git", "-C", worktreeDir, "checkout", "-B", prShortLink(pr), mergeBaseSHA),
 		exec.Command("git", "-C", worktreeDir, "merge", "--squash", "--no-commit", "FETCH_HEAD"),
-	} {
-		if err := runCmd(c); err != nil {
-			return err
-		}
-	}
-
-	commitMsg := fmt.Sprintf("%s\n\n%s\n\n%s %s\n%s %s\n",
-		pr.GetTitle(),
-		pr.GetBody(),
-		prefixGitFooterLastRev, pr.Head.GetSHA(),
-		prefixGitFooterPR, prShortLink(pr))
-	if cl != nil {
-		commitMsg += fmt.Sprintf("%s %s\n", prefixGitFooterChangeID, cl.ChangeID())
-	}
-	for _, c := range []*exec.Cmd{
-		exec.Command("git", "-C", worktreeDir, "commit", "--author", author, "-m", commitMsg),
+		exec.Command("git", "-C", worktreeDir, "commit", "--author", author, "-m", commitMessage(pr, cl)),
 	} {
 		if err := runCmd(c); err != nil {
 			return err
@@ -631,6 +617,35 @@ See the [Wiki page](https://golang.org/wiki/GerritBot) for more info`,
 	return b.postGitHubMessageNoDup(ctx, repo.GetOwner().GetLogin(), repo.GetName(), pr.GetNumber(), msg)
 }
 
+var changeIdentRE = regexp.MustCompile(`(?m)^Change-Id: (I[0-9a-fA-F]{40})\n?`)
+
+// commitMessage returns the text used when creating the squashed commit for pr.
+// A non-nil cl indicates that pr is associated with an existing Gerrit Change.
+func commitMessage(pr *github.PullRequest, cl *maintner.GerritCL) string {
+	prBody := pr.GetBody()
+	var changeID string
+	if cl != nil {
+		changeID = cl.ChangeID()
+	} else {
+		sms := changeIdentRE.FindStringSubmatch(prBody)
+		if sms != nil {
+			changeID = sms[1]
+			prBody = strings.Replace(prBody, sms[0], "", -1)
+		}
+	}
+
+	msg := fmt.Sprintf("%s\n\n%s\n\n%s %s\n%s %s\n",
+		pr.GetTitle(),
+		prBody,
+		prefixGitFooterLastRev, pr.Head.GetSHA(),
+		prefixGitFooterPR, prShortLink(pr))
+
+	if changeID != "" {
+		msg += fmt.Sprintf("%s %s\n", prefixGitFooterChangeID, changeID)
+	}
+	return msg
+}
+
 func cmdOut(cmd *exec.Cmd) (string, error) {
 	log.Printf("Executing %v", cmd.Args)
 	out, err := cmd.CombinedOutput()

cmd/gerritbot/gerritbot_test.go

@@ -0,0 +1,112 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-github/github"
+	"golang.org/x/build/maintner"
+)
+
+func newPullRequest(title, body string) *github.PullRequest {
+	return &github.PullRequest{
+		Title:  github.String(title),
+		Body:   github.String(body),
+		Number: github.Int(42),
+		Head:   &github.PullRequestBranch{SHA: github.String("deadbeef")},
+		Base: &github.PullRequestBranch{
+			Repo: &github.Repository{
+				Owner: &github.User{
+					Login: github.String("golang"),
+				},
+				Name: github.String("go"),
+			},
+		},
+	}
+}
+
+func newMaintnerCL() *maintner.GerritCL {
+	return &maintner.GerritCL{
+		Commit: &maintner.GitCommit{
+			Msg: `cmd/gerritbot: previous commit messsage
+
+Hello there
+
+Change-Id: If751ce3ffa3a4d5e00a3138211383d12cb6b23fc
+`,
+		},
+	}
+}
+
+func TestCommitMessage(t *testing.T) {
+	testCases := []struct {
+		desc     string
+		pr       *github.PullRequest
+		cl       *maintner.GerritCL
+		expected string
+	}{
+		{
+			"simple",
+			newPullRequest("cmd/gerritbot: title of change", "Body text"),
+			nil,
+			`cmd/gerritbot: title of change
+
+Body text
+
+GitHub-Last-Rev: deadbeef
+GitHub-Pull-Request: golang/go#42
+`,
+		},
+		{
+			"change with Change-Id",
+			newPullRequest("cmd/gerritbot: change with change ID", "Body text"),
+			newMaintnerCL(),
+			`cmd/gerritbot: change with change ID
+
+Body text
+
+GitHub-Last-Rev: deadbeef
+GitHub-Pull-Request: golang/go#42
+Change-Id: If751ce3ffa3a4d5e00a3138211383d12cb6b23fc
+`,
+		},
+		{
+			"Change-Id in body text",
+			newPullRequest("cmd/gerritbot: change with change ID in body text",
+				"Change-Id: I30e0a6ec666a06eae3e8444490d96fabcab3333e"),
+			nil,
+			`cmd/gerritbot: change with change ID in body text
+
+
+
+GitHub-Last-Rev: deadbeef
+GitHub-Pull-Request: golang/go#42
+Change-Id: I30e0a6ec666a06eae3e8444490d96fabcab3333e
+`,
+		},
+		{
+			"Change-Id in body text with an existing CL",
+			newPullRequest("cmd/gerritbot: change with change ID in body text and an existing CL",
+				"Change-Id: I30e0a6ec666a06eae3e8444490d96fabcab3333e"),
+			newMaintnerCL(),
+			`cmd/gerritbot: change with change ID in body text and an existing CL
+
+Change-Id: I30e0a6ec666a06eae3e8444490d96fabcab3333e
+
+GitHub-Last-Rev: deadbeef
+GitHub-Pull-Request: golang/go#42
+Change-Id: If751ce3ffa3a4d5e00a3138211383d12cb6b23fc
+`,
+		},
+	}
+
+	for _, tc := range testCases {
+		if diff := cmp.Diff(commitMessage(tc.pr, tc.cl), tc.expected); diff != "" {
+			t.Errorf("%s: got unexpected commit message (-got +want)\n%s", tc.desc, diff)
+		}
+	}
+}