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

Re: [PATCH v2 00/17] Peace with CRLF

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jan 4, 2016, 12:25 UTC
Message-ID
<alpine.DEB.2.20.1601041244370.14434@virtualbox>
In-Reply-To
<1450303398-25900-1-git-send-email-gitster@pobox.com>
Hi Junio,
On Wed, 16 Dec 2015, Junio C Hamano wrote:
Show 42 quoted lines
> I inspected all the callsites of this function to see if it is safe
> to use such an updated logic at these callsites, and did not find
> anything problematic.  I could update strbuf_getline() in place, but
> just to be extra careful, this series instead introduces another
> helper, strbuf_getline_crlf(), that is aware of this CRLF business,
> and convert the ones that are safe to update as we verify.
> 
>  * This series converts only the callers of strbuf_getline() that
>    would misbehave when fed a file with CRLF-terminated lines and
>    use the data with an unwanted CR appended at the end.  With the
>    update the code should work as intended with such a file, without
>    breaking the expected behaviour when working on a file with
>    LF-terminated lines.
> 
>  * Callers of strbuf_getline() that expect to only read from our own
>    output do not have to accommodate CRLF-terminated lines, but they
>    can be updated to do so safely if we do not rely on the ability
>    to express a payload that has CR at the end.  For example, the
>    insn sheet "rebase -i" uses typically ends each line with a
>    commit log summary that we ourselves do not read or use, so even
>    if the end-user on a platform with LF lines deliberately does
>    insert \r at the end of the line and strbuf_gets() removed that
>    \r from the payload, no unexpected behaviour should happen.
> 
>    This series does not touch them, but it may be a good GSoC
>    microproject to convert them to use strbuf_getline_crlf().
> 
>  * Callers of strbuf_getline() that call strbuf_trim() immediately
>    on the result before doing anything, or otherwise have logic that
>    tolerates whitespaces at the end of the line, can continue using
>    strbuf_getline() and will not misbehave on CRLF-terminated lines.
> 
>    This series does not touch them; converting them to use
>    strbuf_getline_crlf() would a good way to document them as
>    dealing with "text".  While doing so, they can also lose their
>    custom logic that happens to make CRLF-terminated lines work.
> 
>  * A caller of strbuf_getline() that wants to only see LF-terminated
>    lines (in other words, "ABC\r\n" must be treated as a line with 4
>    bytes payload on it, A B C and CR), would be broken if we replace
>    it with strbuf_getline_crlf().  This series does not touch them,
>    and no follow-up series should, either.

Thanks for the detailed explanation. I totally agree that (2) would make for a good micro-project.

While I am pretty convinced that strbuf_getline() that retains a CR makes no sense, it is obviously the correct thing to prove that rather than assume it (after all, some caller might exploit strbuf_getline() for its auto-growing capabilities even on a binary stream when it is known that it cannot contain a 0x0a).

And please accept my gratitude for tackling this project.

Ciao, Dscho

