diff --git a/services/pull/pull.go b/services/pull/pull.go index a8468b5bf3446..6f0318ea49c77 100644 --- a/services/pull/pull.go +++ b/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') } } - } - - // commits list is in reverse chronological order - first := true - 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) - } - - if len(toWrite) > maxSize-stringBuilder.Len() && maxSize > -1 { - toWrite = append(toWrite[:maxSize-stringBuilder.Len()], "..."...) - } - if _, err := stringBuilder.Write(toWrite); err != nil { - log.Error("Unable to write commit message Error: %v", err) - return "" - } + } else { + // use PR's commit messages as squash commit message + // commits list is in reverse chronological order + maxMsgSize := setting.Repository.PullRequest.DefaultMergeMessageSize + for i := len(commits) - 1; i >= 0; i-- { + commit := commits[i] + msg := strings.TrimSpace(commit.CommitMessage) + if msg == "" { + continue + } - if _, err := stringBuilder.WriteRune('\n'); err != nil { - log.Error("Unable to write commit message Error: %v", err) - return "" + // 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 } } + } + // 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() diff --git a/tests/integration/api_repo_file_helpers.go b/tests/integration/api_repo_file_helpers.go index 785416b0f580d..f8d6fc803f192 100644 --- a/tests/integration/api_repo_file_helpers.go +++ b/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", diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 71d084e273d87..e2302fa6ce013 100644 --- a/tests/integration/pull_merge_test.go +++ b/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 +`, + }, + } + + 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) + }) + } + }) +}