BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Frank Schäfer]

The CR marker ^M doesn't show up in '-' lines of diffs when the ending
of the removed line is CR+LF.
It shows up as expected in '-' lines when the ending of the removed line
is CR only.
It also always shows up as expected in '+' lines.

These are the diffs of the 6 possible line ending changes:

LF to CR+LF:
@@ -1,2 +1,2 @@
-aaa
+aaa^M
 bbb
 
CR+LF to LF:
@@ -1,2 +1,2 @@
-aaa            => BUG: should be -aaa^M
+aaa
 bbb

CR to CR+LF:
@@ -1 +1,2 @@
-aaa^Mbbb
+aaa^M
+bbb

CR+LF to CR:
@@ -1,2 +1 @@
-aaa            => BUG: should be -aaa^M
-bbb
+aaa^Mbbb

LF to CR:
@@ -1,2 +1 @@
-aaa
-bbb
+aaa^Mbbb

CR to LF:
@@ -1 +1,2 @@
-aaa^Mbbb
+aaa
+bbb

Tested with version 2.19.1.

Regards,
Frank Schäfer

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 23.11.18 um 19:19 schrieb Frank Schäfer:
> The CR marker ^M doesn't show up in '-' lines of diffs when the ending
> of the removed line is CR+LF.
> It shows up as expected in '-' lines when the ending of the removed line
> is CR only.
> It also always shows up as expected in '+' lines.

Is your repository configured to (1) highlight whitespace errors in diff 
output and (2) to leave CRLF alone in text files?

If so, then it is just a side-effect of this combination, an illusion, 
so to say: The CR in the CRLF combo is trailing whitespace. The 'git 
diff' marks it by inserting an escape sequence to switch the color 
before ^M and another escape sequence to reset to color after ^M. This 
breaks the CRLF combination apart, so that the pager does not process it 
as a combined CRLF sequence; it displays the lone CR as ^M.

It is easy to achieve the opposite effect, i.e., that ^M is not 
displayed. For example with these lines in .git/info/attributes or 
.gitattributes:

*.cpp 
whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab
*.h whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab

Note the cr-at-eol. (There may be shorter versions to achieve a similar 
effect.)

-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Frank Schäfer]

Am 23.11.18 um 22:47 schrieb Johannes Sixt:
> Am 23.11.18 um 19:19 schrieb Frank Schäfer:
>> The CR marker ^M doesn't show up in '-' lines of diffs when the ending
>> of the removed line is CR+LF.
>> It shows up as expected in '-' lines when the ending of the removed line
>> is CR only.
>> It also always shows up as expected in '+' lines.
>
> Is your repository configured to (1) highlight whitespace errors in
> diff output and (2) to leave CRLF alone in text files?
I'm using the default configuration, so whitespace is set to
trailing-space, but cr-at-eol is not set.

>
> If so, then it is just a side-effect of this combination, an illusion,
> so to say: The CR in the CRLF combo is trailing whitespace. The 'git
> diff' marks it by inserting an escape sequence to switch the color
> before ^M and another escape sequence to reset to color after ^M. This
> breaks the CRLF combination apart, so that the pager does not process
> it as a combined CRLF sequence; it displays the lone CR as ^M.
Urghh... so that needs to be fixed.
Why does it work correctly with '+' lines ?

>
> It is easy to achieve the opposite effect, i.e., that ^M is not
> displayed. For example with these lines in .git/info/attributes or
> .gitattributes:
>
> *.cpp
> whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab
> *.h
> whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab
>
> Note the cr-at-eol. (There may be shorter versions to achieve a
> similar effect.)
With cr-at-eol, ^M indeed doesn't show up anymore in '+' lines with
CR+LF line endings. That's correct/expected.
'-' lines with CR+LF line endings are displayed correctly in this case, too.
However, ^M still shows up in '+' and '-' lines with CR line endings.

Hmm... is CR-only line termination supported at all ?
E.g. 'eol' can be set to 'lf' or 'crlf' but not 'cr'...

Regards,
Frank

>
> -- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Torsten Bögershausen]

On Sat, Nov 24, 2018 at 03:51:26PM +0100, Frank Schäfer wrote:
[]
> 
> Hmm... is CR-only line termination supported at all ?
> E.g. 'eol' can be set to 'lf' or 'crlf' but not 'cr'...
> 

No, CR-only is not supported, because:
Nobody was implementing it, and that is probably because
the only question abou CR-only (at least what I remember)
was a when an old Mac OS (not the Mac OS X)
was used (which used to use CR instead of LF).

