Re: [PATCH v4 1/2] ref-filter: handle CRLF at end-of-line more gracefully

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 23, 2020, 00:52 UTC
Message-ID: <xmqq8sbxlq62.fsf@gitster.c.googlers.com>
In-Reply-To: <03b2d7d78a15d15130a68ed1e6092072aa0807cd.1603335680.git.gitgitgadget@gmail.com>

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 16 quoted lines

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> The ref-filter code does not correctly handle commit or tag messages
> that use CRLF as the line terminator. Such messages can be created with
> the `--cleanup=verbatim` option of `git commit` and `git tag`, or by
> using `git commit-tree` directly.
>
> The function `find_subpos` in ref-filter.c looks for two consecutive
> LFs to find the end of the subject line, a sequence which is absent in
> messages using CRLF. This results in the whole message being parsed as
> the subject line (`%(contents:subject)`), and the body of the message
> (`%(contents:body)`) being empty.
>
> Moreover, in `copy_subject`, which wants to return the subject as a
> single line, '\n' is replaced by space, but '\r' is
> untouched.

Honestly, all of the above signal, at least to me, that these objects are designed to use LF terminated lines and nothing else, whether Windows or DOS existed in the same world or not. There is no such thing as commit objects that use CRLF as the line terminator. They are commit objects whose payload has CR at the end of each and every line. Just like there can be commit objects whose payload has trailing SP on each line, or even has binary guck, these things can be created using the "commit --cleanup=verbatim" command, or the "hash-objects" command. It does not mean it is encouraged to create such objects. It does not mean it is sensible to expect them to behave as if these trailing whitespaces (be it SP or CR) are not there.

> This impacts the output of `git branch`, `git tag` and `git
> for-each-ref`.

The answer to that problem description is "then don't" ;-). If you do not want to have trailing whitespaces, you need to clean them up somehow, and we give an easy way to do so with the default --cleanup action. Setting it to verbatim is to decline that easy way offered to you, and it makes it your responsibility to do your own clean-up if you still want to remove the CR at the end of your lines.

Having said all that.

Here is how I explained the topic in the "What's cooking" report.

     A commit and tag object may have CR at the end of each and
     every line (you can create such an object with hash-object or
     using --cleanup=verbatim to decline the default clean-up
     action), but it would make it impossible to have a blank line
     to separate the title from the body of the message.  Be lenient
     and accept a line with lone CR on it as a blank line, too.

Let's not call this change a "bug fix". The phrase you used in your title, "more gracefully", is a very good one.

In the meantime, I've squashed your "oops forgot ||return 1" change into [PATCH 2/2].

Thanks.