From f5d0e1633dce903a4fa69533eb079a34c83b82e3 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Sun, 31 May 2026 12:12:36 +0200 Subject: [PATCH 1/7] fix(pull): preserve squash message trailers and additional commit messages When a PR description already ends with git trailers (e.g. Issue: X, Signed-off-by:), the co-author separator line (---------) was still inserted before the Co-authored-by lines, breaking the trailer block. messageHasTrailers now skips the separator so co-authors are appended directly into the existing trailer block. In PR-description mode (PopulateSquashCommentWithCommitMessages=false), commit messages beyond the oldest were silently dropped. They are now appended as bullet points after the PR description, consistent with the commit-message mode format. The commit-message loop is extracted into formatSquashMergeCommitMessages. Callers that want to skip the oldest commit pass a trimmed slice (commits[:max(0, len(commits)-1)]) instead of a skipFirst bool flag. Co-Authored-By: Nicolas Co-Authored-By: Claude Sonnet 4.6 --- services/pull/pull.go | 74 ++++++++++++++++++++++---------------- services/pull/pull_test.go | 20 +++++++++++ 2 files changed, 63 insertions(+), 31 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 0d4a10ae9ef14..35f9823e2ff76 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -831,45 +831,27 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ authors := make([]string, 0, len(commits)) stringBuilder := strings.Builder{} + messageHasTrailers := false if !setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages { // use PR's title and description as squash commit message message := strings.TrimSpace(pr.Issue.Content) + messageHasTrailers = commitMessageTrailersPattern.MatchString(message) stringBuilder.WriteString(message) - if stringBuilder.Len() > 0 { + additionalCommitMessages := formatSquashMergeCommitMessages(commits[:max(0, len(commits)-1)]) + if additionalCommitMessages != "" { + if stringBuilder.Len() > 0 { + stringBuilder.WriteString("\n\n") + } + stringBuilder.WriteString(additionalCommitMessages) + } else if stringBuilder.Len() > 0 { stringBuilder.WriteRune('\n') - if !commitMessageTrailersPattern.MatchString(message) { - // TODO: this trailer check doesn't work with the separator line added below for the co-authors + if !messageHasTrailers { stringBuilder.WriteRune('\n') } } } else { // use PR's commit messages as squash commit message - // commits list is in reverse chronological order - maxMsgSize := setting.Repository.PullRequest.DefaultMergeMessageSize - for _, commit := range slices.Backward(commits) { - msg := strings.TrimSpace(commit.MessageUTF8()) - if msg == "" { - continue - } - - // This format follows GitHub's squash commit message style, - // even if there are other "* " in the commit message body, they are written as-is. - // Maybe, ideally, we should indent those lines too. - _, _ = fmt.Fprintf(&stringBuilder, "* %s\n\n", msg) - if maxMsgSize > 0 && stringBuilder.Len() >= maxMsgSize { - tmp := stringBuilder.String() - wasValidUtf8 := utf8.ValidString(tmp) - tmp = tmp[:maxMsgSize] + "..." - if wasValidUtf8 { - // If the message was valid UTF-8 before truncation, ensure it remains valid after truncation - // For non-utf8 messages, we can't do much about it, end users should use utf-8 as much as possible - tmp = strings.ToValidUTF8(tmp, "") - } - stringBuilder.Reset() - stringBuilder.WriteString(tmp) - break - } - } + stringBuilder.WriteString(formatSquashMergeCommitMessages(commits)) } // collect co-authors @@ -911,8 +893,7 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ } } - if stringBuilder.Len() > 0 && len(authors) > 0 { - // TODO: this separator line doesn't work with the trailer check (commitMessageTrailersPattern) above + if stringBuilder.Len() > 0 && len(authors) > 0 && !messageHasTrailers { stringBuilder.WriteString("---------\n\n") } @@ -925,6 +906,37 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ return stringBuilder.String() } +func formatSquashMergeCommitMessages(commits []*git.Commit) string { + // commits list is in reverse chronological order + maxMsgSize := setting.Repository.PullRequest.DefaultMergeMessageSize + stringBuilder := strings.Builder{} + for _, commit := range slices.Backward(commits) { + msg := strings.TrimSpace(commit.MessageUTF8()) + if msg == "" { + continue + } + + // This format follows GitHub's squash commit message style, + // even if there are other "* " in the commit message body, they are written as-is. + // Maybe, ideally, we should indent those lines too. + _, _ = fmt.Fprintf(&stringBuilder, "* %s\n\n", msg) + if maxMsgSize > 0 && stringBuilder.Len() >= maxMsgSize { + tmp := stringBuilder.String() + wasValidUtf8 := utf8.ValidString(tmp) + tmp = tmp[:maxMsgSize] + "..." + if wasValidUtf8 { + // If the message was valid UTF-8 before truncation, ensure it remains valid after truncation + // For non-utf8 messages, we can't do much about it, end users should use utf-8 as much as possible + tmp = strings.ToValidUTF8(tmp, "") + } + stringBuilder.Reset() + stringBuilder.WriteString(tmp) + break + } + } + return stringBuilder.String() +} + // GetIssuesAllCommitStatus returns a map of issue ID to a list of all statuses for the most recent commit as well as a map of issue ID to only the commit's latest status func GetIssuesAllCommitStatus(ctx context.Context, issues issues_model.IssueList) (map[int64][]*git_model.CommitStatus, map[int64]*git_model.CommitStatus, error) { if err := issues.LoadPullRequests(ctx); err != nil { diff --git a/services/pull/pull_test.go b/services/pull/pull_test.go index 0648cd383d5cc..ec3116ef61d82 100644 --- a/services/pull/pull_test.go +++ b/services/pull/pull_test.go @@ -11,7 +11,9 @@ import ( repo_model "gitea.dev/models/repo" "gitea.dev/models/unit" "gitea.dev/models/unittest" + "gitea.dev/modules/git" "gitea.dev/modules/gitrepo" + "gitea.dev/modules/setting" "github.com/stretchr/testify/assert" ) @@ -35,6 +37,24 @@ func TestPullRequest_CommitMessageTrailersPattern(t *testing.T) { assert.True(t, commitMessageTrailersPattern.MatchString("Folded value.\n\nFolded-trailer: This is\n a folded\n trailer value\nOther-Trailer: Value")) } +func TestPullRequest_FormatSquashMergeCommitMessages(t *testing.T) { + oldest := &git.Commit{CommitMessage: git.CommitMessage{MessageRaw: "commit msg 1"}} + newest := &git.Commit{CommitMessage: git.CommitMessage{MessageRaw: "commit msg 2\n\nCommit description."}} + + defer func(old int) { setting.Repository.PullRequest.DefaultMergeMessageSize = old }( + setting.Repository.PullRequest.DefaultMergeMessageSize, + ) + setting.Repository.PullRequest.DefaultMergeMessageSize = 0 + + // all commits + assert.Equal(t, "* commit msg 1\n\n* commit msg 2\n\nCommit description.\n\n", + formatSquashMergeCommitMessages([]*git.Commit{newest, oldest})) + + // PR-description mode: pass all-but-oldest so the oldest is not duplicated + assert.Equal(t, "* commit msg 2\n\nCommit description.\n\n", + formatSquashMergeCommitMessages([]*git.Commit{newest})) +} + func TestPullRequest_GetDefaultMergeMessage_InternalTracker(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) From f99aa56c4ff491913787e016a9c8f4c98376a093 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Sun, 31 May 2026 12:57:52 +0200 Subject: [PATCH 2/7] fix --- services/pull/pull.go | 22 ++++++++++------------ services/pull/pull_test.go | 6 ++---- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 35f9823e2ff76..8a2410d87534f 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -12,7 +12,6 @@ import ( "slices" "strings" "time" - "unicode/utf8" "gitea.dev/models/db" git_model "gitea.dev/models/git" @@ -831,11 +830,13 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ authors := make([]string, 0, len(commits)) stringBuilder := strings.Builder{} - messageHasTrailers := false + // trailerBlockAtEnd tracks whether the message currently ends with a Git trailer block. + // When true, we skip the "---------" separator so Co-authored-by lines stay contiguous with it. + trailerBlockAtEnd := false if !setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages { // use PR's title and description as squash commit message message := strings.TrimSpace(pr.Issue.Content) - messageHasTrailers = commitMessageTrailersPattern.MatchString(message) + messageHasTrailers := commitMessageTrailersPattern.MatchString(message) stringBuilder.WriteString(message) additionalCommitMessages := formatSquashMergeCommitMessages(commits[:max(0, len(commits)-1)]) if additionalCommitMessages != "" { @@ -843,11 +844,13 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ stringBuilder.WriteString("\n\n") } stringBuilder.WriteString(additionalCommitMessages) + // appended bullets push the PR-description trailers (if any) out of the trailer-block position } else if stringBuilder.Len() > 0 { stringBuilder.WriteRune('\n') if !messageHasTrailers { stringBuilder.WriteRune('\n') } + trailerBlockAtEnd = messageHasTrailers } } else { // use PR's commit messages as squash commit message @@ -893,7 +896,7 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ } } - if stringBuilder.Len() > 0 && len(authors) > 0 && !messageHasTrailers { + if stringBuilder.Len() > 0 && len(authors) > 0 && !trailerBlockAtEnd { stringBuilder.WriteString("---------\n\n") } @@ -921,14 +924,9 @@ func formatSquashMergeCommitMessages(commits []*git.Commit) string { // Maybe, ideally, we should indent those lines too. _, _ = fmt.Fprintf(&stringBuilder, "* %s\n\n", msg) if maxMsgSize > 0 && stringBuilder.Len() >= maxMsgSize { - tmp := stringBuilder.String() - wasValidUtf8 := utf8.ValidString(tmp) - tmp = tmp[:maxMsgSize] + "..." - if wasValidUtf8 { - // If the message was valid UTF-8 before truncation, ensure it remains valid after truncation - // For non-utf8 messages, we can't do much about it, end users should use utf-8 as much as possible - tmp = strings.ToValidUTF8(tmp, "") - } + // MessageUTF8 already guarantees valid UTF-8, but the byte-offset truncation + // can split a multi-byte rune, so trim any resulting invalid trailing bytes. + tmp := strings.ToValidUTF8(stringBuilder.String()[:maxMsgSize], "") + "..." stringBuilder.Reset() stringBuilder.WriteString(tmp) break diff --git a/services/pull/pull_test.go b/services/pull/pull_test.go index ec3116ef61d82..16ced40631e5b 100644 --- a/services/pull/pull_test.go +++ b/services/pull/pull_test.go @@ -14,6 +14,7 @@ import ( "gitea.dev/modules/git" "gitea.dev/modules/gitrepo" "gitea.dev/modules/setting" + "gitea.dev/modules/test" "github.com/stretchr/testify/assert" ) @@ -41,10 +42,7 @@ func TestPullRequest_FormatSquashMergeCommitMessages(t *testing.T) { oldest := &git.Commit{CommitMessage: git.CommitMessage{MessageRaw: "commit msg 1"}} newest := &git.Commit{CommitMessage: git.CommitMessage{MessageRaw: "commit msg 2\n\nCommit description."}} - defer func(old int) { setting.Repository.PullRequest.DefaultMergeMessageSize = old }( - setting.Repository.PullRequest.DefaultMergeMessageSize, - ) - setting.Repository.PullRequest.DefaultMergeMessageSize = 0 + defer test.MockVariableValue(&setting.Repository.PullRequest.DefaultMergeMessageSize, 0)() // all commits assert.Equal(t, "* commit msg 1\n\n* commit msg 2\n\nCommit description.\n\n", From 9392da4d64451c155d2f21fa2524410fbca4eb54 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 31 May 2026 19:17:57 +0800 Subject: [PATCH 3/7] fix fmt --- services/pull/pull.go | 33 +++++++++++++++++++++++---------- services/pull/pull_test.go | 10 ++++++++++ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 8a2410d87534f..5b260a154207c 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -4,6 +4,7 @@ package pull import ( + "bytes" "context" "errors" "fmt" @@ -12,6 +13,7 @@ import ( "slices" "strings" "time" + "unicode/utf8" "gitea.dev/models/db" git_model "gitea.dev/models/git" @@ -910,9 +912,9 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ } func formatSquashMergeCommitMessages(commits []*git.Commit) string { - // commits list is in reverse chronological order maxMsgSize := setting.Repository.PullRequest.DefaultMergeMessageSize - stringBuilder := strings.Builder{} + sb := &bytes.Buffer{} + // commits list is in reverse chronological order for _, commit := range slices.Backward(commits) { msg := strings.TrimSpace(commit.MessageUTF8()) if msg == "" { @@ -922,17 +924,28 @@ func formatSquashMergeCommitMessages(commits []*git.Commit) string { // This format follows GitHub's squash commit message style, // even if there are other "* " in the commit message body, they are written as-is. // Maybe, ideally, we should indent those lines too. - _, _ = fmt.Fprintf(&stringBuilder, "* %s\n\n", msg) - if maxMsgSize > 0 && stringBuilder.Len() >= maxMsgSize { - // MessageUTF8 already guarantees valid UTF-8, but the byte-offset truncation - // can split a multi-byte rune, so trim any resulting invalid trailing bytes. - tmp := strings.ToValidUTF8(stringBuilder.String()[:maxMsgSize], "") + "..." - stringBuilder.Reset() - stringBuilder.WriteString(tmp) + _, _ = fmt.Fprintf(sb, "* %s\n\n", msg) + if maxMsgSize > 0 && sb.Len() >= maxMsgSize { break } } - return stringBuilder.String() + + buf := sb.Bytes() + buf = bytes.TrimSpace(buf) + if maxMsgSize > 0 && len(buf) > maxMsgSize { + buf = buf[:maxMsgSize] + for { + r, sz := utf8.DecodeLastRune(buf) + if r == utf8.RuneError && sz == 1 { + buf = buf[:len(buf)-1] + continue + } + break + } + buf = append(buf, '.', '.', '.') + } + buf = append(buf, '\n', '\n') + return util.UnsafeBytesToString(buf) } // GetIssuesAllCommitStatus returns a map of issue ID to a list of all statuses for the most recent commit as well as a map of issue ID to only the commit's latest status diff --git a/services/pull/pull_test.go b/services/pull/pull_test.go index 16ced40631e5b..16e4e80252398 100644 --- a/services/pull/pull_test.go +++ b/services/pull/pull_test.go @@ -51,6 +51,16 @@ func TestPullRequest_FormatSquashMergeCommitMessages(t *testing.T) { // PR-description mode: pass all-but-oldest so the oldest is not duplicated assert.Equal(t, "* commit msg 2\n\nCommit description.\n\n", formatSquashMergeCommitMessages([]*git.Commit{newest})) + + utf8Msg := &git.Commit{CommitMessage: git.CommitMessage{MessageRaw: "🌞"}} + setting.Repository.PullRequest.DefaultMergeMessageSize = 3 + assert.Equal(t, "* ...\n\n", formatSquashMergeCommitMessages([]*git.Commit{utf8Msg})) + setting.Repository.PullRequest.DefaultMergeMessageSize = 4 + assert.Equal(t, "* ...\n\n", formatSquashMergeCommitMessages([]*git.Commit{utf8Msg})) + setting.Repository.PullRequest.DefaultMergeMessageSize = 5 + assert.Equal(t, "* ...\n\n", formatSquashMergeCommitMessages([]*git.Commit{utf8Msg})) + setting.Repository.PullRequest.DefaultMergeMessageSize = 6 + assert.Equal(t, "* 🌞\n\n", formatSquashMergeCommitMessages([]*git.Commit{utf8Msg})) } func TestPullRequest_GetDefaultMergeMessage_InternalTracker(t *testing.T) { From da91800c1b332eb1109e248bac78259d2b382cb6 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 31 May 2026 19:25:43 +0800 Subject: [PATCH 4/7] add fixme comment --- services/pull/pull.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 5b260a154207c..e76cb2063b4fa 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -840,11 +840,13 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ message := strings.TrimSpace(pr.Issue.Content) messageHasTrailers := commitMessageTrailersPattern.MatchString(message) stringBuilder.WriteString(message) + // FIXME: is it right? why `len(commits)-1)`? additionalCommitMessages := formatSquashMergeCommitMessages(commits[:max(0, len(commits)-1)]) if additionalCommitMessages != "" { if stringBuilder.Len() > 0 { stringBuilder.WriteString("\n\n") } + // FIXME: is it right? what if `pr.Issue.Content` contains trailers? stringBuilder.WriteString(additionalCommitMessages) // appended bullets push the PR-description trailers (if any) out of the trailer-block position } else if stringBuilder.Len() > 0 { @@ -930,8 +932,7 @@ func formatSquashMergeCommitMessages(commits []*git.Commit) string { } } - buf := sb.Bytes() - buf = bytes.TrimSpace(buf) + buf := bytes.TrimSpace(sb.Bytes()) if maxMsgSize > 0 && len(buf) > maxMsgSize { buf = buf[:maxMsgSize] for { From c8e0ebe74db4420152a04f303706324ae461f81c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 15 Jun 2026 15:24:01 +0800 Subject: [PATCH 5/7] fix --- services/pull/pull.go | 90 +++++++++++++++++--------------------- services/pull/pull_test.go | 22 ++++++++++ 2 files changed, 62 insertions(+), 50 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 8bbbcc2bb1f37..cd641c54c7b35 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -820,42 +820,43 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ return "" } - posterSig := pr.Issue.Poster.NewGitSig().String() + mergeMessage := strings.TrimSpace(pr.Issue.Content) // use PR's title and description as squash commit message + if setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages { + mergeMessage = formatSquashMergeCommitMessages(limitedCommits) // use PR's commit messages as squash commit message + } + coAuthors := collectSquashMergeCommitCoAuthors(ctx, gitRepo, pr, headCommitRef, mergeBaseRef, limit, limitedCommits) + return buildSquashMergeCommitMessages(mergeMessage, coAuthors) +} +func buildSquashMergeCommitMessages(mergeMessage string, coAuthors []string) string { + if len(coAuthors) == 0 { + return mergeMessage + } + + msgContent, msgSep, msgTrailer := git.CommitMessageSplitTrailer(mergeMessage) + if msgTrailer == "" { + msgSep = "\n---------\n" + } + var sb strings.Builder + sb.WriteString(msgContent) + sb.WriteString(msgSep) + if msgTrailer = strings.TrimSpace(msgTrailer); msgTrailer != "" { + sb.WriteString(msgTrailer) + sb.WriteRune('\n') + } + for _, author := range coAuthors { + sb.WriteString(git.CoAuthoredByTrailer + ": ") + sb.WriteString(author) + sb.WriteRune('\n') + } + return sb.String() +} + +func collectSquashMergeCommitCoAuthors(ctx context.Context, gitRepo *git.Repository, pr *issues_model.PullRequest, headCommitRef, mergeBaseRef git.RefName, limitFirst int, limitedCommits []*git.Commit) []string { + posterSig := pr.Issue.Poster.NewGitSig().String() uniqueAuthors := make(container.Set[string]) authors := make([]string, 0, len(limitedCommits)) - stringBuilder := strings.Builder{} - - // trailerBlockAtEnd tracks whether the message currently ends with a Git trailer block. - // When true, we skip the "---------" separator so Co-authored-by lines stay contiguous with it. - trailerBlockAtEnd := false - if !setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages { - // use PR's title and description as squash commit message - message := strings.TrimSpace(pr.Issue.Content) - messageHasTrailers := commitMessageTrailersPattern.MatchString(message) - stringBuilder.WriteString(message) - // FIXME: is it right? why `len(commits)-1)`? - additionalCommitMessages := formatSquashMergeCommitMessages(commits[:max(0, len(commits)-1)]) - if additionalCommitMessages != "" { - if stringBuilder.Len() > 0 { - stringBuilder.WriteString("\n\n") - } - // FIXME: is it right? what if `pr.Issue.Content` contains trailers? - stringBuilder.WriteString(additionalCommitMessages) - // appended bullets push the PR-description trailers (if any) out of the trailer-block position - } else if stringBuilder.Len() > 0 { - stringBuilder.WriteRune('\n') - if !messageHasTrailers { - stringBuilder.WriteRune('\n') - } - trailerBlockAtEnd = messageHasTrailers - } - } else { - // use PR's commit messages as squash commit message - stringBuilder.WriteString(formatSquashMergeCommitMessages(commits)) - } - // collect co-authors for _, commit := range limitedCommits { authorString := commit.Author.String() if uniqueAuthors.Add(authorString) && authorString != posterSig { @@ -869,14 +870,14 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ } // collect the remaining authors - if limit >= 0 && setting.Repository.PullRequest.DefaultMergeMessageAllAuthors { - skip := limit - limit = 30 + if limitFirst >= 0 && setting.Repository.PullRequest.DefaultMergeMessageAllAuthors { + skip := limitFirst + batchLimit := 30 for { - commits, err := gitRepo.CommitsBetween(headCommitRef, mergeBaseRef, limit, skip) + commits, err := gitRepo.CommitsBetween(headCommitRef, mergeBaseRef, batchLimit, skip) if err != nil { log.Error("Unable to get commits between: %s %s Error: %v", pr.HeadBranch, pr.MergeBase, err) - return "" + return authors } if len(commits) == 0 { break @@ -890,21 +891,10 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ } } } - skip += limit + skip += batchLimit } } - - if stringBuilder.Len() > 0 && len(authors) > 0 && !trailerBlockAtEnd { - stringBuilder.WriteString("---------\n\n") - } - - for _, author := range authors { - stringBuilder.WriteString(git.CoAuthoredByTrailer + ": ") - stringBuilder.WriteString(author) - stringBuilder.WriteRune('\n') - } - - return stringBuilder.String() + return authors } func formatSquashMergeCommitMessages(commits []*git.Commit) string { diff --git a/services/pull/pull_test.go b/services/pull/pull_test.go index 16e4e80252398..7b26f1ebaae49 100644 --- a/services/pull/pull_test.go +++ b/services/pull/pull_test.go @@ -116,3 +116,25 @@ func TestPullRequest_GetDefaultMergeMessage_ExternalTracker(t *testing.T) { assert.Equal(t, "Merge pull request 'issue3' (#3) from user2/repo2:branch2 into master", mergeMessage) } + +func TestBuildSquashMergeCommitMessages(t *testing.T) { + cases := []struct { + msg string + coAuthors []string + expected string + }{ + {"title", nil, "title"}, + {"title", []string{"the-user"}, "title\n---------\nCo-authored-by: the-user\n"}, + {"title\n\nKey: val", []string{"the-user"}, "title\n\nKey: val\nCo-authored-by: the-user\n"}, + {"title\n\n----\nKey: val", []string{"the-user"}, "title\n\n----\nKey: val\nCo-authored-by: the-user\n"}, + + {"title\n\nbody", nil, "title\n\nbody"}, + {"title\n\nbody", []string{"the-user"}, "title\n\nbody\n---------\nCo-authored-by: the-user\n"}, + {"title\n\nbody\n\nKey: val", []string{"the-user"}, "title\n\nbody\n\nKey: val\nCo-authored-by: the-user\n"}, + {"title\n\nbody\n\n----\nKey: val", []string{"the-user"}, "title\n\nbody\n\n----\nKey: val\nCo-authored-by: the-user\n"}, + } + for _, c := range cases { + msg := buildSquashMergeCommitMessages(c.msg, c.coAuthors) + assert.Equal(t, c.expected, msg, "msg: %s", c.msg) + } +} From c7474fdb73e6ff829fcabb40aad6c27263795e12 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 15 Jun 2026 15:38:03 +0800 Subject: [PATCH 6/7] add more edge case for git commit message trailer --- modules/git/commit_message.go | 2 +- modules/git/commit_message_test.go | 1 + services/pull/pull_test.go | 9 ++------- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/modules/git/commit_message.go b/modules/git/commit_message.go index 8fd3601f0d09a..e300a7f857a55 100644 --- a/modules/git/commit_message.go +++ b/modules/git/commit_message.go @@ -70,7 +70,7 @@ func (c *CommitMessage) MessageTrailer() CommitMessageTrailerValues { var commitMessageTrailerSplit = sync.OnceValue(func() *regexp.Regexp { // the sep is either something like "\n---\n" or "\n\n" in the body, or at the start of the body like "---\n" - return regexp.MustCompile(`(?s)^(?P.*?)(?P^|^\n|^-{3,}\n|\n-{3,}\n|\n\n)(?P(?:[A-Za-z0-9][-A-Za-z0-9]*:[^\n]*\n?)*)$`) + return regexp.MustCompile(`(?s)^(?P.*?)(?P^|^\n|^-{3,}\n|\n-{3,}\n|\n\n)(?P(?:[A-Za-z0-9][-A-Za-z0-9]*:[^\n]*\n?)*\n*)$`) }) func CommitMessageSplitTrailer(s string) (content, sep, trailer string) { diff --git a/modules/git/commit_message_test.go b/modules/git/commit_message_test.go index 049f1c03f786a..f5893447004f2 100644 --- a/modules/git/commit_message_test.go +++ b/modules/git/commit_message_test.go @@ -26,6 +26,7 @@ func TestCommitMessageTrailer(t *testing.T) { {"a", "a", "", ""}, {"a\n\nk", "a\n\nk", "", ""}, {"a\n\nk:v", "a", "\n\n", "k:v"}, + {"a\n\nk:v\n\n", "a", "\n\n", "k:v\n\n"}, {"a\n--\nk:v", "a\n--\nk:v", "", ""}, {"a\n---\nk:v", "a", "\n---\n", "k:v"}, diff --git a/services/pull/pull_test.go b/services/pull/pull_test.go index 7b26f1ebaae49..71f5aa156e9de 100644 --- a/services/pull/pull_test.go +++ b/services/pull/pull_test.go @@ -44,13 +44,7 @@ func TestPullRequest_FormatSquashMergeCommitMessages(t *testing.T) { defer test.MockVariableValue(&setting.Repository.PullRequest.DefaultMergeMessageSize, 0)() - // all commits - assert.Equal(t, "* commit msg 1\n\n* commit msg 2\n\nCommit description.\n\n", - formatSquashMergeCommitMessages([]*git.Commit{newest, oldest})) - - // PR-description mode: pass all-but-oldest so the oldest is not duplicated - assert.Equal(t, "* commit msg 2\n\nCommit description.\n\n", - formatSquashMergeCommitMessages([]*git.Commit{newest})) + assert.Equal(t, "* commit msg 1\n\n* commit msg 2\n\nCommit description.\n\n", formatSquashMergeCommitMessages([]*git.Commit{newest, oldest})) utf8Msg := &git.Commit{CommitMessage: git.CommitMessage{MessageRaw: "🌞"}} setting.Repository.PullRequest.DefaultMergeMessageSize = 3 @@ -127,6 +121,7 @@ func TestBuildSquashMergeCommitMessages(t *testing.T) { {"title", []string{"the-user"}, "title\n---------\nCo-authored-by: the-user\n"}, {"title\n\nKey: val", []string{"the-user"}, "title\n\nKey: val\nCo-authored-by: the-user\n"}, {"title\n\n----\nKey: val", []string{"the-user"}, "title\n\n----\nKey: val\nCo-authored-by: the-user\n"}, + {"title\n\n----\nKey: val\n\n", []string{"the-user"}, "title\n\n----\nKey: val\nCo-authored-by: the-user\n"}, {"title\n\nbody", nil, "title\n\nbody"}, {"title\n\nbody", []string{"the-user"}, "title\n\nbody\n---------\nCo-authored-by: the-user\n"}, From 314fb5db4509c5c4391f6ed1652141a39cdfb2fb Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 15 Jun 2026 18:32:33 +0800 Subject: [PATCH 7/7] fine tune --- modules/git/commit_message.go | 4 +++- modules/git/commit_message_test.go | 1 + services/pull/pull.go | 9 ++++----- services/pull/pull_test.go | 22 +++------------------- tests/integration/pull_merge_test.go | 2 +- 5 files changed, 12 insertions(+), 26 deletions(-) diff --git a/modules/git/commit_message.go b/modules/git/commit_message.go index e300a7f857a55..0729609d87e4f 100644 --- a/modules/git/commit_message.go +++ b/modules/git/commit_message.go @@ -70,9 +70,11 @@ func (c *CommitMessage) MessageTrailer() CommitMessageTrailerValues { var commitMessageTrailerSplit = sync.OnceValue(func() *regexp.Regexp { // the sep is either something like "\n---\n" or "\n\n" in the body, or at the start of the body like "---\n" - return regexp.MustCompile(`(?s)^(?P.*?)(?P^|^\n|^-{3,}\n|\n-{3,}\n|\n\n)(?P(?:[A-Za-z0-9][-A-Za-z0-9]*:[^\n]*\n?)*\n*)$`) + return regexp.MustCompile(`(?s)^(?P.*?)(?P^|^\n|^-{3,}\n+|\n-{3,}\n+|\n\n)(?P(?:[A-Za-z0-9][-A-Za-z0-9]*:[^\n]*\n?)*\n*)$`) }) +// CommitMessageSplitTrailer tries to split the message by the trailer separator +// content + sep + trailer will reconstruct the original message func CommitMessageSplitTrailer(s string) (content, sep, trailer string) { s = util.NormalizeStringEOL(s) re := commitMessageTrailerSplit() diff --git a/modules/git/commit_message_test.go b/modules/git/commit_message_test.go index f5893447004f2..1d13f81803337 100644 --- a/modules/git/commit_message_test.go +++ b/modules/git/commit_message_test.go @@ -29,6 +29,7 @@ func TestCommitMessageTrailer(t *testing.T) { {"a\n\nk:v\n\n", "a", "\n\n", "k:v\n\n"}, {"a\n--\nk:v", "a\n--\nk:v", "", ""}, {"a\n---\nk:v", "a", "\n---\n", "k:v"}, + {"a\n\n---\n\nk:v", "a\n", "\n---\n\n", "k:v"}, {"k: v", "", "", "k: v"}, {"\nk:v", "", "\n", "k:v"}, diff --git a/services/pull/pull.go b/services/pull/pull.go index cd641c54c7b35..13f8ffa8df65b 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -9,10 +9,10 @@ import ( "errors" "fmt" "io" - "regexp" "slices" "strings" "time" + "unicode" "unicode/utf8" "gitea.dev/models/db" @@ -768,8 +768,6 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re return errors.Join(errs...) } -var commitMessageTrailersPattern = regexp.MustCompile(`(?:^|\n\n)(?:[\w-]+[ \t]*:[^\n]+\n*(?:[ \t]+[^\n]+\n*)*)+$`) - // GetSquashMergeCommitMessages returns the commit messages between head and merge base (if there is one) func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequest) string { if err := pr.LoadIssue(ctx); err != nil { @@ -834,8 +832,9 @@ func buildSquashMergeCommitMessages(mergeMessage string, coAuthors []string) str } msgContent, msgSep, msgTrailer := git.CommitMessageSplitTrailer(mergeMessage) - if msgTrailer == "" { - msgSep = "\n---------\n" + if (msgSep == "" || msgSep == "\n\n") && msgTrailer == "" { + msgContent = strings.TrimRightFunc(msgContent, unicode.IsSpace) + msgSep = "\n\n---------\n\n" } var sb strings.Builder sb.WriteString(msgContent) diff --git a/services/pull/pull_test.go b/services/pull/pull_test.go index 71f5aa156e9de..554c9ed577513 100644 --- a/services/pull/pull_test.go +++ b/services/pull/pull_test.go @@ -21,23 +21,6 @@ import ( // TODO TestPullRequest_PushToBaseRepo -func TestPullRequest_CommitMessageTrailersPattern(t *testing.T) { - // Not a valid trailer section - assert.False(t, commitMessageTrailersPattern.MatchString("")) - assert.False(t, commitMessageTrailersPattern.MatchString("No trailer.")) - assert.False(t, commitMessageTrailersPattern.MatchString("Signed-off-by: Bob \nNot a trailer due to following text.")) - assert.False(t, commitMessageTrailersPattern.MatchString("Message body not correctly separated from trailer section by empty line.\nSigned-off-by: Bob ")) - // Valid trailer section - assert.True(t, commitMessageTrailersPattern.MatchString("Signed-off-by: Bob ")) - assert.True(t, commitMessageTrailersPattern.MatchString("Signed-off-by: Bob \nOther-Trailer: Value")) - assert.True(t, commitMessageTrailersPattern.MatchString("Message body correctly separated from trailer section by empty line.\n\nSigned-off-by: Bob ")) - assert.True(t, commitMessageTrailersPattern.MatchString("Multiple trailers.\n\nSigned-off-by: Bob \nOther-Trailer: Value")) - assert.True(t, commitMessageTrailersPattern.MatchString("Newline after trailer section.\n\nSigned-off-by: Bob \n")) - assert.True(t, commitMessageTrailersPattern.MatchString("No space after colon is accepted.\n\nSigned-off-by:Bob ")) - assert.True(t, commitMessageTrailersPattern.MatchString("Additional whitespace is accepted.\n\nSigned-off-by \t : \tBob ")) - assert.True(t, commitMessageTrailersPattern.MatchString("Folded value.\n\nFolded-trailer: This is\n a folded\n trailer value\nOther-Trailer: Value")) -} - func TestPullRequest_FormatSquashMergeCommitMessages(t *testing.T) { oldest := &git.Commit{CommitMessage: git.CommitMessage{MessageRaw: "commit msg 1"}} newest := &git.Commit{CommitMessage: git.CommitMessage{MessageRaw: "commit msg 2\n\nCommit description."}} @@ -118,13 +101,14 @@ func TestBuildSquashMergeCommitMessages(t *testing.T) { expected string }{ {"title", nil, "title"}, - {"title", []string{"the-user"}, "title\n---------\nCo-authored-by: the-user\n"}, + {"title", []string{"the-user"}, "title\n\n---------\n\nCo-authored-by: the-user\n"}, + {"title\n\n", []string{"the-user"}, "title\n\n---------\n\nCo-authored-by: the-user\n"}, {"title\n\nKey: val", []string{"the-user"}, "title\n\nKey: val\nCo-authored-by: the-user\n"}, {"title\n\n----\nKey: val", []string{"the-user"}, "title\n\n----\nKey: val\nCo-authored-by: the-user\n"}, {"title\n\n----\nKey: val\n\n", []string{"the-user"}, "title\n\n----\nKey: val\nCo-authored-by: the-user\n"}, {"title\n\nbody", nil, "title\n\nbody"}, - {"title\n\nbody", []string{"the-user"}, "title\n\nbody\n---------\nCo-authored-by: the-user\n"}, + {"title\n\nbody", []string{"the-user"}, "title\n\nbody\n\n---------\n\nCo-authored-by: the-user\n"}, {"title\n\nbody\n\nKey: val", []string{"the-user"}, "title\n\nbody\n\nKey: val\nCo-authored-by: the-user\n"}, {"title\n\nbody\n\n----\nKey: val", []string{"the-user"}, "title\n\nbody\n\n----\nKey: val\nCo-authored-by: the-user\n"}, } diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 8469057620ef9..ae1f8a34909d6 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -1272,7 +1272,7 @@ Commit description. commitMessage: `loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong message`, }, }, - expectedMessage: `* looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo...`, + expectedMessage: "* looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo...\n\n", }, { name: "Test Co-authored-by",