[PATCH 0/2] Normalize newlines in merge & interpret-trailer

[PATCH 0/2] Normalize newlines in merge & interpret-trailer

[Luca Weiss via GitGitGadget]

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.

Luca Weiss (2):
  trailer: handle input without trailing newline
  merge: make sure to terminate message with newline

 builtin/merge.c               | 1 +
 t/t7513-interpret-trailers.sh | 7 +++++++
 trailer.c                     | 3 +++
 3 files changed, 11 insertions(+)


base-commit: 75ae10bc75336db031ee58d13c5037b929235912
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1048%2Fz3ntu%2Fmaster-newline-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1048/z3ntu/master-newline-v1
Pull-Request: https://github.com/git/git/pull/1048
-- 
gitgitgadget

[PATCH 1/2] trailer: handle input without trailing newline

[Luca Weiss via GitGitGadget]

From: Luca Weiss <luca@z3ntu.xyz>

Add a corresponding test case for this as well

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 t/t7513-interpret-trailers.sh | 7 +++++++
 trailer.c                     | 3 +++
 2 files changed, 10 insertions(+)

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 &&
 	cat >complex_message_body <<-\EOF &&
 		my subject
 
@@ -676,6 +677,12 @@ test_expect_success 'with message that has an old style conflict block' '
 	test_cmp expected actual
 '
 
+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
+'
+
 test_expect_success 'with commit complex message and trailer args' '
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
diff --git a/trailer.c b/trailer.c
index 7c7cb61a945..f53cf7d07d4 100644
--- a/trailer.c
+++ b/trailer.c
@@ -765,6 +765,9 @@ static void read_input_file(struct strbuf *sb, const char *file)
 		if (strbuf_read(sb, fileno(stdin), 0) < 0)
 			die_errno(_("could not read from stdin"));
 	}
+
+	/* Make sure the input ends with a newline */
+	strbuf_complete_line(sb);
 }
 
 static const char *next_line(const char *str)
-- 
gitgitgadget

Re: [PATCH 1/2] trailer: handle input without trailing newline

[Jeff King]

On Fri, Jul 16, 2021 at 07:43:35AM +0000, Luca Weiss via GitGitGadget wrote:

> From: Luca Weiss <luca@z3ntu.xyz>
> 
> Add a corresponding test case for this as well

The details you put in the cover letter won't make it into the Git
history. This would be a good place to put them.

>  t/t7513-interpret-trailers.sh | 7 +++++++
>  trailer.c                     | 3 +++
>  2 files changed, 10 insertions(+)

That patch itself looks good to me, with one minor style nit:

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

Our preferred style is to omit whitespace between redirection operators
and filenames (i.e., ">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
> +'

Ditto here.

-Peff

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

[Luca Weiss via GitGitGadget]

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",
-- 
gitgitgadget

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

[Phillip Wood]

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",
> 

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

[Luca Weiss]

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

Regards
Luca

p.s. sorry for replying off-list, and if this is wrongly bottom/top posted, email client troubles ;)

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",
>> 
>

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

[Phillip Wood]

Hi Luca

On 16/07/2021 13:37, Luca Weiss wrote:
> 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

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.

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

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.

> Regards
> Luca
> 
> p.s. sorry for replying off-list, and if this is wrongly bottom/top posted, email client troubles ;)

This message seems to have made it onto the list. We normally reply in 
line as I've done here but don't worry if you're having problems with 
your mail client.

I've not had time to look at the first patch properly but from a quick 
glance it seems to be a sensible approach

Best Wishes

Phillip

> 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",
>>>
>>

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

[Jeff King]

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.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Whew.  You got me worried.  Yeah, if fmt_merge_msg() is not
terminating its output, we should fix that, but I do not see such a
breakage in the current code, either.

Thanks.

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

[Phillip Wood]

On 16/07/2021 22:10, Jeff King wrote:
> 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.

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);

