fix: make commit message merge correctly (#38490)

fix #38487

---------

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: bircni <bircni@icloud.com>

modules/git/commit_message.go

@@ -77,8 +77,12 @@ func (c *CommitMessage) MessageTrailer() CommitMessageTrailerValues {
 }
 
 var commitMessageTrailerSplit = sync.OnceValue(func() *regexp.Regexp {
-	// the sep is either something like "\n---\n" or "\n\n" in the body, or at the start of the body like "---\n"
-	return regexp.MustCompile(`(?s)^(?P<content>.*?)(?P<sep>^|^\n|^-{3,}\n+|\n-{3,}\n+|\n\n)(?P<trailer>(?:[A-Za-z0-9][-A-Za-z0-9]*:[^\n]*\n?)*\n*)$`)
+	// ref: https://git-scm.com/docs/git-interpret-trailers
+	// TODO: the regexp is not able to perfectly parse the all kinds of trailers
+	// It was just copied from legacy code, it is not exactly the same as how Git parses the trailer and not quite right in some cases.
+	// For the key characters: it follows RFC 822 field name syntax (or RFC 2822/RFC 5322): printable ASCII characters between 33 and 126 except the colon (:),
+	// but maybe we don't want to make it that complicated, so here we only support some common "symbol-like" characters.
+	return regexp.MustCompile(`(?s)^(?P<content>.*?)(?P<sep>^|^\n|^-{3,}\n+|\n+-{3,}\n+|\n{2,})(?P<trailer>(?:[A-Za-z0-9][-\w]*:[^\n]*(\n\s+[^\n]*)*\n?)*\n*)$`)
 })
 
 // CommitMessageSplitTrailer tries to split the message by the trailer separator
@@ -93,6 +97,41 @@ func CommitMessageSplitTrailer(s string) (content, sep, trailer string) {
 	return v[re.SubexpIndex("content")], v[re.SubexpIndex("sep")], v[re.SubexpIndex("trailer")]
 }
 
+// CommitMessageMerge merges two commit messages with their trailers
+func CommitMessageMerge(m1, m2 string) string {
+	c1, s1, t1 := CommitMessageSplitTrailer(m1)
+	c2, s2, t2 := CommitMessageSplitTrailer(m2)
+	c1, t1 = strings.TrimSpace(c1), strings.TrimSpace(t1)
+	c2, t2 = strings.TrimSpace(c2), strings.TrimSpace(t2)
+	out := strings.Builder{}
+	if c1 != "" && c2 != "" {
+		out.WriteString(c1)
+		out.WriteString("\n\n")
+		out.WriteString(c2)
+	} else if c1 != "" {
+		out.WriteString(c1)
+	} else if c2 != "" {
+		out.WriteString(c2)
+	}
+	if t1 != "" || t2 != "" {
+		sep := util.Iif(t1 == "", s2, s1)
+		sep = util.IfZero(sep, "\n\n")
+		if c1 != "" || c2 != "" {
+			out.WriteString(sep)
+		}
+		if t1 != "" {
+			out.WriteString(t1)
+		}
+		if t1 != "" && t2 != "" {
+			out.WriteString("\n")
+		}
+		if t2 != "" {
+			out.WriteString(t2)
+		}
+	}
+	return out.String()
+}
+
 func CommitMessageParseTrailer(s string) CommitMessageTrailerValues {
 	ret := CommitMessageTrailerValues{}
 	for line := range strings.SplitSeq(util.NormalizeStringEOL(s), "\n") {

modules/git/commit_message_test.go

@@ -26,10 +26,12 @@ func TestCommitMessageTrailer(t *testing.T) {
 		{"a", "a", "", ""},
 		{"a\n\nk", "a\n\nk", "", ""},
 		{"a\n\nk:v", "a", "\n\n", "k:v"},
+		{"a\n\nk:v\n  next-line", "a", "\n\n", "k:v\n  next-line"},
+		{"a\n\nk:v\n  next-line\nother: v", "a", "\n\n", "k:v\n  next-line\nother: v"},
 		{"a\n\nk:v\n\n", "a", "\n\n", "k:v\n\n"},
 		{"a\n--\nk:v", "a\n--\nk:v", "", ""},
-		{"a\n---\nk:v", "a", "\n---\n", "k:v"},
-		{"a\n\n---\n\nk:v", "a\n", "\n---\n\n", "k:v"},
+		{"a\n---\nk:v", "a", "\n---\n", "k:v"}, // TODO: should we support such case? No empty line between "---" and the trailer
+		{"a\n\n---\n\nk:v", "a", "\n\n---\n\n", "k:v"},
 
 		{"k: v", "", "", "k: v"},
 		{"\nk:v", "", "\n", "k:v"},
@@ -127,3 +129,31 @@ func TestCommitMessageParticipants(t *testing.T) {
 		}
 	})
 }
+
+func TestCommitMessageMerge(t *testing.T) {
+	cases := []struct {
+		m1, m2 string
+		out    string
+	}{
+		{"", "", ""},
+		{"msg1", "", "msg1"},
+		{"", "msg2", "msg2"},
+		{"msg1", "msg2", "msg1\n\nmsg2"},
+		{"k1: a", "", "k1: a"},
+		{"", "k2: b", "k2: b"},
+		{"k1: a", "k2: b", "k1: a\nk2: b"},
+		{"msg1", "k2: b", "msg1\n\nk2: b"},
+		{"k1: a", "msg2", "msg2\n\nk1: a"},
+		{"msg1\n\nk1: a", "msg2", "msg1\n\nmsg2\n\nk1: a"},
+		{"msg1\n----\nk1: a", "msg2", "msg1\n\nmsg2\n----\nk1: a"},
+		{"msg1\n\n----\n\nk1: a", "msg2", "msg1\n\nmsg2\n\n----\n\nk1: a"},
+		{"msg1", "msg2\n----\nk2: b", "msg1\n\nmsg2\n----\nk2: b"},
+		{"msg1", "msg2\n\nk2: b", "msg1\n\nmsg2\n\nk2: b"},
+		{"msg1\n\nk1: a", "msg2\n\nk2: b", "msg1\n\nmsg2\n\nk1: a\nk2: b"},
+	}
+
+	for i, c := range cases {
+		out := CommitMessageMerge(c.m1, c.m2)
+		assert.Equal(t, c.out, out, "idx=%d, m1=%q m2=%q", i, c.m1, c.m2)
+	}
+}

routers/web/repo/pull_merge_form.go

@@ -10,6 +10,8 @@ import (
 	pull_model "gitea.dev/models/pull"
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unit"
+	"gitea.dev/modules/git"
+	"gitea.dev/modules/log"
 	"gitea.dev/modules/svg"
 	"gitea.dev/modules/templates"
 	"gitea.dev/modules/util"
@@ -62,20 +64,23 @@ func (prInfo *pullRequestViewInfo) prepareMergeBoxFormProps(ctx *context.Context
 		hasPendingPullRequestMergeTip = ctx.Locale.Tr("repo.pulls.auto_merge_has_pending_schedule", pendingPullRequestMerge.Doer.Name, createdPRMergeStr)
 	}
 
-	defaultMergeTitle, defaultMergeBody, err := pull_service.GetDefaultMergeMessage(ctx, ctx.Repo.GitRepo, pull, mergeStyle)
-	if err != nil && !errors.Is(err, util.ErrNotExist) {
-		ctx.ServerError("GetDefaultMergeMessage", err)
-		return
-	}
-	defaultSquashMergeTitle, defaultSquashMergeBody, err := pull_service.GetDefaultMergeMessage(ctx, ctx.Repo.GitRepo, pull, repo_model.MergeStyleSquash)
-	if err != nil && !errors.Is(err, util.ErrNotExist) {
-		ctx.ServerError("GetDefaultSquashMergeMessage", err)
-		return
-	}
-
+	var defaultMergeTitle, defaultMergeBody string
+	var defaultSquashMergeTitle, defaultSquashMergeBody string
 	var defaultSquashMergeCommitMessages string
 	if !prInfo.IsPullRequestBroken {
-		defaultSquashMergeCommitMessages = pull_service.GetSquashMergeCommitMessages(ctx, pull)
+		var err error
+		defaultMergeTitle, defaultMergeBody, err = pull_service.GetDefaultMergeMessage(ctx, ctx.Repo.GitRepo, pull, mergeStyle)
+		if err != nil && !errors.Is(err, util.ErrNotExist) {
+			log.Error("GetDefaultMergeMessage for style %s failed, error: %v", mergeStyle, err)
+		}
+		defaultSquashMergeTitle, defaultSquashMergeBody, err = pull_service.GetDefaultMergeMessage(ctx, ctx.Repo.GitRepo, pull, repo_model.MergeStyleSquash)
+		if err != nil && !errors.Is(err, util.ErrNotExist) {
+			log.Error("GetDefaultMergeMessage for squash failed, error: %v", err)
+		}
+		defaultSquashMergeCommitMessages, err = pull_service.GetSquashMergeCommitMessages(ctx, pull)
+		if err != nil && !errors.Is(err, util.ErrNotExist) {
+			log.Error("GetSquashMergeCommitMessages failed, error: %v", err)
+		}
 	}
 
 	allOverridableChecksOk := !prInfo.MergeBoxData.hasOverridableBlockers
@@ -106,7 +111,6 @@ func (prInfo *pullRequestViewInfo) prepareMergeBoxFormProps(ctx *context.Context
 
 	// if this pr can be merged now, then hide the auto merge
 	generalHideAutoMerge := prInfo.MergeBoxData.canMergeNow && allOverridableChecksOk
-
 	var mergeStyles []any
 	if pull.IsStatusMergeable() {
 		mergeStyles = []any{
@@ -138,7 +142,7 @@ func (prInfo *pullRequestViewInfo) prepareMergeBoxFormProps(ctx *context.Context
 				"allowed":               prConfig.AllowSquash,
 				"textDoMerge":           ctx.Locale.Tr("repo.pulls.squash_merge_pull_request"),
 				"mergeTitleFieldText":   defaultSquashMergeTitle,
-				"mergeMessageFieldText": defaultSquashMergeCommitMessages + defaultSquashMergeBody,
+				"mergeMessageFieldText": git.CommitMessageMerge(defaultSquashMergeCommitMessages, defaultSquashMergeBody),
 				"hideAutoMerge":         generalHideAutoMerge,
 			},
 			map[string]any{

services/pull/pull.go

@@ -777,30 +777,25 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re
 }
 
 // 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, err error) {
 	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 "", err
 	}
 
 	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 "", err
 	}
 
 	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 "", err
 		}
 	}
 
 	gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.HeadRepo)
 	if err != nil {
-		log.Error("Unable to open head repository: Error: %v", err)
-		return ""
+		return "", err
 	}
 	defer closer.Close()
 
