git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: phillip.wood@dunelm.org.uk
Cc: Luca Weiss <luca@z3ntu.xyz>,
	Luca Weiss via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Denton Liu <liu.denton@gmail.com>
Subject: Re: [PATCH 2/2] merge: make sure to terminate message with newline
Date: Fri, 16 Jul 2021 15:33:17 -0400	[thread overview]
Message-ID: <YPHe/W7+Oh63NpB0@coredump.intra.peff.net> (raw)
In-Reply-To: <16229b1d-e4a6-7a8d-8ea0-ae7c3f13075d@gmail.com>

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

> > 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.

> 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

  reply	other threads:[~2021-07-16 19:34 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-16  7:43 [PATCH 0/2] Normalize newlines in merge & interpret-trailer Luca Weiss via GitGitGadget
2021-07-16  7:43 ` [PATCH 1/2] trailer: handle input without trailing newline Luca Weiss via GitGitGadget
2021-07-16 19:35   ` Jeff King
2021-07-16  7:43 ` [PATCH 2/2] merge: make sure to terminate message with newline Luca Weiss via GitGitGadget
2021-07-16 10:23   ` Phillip Wood
2021-07-16 12:37     ` Luca Weiss
2021-07-16 17:30       ` Phillip Wood
2021-07-16 19:33         ` Jeff King [this message]
2021-07-16 20:34           ` Junio C Hamano
2021-07-16 21:10             ` Jeff King
2021-07-16 22:13               ` Junio C Hamano
2021-07-17 13:40               ` Phillip Wood
2021-07-17 17:47                 ` Jeff King
2021-07-21 10:41                   ` Luca Weiss
2021-08-26 18:32                   ` Luca Weiss
2021-07-16 20:20   ` Junio C Hamano
2021-07-16 22:10 ` [PATCH 0/2] Normalize newlines in merge & interpret-trailer Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YPHe/W7+Oh63NpB0@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=liu.denton@gmail.com \
    --cc=luca@z3ntu.xyz \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).