Normalize newlines in merge & interpret-trailer

From: Luca Weiss <[email protected]>

Signed-off-by Luca Weiss <[email protected]>

---
  builtin/merge.c | 1 +
  1 file changed, 1 insertion(+)

builtin/merge.c +1 -0

index a8a843b1f54..646bb49367f 100644
@@ -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

Quoted 20 lines · Luca Weiss via GitGitGadget

From: Luca Weiss <[email protected]>

Signed-off-by Luca Weiss <[email protected]>

---
  builtin/merge.c | 1 +
  1 file changed, 1 insertion(+)

builtin/merge.c +1 -0

index a8a843b1f54..646bb49367f 100644
@@ -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).

Regards
Luca

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

Quoted 45 lines

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

Quoted 20 lines · Luca Weiss via GitGitGadget

From: Luca Weiss <[email protected]>

Signed-off-by Luca Weiss <[email protected]>

---
   builtin/merge.c | 1 +
   1 file changed, 1 insertion(+)

builtin/merge.c +1 -0

index a8a843b1f54..646bb49367f 100644
@@ -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",

Quoted 15 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

Quoted 1 line

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

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.

From: Luca Weiss <[email protected]>

Add a corresponding test case for this as well

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

From: Luca Weiss <[email protected]>

Signed-off-by Luca Weiss <[email protected]>

---
 builtin/merge.c | 1 +
 1 file changed, 1 insertion(+)

builtin/merge.c +0 -0

index a8a843b1f54..646bb49367f 100644

builtin/merge.c +0 -0

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

Quoted 10 lines · Jeff King

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.

Quoted 11 lines

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.

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.

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.

Quoted 28 lines · Junio C Hamano

Quoted 10 lines · Jeff King

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

Quoted 3 lines

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

Quoted 14 lines

Quoted 11 lines

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.

Quoted 9 lines

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

Quoted 2 lines

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

Quoted 11 lines · Phillip Wood

Quoted 2 lines

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