Use GitHub-style commit message for squash merge (#35987)

services/pull/pull.go

@@ -13,6 +13,7 @@ import (
 	"regexp"
 	"strings"
 	"time"
+	"unicode/utf8"
 
 	"code.gitea.io/gitea/models/db"
 	git_model "code.gitea.io/gitea/models/git"
@@ -838,51 +839,53 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
 	stringBuilder := strings.Builder{}
 
 	if !setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages {
+		// use PR's title and description as squash commit message
 		message := strings.TrimSpace(pr.Issue.Content)
 		stringBuilder.WriteString(message)
 		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
 				stringBuilder.WriteRune('\n')
 			}
 		}
-	}
-
+	} else {
+		// use PR's commit messages as squash commit message
 		// commits list is in reverse chronological order
-	first := true
+		maxMsgSize := setting.Repository.PullRequest.DefaultMergeMessageSize
 		for i := len(commits) - 1; i >= 0; i-- {
 			commit := commits[i]
-
-		if setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages {
-			maxSize := setting.Repository.PullRequest.DefaultMergeMessageSize
-			if maxSize < 0 || stringBuilder.Len() < maxSize {
-				var toWrite []byte
-				if first {
-					first = false
-					toWrite = []byte(strings.TrimPrefix(commit.CommitMessage, pr.Issue.Title))
-				} else {
-					toWrite = []byte(commit.CommitMessage)
+			msg := strings.TrimSpace(commit.CommitMessage)
+			if msg == "" {
+				continue
 			}
 
-				if len(toWrite) > maxSize-stringBuilder.Len() && maxSize > -1 {
-					toWrite = append(toWrite[:maxSize-stringBuilder.Len()], "..."...)
+			// 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, "")
 				}
-				if _, err := stringBuilder.Write(toWrite); err != nil {
-					log.Error("Unable to write commit message Error: %v", err)
-					return ""
-				}
-
-				if _, err := stringBuilder.WriteRune('\n'); err != nil {
-					log.Error("Unable to write commit message Error: %v", err)
-					return ""
+				stringBuilder.Reset()
+				stringBuilder.WriteString(tmp)
+				break
 			}
 		}
 	}
 
+	// collect co-authors
+	for _, commit := range commits {
 		authorString := commit.Author.String()
 		if uniqueAuthors.Add(authorString) && authorString != posterSig {
 			// Compare use account as well to avoid adding the same author multiple times
-			// times when email addresses are private or multiple emails are used.
+			// when email addresses are private or multiple emails are used.
 			commitUser, _ := user_model.GetUserByEmail(ctx, commit.Author.Email)
 			if commitUser == nil || commitUser.ID != pr.Issue.Poster.ID {
 				authors = append(authors, authorString)
@@ -890,12 +893,12 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
 		}
 	}
 
-	// Consider collecting the remaining authors
+	// collect the remaining authors
 	if limit >= 0 && setting.Repository.PullRequest.DefaultMergeMessageAllAuthors {
 		skip := limit
 		limit = 30
 		for {
-			commits, err := gitRepo.CommitsBetweenLimit(headCommit, mergeBase, limit, skip)
+			commits, err = gitRepo.CommitsBetweenLimit(headCommit, mergeBase, limit, skip)
 			if err != nil {
 				log.Error("Unable to get commits between: %s %s Error: %v", pr.HeadBranch, pr.MergeBase, err)
 				return ""
@@ -916,19 +919,15 @@ 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
+		stringBuilder.WriteString("---------\n\n")
+	}
+
 	for _, author := range authors {
-		if _, err := stringBuilder.WriteString("Co-authored-by: "); err != nil {
-			log.Error("Unable to write to string builder Error: %v", err)
-			return ""
-		}
-		if _, err := stringBuilder.WriteString(author); err != nil {
-			log.Error("Unable to write to string builder Error: %v", err)
-			return ""
-		}
-		if _, err := stringBuilder.WriteRune('\n'); err != nil {
-			log.Error("Unable to write to string builder Error: %v", err)
-			return ""
-		}
+		stringBuilder.WriteString("Co-authored-by: ")
+		stringBuilder.WriteString(author)
+		stringBuilder.WriteRune('\n')
 	}
 
 	return stringBuilder.String()

tests/integration/api_repo_file_helpers.go

@@ -19,6 +19,9 @@ import (
 
 type createFileInBranchOptions struct {
 	OldBranch, NewBranch string
+	CommitMessage        string
+	CommitterName        string
+	CommitterEmail       string
 }
 
 func testCreateFileInBranch(t *testing.T, user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) *api.FilesResponse {
@@ -29,7 +32,17 @@ func testCreateFileInBranch(t *testing.T, user *user_model.User, repo *repo_mode
 
 func createFileInBranch(user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) (*api.FilesResponse, error) {
 	ctx := context.TODO()
-	opts := &files_service.ChangeRepoFilesOptions{OldBranch: createOpts.OldBranch, NewBranch: createOpts.NewBranch}
+	opts := &files_service.ChangeRepoFilesOptions{
+		OldBranch: createOpts.OldBranch,
+		NewBranch: createOpts.NewBranch,
+		Message:   createOpts.CommitMessage,
+	}
+	if createOpts.CommitterName != "" || createOpts.CommitterEmail != "" {
+		opts.Committer = &files_service.IdentityOptions{
+			GitUserName:  createOpts.CommitterName,
+			GitUserEmail: createOpts.CommitterEmail,
+		}
+	}
 	for path, content := range files {
 		opts.Files = append(opts.Files, &files_service.ChangeRepoFile{
 			Operation:     "create",

tests/integration/pull_merge_test.go

@@ -33,6 +33,7 @@ import (
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/test"
 	"code.gitea.io/gitea/modules/translation"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/services/automerge"
 	"code.gitea.io/gitea/services/automergequeue"
 	pull_service "code.gitea.io/gitea/services/pull"
@@ -41,6 +42,7 @@ import (
 	files_service "code.gitea.io/gitea/services/repository/files"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 type MergeOptions struct {
@@ -1212,3 +1214,140 @@ func TestPullSquashMergeEmpty(t *testing.T) {
 		})
 	})
 }
+
+func TestPullSquashMessage(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
+		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+		user2Session := loginUser(t, user2.Name)
+
+		defer test.MockVariableValue(&setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages, true)()
+		defer test.MockVariableValue(&setting.Repository.PullRequest.DefaultMergeMessageSize, 80)()
+
+		repo, err := repo_service.CreateRepository(t.Context(), user2, user2, repo_service.CreateRepoOptions{
+			Name:          "squash-message-test",
+			Description:   "Test squash message",
+			AutoInit:      true,
+			Readme:        "Default",
+			DefaultBranch: "main",
+		})
+		require.NoError(t, err)
+
+		type commitInfo struct {
+			userName      string
+			commitMessage string
+		}
+
+		testCases := []struct {
+			name            string
+			commitInfos     []*commitInfo
+			expectedMessage string
+		}{
+			{
+				name: "Single-line messages",
+				commitInfos: []*commitInfo{
+					{
+						userName:      user2.Name,
+						commitMessage: "commit msg 1",
+					},
+					{
+						userName:      user2.Name,
+						commitMessage: "commit msg 2",
+					},
+				},
+				expectedMessage: `* commit msg 1
+
+* commit msg 2
+
+`,
+			},
+			{
+				name: "Multiple-line messages",
+				commitInfos: []*commitInfo{
+					{
+						userName: user2.Name,
+						commitMessage: `commit msg 1
+
+Commit description.`,
+					},
+					{
+						userName: user2.Name,
+						commitMessage: `commit msg 2
+
+- Detail 1
+- Detail 2`,
+					},
+				},
+				expectedMessage: `* commit msg 1
+
+Commit description.
+
+* commit msg 2
+
+- Detail 1
+- Detail 2
+
+`,
+			},
+			{
+				name: "Too long message",
+				commitInfos: []*commitInfo{
+					{
+						userName:      user2.Name,
+						commitMessage: `loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong message`,
+					},
+				},
+				expectedMessage: `* looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo...`,
+			},
+			{
+				name: "Test Co-authored-by",
+				commitInfos: []*commitInfo{
+					{
+						userName:      user2.Name,
+						commitMessage: "commit msg 1",
+					},
+					{
+						userName:      "user4",
+						commitMessage: "commit msg 2",
+					},
+				},
+				expectedMessage: `* commit msg 1
+
+* commit msg 2
+
+---------
+
+Co-authored-by: user4 <user4@example.com>
+`,
+			},
+		}
+
+		for tcNum, tc := range testCases {
+			t.Run(tc.name, func(t *testing.T) {
+				branchName := "test-branch-" + strconv.Itoa(tcNum)
+				for infoIdx, info := range tc.commitInfos {
+					createFileOpts := createFileInBranchOptions{
+						CommitMessage:  info.commitMessage,
+						CommitterName:  info.userName,
+						CommitterEmail: util.Iif(info.userName != "", info.userName+"@example.com", ""),
+						OldBranch:      util.Iif(infoIdx == 0, "main", branchName),
+						NewBranch:      branchName,
+					}
+					testCreateFileInBranch(t, user2, repo, createFileOpts, map[string]string{"dummy-file-" + strconv.Itoa(infoIdx): "dummy content"})
+				}
+				resp := testPullCreateDirectly(t, user2Session, createPullRequestOptions{
+					BaseRepoOwner: user2.Name,
+					BaseRepoName:  repo.Name,
+					BaseBranch:    repo.DefaultBranch,
+					HeadBranch:    branchName,
+					Title:         "Pull for " + branchName,
+				})
+				elems := strings.Split(test.RedirectURL(resp), "/")
+				pullIndex, err := strconv.ParseInt(elems[4], 10, 64)
+				assert.NoError(t, err)
+				pullRequest := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, Index: pullIndex})
+				squashMergeCommitMessage := pull_service.GetSquashMergeCommitMessages(t.Context(), pullRequest)
+				assert.Equal(t, tc.expectedMessage, squashMergeCommitMessage)
+			})
+		}
+	})
+}