Previous: Johannes SchindelinNext: Junio C Hamano
Message 53 of 114 in “revisions --stdin: accept CRLF line terminators”
  1. revisions --stdin: accept CRLF line terminatorsJohannes Sixt, Aug 11, 2015
  2. Junio C HamanoAug 11, 2015
  3. Junio C HamanoAug 11, 2015
  4. Johannes SixtAug 12, 2015
  5. 00/17 Peace with CRLFJunio C Hamano, Oct 28, 2015
  6. 01/17 strbuf: add strbuf_gets()Junio C Hamano, Oct 28, 2015
  7. 02/17 check-attr, check-ignore, checkout-index: read paths with strbuf_gets()Junio C Hamano, Oct 28, 2015
  8. 03/17 update-index: read --index-info with strbuf_gets()Junio C Hamano, Oct 28, 2015
  9. 04/17 update-index: read list of paths with strbuf_gets() under --stdinJunio C Hamano, Oct 28, 2015
  10. 05/17 mktree: read textual tree representation with strbuf_gets()Junio C Hamano, Oct 28, 2015
  11. 06/17 hash-object: read --stdin-paths with strbuf_gets()Junio C Hamano, Oct 28, 2015
  12. 07/17 revision: read --stdin with strbuf_gets()Junio C Hamano, Oct 28, 2015
  13. 08/17 rev-parse: read parseopt spec with strbuf_gets()Junio C Hamano, Oct 28, 2015
  14. 09/17 ident.c: read /etc/mailname with strbuf_gets()Junio C Hamano, Oct 28, 2015
  15. 10/17 remote.c: read $GIT_DIR/remotes/* with strbuf_gets()Junio C Hamano, Oct 28, 2015
  16. 11/17 clone/sha1_file: read info/alternates with strbuf_gets()Junio C Hamano, Oct 28, 2015
  17. 12/17 transport-helper: read helper response with strbuf_gets()Junio C Hamano, Oct 28, 2015
  18. 13/17 cat-file: read batch stream with strbuf_gets()Junio C Hamano, Oct 28, 2015
  19. 14/17 column: read lines with strbuf_gets()Junio C Hamano, Oct 28, 2015
  20. 15/17 send-pack: read list of refs with strbuf_gets()Junio C Hamano, Oct 28, 2015
  21. 16/17 grep: read -f file with strbuf_gets()Junio C Hamano, Oct 28, 2015
  22. 17/17 test-sha1-array: read command stream with strbuf_gets()Junio C Hamano, Oct 28, 2015
  23. 00/17 Peace with CRLFJunio C Hamano, Dec 16, 2015
  24. 01/17 strbuf: make strbuf_getline_crlf() globalJunio C Hamano, Dec 16, 2015
  25. Johannes SchindelinJan 4, 2016
  26. Junio C HamanoJan 4, 2016
  27. 02/17 check-attr, check-ignore, checkout-index: read paths with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  28. Johannes SchindelinJan 4, 2016
  29. Johannes SchindelinJan 4, 2016
  30. Junio C HamanoJan 4, 2016
  31. 03/17 update-index: read --index-info with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  32. Johannes SchindelinJan 4, 2016
  33. Junio C HamanoJan 4, 2016
  34. 04/17 update-index: read list of paths with strbuf_getline_crlf() under --stdinJunio C Hamano, Dec 16, 2015
  35. Johannes SchindelinJan 4, 2016
  36. 05/17 mktree: read textual tree representation with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  37. Johannes SchindelinJan 4, 2016
  38. 06/17 hash-object: read --stdin-paths with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  39. 07/17 revision: read --stdin with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  40. 08/17 rev-parse: read parseopt spec with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  41. 09/17 ident.c: read /etc/mailname with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  42. 10/17 remote.c: read $GIT_DIR/remotes/* with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  43. 11/17 clone/sha1_file: read info/alternates with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  44. 12/17 transport-helper: read helper response with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  45. 13/17 cat-file: read batch stream with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  46. 14/17 column: read lines with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  47. 15/17 send-pack: read list of refs with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  48. 16/17 grep: read -f file with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  49. Johannes SchindelinJan 4, 2016
  50. Junio C HamanoJan 4, 2016
  51. 17/17 test-sha1-array: read command stream with strbuf_getline_crlf()Junio C Hamano, Dec 16, 2015
  52. Johannes SchindelinJan 4, 2016
  53. Johannes SchindelinJan 4, 2016
  54. 0/9 Preview of "Peace with CRLF" rerolledJunio C Hamano, Jan 14, 2016
  55. 1/9 strbuf: miniscule style fixJunio C Hamano, Jan 14, 2016
  56. 2/9 strbuf: make strbuf_getline_crlf() globalJunio C Hamano, Jan 14, 2016
  57. 3/9 strbuf: introduce strbuf_getline_{lf,nul}()Junio C Hamano, Jan 14, 2016
  58. 4/9 mktree: there are only two line terminatorsJunio C Hamano, Jan 14, 2016
  59. Jeff KingJan 14, 2016
  60. 5/9 check-attr: there are only two line terminatorsJunio C Hamano, Jan 14, 2016
  61. 6/9 check-ignore: there are only two line terminatorsJunio C Hamano, Jan 14, 2016
  62. 7/9 update-index: there are only two line terminatorsJunio C Hamano, Jan 14, 2016
  63. Jeff KingJan 14, 2016
  64. 8/9 checkout-index: there are only two line terminatorsJunio C Hamano, Jan 14, 2016
  65. Jeff KingJan 14, 2016
  66. Junio C HamanoJan 14, 2016
  67. Jeff KingJan 14, 2016
  68. 9/9 strbuf: give strbuf_getline() to the "most text friendly" variantJunio C Hamano, Jan 14, 2016
  69. 9/9 strbuf: retire strbuf_getline() for nowJunio C Hamano, Jan 14, 2016
  70. Junio C HamanoJan 14, 2016
  71. Jeff KingJan 14, 2016
  72. 00/21 Peace with CRLFJunio C Hamano, Jan 14, 2016
  73. 01/21 strbuf: miniscule style fixJunio C Hamano, Jan 14, 2016
  74. 02/21 strbuf: make strbuf_getline_crlf() globalJunio C Hamano, Jan 14, 2016
  75. 03/21 strbuf: introduce strbuf_getline_{lf,nul}()Junio C Hamano, Jan 14, 2016
  76. 04/21 mktree: there are only two possible line terminationsJunio C Hamano, Jan 14, 2016
  77. 05/21 check-attr: there are only two possible line terminationsJunio C Hamano, Jan 14, 2016
  78. Jeff KingJan 15, 2016
  79. Jeff KingJan 15, 2016
  80. Junio C HamanoJan 15, 2016
  81. Jeff KingJan 15, 2016
  82. Junio C HamanoJan 15, 2016
  83. 06/21 check-ignore: there are only two possible line terminationsJunio C Hamano, Jan 14, 2016
  84. 07/21 update-index: there are only two possible line terminationsJunio C Hamano, Jan 14, 2016
  85. 08/21 checkout-index: there are only two possible line terminationsJunio C Hamano, Jan 14, 2016
  86. Jeff KingJan 15, 2016
  87. Jeff KingJan 15, 2016
  88. Junio C HamanoJan 15, 2016
  89. Junio C HamanoJan 15, 2016
  90. 09/21 strbuf: give strbuf_getline() to the "most text friendly" variantJunio C Hamano, Jan 14, 2016
  91. Eric SunshineJan 15, 2016
  92. Junio C HamanoJan 15, 2016
  93. 10/21 hash-object: read --stdin-paths with strbuf_getline()Junio C Hamano, Jan 14, 2016
  94. Jeff KingJan 15, 2016
  95. 11/21 revision: read --stdin with strbuf_getline()Junio C Hamano, Jan 14, 2016
  96. Eric SunshineJan 15, 2016
  97. Junio C HamanoJan 15, 2016
  98. Jeff KingJan 15, 2016
  99. 12/21 rev-parse: read parseopt spec with strbuf_getline()Junio C Hamano, Jan 14, 2016
  100. 13/21 ident.c: read /etc/mailname with strbuf_getline()Junio C Hamano, Jan 14, 2016
  101. 14/21 remote.c: read $GIT_DIR/remotes/* with strbuf_getline()Junio C Hamano, Jan 14, 2016
  102. 15/21 clone/sha1_file: read info/alternates with strbuf_getline()Junio C Hamano, Jan 14, 2016
  103. Eric SunshineJan 15, 2016
  104. 16/21 transport-helper: read helper response with strbuf_getline()Junio C Hamano, Jan 14, 2016
  105. Jeff KingJan 15, 2016
  106. 17/21 cat-file: read batch stream with strbuf_getline()Junio C Hamano, Jan 14, 2016
  107. Jeff KingJan 15, 2016
  108. 18/21 column: read lines with strbuf_getline()Junio C Hamano, Jan 14, 2016
  109. Jeff KingJan 15, 2016
  110. 19/21 send-pack: read list of refs with strbuf_getline()Junio C Hamano, Jan 14, 2016
  111. 20/21 grep: read -f file with strbuf_getline()Junio C Hamano, Jan 14, 2016
  112. 21/21 test-sha1-array: read command stream with strbuf_getline()Junio C Hamano, Jan 14, 2016
  113. Jeff KingJan 15, 2016
  114. Jeff KingJan 15, 2016

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.