patch, 2 partsNormalize newlines in merge & interpret-trailer

> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index 04885d0a5e5..ff5f1724ad0 100755
> --- a/t/t7513-interpret-trailers.sh
> +++ b/t/t7513-interpret-trailers.sh
> @@ -17,6 +17,7 @@ test_expect_success 'setup' '
>  
>  		body
>  	EOF
> +	printf "subject\n\nbody" > basic_message_no_eol &&

> +test_expect_success 'with message without trailing newline twice' '
> +	git interpret-trailers --trailer "Cc: Peff" basic_message_no_eol > intermediary &&
> +	git interpret-trailers --trailer "Cc: Peff" intermediary > actual &&
> +	test_cmp intermediary actual
> +'

> From: Luca Weiss <luca@z3ntu.xyz>
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>   builtin/merge.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a8a843b1f54..646bb49367f 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -867,6 +867,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>   	}
>   	if (signoff)
>   		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
> +	strbuf_complete_line(&msg);
>   	write_merge_heads(remoteheads);
>   	write_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);
>   	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
> 

>
>Hi Luca
>
>Thanks for your patches. It would be very helpful to have the 
>explanation from the cover letter in the commit messages for both 
>commits to explain why this change is being made, otherwise that 
>information will not appear in the history.
>
>The cover letter says this happened when using '--no-edit', but unless 
>I've missed something 'git merge --no-edit' creates its message using 
>fmt_merge_msg() which calls strbuf_complete_line() just before it 
>returns. append_signoff() and 'merge -m' always terminate the message 
>with a newline. The only path I found that does not ensure the message 
>ends with a newline before calling the prepare-commit-msg hook is when 
>using '-F' and I suspect that may have been a deliberate decision but it 
>could be an oversight. In any case we would want to make sure that 'git 
>commit -F' and 'git merge -F' to behave the same which I think they do 
>at the moment.
>
>Best Wishes
>
>Phillip
>
>On 16/07/2021 08:43, Luca Weiss via GitGitGadget wrote:
>> From: Luca Weiss <luca@z3ntu.xyz>
>> 
>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>> ---
>>   builtin/merge.c | 1 +
>>   1 file changed, 1 insertion(+)
>> 
>> diff --git a/builtin/merge.c b/builtin/merge.c
>> index a8a843b1f54..646bb49367f 100644
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -867,6 +867,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>>   	}
>>   	if (signoff)
>>   		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
>> +	strbuf_complete_line(&msg);
>>   	write_merge_heads(remoteheads);
>>   	write_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);
>>   	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
>> 
>

> Hi Phillip,
> 
> So the behavior that I have observed is the following:
> 
> I've added a usleep for ~10 seconds instead of the line I added and
> .git/MERGE_MSG was not terminated with a newline.
> This didn't change when using --log
> It was properly handled with --signoff and the trailer was added
> correctly.
> 
> 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

> With my patch(es) this works properly.
> 
> If you have any other ideas on how to fix this, I am open for
> suggestions :)
> 
> Otherwise I'll try to add more detail to the individual commit
> messages (I deliberately kept the "unnecessary" detail out of the actual
> commit messages before).

