From 70865730e67fa73582f910bf4f2e44445812856c Mon Sep 17 00:00:00 2001 From: Robert Wolff Date: Sat, 7 Feb 2026 12:58:26 +0100 Subject: [PATCH] fix(ui)!: remove squash merge committer trailer admin option (#11096) fix(ui)!: Remove the instance configuration option `repository.pull-request.ADD_CO_COMMITTER_TRAILERS` (was enabled by default). It was responsible for addition of unexpected trailers to commit messages in squash merges. These trailers were `Co-authored-by: ` and `Co-committed-by: `. Both used the pull request author as value, who is also assigned as the author of the squash merge commit, which they were just repeating. Furthermore, `Co-committed-by: ` is an uncommon commit trailer, and there is only one committer for a commit. The trailers were being added by Forgejo while performing the merge, bypassing user input in the UI and weren't shown in it. See further description and more examples in [#11097](https://codeberg.org/forgejo/forgejo/issues/11097). Closes: #11097 Closes: Codeberg/Community#2030 Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11096 Reviewed-by: 0ko <0ko@noreply.codeberg.org> Reviewed-by: Gusted Co-authored-by: Robert Wolff Co-committed-by: Robert Wolff --- custom/conf/app.example.ini | 3 --- modules/setting/repository.go | 3 --- release-notes/11096.md | 1 + services/pull/merge.go | 35 ----------------------------------- services/pull/merge_squash.go | 6 ------ services/pull/merge_test.go | 25 ------------------------- 6 files changed, 1 insertion(+), 72 deletions(-) create mode 100644 release-notes/11096.md diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 568091b058..b7aa3571df 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1132,9 +1132,6 @@ LEVEL = Info ;; If an squash commit's comment should be populated with the commit messages of the squashed commits ;POPULATE_SQUASH_COMMENT_WITH_COMMIT_MESSAGES = false ;; -;; Add co-authored-by and co-committed-by trailers if committer does not match author -;ADD_CO_COMMITTER_TRAILERS = true -;; ;; Retarget child pull requests to the parent pull request branch target on merge of parent pull request. It only works on merged PRs where the head and base branch target the same repo. ;RETARGET_CHILDREN_ON_MERGE = true diff --git a/modules/setting/repository.go b/modules/setting/repository.go index 7e774f0139..001032e3cb 100644 --- a/modules/setting/repository.go +++ b/modules/setting/repository.go @@ -96,7 +96,6 @@ var ( DefaultMergeMessageOfficialApproversOnly bool DefaultUpdateStyle string PopulateSquashCommentWithCommitMessages bool - AddCoCommitterTrailers bool RetargetChildrenOnMerge bool } `ini:"repository.pull-request"` @@ -227,7 +226,6 @@ var ( DefaultMergeMessageOfficialApproversOnly bool DefaultUpdateStyle string PopulateSquashCommentWithCommitMessages bool - AddCoCommitterTrailers bool RetargetChildrenOnMerge bool }{ WorkInProgressPrefixes: []string{"WIP:", "[WIP]"}, @@ -243,7 +241,6 @@ var ( DefaultMergeMessageOfficialApproversOnly: true, DefaultUpdateStyle: "merge", PopulateSquashCommentWithCommitMessages: false, - AddCoCommitterTrailers: true, RetargetChildrenOnMerge: true, }, diff --git a/release-notes/11096.md b/release-notes/11096.md new file mode 100644 index 0000000000..6db7331bd4 --- /dev/null +++ b/release-notes/11096.md @@ -0,0 +1 @@ +fix(ui)!: Remove the instance configuration option `repository.pull-request.ADD_CO_COMMITTER_TRAILERS` (was enabled by default). It was responsible for addition of unexpected trailers to commit messages in squash merges. These trailers were `Co-authored-by: ` and `Co-committed-by: `. Both used the pull request author as value, who is also assigned as the author of the squash merge commit, which they were just repeating. Furthermore, `Co-committed-by: ` is an uncommon commit trailer, and there is only one committer for a commit. The trailers were being added by Forgejo while performing the merge, bypassing user input in the UI and weren't shown in it. See further description and more examples in [#11097](https://codeberg.org/forgejo/forgejo/issues/11097). diff --git a/services/pull/merge.go b/services/pull/merge.go index 0977623bfb..1bcd11406c 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -208,41 +208,6 @@ func GetDefaultMergeMessage(ctx context.Context, baseGitRepo *git.Repository, pr return getMergeMessage(ctx, baseGitRepo, pr, mergeStyle, nil) } -func AddCommitMessageTrailer(message, tailerKey, tailerValue string) string { - trailerLine := tailerKey + ": " + tailerValue - message = strings.ReplaceAll(message, "\r\n", "\n") - message = strings.ReplaceAll(message, "\r", "\n") - if strings.Contains(message, "\n"+trailerLine+"\n") || strings.HasSuffix(message, "\n"+trailerLine) { - return message - } - - if !strings.HasSuffix(message, "\n") { - message += "\n" - } - lastNewLine := strings.LastIndexByte(message[:len(message)-1], '\n') - keyEnd := -1 - if lastNewLine != -1 { - keyEnd = strings.IndexByte(message[lastNewLine:], ':') - if keyEnd != -1 { - keyEnd += lastNewLine - } - } - var lastLineKey string - if lastNewLine != -1 && keyEnd != -1 { - lastLineKey = message[lastNewLine+1 : keyEnd] - } - - isLikelyTrailerLine := lastLineKey != "" && unicode.IsUpper(rune(lastLineKey[0])) && strings.Contains(message, "-") - for i := 0; isLikelyTrailerLine && i < len(lastLineKey); i++ { - r := rune(lastLineKey[i]) - isLikelyTrailerLine = unicode.IsLetter(r) || unicode.IsDigit(r) || r == '-' - } - if !strings.HasSuffix(message, "\n\n") && !isLikelyTrailerLine { - message += "\n" - } - return message + trailerLine -} - // Merge merges pull request to base repository. // Caller should check PR is ready to be merged (review and status checks) func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, wasAutoMerged bool) error { diff --git a/services/pull/merge_squash.go b/services/pull/merge_squash.go index 898f4c977d..715dc7d4cb 100644 --- a/services/pull/merge_squash.go +++ b/services/pull/merge_squash.go @@ -11,7 +11,6 @@ import ( "forgejo.org/modules/container" "forgejo.org/modules/git" "forgejo.org/modules/log" - "forgejo.org/modules/setting" ) // doMergeStyleSquash gets a commit author signature for squash commits @@ -63,11 +62,6 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error { return err } - if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() { - // add trailer - message = AddCommitMessageTrailer(message, "Co-authored-by", sig.String()) - message = AddCommitMessageTrailer(message, "Co-committed-by", sig.String()) // FIXME: this one should be removed, it is not really used or widely used - } cmdCommit := git.NewCommand(ctx, "commit"). AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email). AddOptionFormat("--message=%s", message) diff --git a/services/pull/merge_test.go b/services/pull/merge_test.go index 0766a46cea..62d6604c98 100644 --- a/services/pull/merge_test.go +++ b/services/pull/merge_test.go @@ -138,31 +138,6 @@ func Test_expandDefaultMergeMessage(t *testing.T) { } } -func TestAddCommitMessageTailer(t *testing.T) { - // add tailer for empty message - assert.Equal(t, "\n\nTest-tailer: TestValue", AddCommitMessageTrailer("", "Test-tailer", "TestValue")) - - // add tailer for message without newlines - assert.Equal(t, "title\n\nTest-tailer: TestValue", AddCommitMessageTrailer("title", "Test-tailer", "TestValue")) - assert.Equal(t, "title\n\nNot tailer: xxx\n\nTest-tailer: TestValue", AddCommitMessageTrailer("title\n\nNot tailer: xxx", "Test-tailer", "TestValue")) - assert.Equal(t, "title\n\nNotTailer: xxx\n\nTest-tailer: TestValue", AddCommitMessageTrailer("title\n\nNotTailer: xxx", "Test-tailer", "TestValue")) - assert.Equal(t, "title\n\nnot-tailer: xxx\n\nTest-tailer: TestValue", AddCommitMessageTrailer("title\n\nnot-tailer: xxx", "Test-tailer", "TestValue")) - - // add tailer for message with one EOL - assert.Equal(t, "title\n\nTest-tailer: TestValue", AddCommitMessageTrailer("title\n", "Test-tailer", "TestValue")) - - // add tailer for message with two EOLs - assert.Equal(t, "title\n\nTest-tailer: TestValue", AddCommitMessageTrailer("title\n\n", "Test-tailer", "TestValue")) - - // add tailer for message with existing tailer (won't duplicate) - assert.Equal(t, "title\n\nTest-tailer: TestValue", AddCommitMessageTrailer("title\n\nTest-tailer: TestValue", "Test-tailer", "TestValue")) - assert.Equal(t, "title\n\nTest-tailer: TestValue\n", AddCommitMessageTrailer("title\n\nTest-tailer: TestValue\n", "Test-tailer", "TestValue")) - - // add tailer for message with existing tailer and different value (will append) - assert.Equal(t, "title\n\nTest-tailer: v1\nTest-tailer: v2", AddCommitMessageTrailer("title\n\nTest-tailer: v1", "Test-tailer", "v2")) - assert.Equal(t, "title\n\nTest-tailer: v1\nTest-tailer: v2", AddCommitMessageTrailer("title\n\nTest-tailer: v1\n", "Test-tailer", "v2")) -} - func prepareLoadMergeMessageTemplates(targetDir string) error { for _, template := range []string{"MERGE", "REBASE", "REBASE-MERGE", "SQUASH", "MANUALLY-MERGED", "REBASE-UPDATE-ONLY"} { file, err := os.Create(path.Join(targetDir, template+"_TEMPLATE.md"))