[gitea] Always use an empty line to separate the commit message and trailer (#8041)

This is a port of a gitea PR: go-gitea/gitea#34512.

I have added some copy-editing commits on top for cleanliness.
I haven't tested the changes manually and only relied on the existing automated test.

## Checklist
### Tests

- I added test coverage for Go changes...
  - [x] in their respective `*_test.go` for unit tests.
  - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [x] I do not want this change to show in the release notes.
- [ ] I want the title to show in the release notes with a link to this pull request.
- [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.

Co-authored-by: Jim Lin <[email protected]>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8041
Reviewed-by: Earl Warren <[email protected]>
Co-authored-by: Antonin Delpeuch <[email protected]>
Co-committed-by: Antonin Delpeuch <[email protected]>

Author: 2 people

services/pull/merge.go

@@ -14,6 +14,7 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+	"unicode"
 
 	"forgejo.org/models"
 	"forgejo.org/models/db"
@@ -169,6 +170,41 @@ 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 {

services/pull/merge_squash.go

@@ -5,7 +5,6 @@ package pull
 
 import (
 	"fmt"
-	"strings"
 
 	repo_model "forgejo.org/models/repo"
 	user_model "forgejo.org/models/user"
@@ -67,10 +66,8 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error {
 
 	if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() {
 		// add trailer
-		if !strings.Contains(message, fmt.Sprintf("Co-authored-by: %s", sig.String())) {
-			message += fmt.Sprintf("\nCo-authored-by: %s", sig.String())
-		}
-		message += fmt.Sprintf("\nCo-committed-by: %s\n", sig.String())
+		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).

services/pull/merge_test.go

@@ -65,3 +65,28 @@ 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"))
+}