From: Junio C Hamano <hidden> Date: 2021-07-16 20:35:05
Jeff King [off-list ref] 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.
From: Jeff King <hidden> Date: 2021-07-16 21:10:03
On Fri, Jul 16, 2021 at 01:34:58PM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
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).
quoted
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
On Fri, Jul 16, 2021 at 01:34:58PM -0700, Junio C Hamano wrote:
quoted
Jeff King [off-list ref] writes:
quoted
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).
quoted
quoted
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.
From: Jeff King <hidden> Date: 2021-07-17 17:47:23
On Sat, Jul 17, 2021 at 02:40:55PM +0100, Phillip Wood wrote:
quoted
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
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:
quoted
quoted
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
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