Re: [Bug] commit-tree shouldn't append an extra newline to commit messages

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 5, 2017, 21:56 UTC
Message-ID: <xmqq3780byhw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170905170311.yhcksrw2bxevd3hk@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 22 quoted lines

> On Tue, Sep 05, 2017 at 12:57:21PM -0400, Ross Kabus wrote:
>
>> On Tue, Sep 5, 2017 at 11:36 AM, Jeff King <peff@peff.net> wrote:
>> 
>> > So I'd argue that "git commit -F" is doing a reasonable
>> > thing, and "commit-tree -F" should probably change to match it (because
>> > it's inconsistent, and because if anything the plumbing commit-tree
>> > should err more on the side of not touching its input than the porcelain
>> > commit command).
>> 
>> I would agree that "commit-tree -F" should change to match the behavior of
>> "git commit -F --cleanup=verbatim". I feel pretty strongly that this type of
>> cleanup logic shouldn't be done in a plumbing command, though I'm not sure
>> it is a big enough deal to change behavior/compatibility for everyone.
>
> OK. Do you want to try your hand at a patch?
>
>> Yup, confusion #2. I was using "-F -" which I see now is a different code
>> path. Reading via stdin without "-F -" _is_ the verbatim option. This
>> difference burned someone else on the mailing list as well. See:
>
> Ah, OK, your confusion makes more sense now.

Yeah, my initial knee-jerk reaction when I started reading the early part of the thread was that the use of strbuf_complete_line() is a strong enough sign that this was intended behaviour, but the "we get different results between reading from standard input and pretending '-' is a file and reading from it" made me change my mind. I agree that dropping strbuf_complete_line() call from that codepath (and nowhere else, especially the "-m" oneline thing [*1*]) would be a backward incompatible change that is acceptable.

Here is what I am preparing to add to the What's cooking report when such a patch materializes ;-)

 * "git commit-tree -F $file" used to add terminating newline if the
   input ends with an incomplete line, but when the command reads
   the input from its standard input, we recorded what was given
   verbatim.  The former has been changed to record the input with
   an incomplete line to make them consistent.

Something like that probably needs to be added to the release notes but I am way being ahead of myself ;-)

Thanks, both, for sensible discussion.

[Footnote]

*1* The message from Ross you are responding to talks about
    "cleanup", but I tend to view the calls to *_complete_line() as
    more for "the end-user convenience".  "-m" meant to take a
    one-liner from the command line should have it because exactly
    as you said, having to pass the terminating newline from the
    command line with "-m" is awkward.  On the other hand, if the
    user/caller prepared the exact message in a file and wants to
    feed it either via "-F" or from the standard input, I agree that
    "the end-user convenience" would get in the way and it is
    preferrable to avoid it in the plumbing layer.