Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 11 additions & 14 deletions services/pull/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,8 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
authors := make([]string, 0, len(commits))
stringBuilder := strings.Builder{}

if !setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages {
populateWithCommits := setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages
if !populateWithCommits {
message := strings.TrimSpace(pr.Issue.Content)
stringBuilder.WriteString(message)
if stringBuilder.Len() > 0 {
Expand All @@ -849,33 +850,25 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
}

// 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 {
if populateWithCommits {
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 strings.TrimSpace(commit.CommitMessage) == "" {
continue
}

toWrite := fmt.Appendf(nil, "* %s\n", 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 ""
}

if _, err := stringBuilder.WriteRune('\n'); err != nil {
log.Error("Unable to write commit message Error: %v", err)
return ""
}
}
}

Expand Down Expand Up @@ -916,6 +909,10 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
}
}

if populateWithCommits && stringBuilder.Len() > 0 && len(authors) > 0 {
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)
Expand Down
24 changes: 24 additions & 0 deletions tests/integration/editor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,21 @@ func testCreateFile(t *testing.T, session *TestSession, user, repo, baseBranchNa
})
}

func testCreateFileWithCommitMessage(t *testing.T, session *TestSession, user, repo, baseBranchName, newBranchName, filePath, content, commitSummary, commitMessage string) {
commitChoice := "direct"
if newBranchName != "" && newBranchName != baseBranchName {
commitChoice = "commit-to-new-branch"
}
testEditorActionEdit(t, session, user, repo, "_new", baseBranchName, "", map[string]string{
"tree_path": filePath,
"content": content,
"commit_choice": commitChoice,
"new_branch_name": newBranchName,
"commit_summary": commitSummary,
"commit_message": commitMessage,
})
}

func testEditorProtectedBranch(t *testing.T) {
session := loginUser(t, "user2")
// Change the "master" branch to "protected"
Expand Down Expand Up @@ -139,6 +154,15 @@ func testEditFile(t *testing.T, session *TestSession, user, repo, branch, filePa
})
}

func testEditFileWithCommitMessage(t *testing.T, session *TestSession, user, repo, branch, filePath, newContent, commitSummary, commitMessage string) {
testEditorActionEdit(t, session, user, repo, "_edit", branch, filePath, map[string]string{
"content": newContent,
"commit_choice": "direct",
"commit_summary": commitSummary,
"commit_message": commitMessage,
})
}

func testEditFileToNewBranch(t *testing.T, session *TestSession, user, repo, branch, targetBranch, filePath, newContent string) {
testEditorActionEdit(t, session, user, repo, "_edit", branch, filePath, map[string]string{
"content": newContent,
Expand Down
179 changes: 179 additions & 0 deletions tests/integration/pull_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
auth_model "code.gitea.io/gitea/models/auth"
git_model "code.gitea.io/gitea/models/git"
issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/perm"
pull_model "code.gitea.io/gitea/models/pull"
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unittest"
Expand Down Expand Up @@ -1180,3 +1181,181 @@ func TestPullNonMergeForAdminWithBranchProtection(t *testing.T) {
session.MakeRequest(t, mergeReq, http.StatusMethodNotAllowed)
})
}

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)
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
user4Session := loginUser(t, user4.Name)

sessions := map[string]*TestSession{
user2.Name: user2Session,
user4.Name: user4Session,
}

// Enable POPULATE_SQUASH_COMMENT_WITH_COMMIT_MESSAGES
resetFunc1 := test.MockVariableValue(&setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages, true)
defer resetFunc1()
// Set DEFAULT_MERGE_MESSAGE_SIZE
resetFunc2 := test.MockVariableValue(&setting.Repository.PullRequest.DefaultMergeMessageSize, 512)
defer resetFunc2()

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",
})
assert.NoError(t, err)
doAPIAddCollaborator(NewAPITestContext(t, repo.OwnerName, repo.Name, auth_model.AccessTokenScopeWriteRepository), user4.Name, perm.AccessModeWrite)(t)

type commitInfo struct {
userName string
commitSummary string
commitMessage string
}

