Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
29 changes: 23 additions & 6 deletions routers/web/repo/issue_view.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"net/url"
"sort"
"strconv"
"strings"

activities_model "code.gitea.io/gitea/models/activities"
"code.gitea.io/gitea/models/db"
Expand Down Expand Up @@ -387,8 +388,10 @@ func ViewIssue(ctx *context.Context) {
prepareIssueViewSidebarTimeTracker,
prepareIssueViewSidebarDependency,
prepareIssueViewSidebarPin,
func(ctx *context.Context, issue *issues_model.Issue) { preparePullViewPullInfo(ctx, issue) },
preparePullViewReviewAndMerge,
func(ctx *context.Context, issue *issues_model.Issue) {
compareInfo := preparePullViewPullInfo(ctx, issue)
preparePullViewReviewAndMerge(ctx, issue, compareInfo)
},
}

for _, prepareFunc := range prepareFuncs {
Expand Down Expand Up @@ -440,8 +443,8 @@ func ViewPullMergeBox(ctx *context.Context) {
ctx.NotFound(nil)
return
}
preparePullViewPullInfo(ctx, issue)
preparePullViewReviewAndMerge(ctx, issue)
compareInfo := preparePullViewPullInfo(ctx, issue)
preparePullViewReviewAndMerge(ctx, issue, compareInfo)
ctx.Data["PullMergeBoxReloading"] = issue.PullRequest.IsChecking()

// TODO: it should use a dedicated struct to render the pull merge box, to make sure all data is prepared correctly
Expand Down Expand Up @@ -822,7 +825,7 @@ func prepareIssueViewCommentsAndSidebarParticipants(ctx *context.Context, issue
ctx.Data["NumParticipants"] = len(participants)
}

func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Issue) {
func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Issue, compareInfo *pull_service.CompareInfo) {
getBranchData(ctx, issue)
if !issue.IsPull {
return
Expand Down Expand Up @@ -933,8 +936,22 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss
ctx.ServerError("GetDefaultSquashMergeMessage", err)
return
}
_, coAuthors := pull_service.GetSquashMergeCommitMessages(ctx, pull)

defaultSquashMergeBody = fmt.Sprintf("%s%s", defaultSquashMergeBody, coAuthors)

commitsBuilder := strings.Builder{}
for _, c := range compareInfo.Commits {
commitsBuilder.WriteString("* ")
commitsBuilder.WriteString(c.CommitMessage)
commitsBuilder.WriteRune('\n')
}

ctx.Data["DefaultSquashMergeMessage"] = defaultSquashMergeMessage
ctx.Data["DefaultSquashMergeBody"] = defaultSquashMergeBody
ctx.Data["DefaultSquashMergeBody"] = fmt.Sprintf("--------------------\n%s%s", commitsBuilder.String(), defaultSquashMergeBody)
Copy link
Member

Choose a reason for hiding this comment

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

The --------------------\n should be ignored if the pull request body is empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

done b09ada6

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it still needs more tests, to test various edge cases.

The test can clearly test this function's logic.

The existing tests in this PR (parse HTML pages) seem quite fragile and difficult to read. I guess you will also have difficulty to understand your code after 3 months.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll continue to refine this pr over the weekend

if len(pull.Issue.Content) == 0 {
ctx.Data["DefaultSquashMergeBody"] = fmt.Sprintf("%s%s", commitsBuilder.String(), defaultSquashMergeBody)
}

pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pull.BaseRepoID, pull.BaseBranch)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ func prepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *pull_
ctx.ServerError("IsUserAllowedToUpdate", err)
return nil
}
ctx.Data["GetCommitMessages"] = pull_service.GetSquashMergeCommitMessages(ctx, pull)
ctx.Data["GetCommitMessages"], _ = pull_service.GetSquashMergeCommitMessages(ctx, pull)
} else {
ctx.Data["GetCommitMessages"] = ""
}
Expand Down
39 changes: 20 additions & 19 deletions services/pull/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -784,30 +784,30 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re
var commitMessageTrailersPattern = regexp.MustCompile(`(?:^|\n\n)(?:[\w-]+[ \t]*:[^\n]+\n*(?:[ \t]+[^\n]+\n*)*)+$`)

