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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 16, 2021, 20:34 UTC
Message-ID: <xmqq8s26q9ot.fsf@gitster.g>
In-Reply-To: <YPHe/W7+Oh63NpB0@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 10 quoted lines

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

Show 11 quoted 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.