And, such feature may be implemented by writing a filter,
replace CR with LF as "clean" and "LF" with "CR" for smudge.

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 24.11.18 um 15:51 schrieb Frank Schäfer:
> Am 23.11.18 um 22:47 schrieb Johannes Sixt:
>> Am 23.11.18 um 19:19 schrieb Frank Schäfer:
>>> The CR marker ^M doesn't show up in '-' lines of diffs when the ending
>>> of the removed line is CR+LF.
>>> It shows up as expected in '-' lines when the ending of the removed line
>>> is CR only.
>>> It also always shows up as expected in '+' lines.
>>
>> Is your repository configured to (1) highlight whitespace errors in
>> diff output and (2) to leave CRLF alone in text files?
> I'm using the default configuration, so whitespace is set to
> trailing-space, but cr-at-eol is not set.
> 
>>
>> If so, then it is just a side-effect of this combination, an illusion,
>> so to say: The CR in the CRLF combo is trailing whitespace. The 'git
>> diff' marks it by inserting an escape sequence to switch the color
>> before ^M and another escape sequence to reset to color after ^M. This
>> breaks the CRLF combination apart, so that the pager does not process
>> it as a combined CRLF sequence; it displays the lone CR as ^M.
> Urghh... so that needs to be fixed.
> Why does it work correctly with '+' lines ?

I don't think that there is anything to fix. If you have a file with 
CRLF in it, but you did not declare to Git that CRLF is the expected 
end-of-line indicator, then the CR *is* trailing whitespace (because the 
line ends at LF), and 'git diff' highlights it.

-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Frank Schäfer]

Am 24.11.18 um 23:07 schrieb Johannes Sixt:
> I don't think that there is anything to fix. If you have a file with
> CRLF in it, but you did not declare to Git that CRLF is the expected
> end-of-line indicator, then the CR *is* trailing whitespace (because
> the line ends at LF), and 'git diff' highlights it. 

Sure, it's correct to highlight it.
But it doesn't highlight it in removed lines, just in added lines.
I can see no good reason why removed and added lines should be treated
differently.

Let me give you two real-life examples:
 
1) If CR+LF line termination is used in a file, changing the content of
a line (but not its termination) currently produces a diff like

-something
+something_new^M

which causes the user to think he has changed the line ending (added a
CR) although he didn't.

2) If someone/something unintentionally changes the line termination
from CR+LF to LF, it doesn't show up in the diff:

-something
+something

I don't think this behavior makes sense.
At least it's IMHO not what users expect to see.
They want to see what's really going on, not to get confused.

Regards,
Frank

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 25.11.18 um 15:03 schrieb Frank Schäfer:
> Am 24.11.18 um 23:07 schrieb Johannes Sixt:
>> I don't think that there is anything to fix. If you have a file with
>> CRLF in it, but you did not declare to Git that CRLF is the expected
>> end-of-line indicator, then the CR *is* trailing whitespace (because
>> the line ends at LF), and 'git diff' highlights it.
> 
> Sure, it's correct to highlight it.
> But it doesn't highlight it in removed lines, just in added lines.
> I can see no good reason why removed and added lines should be treated
> differently.

But incorrect whitespace is never highlighted in removed lines, why 
should CR be an exception?

> 1) If CR+LF line termination is used in a file, changing the content of
> a line (but not its termination) currently produces a diff like
> 
> -something
> +something_new^M
> 
> which causes the user to think he has changed the line ending (added a
> CR) although he didn't.

But this is not limited to CR at EOL:

-<SP><TAB>something
+<SP><TAB>something_new

will also show the incorrect whitespace highlighted only for the + line.

> 2) If someone/something unintentionally changes the line termination
> from CR+LF to LF, it doesn't show up in the diff:
> 
> -something
> +something

Same here for other cases, for example

-something<SP>
+something

will not have on obvious indicator that whitespace was corrected.

If you are worried about a change in EOL style, you should better listen 
to your other tools. Either it is important, or it is not. If it is, 
they will report it to you. If it isn't, why care?

-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> But incorrect whitespace is never highlighted in removed lines, why
> should CR be an exception?
> ...
> Same here for other cases, for example
> 
> -something<SP>
> +something
>
> will not have on obvious indicator that whitespace was corrected.

All correct, but misses one point in Frank's original report, which
observed

	-something
	+something_new^M

with ^M highlighted for whitespace error.  The highlighting is
correct.  But notice lack of caret-em on the preimage line?

It turns out that we show something like this

    <RED>-something<RESET> CR LF

for the preimage line, while showing something like this

    <GREEN>+something_new<RESET><BG-RED> CR <RESET> LF

