git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: phillip.wood@dunelm.org.uk, Luca Weiss <luca@z3ntu.xyz>,
	Luca Weiss via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Denton Liu <liu.denton@gmail.com>
Subject: Re: [PATCH 2/2] merge: make sure to terminate message with newline
Date: Fri, 16 Jul 2021 13:34:58 -0700	[thread overview]
Message-ID: <xmqq8s26q9ot.fsf@gitster.g> (raw)
In-Reply-To: <YPHe/W7+Oh63NpB0@coredump.intra.peff.net> (Jeff King's message of "Fri, 16 Jul 2021 15:33:17 -0400")

Jeff King <peff@peff.net> 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.

  reply	other threads:[~2021-07-16 20:35 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-16  7:43 [PATCH 0/2] Normalize newlines in merge & interpret-trailer Luca Weiss via GitGitGadget
2021-07-16  7:43 ` [PATCH 1/2] trailer: handle input without trailing newline Luca Weiss via GitGitGadget
2021-07-16 19:35   ` Jeff King
2021-07-16  7:43 ` [PATCH 2/2] merge: make sure to terminate message with newline Luca Weiss via GitGitGadget
2021-07-16 10:23   ` Phillip Wood
2021-07-16 12:37     ` Luca Weiss
2021-07-16 17:30       ` Phillip Wood
2021-07-16 19:33         ` Jeff King
2021-07-16 20:34           ` Junio C Hamano [this message]
2021-07-16 21:10             ` Jeff King
2021-07-16 22:13               ` Junio C Hamano
2021-07-17 13:40               ` Phillip Wood
2021-07-17 17:47                 ` Jeff King
2021-07-21 10:41                   ` Luca Weiss
2021-08-26 18:32                   ` Luca Weiss
2021-07-16 20:20   ` Junio C Hamano
2021-07-16 22:10 ` [PATCH 0/2] Normalize newlines in merge & interpret-trailer Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq8s26q9ot.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=liu.denton@gmail.com \
    --cc=luca@z3ntu.xyz \
    --cc=peff@peff.net \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).