testCases := []struct {
name string
commitInfos []*commitInfo
expectedMessage string
}{
{
name: "Only summaries",
commitInfos: []*commitInfo{
{
userName: user2.Name,
commitSummary: "Implement the login endpoint",
},
{
userName: user2.Name,
commitSummary: "Validate request body",
},
},
expectedMessage: `* Implement the login endpoint

* Validate request body

`,
},
{
name: "Summaries and messages",
commitInfos: []*commitInfo{
{
userName: user2.Name,
commitSummary: "Refactor user service",
commitMessage: `Implement the login endpoint.
Validate request body.`,
},
{
userName: user2.Name,
commitSummary: "Add email notification service",
commitMessage: `Implements a new email notification module.

- Supports templating
- Supports HTML and plain text modes
- Includes retry logic`,
},
},
expectedMessage: `* Refactor user service

Implement the login endpoint.
Validate request body.

* Add email notification service

Implements a new email notification module.

- Supports templating
- Supports HTML and plain text modes
- Includes retry logic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really a good result?

* Refactor user service
...
- Supports templating
* (others...)

Not sure whether GitHub does so. Even if GitHub does so, I don't think GitHub was right or we need to blindly follow GitHub

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't quite understand where the problem is. We add * before every commit message and - comes from the commit message content.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:

commit1:

a
* a
* c

commit2:

x
* y
* z

The current code generates:

* a
* b
* c
* x
* y
* z

It looks like there are 6 commits.

But ideally it should be:

* a
  * b
  * c
* x
  * y
  * z

Just my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, GitHub does the same thing.

c7ede32e4a4b36236d0b4826d3045952

Using * in commit messages can make it difficult to distinguish between multiple messages. However, since the squash commit message is editable, I think that users can manually fix such issues if necessary.


`,
},
{
name: "Long Message",
commitInfos: []*commitInfo{
{
userName: user2.Name,
commitSummary: "Add advanced validation logic for user onboarding",
commitMessage: `This commit introduces a comprehensive validation layer for the user onboarding flow.
The primary goal is to ensure that all input data is strictly validated before being processed by downstream services.
This improves system reliability and significantly reduces runtime exceptions in the registration pipeline.

The validation logic includes:

1. Email format checking using RFC 5322-compliant patterns.
2. Username length and character limitation enforcement.
3. Password strength enforcement, including:
- Minimum length checks
- Mixed character type detection
- Optional entropy-based scoring
4. Optional phone number validation using region-specific rules.
`,
},
},
expectedMessage: `* Add advanced validation logic for user onboarding

This commit introduces a comprehensive validation layer for the user onboarding flow.
The primary goal is to ensure that all input data is strictly validated before being processed by downstream services.
This improves system reliability and significantly reduces runtime exceptions in the registration pipeline.

The validation logic includes:

1. Email format checking using RFC 5322-compliant patterns.
2. Username length and character limitation enfor...`,
},
{
name: "Test Co-authored-by",
commitInfos: []*commitInfo{
{
userName: user2.Name,
commitSummary: "Implement the login endpoint",
},
{
userName: user4.Name,
commitSummary: "Validate request body",
},
},
expectedMessage: `* Implement the login endpoint

* Validate request body

---------

Co-authored-by: user4 <[email protected]>
`,
},
}

for tcNum, tc := range testCases {
branchName := "test-branch-" + strconv.Itoa(tcNum)
fileName := fmt.Sprintf("test-file-%d.txt", tcNum)
t.Run(tc.name, func(t *testing.T) {
for infoIdx, info := range tc.commitInfos {
content := "content-" + strconv.Itoa(infoIdx)
if infoIdx == 0 {
testCreateFileWithCommitMessage(t, sessions[info.userName], repo.OwnerName, repo.Name, repo.DefaultBranch, branchName, fileName, content, info.commitSummary, info.commitMessage)
} else {
testEditFileWithCommitMessage(t, sessions[info.userName], repo.OwnerName, repo.Name, branchName, fileName, content, info.commitSummary, info.commitMessage)
}
}
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.Atoi(elems[4])
assert.NoError(t, err)
pullRequest := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, Index: int64(pullIndex)})
squashMergeCommitMessage := pull_service.GetSquashMergeCommitMessages(t.Context(), pullRequest)
assert.Equal(t, tc.expectedMessage, squashMergeCommitMessage)
})
}
})
}
Loading