This has been the case since the beginning of the builtin merge[1]. I 
assume it was trying to emulate the result of a command substitution in 
the shell version.

Best Wishes

Phillip

[1] See 
https://lore.kernel.org16229b1d-e4a6-7a8d-8ea0-ae7c3f13075d@gmail.com/ 
for more details of my archaeology on this.

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



> 
> -Peff
> 

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

[Jeff King]

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

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

[Luca Weiss]

On Samstag, 17. Juli 2021 19:47:20 CEST Jeff King wrote:
> 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

Hi Peff,

if you have a good idea on how to create these patches, feel free to do so.
If not, I can take a shot at it this or next week.

Regards
Luca

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

[Luca Weiss]

Hi Peff and others,

On Samstag, 17. Juli 2021 19:47:20 CEST Jeff King wrote:
> [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

I haven't found time to revisit the patches yet but I have found another 
unexpected behavior with git merge that is shown with these commands:

# git needs to be set up to sign commits with gpg
mkdir /tmp/test
cd /tmp/test
git init
git commit --allow-empty -m "Foo1"
git commit --allow-empty -m "Foo2"
git tag -s tag_foo -m "foooo!"
git checkout HEAD~1
git merge --no-ff --no-edit --log -m "Merge tag_foo" tag_foo
# git show

There are two problems with the resulting commit message:
* The newline between -m message and tag message is missing
* There's a big pgp signature (BEGIN PGP SIGNATURE) block included in the 
commit message. When using an editor this would be removed because the text 
starts with comments.

I don't think I have enough insight into where to fix this so I'd appreciate if 
somebody else could fix it :)

Regards
Luca

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

[Junio C Hamano]

"Luca Weiss via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Luca Weiss <luca@z3ntu.xyz>

Please explain what you are trying to fix here.  The title hints
that this change is to deal with an message that ends in an
incomplete line, but where does that incomplete line come?

I find it somewhat important to know that details especially because
in this patch ...

> 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);

... the placement of this call looks dubious.

Before the context of this hunk, we 

 - started with an empty msg at the beginning of this function;

 - added merge_msg to msg, where merge_msg came from options like -F
   and -m;

 - optionally added to msg comments to be shown in the editor; and

 - optionally called append_signoff() to add s-o-b.

At which step do we make msg end with an incomplete line and under
what condition?

If it is after processing -F/-m and if we did not call the editor,
for example, we "append" the sign-off after that incomplete line and
your new call to strbuf_complete_line() would be too late if that is
the case, wouldn't it?  I do not know if that is, since your commit
log message lacks meaningful details, and that is exactly why people
want you to explain what breakage you observed and what your fix is.

Thanks.

>  	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",

Re: [PATCH 0/2] Normalize newlines in merge & interpret-trailer

[Junio C Hamano]

"Luca Weiss via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

When you fold some of what you wrote in the above into the proposed
commit log message proper when you send an updated version of the
series, please pay special attention to the phrases like "empty
line", "normalize newline" and "terminated by a newline".

 - As there are some folks who use Git on Windows on this list, when
   we say "normalize the newlines", they will think of CRLF vs LF,
   but I do not think that is what you are talking about here.

 - As I asked in my review of one of your patches, please explain
   where the incomplete line comes from (e.g. saying "if the user
   ends the edited log message with an incomplete line" would make
   it clear how we missed such an incomplete line to come into the
   system).

 - I am guessing that "without an empty line" is because we usually
   append trailers with one newline after the last line of the log
   message, with the expectation that the existing log message ends
   with a complete line, but an incomplete line at the end of the
   log message absorbs the newline and makes it as part of the last
   line that is now a complete line?  And a trailer block that is
   not separated with a blank line from the last paragraph of the
   message body is not taken as a trailer block, causing the later
   parsing to fail, but from your description it was unclear how
   the trailer block is added without the paragraph break.

Thanks.