for the postimage line.  

Because CR on the postimage line, thanks to highlighting, appears
alone separate from the LF, it is shown as two-letter caret-em
sequence to the user.

On the other hand, because CR and LF appear next to each other on
the preimage line, the pager and/or the terminal behaves as if CR is
not even there and that is where Frank's complaint comes from, I think.

The code is doing the right thing by showing CR, but it is hidden by
the pager and/or the terminal.

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 26.11.18 um 04:04 schrieb Junio C Hamano:
> It appears to me that what Frank sees is not "^M highlighted for
> whitespace breakage appears only on postimage lines, while ^M is
> shown but not highlighted on preimage lines".  My reading of the
> above is "Not just without highlight, ^M is just *NOT* shown on the
> preimage line".
> 
> That does not sound right.  I would understand it if both lines
> showed ^M at the end, and only the one on the postimage line had it
> highlighted as a trailing-whitespace.

I agree that this is a (small?) weakness. But...

> When we are producing a colored output, we know we are *not* writing
> for machines, so one way to do it would be to turn CR at the end of
> the line into two letter "^" "M" sequence on our end, without relying
> on the terminal and/or the pager.  I dunno.

... this goes too far, IMO. It is the pager's task to decode control 
characters.

-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 26.11.18 um 04:04 schrieb Junio C Hamano:
>>
>> That does not sound right.  I would understand it if both lines
>> showed ^M at the end, and only the one on the postimage line had it
>> highlighted as a trailing-whitespace.
>
> I agree that this is a (small?) weakness. But...
>
>> When we are producing a colored output, we know we are *not* writing
>> for machines, so one way to do it would be to turn CR at the end of
>> the line into two letter "^" "M" sequence on our end, without relying
>> on the terminal and/or the pager.  I dunno.
>
> ... 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.

Then, what Frank saw in the troublesome output would become

	<RED> -something <RESET> CR <RESET> LF
	<GREEN> +something_new <RESET> <BG_RED> CR <RESET> LF

and we'll let the existing pager+terminal magic turn that trailing
CR on the preimage line into caret-em, just like the trailing CR on
the postimage line is already shown as caret-em with the current
output.

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.

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

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.
> 
> Then, what Frank saw in the troublesome output would become
> 
> 	<RED> -something <RESET> CR <RESET> LF
> 	<GREEN> +something_new <RESET> <BG_RED> CR <RESET> LF
> 
> and we'll let the existing pager+terminal magic turn that trailing
> CR on the preimage line into caret-em, just like the trailing CR on
> the postimage line is already shown as caret-em with the current
> output.

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.

> 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.

-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Frank Schäfer]

Am 27.11.18 um 19:15 schrieb Johannes Sixt:
> 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.
>>
>> Then, what Frank saw in the troublesome output would become
>>
>>     <RED> -something <RESET> CR <RESET> LF
>>     <GREEN> +something_new <RESET> <BG_RED> CR <RESET> LF
>>
>> and we'll let the existing pager+terminal magic turn that trailing
>> CR on the preimage line into caret-em, just like the trailing CR on
>> the postimage line is already shown as caret-em with the current
>> output.
>
> 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.
>
>> 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.
... but that also turns on displaying normal space/tab errors in removed
lines.
So to make both of us happy, we would need separate switches.

Any chance this can be done easily enough ?

Regards,
Frank

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> 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.

>> 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.

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Frank Schäfer]

Hi Junio,

Am 29.11.18 um 03:11 schrieb Junio C Hamano:
[...]
> 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.
You mean
     ...
     <RESET> *after* a CR that comes immediately before a LF."


OK, AFAICS this produces the desired output in all cases if eol=lf.

Now what about the case eol=crlf ?
Keeping the current behavior of '-' lines is correct.
But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ?

With other words:
"If CR comes immediately before a LF, do the following with *all* lines:
<RESET> after the CR if eol=lf but do not <RESET> after the CR if eol=crlf."

Regards,
Frank

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 02.12.18 um 20:31 schrieb Frank Schäfer:
> Am 29.11.18 um 03:11 schrieb Junio C Hamano:
> [...]
>> 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.
> You mean
>       ...
>       <RESET> *after* a CR that comes immediately before a LF."
> 
> 
> OK, AFAICS this produces the desired output in all cases if eol=lf.
> 
> Now what about the case eol=crlf ?
> Keeping the current behavior of '-' lines is correct.
> But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ?

That can be achieved with whitespace=cr-at-eol.

> With other words:
> "If CR comes immediately before a LF, do the following with *all* lines:
> <RESET> after the CR if eol=lf but do not <RESET> after the CR if eol=crlf."