// GetSquashMergeCommitMessages returns the commit messages between head and merge base (if there is one)
func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequest) string {
func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequest) (string, string) {
if err := pr.LoadIssue(ctx); err != nil {
log.Error("Cannot load issue %d for PR id %d: Error: %v", pr.IssueID, pr.ID, err)
return ""
return "", ""
}

if err := pr.Issue.LoadPoster(ctx); err != nil {
log.Error("Cannot load poster %d for pr id %d, index %d Error: %v", pr.Issue.PosterID, pr.ID, pr.Index, err)
return ""
return "", ""
}

if pr.HeadRepo == nil {
var err error
pr.HeadRepo, err = repo_model.GetRepositoryByID(ctx, pr.HeadRepoID)
if err != nil {
log.Error("GetRepositoryByIdCtx[%d]: %v", pr.HeadRepoID, err)
return ""
return "", ""
}
}

gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.HeadRepo)
if err != nil {
log.Error("Unable to open head repository: Error: %v", err)
return ""
return "", ""
}
defer closer.Close()

Expand All @@ -818,34 +818,35 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
pr.HeadCommitID, err = gitRepo.GetRefCommitID(pr.GetGitHeadRefName())
if err != nil {
log.Error("Unable to get head commit: %s Error: %v", pr.GetGitHeadRefName(), err)
return ""
return "", ""
}
headCommit, err = gitRepo.GetCommit(pr.HeadCommitID)
}
if err != nil {
log.Error("Unable to get head commit: %s Error: %v", pr.HeadBranch, err)
return ""
return "", ""
}

mergeBase, err := gitRepo.GetCommit(pr.MergeBase)
if err != nil {
log.Error("Unable to get merge base commit: %s Error: %v", pr.MergeBase, err)
return ""
return "", ""
}

limit := setting.Repository.PullRequest.DefaultMergeMessageCommitsLimit

commits, err := gitRepo.CommitsBetweenLimit(headCommit, mergeBase, limit, 0)
if err != nil {
log.Error("Unable to get commits between: %s %s Error: %v", pr.HeadBranch, pr.MergeBase, err)
return ""
return "", ""
}

posterSig := pr.Issue.Poster.NewGitSig().String()

uniqueAuthors := make(container.Set[string])
authors := make([]string, 0, len(commits))
stringBuilder := strings.Builder{}
coAuthorStringBuilder := strings.Builder{}

if !setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages {
message := strings.TrimSpace(pr.Issue.Content)
Expand Down Expand Up @@ -879,12 +880,12 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
}
if _, err := stringBuilder.Write(toWrite); err != nil {
log.Error("Unable to write commit message Error: %v", err)
return ""
return "", ""
}

if _, err := stringBuilder.WriteRune('\n'); err != nil {
log.Error("Unable to write commit message Error: %v", err)
return ""
return "", ""
}
}
}
Expand All @@ -908,7 +909,7 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
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 ""
return "", ""
}
if len(commits) == 0 {
break
Expand All @@ -927,21 +928,21 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
}

