Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 29, 2018, 02:11 UTC
- Message-ID
- <xmqqk1kwr5tp.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <3e24a770-47fc-50e4-d757-1e4a28dcd019@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
Show 13 quoted lines
> Am 27.11.18 um 00:31 schrieb Junio C Hamano: >> Johannes Sixt <j6t@kdbg.org> writes: >>> Am 26.11.18 um 04:04 schrieb Junio C Hamano: >>> ... this goes too far, IMO. It is the pager's task to decode control >>> characters. >> >> It was tongue-in-cheek suggestion to split a CR into caret-em on our >> end, but we'd get essentially the same visual effect if we added a >> rule: >> >> When producing a colored output (not limited to whitespace >> error coloring of diff output), insert <RESET> before a CR >> that comes immediately before a LF.
This was misspoken a bit. Let's revise it to
When producing a colored output (not limited to whitespace error coloring of diff output) for contents that are not marked as eol=crlf (and other historical means), insert <RESET> before a CR that comes immediately before a LF.
to exempt CR in CRLF sequence that the contents and the user agree to be part of the end-of-line marker.
> I wouldn't want that to happen for all output (context lines, - lines, > + lines): I really am not interested to see all the CRs in my CRLF > files.
So your CRLF files will not give caret-em.
Show 9 quoted lines
>> And a good thing is that I do not think that new rule is doing any >> decode of control chars on our end. We are just producing colored >> output normally. > > But we already have it, as Brian pointed out: > > git diff --ws-error-highlight=old,new > > or by setting diff.wsErrorHighlight accordingly.
Not really. If the context lines end with CRLF and the material is not CRLF, I do not think CR at the end of them should be highlighted as whitespace errors (as we tell to detect errors on "old" and "new" lines), but I think we still should make an effort to help them be visible to the users. Otherwise, next Frank will come and complain after seeing
-something some common thing +something_new^M
With the suggested change, you can distinguish the following two (and use your imagination that there are many "some common thing" lines), which would help the user guide if the file should be marked as CRLF file, or the contents mistakenly has some lines that end with CRLF by mistake. The corrective action between the two cases would vastly be different.
-something^M -something some common thing^M some common thing some common thing^M some common thing some common thing^M some common thing +something_new^M +something_new^M
Without, both would look like the RHS of the illustration, and with the "highlight both", you'd only get an extra caret-em on the removed line, and need to judge without the help from common context lines.
Besides, --ws-error-highlight shows all whitespace errors, and showing CR as caret-em is mere side effect of the interaction between its coloring and the pager.