> On July 16, 2021 12:23:06 PM GMT+02:00, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Luca
>>
>> Thanks for your patches. It would be very helpful to have the
>> explanation from the cover letter in the commit messages for both
>> commits to explain why this change is being made, otherwise that
>> information will not appear in the history.
>>
>> The cover letter says this happened when using '--no-edit', but unless
>> I've missed something 'git merge --no-edit' creates its message using
>> fmt_merge_msg() which calls strbuf_complete_line() just before it
>> returns. append_signoff() and 'merge -m' always terminate the message
>> with a newline. The only path I found that does not ensure the message
>> ends with a newline before calling the prepare-commit-msg hook is when
>> using '-F' and I suspect that may have been a deliberate decision but it
>> could be an oversight. In any case we would want to make sure that 'git
>> commit -F' and 'git merge -F' to behave the same which I think they do
>> at the moment.
>>
>> Best Wishes
>>
>> Phillip
>>
>> On 16/07/2021 08:43, Luca Weiss via GitGitGadget wrote:
>>> From: Luca Weiss <luca@z3ntu.xyz>
>>>
>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>>> ---
>>>    builtin/merge.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/builtin/merge.c b/builtin/merge.c
>>> index a8a843b1f54..646bb49367f 100644
>>> --- a/builtin/merge.c
>>> +++ b/builtin/merge.c
>>> @@ -867,6 +867,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>>>    	}
>>>    	if (signoff)
>>>    		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
>>> +	strbuf_complete_line(&msg);
>>>    	write_merge_heads(remoteheads);
>>>    	write_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);
>>>    	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
>>>
>>

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

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

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

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> To be quite honest, I think this patch is a half-way solution and I
> am not sure if it is better than either of the two "purist"
> extremes:
> 
>  * If the hooks want to see messages with as little loss of
>    information from the original as possible, we should give them
>    without clean-up (which you pointed out above) *and* without this
>    patch.
> 
>  * If the hooks want to see messages as canonicalized as people
>    would see in normal "git log" output, we should be passing the
>    full clean-up to lose even comments and in such a case there is
>    no need for this "always terminate" patch (we'd instead do far
>    more).
> 
> Between the two approaches, both are purists' view, I'd prefer the
> former, but from that stance, the conclusion would become that there
> is no need to do anything, which may be a bit unsatisfactory.

> > 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.
> 
> Hmph.
> 
> If the user tells us "refrain from touching my message as much as
> possible" and feeds us a proposed log message that ends with an
> incomplete line, I would think they expect us not to turn it into a
> complete line, and I would think doing this change only when cleanup
> is in effect would make sense.  This is especially true for users
> who do not let any hooks to interfere.  They used to get their
> incomplete lines intact, now their incomplete lines will
> unconditionally get completed.  I am not sure if I would want to
> defend this change from their complaints.

> which would cover that case. But Phillip mentioned that our own
> fmt_merge_msg() does not include a newline. So it would not be the user
> feeding us an incomplete line, but rather Git feeding it.
>
> And that was
> what I suggested should be corrected (which I suppose would be in
> addition to correcting lines from the user).
>
> However, I see a call to strbuf_complete_line() at the end of
> fmt_merge_msg(), so I am puzzled about what he meant.

