git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Philippe Blain <levraiphilippeblain@gmail.com>
Date
Oct 23, 2020, 01:46 UTC
Message-ID
<FFAF7079-C759-43F3-96AA-CAF3B73B55B4@gmail.com>
In-Reply-To
<xmqq8sbxlq62.fsf@gitster.c.googlers.com>
Hi Junio,
Show 43 quoted lines
> Le 22 oct. 2020 à 20:52, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> 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.

I agree with you on that : if you are creating the object yourself, you should let the default cleanup take place.

But as a lot of projects use GitHub, GitLab or similar services to accept contributions, and let these web systems perform the "merge" (or rebase or whatever) operation to integrate these contributions; maintainers sometime choose to not always have complete control on all objects that become part of the canonical history of their repository. And as I wrote in [1], GitLab was creating commits using CRLF up until 9.2... [2]. So for these poor projects that are now stuck with these CRLFs in their merge commit messages, I think it's good that Git handles these correctly.

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

Just for the sake of searchability, I think it would be good to have CRLF spelled out in this topic description (since I gather this is what ends up in the release notes). But I don't feel that strongly about that.

> Let's not call this change a "bug fix".  The phrase you used in your
> title, "more gracefully", is a very good one.
It was your suggestion ;) 
> In the meantime, I've squashed your "oops forgot ||return 1" change
> into [PATCH 2/2].
Thanks for squashing it in.

Cheers, Philippe.

[1] https://lore.kernel.org/git/63755050-10A5-4A46-9BB3-8207E055692C@gmail.com/ [2] https://gitlab.com/gitlab-org/gitlab-foss/-/issues/31671

Previous: Junio C HamanoNext: Junio C Hamano
Message 33 of 38 in “Teach ref-filter API to correctly handle CRLF in messages”
  1. 0/3 Teach ref-filter API to correctly handle CRLF in messagesPhilippe Blain via GitGitGadget, Mar 8, 2020
  2. 3/3 log: add tests for messages containing CRLF to t4202Philippe Blain via GitGitGadget, Mar 8, 2020
  3. 1/3 t: add lib-crlf-messages.sh for messages containing CRLFPhilippe Blain via GitGitGadget, Mar 8, 2020
  4. 2/3 ref-filter: teach the API to correctly handle CRLFPhilippe Blain via GitGitGadget, Mar 8, 2020
  5. Junio C HamanoMar 9, 2020
  6. Philippe BlainMar 10, 2020
  7. 0/3 Teach ref-filter API to correctly handle CRLF in messagesPhilippe Blain via GitGitGadget, Mar 10, 2020
  8. 1/3 t: add lib-crlf-messages.sh for messages containing CRLFPhilippe Blain via GitGitGadget, Mar 10, 2020
  9. 2/3 ref-filter: fix the API to correctly handle CRLFPhilippe Blain via GitGitGadget, Mar 10, 2020
  10. Junio C HamanoMar 10, 2020
  11. 3/3 log: add tests for messages containing CRLF to t4202Philippe Blain via GitGitGadget, Mar 10, 2020
  12. Junio C HamanoMar 10, 2020
  13. Junio C HamanoMar 10, 2020
  14. 0/3 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 12, 2020
  15. 2/3 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 12, 2020
  16. Junio C HamanoOct 12, 2020
  17. Philippe BlainOct 14, 2020
  18. 1/3 t: add lib-crlf-messages.sh for messages containing CRLFPhilippe Blain via GitGitGadget, Oct 12, 2020
  19. Junio C HamanoOct 12, 2020
  20. Philippe BlainOct 14, 2020
  21. Eric SunshineOct 12, 2020
  22. Philippe BlainOct 14, 2020
  23. Eric SunshineOct 14, 2020
  24. Philippe BlainOct 14, 2020
  25. Eric SunshineOct 14, 2020
  26. Philippe BlainOct 22, 2020
  27. 3/3 log, show: add tests for messages containing CRLFPhilippe Blain via GitGitGadget, Oct 12, 2020
  28. 0/2 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 22, 2020
  29. 2/2 log, show: add tests for messages containing CRLFPhilippe Blain via GitGitGadget, Oct 22, 2020
  30. Philippe BlainOct 22, 2020
  31. 1/2 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 22, 2020
  32. Junio C HamanoOct 23, 2020
  33. Philippe BlainOct 23, 2020
  34. Junio C HamanoOct 28, 2020
  35. Philippe BlainOct 29, 2020
  36. 0/2 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 29, 2020
  37. 1/2 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 29, 2020
  38. 2/2 log, show: add tests for messages containing CRLFPhilippe Blain via GitGitGadget, Oct 29, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.