Why? It is the pager's duty to highlight CR, IMO. If it doesn't, but the 
user wants to see them, then they are using the wrong pager or the wrong 
pager settings.

As far as I am concerned, I do not have any of my files marked as 
eol=crlf, but I do *not* want to see ^M in the pager. I.e., having git 
insert <RESET> between CR and LF would do the wrong thing for me.

-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Frank Schäfer]

Am 02.12.18 um 22:22 schrieb Johannes Sixt:
> Am 02.12.18 um 20:31 schrieb Frank Schäfer:
>> With other words:
>> "If CR comes immediately before a LF, do the following with *all* lines:
>> <RESET> after the CR if eol=lf but do not <RESET> after the CR if
>> eol=crlf."
>
> Why? It is the pager's duty to highlight CR, IMO. If it doesn't, but
> the user wants to see them, then they are using the wrong pager or the
> wrong pager settings.
AFAIU Junios explanation it's not the pagers fault.

>
> As far as I am concerned, I do not have any of my files marked as
> eol=crlf, but I do *not* want to see ^M in the pager. I.e., having git
> insert <RESET> between CR and LF would do the wrong thing for me.
>
But doing the same thing in added lines is doing the right thing for you ?
Or are you suggesting to fix the behavior of added lines instead ?
In any case, inconsistent behavior is not what we want.

Regards,
Frank

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 05.12.18 um 20:29 schrieb Frank Schäfer:
> 
> Am 02.12.18 um 22:22 schrieb Johannes Sixt:
>> Am 02.12.18 um 20:31 schrieb Frank Schäfer:
>>> With other words:
>>> "If CR comes immediately before a LF, do the following with *all* lines:
>>> <RESET> after the CR if eol=lf but do not <RESET> after the CR if
>>> eol=crlf."
>>
>> Why? It is the pager's duty to highlight CR, IMO. If it doesn't, but
>> the user wants to see them, then they are using the wrong pager or the
>> wrong pager settings.
> AFAIU Junios explanation it's not the pagers fault.

Then Junio and I are in disagreement. IMO, Git does not have to be more 
clever than the pager when it comes to presentation of text.

>> As far as I am concerned, I do not have any of my files marked as
>> eol=crlf, but I do *not* want to see ^M in the pager. I.e., having git
>> insert <RESET> between CR and LF would do the wrong thing for me.
>>
> But doing the same thing in added lines is doing the right thing for you ?

Yes, I think so. As long as I'm not telling Git that my files are CRLF 
when they actual are, then the CR before LF is a whitespace error. 
Nevertheless, I do *NOT* want Git to outwit my pager by inserting 
<RESET> between CR and LF all the time so that it is forced to treat the 
lone CR like a control character that is to be made visible.

> Or are you suggesting to fix the behavior of added lines instead ?
> In any case, inconsistent behavior is not what we want.

I'm suggesting that users who knowingly store CRLF files in the 
database, but do not want to see ^M in added lines have to use 
whitespace=cr-at-eol and that's all. I do not see inconsistency.

-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Junio C Hamano]

Frank Schäfer <fschaefer.oss@googlemail.com> writes:

> Hi Junio,
>
> Am 29.11.18 um 03:11 schrieb Junio C Hamano:
> [...]
>> 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.
> You mean
>      ...
>      <RESET> *after* a CR that comes immediately before a LF."
>
> OK, AFAICS this produces the desired output in all cases if eol=lf.

OK, yeah, I think I meant "after", i.e. ... CR <RESET> LF, in order
to force CR to be separated from LF.

> Now what about the case eol=crlf ?

I have no strong opinions, other than that "LF in repository, CRLF
in working tree" would make the issue go away (when it is solved for
EOL=LF like the above, that is).

> Keeping the current behavior of '-' lines is correct.
> But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ?

If "LF in repository, CRLF in working tree" is done, there would not
be ^M at the end of the line, not just for removed lines, but also
for added lines, no?

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Frank Schäfer]

