Re: question: how to ignore extral CR when diff dos format files with 'color=auto'

From dae20e25960c73bd7ccc0939fe096bb68a009fb5 Mon Sep 17 00:00:00 2001
From: erwangg <redacted>
Date: Wed, 27 Aug 2008 12:22:43 +0800
Subject: [PATCH] ingore cr at eol when diff with color=auto

---

Thanks.  A few comments.

decl-after-statement.

+	if (has_trailing_return)
+        len--;
+
 	fputs(set, file);
 	fwrite(line, len, 1, file);
 	fputs(reset, file);
+
+    if (has_trailing_return)
+        fputc('\r', file);
 	if (has_trailing_newline)
 		fputc('\n', file);
 }

We indent with tab which jumps the cursor to next column that is multiple
of 8.

Otherwise this hunk is good.

I do not think you want to force WS_CR_AT_EOL here.

That is a policy issue (not "git policy", but the policy of the contents
that is managed by git, in other words, your repository) that you should
control with core.whitespace (and if you want finer grain control, then
with .gitattributes).

Your patch is whitespace damanged, and has style issues, and lacks any
comment to defend the change.  It does not have a Sign-off, either.  Even
though the intent is good, I cannot apply this as-is.  Please look at
Documentation/SubmittingPatches and emulate patches from other git
regulars.

You say "when color=auto" in the patch title, but there is nothing that
restricts the effect of this change only to that case in your patch.  A
comment to defend the change should address things like that, and here is
how I would write it.

    When the tracked contents have CRLF line endings, colored diff output
    shows "^M" at the end of output lines, which is distracting, even
    though the pager we use by default ("less") knows to hide them.

    The problem is that "less" hides a carriage-return only at the end of
    the line, immediately before a line feed.  The colored diff output
    does not take this into account, and emits four element sequence for
    each line:

       - force this color;
       - the line up to but not including the terminating line feed;
       - reset color
       - line feed.

    By including the carriage return at the end of the line in the second
    item, we are breaking the smart our pager has in order not to show
    "^M".  This can be fixed by changing the sequence to:

       - force this color;
       - the line up to but not including the terminating end-of-line;
       - reset color
       - end-of-line.

    where end-of-line is either a single linefeed or a CRLF pair.
    When the output is not colored, "force this color" and "reset color"
    sequences are both empty, so we won't have this problem with or
    without this patch.    

If we drop the hunk to emit_add_line() that forces WS_CR_AT_EOL, I find
that the intent and the approach of the patch is quite good.  I suspect
that combined-diff output routines have the same issue that you need to
fix the same way, but I didn't look.