@@ -810,8 +805,7 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
 	} else {
 		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 "", err
 		}
 		headCommitRef = git.RefNameFromCommit(pr.HeadCommitID)
 	}
@@ -822,8 +816,7 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
 
 	limitedCommits, err := gitRepo.CommitsBetween(headCommitRef, mergeBaseRef, limit)
 	if err != nil {
-		log.Error("Unable to get commits between: %s %s Error: %v", pr.HeadBranch, pr.MergeBase, err)
-		return ""
+		return "", err
 	}
 
 	mergeMessage := strings.TrimSpace(pr.Issue.Content) // use PR's title and description as squash commit message
@@ -831,7 +824,7 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ
 		mergeMessage = formatSquashMergeCommitMessages(limitedCommits) // use PR's commit messages as squash commit message
 	}
 	coAuthors := collectSquashMergeCommitCoAuthors(ctx, gitRepo, pr, headCommitRef, mergeBaseRef, limit, limitedCommits)
-	return buildSquashMergeCommitMessages(mergeMessage, coAuthors)
+	return buildSquashMergeCommitMessages(mergeMessage, coAuthors), nil
 }
 
 func buildSquashMergeCommitMessages(mergeMessage string, coAuthors []string) string {

tests/integration/pull_merge_test.go

@@ -1321,7 +1321,8 @@ Co-authored-by: user4 <user4@example.com>
 				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)
+				squashMergeCommitMessage, err := pull_service.GetSquashMergeCommitMessages(t.Context(), pullRequest)
+				assert.NoError(t, err)
 				assert.Equal(t, tc.expectedMessage, squashMergeCommitMessage)
 			})
 		}