Am 03.12.18 um 02:15 schrieb Junio C Hamano:
> Frank Schäfer <fschaefer.oss@googlemail.com> writes:
>
>> Hi Junio,
>>
>> Am 29.11.18 um 03:11 schrieb Junio C Hamano:
>> [...]
>>> 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.
>> You mean
>>      ...
>>      <RESET> *after* a CR that comes immediately before a LF."
>>
>> OK, AFAICS this produces the desired output in all cases if eol=lf.
> OK, yeah, I think I meant "after", i.e. ... CR <RESET> LF, in order
> to force CR to be separated from LF.
>
>> Now what about the case eol=crlf ?
> I have no strong opinions, other than that "LF in repository, CRLF
> in working tree" would make the issue go away (when it is solved for
> EOL=LF like the above, that is).
>
>> Keeping the current behavior of '-' lines is correct.
>> But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ?
> If "LF in repository, CRLF in working tree" is done, there would not
> be ^M at the end of the line, not just for removed lines, but also
> for added lines, no?
Just to be sure that I'm not missing anything here:
What's your definition of "LF in repository, CRLF in working tree" in
terms of config parameters ?

Regards,
Frank

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Junio C Hamano]

Frank Schäfer <fschaefer.oss@googlemail.com> writes:

> Just to be sure that I'm not missing anything here:
> What's your definition of "LF in repository, CRLF in working tree" in
> terms of config parameters ?

:::Documentation/config/core.txt:::

core.autocrlf::
	Setting this variable to "true" is the same as setting
	the `text` attribute to "auto" on all files and core.eol to "crlf".
	Set to true if you want to have `CRLF` line endings in your
	working directory and the repository has LF line endings.
	This variable can be set to 'input',
	in which case no output conversion is performed.

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Frank Schäfer]

Am 06.12.18 um 01:58 schrieb Junio C Hamano:
> Frank Schäfer <fschaefer.oss@googlemail.com> writes:
>
>> Just to be sure that I'm not missing anything here:
>> What's your definition of "LF in repository, CRLF in working tree" in
>> terms of config parameters ?
> :::Documentation/config/core.txt:::
>
> core.autocrlf::
> 	Setting this variable to "true" is the same as setting
> 	the `text` attribute to "auto" on all files and core.eol to "crlf".
> 	Set to true if you want to have `CRLF` line endings in your
> 	working directory and the repository has LF line endings.
> 	This variable can be set to 'input',
> 	in which case no output conversion is performed.
Argh... crap. I was missing that I have to set the "text" attribute
manually...
Thats why core.eol=crlf didn't make a difference in my tests. :/

Let me thoroughly re-validate all cases.
I will likely not find the time for that before Monday, but I think that
break could be helpful. ;)

Regards,
Frank

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Frank Schäfer]

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:
>>> That does not sound right.  I would understand it if both lines
>>> showed ^M at the end, and only the one on the postimage line had it
>>> highlighted as a trailing-whitespace.
>> I agree that this is a (small?) weakness. But...
>>
>>> When we are producing a colored output, we know we are *not* writing
>>> for machines, so one way to do it would be to turn CR at the end of
>>> the line into two letter "^" "M" sequence on our end, without relying
>>> on the terminal and/or the pager.  I dunno.
>> ... 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.
>
> Then, what Frank saw in the troublesome output would become
>
> 	<RED> -something <RESET> CR <RESET> LF
> 	<GREEN> +something_new <RESET> <BG_RED> CR <RESET> LF
>
> and we'll let the existing pager+terminal magic turn that trailing
> CR on the preimage line into caret-em, just like the trailing CR on
> the postimage line is already shown as caret-em with the current
> output.
>
> 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.

Hmm... I think I now understand what caused the confusion here.
It was my mistake: I didn't consider that EOL characters are whitespace
characters, too. :/

Nevertheless, I still think that eol (CR, LF) and "normal" whitespace
(space, tab) should be distinguished and marked/displayed differently,
because they are playing different roles.
Your suggestion seems to be a good solution for that.

Regards,
Frank

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

On Sun, Nov 25, 2018 at 03:03:11PM +0100, Frank Schäfer wrote:
> Am 24.11.18 um 23:07 schrieb Johannes Sixt:
> > I don't think that there is anything to fix. If you have a file with
> > CRLF in it, but you did not declare to Git that CRLF is the expected
> > end-of-line indicator, then the CR *is* trailing whitespace (because
> > the line ends at LF), and 'git diff' highlights it. 
> 
> Sure, it's correct to highlight it.
> But it doesn't highlight it in removed lines, just in added lines.
> I can see no good reason why removed and added lines should be treated
> differently.

The default behavior is to highlight whitespace errors only in new
lines, because the assumption is that while you don't want to introduce
new errors, you can't do anything about old mistakes without rewriting
history.

I agree that in many circumstances, such as code review, this may be
undesirable.  In the past, I've done code reviews where I may let
existing trailing whitespace go but am strict about not introducing
new trailing whitespace, and being able to see both is helpful.

If you want to see whitespace errors in both the old and the new, use
--ws-error-highlight or set diff.wsErrorHighlight.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]