Bug: Problem with CRLF line ending in git-diff with coloring

From: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
Date: Feb 9, 2014, 11:01 UTC
Message-ID: <20140209110155.GB16189@scotty.home>

Good morning,

when diffing output where files have CRLF line ending, the coloring seems wrong, because in changed lines the CR (^M) is highlighted, even if the line ending has not changed.

The diff engine itself is correct.

I added a test case to show this behaviour.

The problem seems to come from emit_add_line() where ws_check_emit() is called. The parameter ecbdata->ws_rule has not set WS_CR_AT_EOL. In this case ws_check_emit() handles the CR at eol as whitespace character and therfore highlights it. This seems wrong for files with CRLF lineending.

,----
| static void emit_add_line(const char *reset,
| 			  struct emit_callback *ecbdata,
| 			  const char *line, int len)
| {
|         const char *ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
|         ...
|         
| 	if (!*ws)
|         ...
| 	else {
| 		/* Emit just the prefix, then the rest. */
| 		emit_line_0(ecbdata->opt, set, reset, '+', "", 0);
| 		ws_check_emit(line, len, ecbdata->ws_rule,
| 			      ecbdata->opt->file, set, reset, ws);
| 	}
| }
`----

If WS_CR_AT_EOL is set in ecbdata->ws_rule, it works correctly, but this seems not the right solutions. (Sorry, but I'm not deep enough in the code to propose a solution.)

Another nitpick: While writing the test it was unclear for me where the color start and end sequences will be put. Here is a difference between old lines and new lines, because old lines will be printed with emit_line_0() and new lines with emit_line_0() + ws_check_emit(). So in case of new lines the "+" itself is enclosed by the color sequences, where in case of the old lines the whole line is enclosed by the color sequences.

I tested this with 6a7071958620dad (Git 1.9.0-rc3), but this is also wrong in older versions.

With kind regards, Stefan

---
 t/t4060-diff-eol.sh | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 81 insertions(+), 0 deletion(-)
 create mode 100755 t/t4060-diff-eol.sh

diff --git a/t/t4060-diff-eol.sh b/t/t4060-diff-eol.sh
new file mode 100755
index 0000000..8cf9a69
--- /dev/null
+++ b/t/t4060-diff-eol.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+#
+# Copyright (c) 2014 Stefan-W. Hahn
+#
+
+test_description='Test coloring of diff with CRLF line ending.
+
+'
+. ./test-lib.sh
+
+get_color ()
+{
+	git config --get-color "$1"
+}
+
+tr 'Q' '\015' << EOF > x
+Zeile 1Q
+Zeile 2Q
+Zeile 3Q
+EOF
+
+git update-index --add x
+
+tr 'Q' '\015' << EOF > x
+Zeile 1Q
+Zeile 22Q
+Zeile 3Q
+EOF
+
+tr 'Q' '\015' << EOF > expect
+diff --git a/x b/x
+index 3411cc1..68a4b2c 100644
+--- a/x
++++ b/x
+@@ -1,3 +1,3 @@
+ Zeile 1Q
+-Zeile 2Q
++Zeile 22Q
+ Zeile 3Q
+EOF
+
+
+git -c color.diff=false diff > out
+test_expect_success "diff files ending with CRLF without color" '
+        test_cmp expect out'
+
+test_expect_success setup '
+        git config color.diff.plain black &&
+        git config color.diff.meta blue &&
+        git config color.diff.frag yellow &&
+        git config color.diff.func normal &&
+        git config color.diff.old red &&
+        git config color.diff.new green &&
+        git config color.diff.commit normal &&
+	c_reset=$(git config --get-color no.such.color reset) &&
+	c_plain=$(get_color color.diff.plain) &&
+	c_meta=$(get_color color.diff.meta) &&
+	c_frag=$(get_color color.diff.frag) &&
+	c_func=$(get_color color.diff.func) &&
+	c_old=$(get_color color.diff.old) &&
+	c_new=$(get_color color.diff.new) &&
+	c_commit=$(get_color color.diff.commit) &&
+	c_whitespace=$(get_color color.diff.whitespace)
+'
+
+tr 'Q' '\015' << EOF > expect
+${c_meta}diff --git a/x b/x${c_reset}
+${c_meta}index 3411cc1..68a4b2c 100644${c_reset}
+${c_meta}--- a/x${c_reset}
+${c_meta}+++ b/x${c_reset}
+${c_frag}@@ -1,3 +1,3 @@${c_reset}
+${c_plain} Zeile 1${c_reset}Q
+${c_old}-Zeile 2${c_reset}Q
+${c_new}+${c_reset}${c_new}Zeile 22${c_reset}Q
+${c_plain} Zeile 3${c_reset}Q
+EOF
+
+git -c color.diff=always diff > out
+test_expect_success "diff files ending with CRLF with color coding" 'test_cmp expect out'
+
+test_done

-- 
1.8.3.2.733.gf8abaeb



-- 
Stefan-W. Hahn                          It is easy to make things.
                                        It is hard to make things simple.