Commit messages do not include an ending carriage return

[talios]

Describe the bug

When making a commit, the message does not include a terminating carriage return in it's message, this
causes problems with tooling/hooks that update the commit message and add footers/trailers (such as the Gerrit code review tool).

This means you end up with a message like:

build(bug-1234): Updated dependencies
Change-Id: XXXXXXX

which doesn't include a blank line separating the commit message from it's footers.

To Reproduce

Steps

• Make a commit, any commit with -e to edit the message
• Note in the editor there's no carriage return in the message.

Expected behavior

• A carriage return/linefeed in the editor at the end of the line.

Actual behavior

• No new line.

Versions

Version:        1.2.0
Go version:     go1.14.12
Git commit:     180ee44
Built:          2020-11-21T13:06:18Z
OS/Arch:        darwin/amd64⏎ 

[b4nst]

Hi, thanks for reporting. I'm not sure about the behavior here. Doesn't adding a carriage return would break the Commit message with no body? I want to keep the git commit default behavior and I though it was to let the CR/LF to the hooks/tools?
I may be totally wrong here, in that case this should be a quick fix but I want to be sure about it first.

Whatever the decision, I will at least add an option (tug options are managed through git) to add this CR/LF automatically since it seems to be useful to at least you.

[talios]

A quick test using the standard git -m command for a "commit message with no body" including a signoff gives a blank line between the summary and body (my understanding of the git commit format - much like SMTP headers): summary, blank line, body, blank line, trailers.

/t/testing on  master [+] ❯ git commit -a -m "Test" -s
[master (root-commit) d100763] Test
 1 file changed, 2 insertions(+)
 create mode 100644 test.txt
/t/testing on  master ❯ git log
commit d10076354de294a79c2114e2a5f5aa5399aefdc5 (HEAD -> master)
Author: Mark Derricutt <mark@talios.com>
Date:   Thu Jan 28 20:03:16 2021 +1300

    Test

    Signed-off-by: Mark Derricutt <mark@talios.com>

Interesting - reading the docs for git-interpret-trailers at http://manpages.ubuntu.com/manpages/cosmic/man1/git-interpret-trailers.1.html :

By default the new trailer will appear at the end of all the existing trailers. If there is no existing trailer, the new trailer will appear after the commit message part of the output, and, if there is no line with only spaces at the end of the commit message part, one blank line will be added before the new trailer.

From this one could argue that it's up to the hook script from Gerrit to add that blank line - but it appears that tug's behaviour is different to git.

[talios]

Interesting - using tug against most of our projects repositories - some which use a newer version of Gerrit's commit hook, I see that my Change-Id trailers seen to get added correctly with a blank line ( the hook adds it ).

So this may not be required if I just update my hooks - tho it may be useful to have regardless.

[b4nst]

In your test you used -s option. In my understanding the blank line is added with the -s option, not after the subject. That can be reproduced with multiple -m options:

➜  git commit --allow-empty -m "First line" -m "Second line" -m "Third line"
[master fa47988] First line
➜  git log
commit fa479888ea93683f35ac8e7edb2c99920c814c30 (HEAD -> master)
Author: foo <foo@bar.com>
Date:   Sat Jan 30 11:03:27 2021 +0100

    First line

    Second line

    Third line

But I think tug commit build -s bug-1234 Updated dependencies should reproduce git commit --allow-empty -m "build(bug-1234): Updated dependencies" which it actually does:

commit 6f11f475ef53347b57474abce680476780034644 (HEAD -> master)  <---- turbogit
Author: foo <foo@bar.com>
Date:   Sat Jan 30 11:11:04 2021 +0100

    build(bug-1234): Updated dependencies

commit db5856c9696c224bf1ebaa507675653a413715e8                   <---- git commit
Author: foo <foo@bar.com>
Date:   Sat Jan 30 11:09:06 2021 +0100

    build(bug-1234): Updated dependencies

Do you agree with that?

[b4nst]

Closing since I think it's the expected behavior.