Re: [PATCH 2/2] merge: make sure to terminate message with newline

From: Jeff King <peff@peff.net>
Date: Jul 16, 2021, 19:33 UTC
Message-ID: <YPHe/W7+Oh63NpB0@coredump.intra.peff.net>
In-Reply-To: <16229b1d-e4a6-7a8d-8ea0-ae7c3f13075d@gmail.com>

On Fri, Jul 16, 2021 at 06:30:50PM +0100, Phillip Wood wrote:

Show 30 quoted lines

> > I have a simple reproducer here:
> > 
> > mkdir /tmp/test
> > cd /tmp/test
> > git init
> > echo 'dest="$1.tmp"; git interpret-trailers --trailer "Foo: Bar" < "$1" > "${dest}"; mv "${dest}" "$1"' > .git/hooks/commit-msg
> > chmod +x .git/hooks/commit-msg
> > git commit --allow-empty -m "Initial commit"
> > sleep 1
> > git switch -c foobar
> > git commit --allow-empty -m "Foo1"
> > sleep 1
> > git commit --allow-empty -m "Foo2"
> > git switch master
> > git merge --no-ff --no-edit foobar
> > # look at merge commit message now
> 
> Thanks for the reproducer, I can confirm it shows the bug for me. What I
> missed this morning was that we promptly chop the '\n' off the end of the
> message we get back from fmt_merge_msg(). I've looked through the history
> and this behavior dates back to the beginning of the builtin merge added in
> 1c7b76be7d ("Build in merge", 2008-07-07). Back then we added a newline to
> the end of the message before writing .git/MERGE_MSG or committing in
> finish_automerge() but merge_trivial() did not add a new line before
> committing. Commit 66f4b98ad9 ("Teach merge the '[-e|--edit]' option",
> 2011-10-08) added prepare_to_commit() which added the newline and was called
> by both finish_automerge() and merge_trivial(). This behavior was changed in
> d540b70c85 ("merge: cleanup messages like commit", 2019-04-17) after which
> we only added a newline if the message was going to be edited. I've cc'd
> Denton to see if he remembers if this was intentional or not.

I think we still end up calling cleanup_message() on the result before using it as the final message, and that will fix any missing newline. But we feed the intermediate state before then to the hooks (which is exactly where one might expect to use interpret-trailers).

I'm not sure if we should be doing a preemptive call to cleanup_message() before feeding the hooks (we'd still need to do the final one, to clean up whatever the hooks return to us). I guess probably not, because I think that would remove comments, as well. So adding in just the missing newline is probably better.

Show 5 quoted lines

> I suspect the best way to fix this is to stop stripping the newline that is
> added by fmt_merge_msg() and remove the line in prepare_to_commit() that
> adds the newline when editing. That would leave '-F' untouched so it would
> still not add missing newline in that case - I'm not sure if that is
> desirable or not but I think it matches what 'git commit -F' does.

Since it will usually be added back in by cleanup_message() anyway, I think it's OK to just add it preemptively. The exception would be if the user asked for no cleanup at all. So making it conditional on cleanup_mode would work, whether -F or not.

I suppose that does mean people turning off cleanup mode would get a message without a newline from fmt_merge_msg(), though, which is perhaps unexpected.

So maybe just keeping the newline there, as you suggest, is the best way.

> The commit message should explain why you're making the change - that is not
> unnecessary detail but essential context to help others reading the history
> in the future to understand the reason for the change.

Yes. A summary of the problem, the reasoning, and the discussion here would be appropriate for the commit message.

-Peff