Always use an empty line to separate the commit message and trailer #34512
Conversation
If the message from form.MergeMessageField is empty, we will miss a "\n" between the title and the "Co-authored-by:" line. The title and message should have a blank line between of them.
routers/web/repo/pull.go
Outdated
| @@ -1109,8 +1109,9 @@ func MergePullRequest(ctx *context.Context) { | ||
| } | ||
|
|
||
| form.MergeMessageField = strings.TrimSpace(form.MergeMessageField) | ||
| message += "\n" |
There was a problem hiding this comment.
But if form.MergeMessageField was empty, then there will be a unnecessary \n used for the empty message?
There was a problem hiding this comment.
We encountered a case where the commit message is empty, and the code at this line appends "\nCo-authored-by: " + sig.String(), using only one \n instead of two. As a result, there is no extra blank line between the commit title and the message body.
Would it be acceptable to always add a \n after the commit title, even if the message body is completely empty (i.e., not even the auto-appended Co-authored-by: ... line)?
There was a problem hiding this comment.
Would it be acceptable to always add a \n after the commit title, even if the message body is completely empty (i.e., not even the auto-appended
Co-authored-by: ...line)?
Could you design some test cases to show the expected output for each case?
There was a problem hiding this comment.
Here is the case we encountered. The commit title and Co-authored-by: line don't have an extra blank line between of them if the commit message is empty. Then it looks like from git log command
and what we see from tig
I'm not familiar with gitea project. Can you guide me how can I add the testcase for my change? Thanks.
There was a problem hiding this comment.
I think we can do this: 669cccd
Then the \n can be handled correctly for all cases.
There was a problem hiding this comment.
Maybe it's better to have a function like addCommitMessageTailer("Co-authored-by", "..."), and let it handle all cases.
There was a problem hiding this comment.
Maybe it's better to have a function like
addCommitMessageTailer("Co-authored-by", "..."), and let it handle all cases.
Try this one: 26e1f78
There was a problem hiding this comment.
Does the new change work?
There was a problem hiding this comment.
It looks good to me. Thanks.
…o-gitea#34512) If the message from form.MergeMessageField is empty, we will miss a "\n" between the title and the "Co-authored-by:" line. The title and message should have a blank line between of them. --------- Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
…34512) (#34578) Backport #34512 by tclin914 If the message from form.MergeMessageField is empty, we will miss a "\n" between the title and the "Co-authored-by:" line. The title and message should have a blank line between of them. Co-authored-by: Jim Lin <jim@andestech.com> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
…railer (#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 <jim@andestech.com> Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8041 Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org> Co-authored-by: Antonin Delpeuch <antonin@delpeuch.eu> Co-committed-by: Antonin Delpeuch <antonin@delpeuch.eu>
* giteaofficial/main: Refactor some tests (go-gitea#34580) Do not mutate incoming options to SearchRepositoryByName (go-gitea#34553) Fix/improve avatar sync from LDAP (go-gitea#34573) Fix some trivial problems (go-gitea#34579) Retain issue sort type when a keyword search is introduced (go-gitea#34559) Always use an empty line to separate the commit message and trailer (go-gitea#34512) Fix line-button issue after file selection in file tree (go-gitea#34574) [skip ci] Updated translations via Crowdin Fix doctor deleting orphaned issues attachments (go-gitea#34142) [skip ci] Updated translations via Crowdin Fix actions skipped commit status indicator (go-gitea#34507) Clean up "file-view" related styles (go-gitea#34558) Add "View workflow file" to Actions list page (go-gitea#34538) Do not mutate incoming options to RenderUserSearch and SearchUsers (go-gitea#34544) Add webhook assigning test and fix possible bug (go-gitea#34420) Fix possible nil description of pull request when migrating from CodeCommit (go-gitea#34541) Refactor commit reader (go-gitea#34542)
If the message from form.MergeMessageField is empty, we will miss a "\n" between the title and the "Co-authored-by:" line. The title and message should have a blank line between of them.