From f5d0e1633dce903a4fa69533eb079a34c83b82e3 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Sun, 31 May 2026 12:12:36 +0200 Subject: [PATCH] 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 0d4a10ae9e..35f9823e2f 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 0648cd383d..ec3116ef61 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})