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 [off-list ref] writes:

Am 27.11.18 um 00:31 schrieb Junio C Hamano:

quoted

Johannes Sixt [off-list ref] writes:

quoted

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.

quoted

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.

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

Am 02.12.18 um 20:31 schrieb Frank Schäfer:

Am 29.11.18 um 03:11 schrieb Junio C Hamano:
[...]

quoted

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

Am 02.12.18 um 22:22 schrieb Johannes Sixt:

Am 02.12.18 um 20:31 schrieb Frank Schäfer:

quoted

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

Am 05.12.18 um 20:29 schrieb Frank Schäfer:

Am 02.12.18 um 22:22 schrieb Johannes Sixt:

quoted

Am 02.12.18 um 20:31 schrieb Frank Schäfer:

quoted

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.

quoted

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