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@xxxxxxxxx> 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@xxxxxxxxx>
Signed-off-by: Luca Weiss <luca@xxxxxxxxx>
---
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",
