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

On 16/07/2021 22:10, Jeff King wrote:

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.

-Peff