Skip to content

Always use an empty line to separate the commit message and trailer #34512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 2, 2025
Merged

Always use an empty line to separate the commit message and trailer #34512

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cfd13c1
Add "\n" after title unconditionally in case the message is empty
tclin914 May 21, 2025
669cccd
fix
wxiaoguang May 26, 2025
26e1f78
fix
wxiaoguang May 27, 2025
6364056
improve
wxiaoguang May 27, 2025
d296520
improve
wxiaoguang May 27, 2025
351dfb4
Merge branch 'main' into message-blank-line
wxiaoguang Jun 2, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion routers/web/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,8 +1109,9 @@ func MergePullRequest(ctx *context.Context) {
}

form.MergeMessageField = strings.TrimSpace(form.MergeMessageField)
message += "\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if form.MergeMessageField was empty, then there will be a unnecessary \n used for the empty message?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
螢幕擷取畫面 2025-05-26 154352
and what we see from tig
image

I'm not familiar with gitea project. Can you guide me how can I add the testcase for my change? Thanks.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do this: 669cccd

Then the \n can be handled correctly for all cases.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to have a function like addCommitMessageTailer("Co-authored-by", "..."), and let it handle all cases.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to have a function like addCommitMessageTailer("Co-authored-by", "..."), and let it handle all cases.

Try this one: 26e1f78
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the new change work?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tclin914 ping ~~
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. Thanks.
if len(form.MergeMessageField) > 0 {
message += "\n\n" + form.MergeMessageField
message += "\n" + form.MergeMessageField
}

if form.MergeWhenChecksSucceed {
Expand Down