fix(pull): preserve squash message trailers and additional commit messages

services/pull/pull.go

@@ -4,6 +4,7 @@
 package pull
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -831,45 +832,33 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
 	authors := make([]string, 0, len(commits))
 	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)
-		if stringBuilder.Len() > 0 {
+		// 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 !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')
 			}
+			trailerBlockAtEnd = messageHasTrailers
 		}
 	} 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 +900,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 && !trailerBlockAtEnd {
 		stringBuilder.WriteString("---------\n\n")
 	}
 
@@ -925,6 +913,42 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
 	return stringBuilder.String()
 }
 
+func formatSquashMergeCommitMessages(commits []*git.Commit) string {
+	maxMsgSize := setting.Repository.PullRequest.DefaultMergeMessageSize
+	sb := &bytes.Buffer{}
+	// commits list is in reverse chronological order
+	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(sb, "* %s\n\n", msg)
+		if maxMsgSize > 0 && sb.Len() >= maxMsgSize {
+			break
+		}
+	}
+
+	buf := bytes.TrimSpace(sb.Bytes())
+	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
 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 {

services/pull/pull_test.go

@@ -11,7 +11,10 @@ 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"
+	"gitea.dev/modules/test"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -35,6 +38,31 @@ 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 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}))
+
+	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) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2})