for _, author := range authors {
if _, err := stringBuilder.WriteString("Co-authored-by: "); err != nil {
if _, err := coAuthorStringBuilder.WriteString("Co-authored-by: "); err != nil {
log.Error("Unable to write to string builder Error: %v", err)
return ""
return "", ""
}
if _, err := stringBuilder.WriteString(author); err != nil {
if _, err := coAuthorStringBuilder.WriteString(author); err != nil {
log.Error("Unable to write to string builder Error: %v", err)
return ""
return "", ""
}
if _, err := stringBuilder.WriteRune('\n'); err != nil {
if _, err := coAuthorStringBuilder.WriteRune('\n'); err != nil {
log.Error("Unable to write to string builder Error: %v", err)
return ""
return "", ""
}
}

return stringBuilder.String()
return stringBuilder.String(), coAuthorStringBuilder.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
Expand Down
12 changes: 9 additions & 3 deletions tests/integration/pull_create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
"github.com/stretchr/testify/assert"
)

func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSelf bool, targetBranch, sourceBranch, title string) *httptest.ResponseRecorder {
func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSelf bool, targetBranch, sourceBranch, title string, contents ...string) *httptest.ResponseRecorder {
req := NewRequest(t, "GET", path.Join(user, repo))
resp := session.MakeRequest(t, req, http.StatusOK)

Expand Down Expand Up @@ -52,9 +52,15 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSel
htmlDoc = NewHTMLParser(t, resp.Body)
link, exists = htmlDoc.doc.Find("form.ui.form").Attr("action")
assert.True(t, exists, "The template has changed")

content := ""
if len(contents) > 0 {
content = contents[0]
}
req = NewRequestWithValues(t, "POST", link, map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"title": title,
"_csrf": htmlDoc.GetCSRF(),
"title": title,
"content": content,
})
resp = session.MakeRequest(t, req, http.StatusOK)
return resp
Expand Down
128 changes: 121 additions & 7 deletions tests/integration/pull_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"os"
"path"
"path/filepath"
"regexp"
"strconv"
"strings"
"testing"
Expand Down Expand Up @@ -40,19 +41,27 @@ import (
commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus"
files_service "code.gitea.io/gitea/services/repository/files"

"github.com/PuerkitoBio/goquery"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string, mergeStyle repo_model.MergeStyle, deleteBranch bool) *httptest.ResponseRecorder {
func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string, mergeStyle repo_model.MergeStyle, deleteBranch bool, messages ...string) *httptest.ResponseRecorder {
req := NewRequest(t, "GET", path.Join(user, repo, "pulls", pullnum))
resp := session.MakeRequest(t, req, http.StatusOK)

htmlDoc := NewHTMLParser(t, resp.Body)
link := path.Join(user, repo, "pulls", pullnum, "merge")

message := ""
if len(messages) > 0 {
message = messages[0]
}

options := map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"do": string(mergeStyle),
"_csrf": htmlDoc.GetCSRF(),
"do": string(mergeStyle),
"merge_message_field": message,
}

if deleteBranch {
Expand Down Expand Up @@ -165,11 +174,116 @@ func TestPullSquash(t *testing.T) {
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited!)\n")

resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title")

elem := strings.Split(test.RedirectURL(resp), "/")
resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title", "This is a pull content")
prURL := test.RedirectURL(resp)
elem := strings.Split(prURL, "/")
assert.Equal(t, "pulls", elem[3])
testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleSquash, false)

resp = session.MakeRequest(t, NewRequest(t, "GET", prURL), http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
dataURL, exists := htmlDoc.doc.Find("#allow-edits-from-maintainers").Attr("data-url")
assert.True(t, exists)
req := NewRequestWithValues(t, "POST", dataURL+"/set_allow_maintainer_edit", map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"allow_maintainer_edit": "true",
})
session.MakeRequest(t, req, http.StatusOK)

user2Session := loginUser(t, "user2")
resp = user2Session.MakeRequest(t, NewRequest(t, "GET", prURL+"/files"), http.StatusOK)
htmlDoc = NewHTMLParser(t, resp.Body)
nodes := htmlDoc.doc.Find(".diff-file-box[data-new-filename=\"README.md\"] .diff-file-header-actions .tippy-target a")
if assert.Equal(t, 2, nodes.Length()) {
assert.Equal(t, "Edit File", nodes.Last().Text())
editFileLink, exists := nodes.Last().Attr("href")
if assert.True(t, exists) {
// edit the file
resp := user2Session.MakeRequest(t, NewRequest(t, "GET", editFileLink), http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
lastCommit := htmlDoc.GetInputValueByName("last_commit")
assert.NotEmpty(t, lastCommit)
req := NewRequestWithValues(t, "POST", editFileLink, map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"last_commit": lastCommit,
"tree_path": "README.md",
"content": "Hello, World (Edite!!)",
"commit_summary": "user2 updated the file",
"commit_choice": "direct",
})
resp = user2Session.MakeRequest(t, req, http.StatusOK)
assert.NotEmpty(t, test.RedirectURL(resp))
}
}
resp = user2Session.MakeRequest(t, NewRequest(t, "GET", prURL+"/files"), http.StatusOK)
htmlDoc = NewHTMLParser(t, resp.Body)
nodes = htmlDoc.doc.Find(".diff-file-box[data-new-filename=\"README.md\"] .diff-file-header-actions .tippy-target a")
if assert.Equal(t, 2, nodes.Length()) {
assert.Equal(t, "Edit File", nodes.Last().Text())
editFileLink, exists := nodes.Last().Attr("href")
if assert.True(t, exists) {
// edit the file
resp := user2Session.MakeRequest(t, NewRequest(t, "GET", editFileLink), http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
lastCommit := htmlDoc.GetInputValueByName("last_commit")
assert.NotEmpty(t, lastCommit)
req := NewRequestWithValues(t, "POST", editFileLink, map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"last_commit": lastCommit,
"tree_path": "README.md",
"content": "Hello, World (Edite!!!)",
"commit_summary": "user2 updated the file!",
"commit_choice": "direct",
})
resp = user2Session.MakeRequest(t, req, http.StatusOK)
assert.NotEmpty(t, test.RedirectURL(resp))
}
}

resp = session.MakeRequest(t, NewRequest(t, "GET", prURL+"/merge_box"), http.StatusOK)
htmlDoc = NewHTMLParser(t, resp.Body)
message := ""
htmlDoc.doc.Find("script").Each(func(i int, s *goquery.Selection) {
scriptContent := s.Text()
re := regexp.MustCompile(`const\s+defaultSquashMergeMessage\s*=\s*"(.*?)"\s*;`)
matches := re.FindStringSubmatch(scriptContent)
if len(matches) > 1 {
message = matches[1]
}
})

testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleSquash, false, message)

