From 895d848ff421a6e6798dc64417af7118c30bc53d Mon Sep 17 00:00:00 2001 From: Giteabot Date: Fri, 17 Jul 2026 01:09:01 -0700 Subject: [PATCH] fix: make commit message merge correctly (#38490) (#38502) Backport #38490 fix #38487 Co-authored-by: wxiaoguang Co-authored-by: silverwind Co-authored-by: bircni --- modules/git/commit_message.go | 43 ++++++++++++++++++++++++++-- modules/git/commit_message_test.go | 34 ++++++++++++++++++++-- routers/web/repo/pull_merge_form.go | 32 ++++++++++++--------- services/pull/pull.go | 23 ++++++--------- tests/integration/pull_merge_test.go | 3 +- 5 files changed, 101 insertions(+), 34 deletions(-) diff --git a/modules/git/commit_message.go b/modules/git/commit_message.go index e03bca3f5e7..14c7b8f7e23 100644 --- a/modules/git/commit_message.go +++ b/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.*?)(?P^|^\n|^-{3,}\n+|\n-{3,}\n+|\n\n)(?P(?:[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.*?)(?P^|^\n|^-{3,}\n+|\n+-{3,}\n+|\n{2,})(?P(?:[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") { diff --git a/modules/git/commit_message_test.go b/modules/git/commit_message_test.go index e16fa19e8b8..a602185d562 100644 --- a/modules/git/commit_message_test.go +++ b/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) + } +} diff --git a/routers/web/repo/pull_merge_form.go b/routers/web/repo/pull_merge_form.go index 668717604f7..fe12aa6105b 100644 --- a/routers/web/repo/pull_merge_form.go +++ b/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{ diff --git a/services/pull/pull.go b/services/pull/pull.go index 79a07a9348e..6af56b7e8f9 100644 --- a/services/pull/pull.go +++ b/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 { diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index ae1f8a34909..e26676f7721 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -1321,7 +1321,8 @@ Co-authored-by: user4 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) }) }