> On Fri, Jul 16, 2021 at 01:34:58PM -0700, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>> 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.
>>
>> To be quite honest, I think this patch is a half-way solution and I
>> am not sure if it is better than either of the two "purist"
>> extremes:
>>
>>   * If the hooks want to see messages with as little loss of
>>     information from the original as possible, we should give them
>>     without clean-up (which you pointed out above) *and* without this
>>     patch.
>>
>>   * If the hooks want to see messages as canonicalized as people
>>     would see in normal "git log" output, we should be passing the
>>     full clean-up to lose even comments and in such a case there is
>>     no need for this "always terminate" patch (we'd instead do far
>>     more).
>>
>> Between the two approaches, both are purists' view, I'd prefer the
>> former, but from that stance, the conclusion would become that there
>> is no need to do anything, which may be a bit unsatisfactory.
> 
> Yes, I agree with all of that (including the "as little loss of
> information as possible" preference).
> 
>>> 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.
>>
>> Hmph.
>>
>> If the user tells us "refrain from touching my message as much as
>> possible" and feeds us a proposed log message that ends with an
>> incomplete line, I would think they expect us not to turn it into a
>> complete line, and I would think doing this change only when cleanup
>> is in effect would make sense.  This is especially true for users
>> who do not let any hooks to interfere.  They used to get their
>> incomplete lines intact, now their incomplete lines will
>> unconditionally get completed.  I am not sure if I would want to
>> defend this change from their complaints.
> 
> Right, what I was suggesting was:
> 
>    if (cleanup_mode != COMMIT_MSG_CLEANUP_NONE)
> 	strbuf_complete(&msg);
> 
> which would cover that case. But Phillip mentioned that our own
> fmt_merge_msg() does not include a newline.

> So it would not be the user
> feeding us an incomplete line, but rather Git feeding it. And that was
> what I suggested should be corrected (which I suppose would be in
> addition to correcting lines from the user).
> 
> However, I see a call to strbuf_complete_line() at the end of
> fmt_merge_msg(), so I am puzzled about what he meant.

> > which would cover that case. But Phillip mentioned that our own
> > fmt_merge_msg() does not include a newline.
> 
> I mentioned that we remove the newline that is added by fmt_merge_msg(), not
> that there is no newline added by fmt_merge_msg() - maybe I wasn't clear
> enough. In builtin/merge.c:prepare_merge_message() we do
> 
> 	fmt_merge_msg(merge_names, merge_msg, &opts);
> 	if (merge_msg->len)
> 		strbuf_setlen(merge_msg, merge_msg->len - 1);

> On Sat, Jul 17, 2021 at 02:40:55PM +0100, Phillip Wood wrote:
> > > which would cover that case. But Phillip mentioned that our own
> > > fmt_merge_msg() does not include a newline.
> > 
> > I mentioned that we remove the newline that is added by fmt_merge_msg(),
> > not that there is no newline added by fmt_merge_msg() - maybe I wasn't
> > clear enough. In builtin/merge.c:prepare_merge_message() we do
> > 
> > 	fmt_merge_msg(merge_names, merge_msg, &opts);
> > 	if (merge_msg->len)
> > 	
> > 		strbuf_setlen(merge_msg, merge_msg->len - 1);
> 
> Of maybe I didn't read carefully enough. :)
> 
> Either way, thanks for clarifying. Doing something like:
> 
>   cat >.git/hooks/commit-msg <<\EOF
>   #!/bin/sh
>   xxd "$1"
>   EOF
>   chmod +x .git/hooks/commit-msg
> 
>   git merge --no-edit ...
> 
> shows off the problem; the hook sees that intermediate state.
> 
> Likewise if we do:
> 
>   git merge -m "foo" ...
> 
> which similarly suppresses the editor. There are actually two
> interesting cases here:
> 
>   - if merge.log is not set, then we'd see "foo" with no newline
> 
>   - if it is set, we'll get a newline after "foo", but with no newline
>     after the log data
> 
> Likewise for:
> 
>   printf foo >no-newline
>   git merge -F no-newline ...
> 
> So I think we'd probably want to see a 3-patch series:
> 
>   1. Make interpret-trailers handle input missing the final newline.
>      This isn't strictly necessary after patches 2 and 3, but it makes
>      sense to be more robust with unexpected input.
> 
>   2. Drop the newline-stripping from prepare_merge_message(). The
>      examples above show some ways we could cover this in the tests.
>      This will help --no-edit case, but also using merge.log with "-m"
>      or "-F".
> 
>   3. Teach prepare_to_commit() to add the extra newline before letting
>      hooks see the message. This should probably be done only when
>      cleanup_mode != COMMIT_MSG_CLEANUP_NONE.
> 
> Luca, do you want to try revising your series in that direction?
> 
> -Peff

> [cut]
> So I think we'd probably want to see a 3-patch series:
> 
>   1. Make interpret-trailers handle input missing the final newline.
>      This isn't strictly necessary after patches 2 and 3, but it makes
>      sense to be more robust with unexpected input.
> 
>   2. Drop the newline-stripping from prepare_merge_message(). The
>      examples above show some ways we could cover this in the tests.
>      This will help --no-edit case, but also using merge.log with "-m"
>      or "-F".
> 
>   3. Teach prepare_to_commit() to add the extra newline before letting
>      hooks see the message. This should probably be done only when
>      cleanup_mode != COMMIT_MSG_CLEANUP_NONE.
> 
> Luca, do you want to try revising your series in that direction?
> 
> -Peff

> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>  builtin/merge.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a8a843b1f54..646bb49367f 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -867,6 +867,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>  	}
>  	if (signoff)
>  		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
> +	strbuf_complete_line(&msg);

> These two patches fix a problem where the trailer would be appended to the
> commit message without an empty line, so parsing the trailers again
> afterwards would fail.
>
> In practice either one of the patches fixes the exact behavior I see but in
> both cases it makes sense to normalize the newlines.
>
> The exact use case where this issue was found is a "git merge --no-edit"
> with a commit-msg hook that adds a trailer immediately afterwards. The input
> the commit-msg script gets is not terminated by a newline (which is fixed by
> the second commit) while the first one makes interpret-trailer capable of
> handling such input without a final newline.