req = NewRequest(t, "GET", "/user2/repo1/src/branch/master/")
resp = user2Session.MakeRequest(t, req, http.StatusOK)
htmlDoc = NewHTMLParser(t, resp.Body)
commitHref := htmlDoc.doc.Find(".latest-commit .commit-id-short").AttrOr("href", "")
assert.NotEmpty(t, commitHref)

req = NewRequest(t, "GET", commitHref)
resp = user2Session.MakeRequest(t, req, http.StatusOK)
htmlDoc = NewHTMLParser(t, resp.Body)
prTitle, exists := htmlDoc.doc.Find(".commit-summary").Attr("title")
assert.True(t, exists)
assert.Contains(t, prTitle, "This is a pull title")

commitBody := htmlDoc.doc.Find(".commit-body").Text()
assert.NotEmpty(t, commitBody)
assert.Contains(t, commitBody, "--------------------")

req = NewRequest(t, "GET", fmt.Sprintf("/user2/repo1/pulls/%s/commits/list", elem[4]))
resp = user2Session.MakeRequest(t, req, http.StatusOK)

var pullCommitList struct {
Commits []pull_service.CommitInfo `json:"commits"`
LastReviewCommitSha string `json:"last_review_commit_sha"`
}
DecodeJSON(t, resp, &pullCommitList)

require.Len(t, pullCommitList.Commits, 4)

for _, commit := range pullCommitList.Commits {
assert.Contains(t, commitBody, "* "+commit.Summary)
}

hookTasks, err = webhook.HookTasks(t.Context(), 1, 1)
assert.NoError(t, err)
Expand Down