[autoroll] Add better handling for git trailers
Change-Id: I62b6998c1063d9b242b5187581bc359c79722474
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/373740
Commit-Queue: Eric Boren <borenet@google.com>
Reviewed-by: Ravi Mistry <rmistry@google.com>
diff --git a/autoroll/go/repo_manager/deps_repo_manager_test.go b/autoroll/go/repo_manager/deps_repo_manager_test.go
index 0f63fb3..ac7949e 100644
--- a/autoroll/go/repo_manager/deps_repo_manager_test.go
+++ b/autoroll/go/repo_manager/deps_repo_manager_test.go
@@ -41,7 +41,7 @@
blah blah blah
`
- fakeCommitMsgMock = fakeCommitMsg + "Change-Id: 123"
+ fakeCommitMsgMock = fakeCommitMsg + "\nChange-Id: 123"
)
var (
diff --git a/go/gerrit/change_edit_helpers.go b/go/gerrit/change_edit_helpers.go
index cb775c6..1ab523d 100644
--- a/go/gerrit/change_edit_helpers.go
+++ b/go/gerrit/change_edit_helpers.go
@@ -4,6 +4,7 @@
"context"
"strings"
+ "go.skia.org/infra/go/git"
"go.skia.org/infra/go/skerr"
)
@@ -36,7 +37,10 @@
if err != nil {
return nil, skerr.Wrapf(err, "failed to create change")
}
- commitMsg = strings.TrimSpace(commitMsg) + "\n" + "Change-Id: " + ci.ChangeId
+ commitMsg, err = git.AddTrailer(commitMsg, "Change-Id: "+ci.ChangeId)
+ if err != nil {
+ return nil, skerr.Wrap(err)
+ }
if err := EditChange(ctx, g, ci, func(ctx context.Context, g GerritInterface, ci *ChangeInfo) error {
if err := g.SetCommitMessage(ctx, ci, commitMsg); err != nil {
return skerr.Wrapf(err, "failed to set commit message to:\n\n%s\n\n", commitMsg)
diff --git a/go/git/util.go b/go/git/util.go
index 2c292ff..403d7c9 100644
--- a/go/git/util.go
+++ b/go/git/util.go
@@ -7,6 +7,7 @@
"os"
"path"
"path/filepath"
+ "regexp"
"strconv"
"strings"
"time"
@@ -31,6 +32,10 @@
DefaultRemoteBranch = git_common.DefaultRemoteBranch
)
+// This regex is taken from:
+// https://source.chromium.org/chromium/infra/infra/+/master:go/src/go.chromium.org/luci/common/git/footer/footer.go?q=%22%5E%5Cs*(%5B%5Cw-%5D%2B):%20*(.*)$%22&ss=chromium
+var trailerRegex = regexp.MustCompile(`^\s*([\w-]+): *(.*)$`)
+
// Types of git objects.
const (
ObjectTypeBlob ObjectType = "blob"
@@ -197,3 +202,70 @@
Sys: nil,
}.Get(), nil
}
+
+// SplitTrailers splits a commit message into a main commit message body and
+// trailers footer. Assumes that the commit message is already well-formed with
+// respect to trailers, ie. there is an empty line between the last body
+// paragraph and the single trailers paragraph, which contains only lines in
+// "key: value" format.
+func SplitTrailers(commitMsg string) ([]string, []string) {
+ // Split the commit message into paragraphs.
+ if commitMsg == "" {
+ return []string{}, []string{}
+ }
+ lines := strings.Split(strings.TrimSpace(commitMsg), "\n")
+ paragraphs := [][]string{}
+ var paragraph []string
+ for _, line := range lines {
+ paragraph = append(paragraph, line)
+ if line == "" {
+ if len(paragraph) > 0 {
+ paragraphs = append(paragraphs, paragraph)
+ }
+ paragraph = []string{}
+ }
+ }
+ if len(paragraph) > 0 {
+ paragraphs = append(paragraphs, paragraph)
+ }
+
+ // If the last paragraph consists of valid trailers, split off those lines,
+ // otherwise include them as part of the main commit message body.
+ if len(paragraphs) < 1 {
+ return lines, []string{}
+ }
+ trailerLines := paragraphs[len(paragraphs)-1]
+ for _, line := range trailerLines {
+ if !trailerRegex.MatchString(line) {
+ // At least one line in the last paragraph does not fit the trailer
+ // format; assume there are no trailers.
+ return lines, []string{}
+ }
+ }
+ bodyLines := make([]string, 0, len(paragraphs[0]))
+ for _, paragraph := range paragraphs[:len(paragraphs)-1] {
+ bodyLines = append(bodyLines, paragraph...)
+ }
+ return bodyLines, trailerLines
+}
+
+// JoinTrailers joins a main commit message body with a trailers footer.
+func JoinTrailers(bodyLines, trailers []string) string {
+ commitMsg := make([]string, 0, len(bodyLines)+len(trailers)+1)
+ commitMsg = append(commitMsg, bodyLines...)
+ if len(commitMsg) > 0 && commitMsg[len(commitMsg)-1] != "" {
+ commitMsg = append(commitMsg, "")
+ }
+ commitMsg = append(commitMsg, trailers...)
+ return strings.Join(commitMsg, "\n")
+}
+
+// AddTrailer adds a trailer to the given commit message.
+func AddTrailer(commitMsg, trailer string) (string, error) {
+ if !trailerRegex.MatchString(trailer) {
+ return "", skerr.Fmt("%q is not a valid git trailer", trailer)
+ }
+ body, trailers := SplitTrailers(commitMsg)
+ trailers = append(trailers, trailer)
+ return JoinTrailers(body, trailers), nil
+}
diff --git a/go/git/util_test.go b/go/git/util_test.go
index 3849751..213d3bb 100644
--- a/go/git/util_test.go
+++ b/go/git/util_test.go
@@ -25,3 +25,117 @@
require.NoError(t, err)
assert.Equal(t, "github.com/skia-dev/textfiles", normGitWithExt)
}
+
+func TestSplitTrailers(t *testing.T) {
+ unittest.SmallTest(t)
+
+ test := func(commitMsg string, expectBody, expectTrailers []string) {
+ actualBody, actualTrailers := SplitTrailers(commitMsg)
+ require.Equal(t, expectBody, actualBody)
+ require.Equal(t, expectTrailers, actualTrailers)
+ }
+
+ test("", []string{}, []string{})
+ test("Hello World", []string{"Hello World"}, []string{})
+ test(`Hello World
+ `, []string{"Hello World"}, []string{})
+ test(`Hello World
+
+Paragraph 2
+`, []string{"Hello World", "", "Paragraph 2"}, []string{})
+ test(`Hello World
+
+Trailer-Key: trailer-value`, []string{"Hello World", ""}, []string{"Trailer-Key: trailer-value"})
+ test(`Hello World
+
+Trailer-Key: trailer-value
+`, []string{"Hello World", ""}, []string{"Trailer-Key: trailer-value"})
+
+ test(`Hello World
+
+Paragraph 2
+
+Trailer-Key: trailer-value
+T2: V2
+Bug: 1234, chromium:5678
+`,
+ []string{"Hello World", "", "Paragraph 2", ""},
+ []string{"Trailer-Key: trailer-value", "T2: V2", "Bug: 1234, chromium:5678"})
+
+ test(`Trailer-Key: trailer-value`, []string{}, []string{"Trailer-Key: trailer-value"})
+}
+
+func TestJoinTrailers(t *testing.T) {
+ unittest.SmallTest(t)
+
+ test := func(body, trailers []string, expect string) {
+ actual := JoinTrailers(body, trailers)
+ require.Equal(t, expect, actual)
+ }
+
+ test([]string{}, []string{}, "")
+ test([]string{"Hello World"}, []string{}, "Hello World\n")
+ test([]string{"Hello World", ""}, []string{}, "Hello World\n")
+ test(
+ []string{"Hello World", "", "Paragraph 2", ""},
+ []string{},
+ `Hello World
+
+Paragraph 2
+`)
+ test([]string{}, []string{"Trailer-Key: trailer-value"}, "Trailer-Key: trailer-value")
+
+ test(
+ []string{"Hello World", "", "Paragraph 2", ""},
+ []string{"Trailer-Key: trailer-value"},
+ `Hello World
+
+Paragraph 2
+
+Trailer-Key: trailer-value`)
+}
+
+func TestAddTrailer(t *testing.T) {
+ unittest.SmallTest(t)
+
+ test := func(commitMsg, trailer, expect, expectErr string) {
+ actual, err := AddTrailer(commitMsg, trailer)
+ require.Equal(t, expect, actual)
+ if expectErr != "" {
+ require.NotNil(t, err)
+ require.Contains(t, err.Error(), expectErr)
+ }
+ }
+
+ test("", "", "", "\"\" is not a valid git trailer")
+ test("", "bogustrailer", "", "\"bogustrailer\" is not a valid git trailer")
+ test("", "Trailer-Key: trailer-value", "Trailer-Key: trailer-value", "")
+ test("Hello World", "Trailer-Key: trailer-value", `Hello World
+
+Trailer-Key: trailer-value`, "")
+
+ test(`Hello World
+
+Paragraph 2
+`,
+ "Trailer-Key: trailer-value",
+ `Hello World
+
+Paragraph 2
+
+Trailer-Key: trailer-value`, "")
+
+ test(`Hello World
+
+Paragraph 2
+
+K1: V1
+`,
+ "Trailer-Key: trailer-value",
+ `Hello World
+
+Paragraph 2
+
+K1: V1
+Trailer-Key: trailer-value`, "")
+}
