diff --git a/services/pull/merge.go b/services/pull/merge.go index a591ce4de2..0977623bfb 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -87,6 +87,19 @@ func getMergeMessage(ctx context.Context, baseGitRepo *git.Repository, pr *issue reviewedOn := fmt.Sprintf("Reviewed-on: %s", issueURL) reviewedBy := pr.GetApprovers(ctx) + body = fmt.Sprintf("%s\n%s", reviewedOn, reviewedBy) + + // Squash merge has a different from other styles. + if mergeStyle == repo_model.MergeStyleSquash { + message = fmt.Sprintf("%s (%s%d)", pr.Issue.Title, issueReference, pr.Issue.Index) + } else if pr.BaseRepoID == pr.HeadRepoID { + message = fmt.Sprintf("Merge pull request '%s' (%s%d) from %s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadBranch, pr.BaseBranch) + } else if pr.HeadRepo == nil { + message = fmt.Sprintf("Merge pull request '%s' (%s%d) from :%s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadBranch, pr.BaseBranch) + } else { + message = fmt.Sprintf("Merge pull request '%s' (%s%d) from %s:%s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseBranch) + } + if mergeStyle != "" { commit, err := baseGitRepo.GetBranchCommit(pr.BaseRepo.DefaultBranch) if err != nil { @@ -155,8 +168,7 @@ func getMergeMessage(ctx context.Context, baseGitRepo *git.Repository, pr *issue vars["ClosingIssues"] = "" } } - message, body = expandDefaultMergeMessage(templateContent, vars) - return message, body, nil + return expandDefaultMergeMessage(templateContent, vars, message, body) } } @@ -165,32 +177,30 @@ func getMergeMessage(ctx context.Context, baseGitRepo *git.Repository, pr *issue return "", "", nil } - body = fmt.Sprintf("%s\n%s", reviewedOn, reviewedBy) - - // Squash merge has a different from other styles. - if mergeStyle == repo_model.MergeStyleSquash { - return fmt.Sprintf("%s (%s%d)", pr.Issue.Title, issueReference, pr.Issue.Index), body, nil - } - - if pr.BaseRepoID == pr.HeadRepoID { - return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadBranch, pr.BaseBranch), body, nil - } - - if pr.HeadRepo == nil { - return fmt.Sprintf("Merge pull request '%s' (%s%d) from :%s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadBranch, pr.BaseBranch), body, nil - } - - return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s:%s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseBranch), body, nil + return message, body, nil } -func expandDefaultMergeMessage(template string, vars map[string]string) (message, body string) { - message = strings.TrimSpace(template) - if splits := strings.SplitN(message, "\n", 2); len(splits) == 2 { - message = splits[0] - body = strings.TrimSpace(splits[1]) +func expandDefaultMergeMessage(template string, vars map[string]string, message, body string) (finalMessage, finalBody string, err error) { + if template == "" { + return message, body, nil } mapping := func(s string) string { return vars[s] } - return os.Expand(message, mapping), os.Expand(body, mapping) + if splits := strings.SplitN(template, "\n", 2); len(splits) == 2 { + var templateTitle string + var templateBody string + if len(splits[0]) == 0 { + templateTitle = message + } else { + templateTitle = os.Expand(strings.TrimSpace(splits[0]), mapping) + } + if len(splits[1]) == 0 { + templateBody = body + } else { + templateBody = os.Expand(strings.TrimRightFunc(splits[1], unicode.IsSpace), mapping) + } + return templateTitle, templateBody, nil + } + return os.Expand(strings.TrimSpace(template), mapping), body, nil } // GetDefaultMergeMessage returns default message used when merging pull request diff --git a/services/pull/merge_test.go b/services/pull/merge_test.go index aa033b8cdb..0766a46cea 100644 --- a/services/pull/merge_test.go +++ b/services/pull/merge_test.go @@ -4,6 +4,7 @@ package pull import ( + "fmt" "os" "path" "strings" @@ -27,6 +28,21 @@ func Test_expandDefaultMergeMessage(t *testing.T) { template string vars map[string]string } + + title := "PullRequestTitle" + description := "Pull\nRequest\nDescription\n" + vars := map[string]string{ + "PullRequestTitle": title, + "PullRequestDescription": description, + } + defaultTitle := "default_title" + defaultBody := "default_body" + expectedTitle := fmt.Sprintf("Merge %s", title) + expectedDescription := fmt.Sprintf("Description: %s", description) + expectedDescriptionMultiLine := fmt.Sprintf("Description:\n\n%s", description) + expectedDescriptionMerged := fmt.Sprintf("\n\nMerge %s\n\nDescription:\n\n%s", title, description) + emptyString := "" + tests := []struct { name string args args @@ -34,47 +50,90 @@ func Test_expandDefaultMergeMessage(t *testing.T) { wantBody string }{ { - name: "single line", - args: args{ - template: "Merge ${PullRequestTitle}", - vars: map[string]string{ - "PullRequestTitle": "PullRequestTitle", - "PullRequestDescription": "Pull\nRequest\nDescription\n", - }, - }, - want: "Merge PullRequestTitle", - wantBody: "", + name: "empty template", + args: args{template: "", vars: vars}, + want: defaultTitle, + wantBody: defaultBody, }, { - name: "multiple lines", - args: args{ - template: "Merge ${PullRequestTitle}\nDescription:\n\n${PullRequestDescription}\n", - vars: map[string]string{ - "PullRequestTitle": "PullRequestTitle", - "PullRequestDescription": "Pull\nRequest\nDescription\n", - }, - }, - want: "Merge PullRequestTitle", - wantBody: "Description:\n\nPull\nRequest\nDescription\n", + name: "single line", + args: args{template: "Merge ${PullRequestTitle}", vars: vars}, + want: expectedTitle, + wantBody: defaultBody, }, { - name: "leading newlines", - args: args{ - template: "\n\n\nMerge ${PullRequestTitle}\n\n\nDescription:\n\n${PullRequestDescription}\n", - vars: map[string]string{ - "PullRequestTitle": "PullRequestTitle", - "PullRequestDescription": "Pull\nRequest\nDescription\n", - }, - }, - want: "Merge PullRequestTitle", - wantBody: "Description:\n\nPull\nRequest\nDescription\n", + name: "empty message (space)", + args: args{template: " ", vars: vars}, + want: emptyString, + wantBody: defaultBody, + }, + { + name: "empty message (with newline)", + args: args{template: " \n", vars: vars}, + want: emptyString, + wantBody: defaultBody, + }, + { + name: "single newline", + args: args{template: "\n", vars: vars}, + want: defaultTitle, + wantBody: defaultBody, + }, + { + name: "empty description (newline)", + args: args{template: "\n\n", vars: vars}, + want: defaultTitle, + wantBody: emptyString, + }, + { + name: "empty description (space)", + args: args{template: "\n ", vars: vars}, + want: defaultTitle, + wantBody: emptyString, + }, + { + name: "empty title and description (spaces)", + args: args{template: " \n ", vars: vars}, + want: emptyString, + wantBody: emptyString, + }, + { + name: "empty title and description (space and newline)", + args: args{template: " \n\n", vars: vars}, + want: emptyString, + wantBody: emptyString, + }, + { + name: "simple message and description", + args: args{template: "Merge ${PullRequestTitle}\nDescription: ${PullRequestDescription}", vars: vars}, + want: expectedTitle, + wantBody: expectedDescription, + }, + { + name: "multiple lines", + args: args{template: "Merge ${PullRequestTitle}\nDescription:\n\n${PullRequestDescription}\n", vars: vars}, + want: expectedTitle, + wantBody: expectedDescriptionMultiLine, + }, + { + name: "description only", + args: args{template: "\nDescription: ${PullRequestDescription}\n", vars: vars}, + want: defaultTitle, + wantBody: expectedDescription, + }, + { + name: "leading newlines", + args: args{template: "\n\n\nMerge ${PullRequestTitle}\n\nDescription:\n\n${PullRequestDescription}\n", vars: vars}, + want: defaultTitle, + wantBody: expectedDescriptionMerged, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, got1 := expandDefaultMergeMessage(tt.args.template, tt.args.vars) - assert.Equalf(t, tt.want, got, "expandDefaultMergeMessage(%v, %v)", tt.args.template, tt.args.vars) - assert.Equalf(t, tt.wantBody, got1, "expandDefaultMergeMessage(%v, %v)", tt.args.template, tt.args.vars) + resultTitle, resultBody, err := expandDefaultMergeMessage(tt.args.template, tt.args.vars, defaultTitle, defaultBody) + require.NoError(t, err) + assert.Equalf(t, tt.want, resultTitle, "Wrong title for test '%s' -> expandDefaultMergeMessage(%q, %q)", tt.name, tt.args.template, tt.args.vars) + assert.Equalf(t, tt.wantBody, resultBody, "Wrong body for test '%s' -> expandDefaultMergeMessage(%q, %q)", tt.name, tt.args.template, tt.args.vars) }) } } diff --git a/services/pull/pull_test.go b/services/pull/pull_test.go index 99607c5b35..bf5c56eda1 100644 --- a/services/pull/pull_test.go +++ b/services/pull/pull_test.go @@ -126,5 +126,5 @@ func TestPullRequest_GetDefaultMergeMessage_GlobalTemplate(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Default merge message template", mergeMessage) - assert.Equal(t, "This line was read from .forgejo/default_merge_message/MERGE_TEMPLATE.md", body) + assert.Equal(t, "\nThis line was read from .forgejo/default_merge_message/MERGE_TEMPLATE.md", body) }