[PATCH 0/8] Add color test for range-diff, simplify diff.c

[PATCH 0/8] Add color test for range-diff, simplify diff.c

[Stefan Beller]

This is based on origin/js/range-diff (c255a588bcd) and is also available
via
  git fetch https://github.com/stefanbeller/git ws_cleanup-ontop-range-diff-2 

This adds some color testing to range-diff and then attempts to make the code
in diff.c around emit_line_0 more readable.

I think we can go further bit more (by e.g. cherry-picking 
"ws: do not reset and set color twice" found at [1]), but that would be feature
work. This series doesn't change any existing tests!

[1] https://github.com/stefanbeller/git/tree/ws_cleanup-ontop-range-diff

Thanks,
Stefan

Stefan Beller (8):
  test_decode_color: understand FAINT and ITALIC
  t3206: add color test for range-diff --dual-color
  diff.c: simplify caller of emit_line_0
  diff.c: reorder arguments for emit_line_ws_markup
  diff.c: add set_sign to emit_line_0
  diff: use emit_line_0 once per line
  diff.c: compute reverse locally in emit_line_0
  diff.c: rewrite emit_line_0 more understandably

 diff.c                  | 94 +++++++++++++++++++++++------------------
 t/t3206-range-diff.sh   | 39 +++++++++++++++++
 t/test-lib-functions.sh |  2 +
 3 files changed, 93 insertions(+), 42 deletions(-)

-- 
2.18.0.345.g5c9ce644c3-goog

[PATCH 1/8] test_decode_color: understand FAINT and ITALIC

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/test-lib-functions.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2b2181dca09..be8244c47b5 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -42,6 +42,8 @@ test_decode_color () {
 		function name(n) {
 			if (n == 0) return "RESET";
 			if (n == 1) return "BOLD";
+			if (n == 2) return "FAINT";
+			if (n == 3) return "ITALIC";
 			if (n == 7) return "REVERSE";
 			if (n == 30) return "BLACK";
 			if (n == 31) return "RED";
-- 
2.18.0.345.g5c9ce644c3-goog

[PATCH 2/8] t3206: add color test for range-diff --dual-color

[Stefan Beller]

The 'expect'ed outcome is taken by running the 'range-diff |decode';
it is not meant as guidance, rather as a documentation of the current
situation.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t3206-range-diff.sh | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 2237c7f4af9..ef652865cd7 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -142,4 +142,43 @@ test_expect_success 'changed message' '
 	test_cmp expected actual
 '
 
+test_expect_success 'simple coloring' '
+	q_to_tab >expect <<-EOF &&
+	<YELLOW>1:  a4b3333 = 1:  f686024 s/5/A/<RESET>
+	<RED>2:  f51d370 <RESET><YELLOW>!<RESET><GREEN> 2:  4ab067d<RESET><YELLOW> s/4/A/<RESET>
+	    <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET>
+	     <RESET>
+	         s/4/A/<RESET>
+	     <RESET>
+	    <REVERSE><GREEN>+<RESET> <BOLD>   Also a silly comment here!<RESET>
+	    <REVERSE><GREEN>+<RESET>
+	     diff --git a/file b/file<RESET>
+	    <RED> --- a/file<RESET>
+	    <GREEN> +++ b/file<RESET>
+	<RED>3:  0559556 <RESET><YELLOW>!<RESET><GREEN> 3:  b9cb956<RESET><YELLOW> s/11/B/<RESET>
+	    <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET>
+	      9<RESET>
+	      10<RESET>
+	    <RED> -11<RESET>
+	    <REVERSE><RED>-<RESET><FAINT;GREEN>+BB<RESET>
+	    <REVERSE><GREEN>+<RESET><BOLD;GREEN>+B<RESET>
+	      12<RESET>
+	      13<RESET>
+	      14<RESET>
+	<RED>4:  d966c5c <RESET><YELLOW>!<RESET><GREEN> 4:  8add5f1<RESET><YELLOW> s/12/B/<RESET>
+	    <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET>
+	    <CYAN> @@<RESET>
+	      9<RESET>
+	      10<RESET>
+	    <REVERSE><RED>-<RESET><FAINT> BB<RESET>
+	    <REVERSE><GREEN>+<RESET> <BOLD>B<RESET>
+	    <RED> -12<RESET>
+	    <GREEN> +B<RESET>
+	      13<RESET>
+	EOF
+	git range-diff changed...changed-message --color --dual-color >actual.raw &&
+	test_decode_color >actual <actual.raw &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.18.0.345.g5c9ce644c3-goog

[PATCH 3/8] diff.c: simplify caller of emit_line_0

[Stefan Beller]

Due to the previous condition we know "set_sign != NULL" at that point.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 272b0b93834..f7251c89cbb 100644
--- a/diff.c
+++ b/diff.c
@@ -997,8 +997,7 @@ static void emit_line_ws_markup(struct diff_options *o,
 		emit_line_0(o, set, 0, reset, sign, line, len);
 	else if (!ws) {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,
-			    sign, "", 0);
+		emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0);
 		emit_line_0(o, set, 0, reset, 0, line, len);
 	} else if (blank_at_eof)
 		/* Blank line at EOF - paint '+' as well */
-- 
2.18.0.345.g5c9ce644c3-goog

[PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup

[Stefan Beller]

The order shall be all colors first, then the content, flags at the end.
The colors are in order.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index f7251c89cbb..8fd2171d808 100644
--- a/diff.c
+++ b/diff.c
@@ -980,9 +980,9 @@ static void dim_moved_lines(struct diff_options *o)
 }
 
 static void emit_line_ws_markup(struct diff_options *o,
-				const char *set, const char *reset,
-				const char *line, int len,
-				const char *set_sign, char sign,
+				const char *set_sign, const char *set,
+				const char *reset,
+				char sign, const char *line, int len,
 				unsigned ws_rule, int blank_at_eof)
 {
 	const char *ws = NULL;
@@ -1066,7 +1066,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else if (c == '-')
 				set = diff_get_color_opt(o, DIFF_FILE_OLD);
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, ' ',
+		emit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:
@@ -1109,7 +1109,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 				set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);
 			flags |= WS_IGNORE_FIRST_SPACE;
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, '+',
+		emit_line_ws_markup(o, set_sign, set, reset, '+', line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
 		break;
@@ -1152,7 +1152,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else
 				set = diff_get_color_opt(o, DIFF_CONTEXT_DIM);
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, '-',
+		emit_line_ws_markup(o, set_sign, set, reset, '-', line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
 		break;
 	case DIFF_SYMBOL_WORDS_PORCELAIN:
-- 
2.18.0.345.g5c9ce644c3-goog

[PATCH 5/8] diff.c: add set_sign to emit_line_0

[Stefan Beller]

For now just change the signature, we'll reason about the actual
change in a follow up patch.

Pass set_sign (which is output before the sign) and set that is setting
the color after the sign. Hence, promote any 'set's to set_sign as
we want to have color before the sign for now.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index 8fd2171d808..a36ed92c54c 100644
--- a/diff.c
+++ b/diff.c
@@ -576,7 +576,7 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
 }
 
 static void emit_line_0(struct diff_options *o,
-			const char *set, unsigned reverse, const char *reset,
+			const char *set_sign, const char *set, unsigned reverse, const char *reset,
 			int first, const char *line, int len)
 {
 	int has_trailing_newline, has_trailing_carriage_return;
@@ -606,9 +606,12 @@ static void emit_line_0(struct diff_options *o,
 	if (len || !nofirst) {
 		if (reverse && want_color(o->use_color))
 			fputs(GIT_COLOR_REVERSE, file);
-		fputs(set, file);
+		if (set_sign && set_sign[0])
+			fputs(set_sign, file);
 		if (first && !nofirst)
 			fputc(first, file);
+		if (set)
+			fputs(set, file);
 		fwrite(line, len, 1, file);
 		fputs(reset, file);
 	}
@@ -621,7 +624,7 @@ static void emit_line_0(struct diff_options *o,
 static void emit_line(struct diff_options *o, const char *set, const char *reset,
 		      const char *line, int len)
 {
-	emit_line_0(o, set, 0, reset, line[0], line+1, len-1);
+	emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);
 }
 
 enum diff_symbol {
@@ -994,17 +997,17 @@ static void emit_line_ws_markup(struct diff_options *o,
 	}
 
 	if (!ws && !set_sign)
-		emit_line_0(o, set, 0, reset, sign, line, len);
+		emit_line_0(o, set, NULL, 0, reset, sign, line, len);
 	else if (!ws) {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0);
-		emit_line_0(o, set, 0, reset, 0, line, len);
+		emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0);
+		emit_line_0(o, set, NULL, 0, reset, 0, line, len);
 	} else if (blank_at_eof)
 		/* Blank line at EOF - paint '+' as well */
-		emit_line_0(o, ws, 0, reset, sign, line, len);
+		emit_line_0(o, ws, NULL, 0, reset, sign, line, len);
 	else {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,
+		emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,
 			    sign, "", 0);
 		ws_check_emit(line, len, ws_rule,
 			      o->file, set, reset, ws);
@@ -1028,7 +1031,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 		context = diff_get_color_opt(o, DIFF_CONTEXT);
 		reset = diff_get_color_opt(o, DIFF_RESET);
 		putc('\n', o->file);
-		emit_line_0(o, context, 0, reset, '\\',
+		emit_line_0(o, context, NULL, 0, reset, '\\',
 			    nneof, strlen(nneof));
 		break;
 	case DIFF_SYMBOL_SUBMODULE_HEADER:
-- 
2.18.0.345.g5c9ce644c3-goog

[PATCH 6/8] diff: use emit_line_0 once per line

[Stefan Beller]

All lines that use emit_line_0 multiple times per line, are combined
into a single call to emit_line_0, making use of the 'set' argument.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/diff.c b/diff.c
index a36ed92c54c..fdad7ffdd77 100644
--- a/diff.c
+++ b/diff.c
@@ -583,10 +583,7 @@ static void emit_line_0(struct diff_options *o,
 	int nofirst;
 	FILE *file = o->file;
 
-	if (first)
-		fputs(diff_line_prefix(o), file);
-	else if (!len)
-		return;
+	fputs(diff_line_prefix(o), file);
 
 	if (len == 0) {
 		has_trailing_newline = (first == '\n');
@@ -606,13 +603,17 @@ static void emit_line_0(struct diff_options *o,
 	if (len || !nofirst) {
 		if (reverse && want_color(o->use_color))
 			fputs(GIT_COLOR_REVERSE, file);
-		if (set_sign && set_sign[0])
-			fputs(set_sign, file);
+		if (set_sign || set)
+			fputs(set_sign ? set_sign : set, file);
 		if (first && !nofirst)
 			fputc(first, file);
-		if (set)
-			fputs(set, file);
-		fwrite(line, len, 1, file);
+		if (len) {
+			if (set_sign && set && set != set_sign)
+				fputs(reset, file);
+			if (set)
+				fputs(set, file);
+			fwrite(line, len, 1, file);
+		}
 		fputs(reset, file);
 	}
 	if (has_trailing_carriage_return)
@@ -999,16 +1000,13 @@ static void emit_line_ws_markup(struct diff_options *o,
 	if (!ws && !set_sign)
 		emit_line_0(o, set, NULL, 0, reset, sign, line, len);
 	else if (!ws) {
-		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0);
-		emit_line_0(o, set, NULL, 0, reset, 0, line, len);
+		emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);
 	} else if (blank_at_eof)
 		/* Blank line at EOF - paint '+' as well */
 		emit_line_0(o, ws, NULL, 0, reset, sign, line, len);
 	else {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,
-			    sign, "", 0);
+		emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0);
 		ws_check_emit(line, len, ws_rule,
 			      o->file, set, reset, ws);
 	}
-- 
2.18.0.345.g5c9ce644c3-goog

[PATCH 7/8] diff.c: compute reverse locally in emit_line_0

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index fdad7ffdd77..f565a2c0c2b 100644
--- a/diff.c
+++ b/diff.c
@@ -576,11 +576,12 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
 }
 
 static void emit_line_0(struct diff_options *o,
-			const char *set_sign, const char *set, unsigned reverse, const char *reset,
+			const char *set_sign, const char *set, const char *reset,
 			int first, const char *line, int len)
 {
 	int has_trailing_newline, has_trailing_carriage_return;
 	int nofirst;
+	int reverse = !!set && !!set_sign;
 	FILE *file = o->file;
 
 	fputs(diff_line_prefix(o), file);
@@ -625,7 +626,7 @@ static void emit_line_0(struct diff_options *o,
 static void emit_line(struct diff_options *o, const char *set, const char *reset,
 		      const char *line, int len)
 {
-	emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);
+	emit_line_0(o, set, NULL, reset, line[0], line+1, len-1);
 }
 
 enum diff_symbol {
@@ -998,15 +999,15 @@ static void emit_line_ws_markup(struct diff_options *o,
 	}
 
 	if (!ws && !set_sign)
-		emit_line_0(o, set, NULL, 0, reset, sign, line, len);
+		emit_line_0(o, set, NULL, reset, sign, line, len);
 	else if (!ws) {
-		emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);
+		emit_line_0(o, set_sign, set, reset, sign, line, len);
 	} else if (blank_at_eof)
 		/* Blank line at EOF - paint '+' as well */
-		emit_line_0(o, ws, NULL, 0, reset, sign, line, len);
+		emit_line_0(o, ws, NULL, reset, sign, line, len);
 	else {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0);
+		emit_line_0(o, set_sign, set, reset, sign, "", 0);
 		ws_check_emit(line, len, ws_rule,
 			      o->file, set, reset, ws);
 	}
@@ -1029,7 +1030,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 		context = diff_get_color_opt(o, DIFF_CONTEXT);
 		reset = diff_get_color_opt(o, DIFF_RESET);
 		putc('\n', o->file);
-		emit_line_0(o, context, NULL, 0, reset, '\\',
+		emit_line_0(o, context, NULL, reset, '\\',
 			    nneof, strlen(nneof));
 		break;
 	case DIFF_SYMBOL_SUBMODULE_HEADER:
-- 
2.18.0.345.g5c9ce644c3-goog

[PATCH 8/8] diff.c: rewrite emit_line_0 more understandably

[Stefan Beller]

emit_line_0 has no nested conditions, but puts out all its arguments
(if set) in order. There is still a slight confusion with set
and set_sign, but let's defer that to a later patch.

'first' used be output always no matter if it was 0, but that got lost
got lost at e8c285c4f9c (diff: add an internal option to dual-color
diffs of diffs, 2018-07-21), as there we broadened the meaning of 'first'
to also signal an early return.

The change in 'emit_line' makes sure that 'first' is never content, but
always under our control, a sign or special character in the beginning
of the line (or 0, in which case we ignore it).

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 73 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/diff.c b/diff.c
index f565a2c0c2b..2bd4d3d6839 100644
--- a/diff.c
+++ b/diff.c
@@ -580,43 +580,52 @@ static void emit_line_0(struct diff_options *o,
 			int first, const char *line, int len)
 {
 	int has_trailing_newline, has_trailing_carriage_return;
-	int nofirst;
 	int reverse = !!set && !!set_sign;
+	int needs_reset = 0;
+
 	FILE *file = o->file;
 
 	fputs(diff_line_prefix(o), file);
 
-	if (len == 0) {
-		has_trailing_newline = (first == '\n');
-		has_trailing_carriage_return = (!has_trailing_newline &&
-						(first == '\r'));
-		nofirst = has_trailing_newline || has_trailing_carriage_return;
-	} else {
-		has_trailing_newline = (len > 0 && line[len-1] == '\n');
-		if (has_trailing_newline)
-			len--;
-		has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
-		if (has_trailing_carriage_return)
-			len--;
-		nofirst = 0;
-	}
-
-	if (len || !nofirst) {
-		if (reverse && want_color(o->use_color))
-			fputs(GIT_COLOR_REVERSE, file);
-		if (set_sign || set)
-			fputs(set_sign ? set_sign : set, file);
-		if (first && !nofirst)
-			fputc(first, file);
-		if (len) {
-			if (set_sign && set && set != set_sign)
-				fputs(reset, file);
-			if (set)
-				fputs(set, file);
-			fwrite(line, len, 1, file);
-		}
-		fputs(reset, file);
+	has_trailing_newline = (len > 0 && line[len-1] == '\n');
+	if (has_trailing_newline)
+		len--;
+
+	has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
+	if (has_trailing_carriage_return)
+		len--;
+
+	if (!len && !first)
+		goto end_of_line;
+
+	if (reverse && want_color(o->use_color)) {
+		fputs(GIT_COLOR_REVERSE, file);
+		needs_reset = 1;
+	}
+
+	if (set_sign || set) {
+		fputs(set_sign ? set_sign : set, file);
+		needs_reset = 1;
 	}
+
+	if (first)
+		fputc(first, file);
+
+	if (!len)
+		goto end_of_line;
+
+	if (set) {
+		if (set_sign && set != set_sign)
+			fputs(reset, file);
+		fputs(set, file);
+		needs_reset = 1;
+	}
+	fwrite(line, len, 1, file);
+	needs_reset |= len > 0;
+
+end_of_line:
+	if (needs_reset)
+		fputs(reset, file);
 	if (has_trailing_carriage_return)
 		fputc('\r', file);
 	if (has_trailing_newline)
@@ -626,7 +635,7 @@ static void emit_line_0(struct diff_options *o,
 static void emit_line(struct diff_options *o, const char *set, const char *reset,
 		      const char *line, int len)
 {
-	emit_line_0(o, set, NULL, reset, line[0], line+1, len-1);
+	emit_line_0(o, set, NULL, reset, 0, line, len);
 }
 
 enum diff_symbol {
-- 
2.18.0.345.g5c9ce644c3-goog

Re: [PATCH 2/8] t3206: add color test for range-diff --dual-color

[Eric Sunshine]

On Fri, Jul 27, 2018 at 11:05 PM Stefan Beller <sbeller@google.com> wrote:
> The 'expect'ed outcome is taken by running the 'range-diff |decode';
> it is not meant as guidance, rather as a documentation of the current
> situation.

I'm not really sure what this is trying to say. It seems _too_ brief.

Did you want a space after the vertical bar before "decode"?

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> +test_expect_success 'simple coloring' '
> +       q_to_tab >expect <<-EOF &&

Why 'q_to_tab'? I don't see any "q"'s in the body.

I also don't see any variable interpolation in the body, so maybe you
want -\EOF instead?

Re: [PATCH 5/8] diff.c: add set_sign to emit_line_0

[Eric Sunshine]

On Fri, Jul 27, 2018 at 11:05 PM Stefan Beller <sbeller@google.com> wrote:
> For now just change the signature, we'll reason about the actual
> change in a follow up patch.
>
> Pass set_sign (which is output before the sign) and set that is setting
> the color after the sign. Hence, promote any 'set's to set_sign as
> we want to have color before the sign for now.

ECANTPARSE: "and set that is setting"

Re: [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably

[Eric Sunshine]

On Fri, Jul 27, 2018 at 11:05 PM Stefan Beller <sbeller@google.com> wrote:
> emit_line_0 has no nested conditions, but puts out all its arguments
> (if set) in order. There is still a slight confusion with set
> and set_sign, but let's defer that to a later patch.
>
> 'first' used be output always no matter if it was 0, but that got lost
> got lost at e8c285c4f9c (diff: add an internal option to dual-color
> diffs of diffs, 2018-07-21), as there we broadened the meaning of 'first'
> to also signal an early return.

s/used be/used to be/

redundant "got lost"

> The change in 'emit_line' makes sure that 'first' is never content, but
> always under our control, a sign or special character in the beginning
> of the line (or 0, in which case we ignore it).
>
> Signed-off-by: Stefan Beller <sbeller@google.com>

Re: [PATCH 2/8] t3206: add color test for range-diff --dual-color

[Stefan Beller]

On Fri, Jul 27, 2018 at 11:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Jul 27, 2018 at 11:05 PM Stefan Beller <sbeller@google.com> wrote:
> > The 'expect'ed outcome is taken by running the 'range-diff |decode';
> > it is not meant as guidance, rather as a documentation of the current
> > situation.
>
> I'm not really sure what this is trying to say. It seems _too_ brief.
>
> Did you want a space after the vertical bar before "decode"?

I am trying to say that this patch was generated by inserting
a "true && test_pause" first and then inside that paused test,
I just run

  source <path>/t/test-lib-functions.sh
  git range-diff changed...changed-message --color --dual-color \
    | test_decode_color

and saved that as the expected outcome, I was prepared to
massage it gently by s/<TAB>/Q/ but that was not needed,
but I forgot the q_to_tab in place.

By adding the test this way, I just want to state "I observed
the functionality as produced in this patch". I do not want
to endorse the coloring scheme or claim it is good (it is,
but I also still have nits to pick). So I tried to briefly say
that the test is essentially "autogenerated" by just observing
output at that point in time.

>
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> > +test_expect_success 'simple coloring' '
> > +       q_to_tab >expect <<-EOF &&
>
> Why 'q_to_tab'? I don't see any "q"'s in the body.
>
> I also don't see any variable interpolation in the body, so maybe you
> want -\EOF instead?

All good suggestions! Thanks for the review, I'll incorporate them.

Thanks,
Stefan

[PATCHv2 0/8] Add color test for range-diff, simplify diff.c

[Stefan Beller]

addressed all of Erics feedback:
* reworded commit messages
* dropped q_to_tab and use cat instead
* use -\EOF isntead of -EOF

Thanks,
Stefan

Stefan Beller (8):
  test_decode_color: understand FAINT and ITALIC
  t3206: add color test for range-diff --dual-color
  diff.c: simplify caller of emit_line_0
  diff.c: reorder arguments for emit_line_ws_markup
  diff.c: add set_sign to emit_line_0
  diff: use emit_line_0 once per line
  diff.c: compute reverse locally in emit_line_0
  diff.c: rewrite emit_line_0 more understandably

 diff.c                  | 94 +++++++++++++++++++++++------------------
 t/t3206-range-diff.sh   | 39 +++++++++++++++++
 t/test-lib-functions.sh |  2 +
 3 files changed, 93 insertions(+), 42 deletions(-)

./git-range-diff ws_cleanup-ontop-range-diff-2@{2.hours.ago}...HEAD >>0000-cover-letter.patch
[dropped changes to range-diff itself]
13:  a02ea020ae7 ! 13:  16f71b43f48 t3206: add color test for range-diff --dual-color
    @@ -2,9 +2,7 @@
     
         t3206: add color test for range-diff --dual-color
     
    -    The 'expect'ed outcome is taken by running the 'range-diff |decode';
    -    it is not meant as guidance, rather as a documentation of the current
    -    situation.
    +    The 'expect'ed outcome has been taken by running the 'range-diff | decode'.
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
     
    @@ -15,8 +13,8 @@
      	test_cmp expected actual
      '
      
    -+test_expect_success 'simple coloring' '
    -+	q_to_tab >expect <<-EOF &&
    ++test_expect_success 'dual-coloring' '
    ++	cat >expect <<-\EOF &&
     +	<YELLOW>1:  a4b3333 = 1:  f686024 s/5/A/<RESET>
     +	<RED>2:  f51d370 <RESET><YELLOW>!<RESET><GREEN> 2:  4ab067d<RESET><YELLOW> s/4/A/<RESET>
     +	    <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET>
14:  c8734075229 = 14:  abd1ec80608 diff.c: simplify caller of emit_line_0
15:  ba98acffcda = 15:  bc29037f4f0 diff.c: reorder arguments for emit_line_ws_markup
16:  5a576baeb49 ! 16:  8f6ee340f1e diff.c: add set_sign to emit_line_0
    @@ -5,9 +5,10 @@
         For now just change the signature, we'll reason about the actual
         change in a follow up patch.
     
    -    Pass set_sign (which is output before the sign) and set that is setting
    -    the color after the sign. Hence, promote any 'set's to set_sign as
    -    we want to have color before the sign for now.
    +    Pass 'set_sign' (which is output before the sign) and 'set' which
    +    controls the color after the first character. Hence, promote any
    +    'set's to 'set_sign' as we want to have color before the sign
    +    for now.
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
     
17:  4e2d5a4c7f3 = 17:  0ab5920a9ab diff: use emit_line_0 once per line
18:  460713e1c3c = 18:  2d05ebdd280 diff.c: compute reverse locally in emit_line_0
19:  e442d722b7f ! 19:  001e6042d81 diff.c: rewrite emit_line_0 more understandably
    @@ -7,9 +7,9 @@
         and set_sign, but let's defer that to a later patch.
     
         'first' used be output always no matter if it was 0, but that got lost
    -    got lost at e8c285c4f9c (diff: add an internal option to dual-color
    -    diffs of diffs, 2018-07-21), as there we broadened the meaning of 'first'
    -    to also signal an early return.
    +    at "diff: add an internal option to dual-color diffs of diffs",
    +    2018-07-21), as there we broadened the meaning of 'first' to also
    +    signal an early return.
     
         The change in 'emit_line' makes sure that 'first' is never content, but
         always under our control, a sign or special character in the beginning

[PATCH 1/8] test_decode_color: understand FAINT and ITALIC

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/test-lib-functions.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2b2181dca09..be8244c47b5 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -42,6 +42,8 @@ test_decode_color () {
 		function name(n) {
 			if (n == 0) return "RESET";
 			if (n == 1) return "BOLD";
+			if (n == 2) return "FAINT";
+			if (n == 3) return "ITALIC";
 			if (n == 7) return "REVERSE";
 			if (n == 30) return "BLACK";
 			if (n == 31) return "RED";
-- 
2.18.0.132.g195c49a2227

[PATCH 2/8] t3206: add color test for range-diff --dual-color

[Stefan Beller]

The 'expect'ed outcome has been taken by running the 'range-diff | decode'.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t3206-range-diff.sh | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 2237c7f4af9..019724e61a0 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -142,4 +142,43 @@ test_expect_success 'changed message' '
 	test_cmp expected actual
 '
 
+test_expect_success 'dual-coloring' '
+	cat >expect <<-\EOF &&
+	<YELLOW>1:  a4b3333 = 1:  f686024 s/5/A/<RESET>
+	<RED>2:  f51d370 <RESET><YELLOW>!<RESET><GREEN> 2:  4ab067d<RESET><YELLOW> s/4/A/<RESET>
+	    <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET>
+	     <RESET>
+	         s/4/A/<RESET>
+	     <RESET>
+	    <REVERSE><GREEN>+<RESET> <BOLD>   Also a silly comment here!<RESET>
+	    <REVERSE><GREEN>+<RESET>
+	     diff --git a/file b/file<RESET>
+	    <RED> --- a/file<RESET>
+	    <GREEN> +++ b/file<RESET>
+	<RED>3:  0559556 <RESET><YELLOW>!<RESET><GREEN> 3:  b9cb956<RESET><YELLOW> s/11/B/<RESET>
+	    <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET>
+	      9<RESET>
+	      10<RESET>
+	    <RED> -11<RESET>
+	    <REVERSE><RED>-<RESET><FAINT;GREEN>+BB<RESET>
+	    <REVERSE><GREEN>+<RESET><BOLD;GREEN>+B<RESET>
+	      12<RESET>
+	      13<RESET>
+	      14<RESET>
+	<RED>4:  d966c5c <RESET><YELLOW>!<RESET><GREEN> 4:  8add5f1<RESET><YELLOW> s/12/B/<RESET>
+	    <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET>
+	    <CYAN> @@<RESET>
+	      9<RESET>
+	      10<RESET>
+	    <REVERSE><RED>-<RESET><FAINT> BB<RESET>
+	    <REVERSE><GREEN>+<RESET> <BOLD>B<RESET>
+	    <RED> -12<RESET>
+	    <GREEN> +B<RESET>
+	      13<RESET>
+	EOF
+	git range-diff changed...changed-message --color --dual-color >actual.raw &&
+	test_decode_color >actual <actual.raw &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.18.0.132.g195c49a2227

[PATCH 3/8] diff.c: simplify caller of emit_line_0

[Stefan Beller]

Due to the previous condition we know "set_sign != NULL" at that point.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 272b0b93834..f7251c89cbb 100644
--- a/diff.c
+++ b/diff.c
@@ -997,8 +997,7 @@ static void emit_line_ws_markup(struct diff_options *o,
 		emit_line_0(o, set, 0, reset, sign, line, len);
 	else if (!ws) {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,
-			    sign, "", 0);
+		emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0);
 		emit_line_0(o, set, 0, reset, 0, line, len);
 	} else if (blank_at_eof)
 		/* Blank line at EOF - paint '+' as well */
-- 
2.18.0.132.g195c49a2227

[PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup

[Stefan Beller]

The order shall be all colors first, then the content, flags at the end.
The colors are in order.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index f7251c89cbb..8fd2171d808 100644
--- a/diff.c
+++ b/diff.c
@@ -980,9 +980,9 @@ static void dim_moved_lines(struct diff_options *o)
 }
 
 static void emit_line_ws_markup(struct diff_options *o,
-				const char *set, const char *reset,
-				const char *line, int len,
-				const char *set_sign, char sign,
+				const char *set_sign, const char *set,
+				const char *reset,
+				char sign, const char *line, int len,
 				unsigned ws_rule, int blank_at_eof)
 {
 	const char *ws = NULL;
@@ -1066,7 +1066,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else if (c == '-')
 				set = diff_get_color_opt(o, DIFF_FILE_OLD);
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, ' ',
+		emit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:
@@ -1109,7 +1109,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 				set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);
 			flags |= WS_IGNORE_FIRST_SPACE;
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, '+',
+		emit_line_ws_markup(o, set_sign, set, reset, '+', line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
 		break;
@@ -1152,7 +1152,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else
 				set = diff_get_color_opt(o, DIFF_CONTEXT_DIM);
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, '-',
+		emit_line_ws_markup(o, set_sign, set, reset, '-', line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
 		break;
 	case DIFF_SYMBOL_WORDS_PORCELAIN:
-- 
2.18.0.132.g195c49a2227

[PATCH 5/8] diff.c: add set_sign to emit_line_0

[Stefan Beller]

For now just change the signature, we'll reason about the actual
change in a follow up patch.

Pass 'set_sign' (which is output before the sign) and 'set' which
controls the color after the first character. Hence, promote any
'set's to 'set_sign' as we want to have color before the sign
for now.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index 8fd2171d808..a36ed92c54c 100644
--- a/diff.c
+++ b/diff.c
@@ -576,7 +576,7 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
 }
 
 static void emit_line_0(struct diff_options *o,
-			const char *set, unsigned reverse, const char *reset,
+			const char *set_sign, const char *set, unsigned reverse, const char *reset,
 			int first, const char *line, int len)
 {
 	int has_trailing_newline, has_trailing_carriage_return;
@@ -606,9 +606,12 @@ static void emit_line_0(struct diff_options *o,
 	if (len || !nofirst) {
 		if (reverse && want_color(o->use_color))
 			fputs(GIT_COLOR_REVERSE, file);
-		fputs(set, file);
+		if (set_sign && set_sign[0])
+			fputs(set_sign, file);
 		if (first && !nofirst)
 			fputc(first, file);
+		if (set)
+			fputs(set, file);
 		fwrite(line, len, 1, file);
 		fputs(reset, file);
 	}
@@ -621,7 +624,7 @@ static void emit_line_0(struct diff_options *o,
 static void emit_line(struct diff_options *o, const char *set, const char *reset,
 		      const char *line, int len)
 {
-	emit_line_0(o, set, 0, reset, line[0], line+1, len-1);
+	emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);
 }
 
 enum diff_symbol {
@@ -994,17 +997,17 @@ static void emit_line_ws_markup(struct diff_options *o,
 	}
 
 	if (!ws && !set_sign)
-		emit_line_0(o, set, 0, reset, sign, line, len);
+		emit_line_0(o, set, NULL, 0, reset, sign, line, len);
 	else if (!ws) {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0);
-		emit_line_0(o, set, 0, reset, 0, line, len);
+		emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0);
+		emit_line_0(o, set, NULL, 0, reset, 0, line, len);
 	} else if (blank_at_eof)
 		/* Blank line at EOF - paint '+' as well */
-		emit_line_0(o, ws, 0, reset, sign, line, len);
+		emit_line_0(o, ws, NULL, 0, reset, sign, line, len);
 	else {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,
+		emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,
 			    sign, "", 0);
 		ws_check_emit(line, len, ws_rule,
 			      o->file, set, reset, ws);
@@ -1028,7 +1031,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 		context = diff_get_color_opt(o, DIFF_CONTEXT);
 		reset = diff_get_color_opt(o, DIFF_RESET);
 		putc('\n', o->file);
-		emit_line_0(o, context, 0, reset, '\\',
+		emit_line_0(o, context, NULL, 0, reset, '\\',
 			    nneof, strlen(nneof));
 		break;
 	case DIFF_SYMBOL_SUBMODULE_HEADER:
-- 
2.18.0.132.g195c49a2227

[PATCH 6/8] diff: use emit_line_0 once per line

[Stefan Beller]

All lines that use emit_line_0 multiple times per line, are combined
into a single call to emit_line_0, making use of the 'set' argument.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/diff.c b/diff.c
index a36ed92c54c..fdad7ffdd77 100644
--- a/diff.c
+++ b/diff.c
@@ -583,10 +583,7 @@ static void emit_line_0(struct diff_options *o,
 	int nofirst;
 	FILE *file = o->file;
 
-	if (first)
-		fputs(diff_line_prefix(o), file);
-	else if (!len)
-		return;
+	fputs(diff_line_prefix(o), file);
 
 	if (len == 0) {
 		has_trailing_newline = (first == '\n');
@@ -606,13 +603,17 @@ static void emit_line_0(struct diff_options *o,
 	if (len || !nofirst) {
 		if (reverse && want_color(o->use_color))
 			fputs(GIT_COLOR_REVERSE, file);
-		if (set_sign && set_sign[0])
-			fputs(set_sign, file);
+		if (set_sign || set)
+			fputs(set_sign ? set_sign : set, file);
 		if (first && !nofirst)
 			fputc(first, file);
-		if (set)
-			fputs(set, file);
-		fwrite(line, len, 1, file);
+		if (len) {
+			if (set_sign && set && set != set_sign)
+				fputs(reset, file);
+			if (set)
+				fputs(set, file);
+			fwrite(line, len, 1, file);
+		}
 		fputs(reset, file);
 	}
 	if (has_trailing_carriage_return)
@@ -999,16 +1000,13 @@ static void emit_line_ws_markup(struct diff_options *o,
 	if (!ws && !set_sign)
 		emit_line_0(o, set, NULL, 0, reset, sign, line, len);
 	else if (!ws) {
-		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0);
-		emit_line_0(o, set, NULL, 0, reset, 0, line, len);
+		emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);
 	} else if (blank_at_eof)
 		/* Blank line at EOF - paint '+' as well */
 		emit_line_0(o, ws, NULL, 0, reset, sign, line, len);
 	else {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,
-			    sign, "", 0);
+		emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0);
 		ws_check_emit(line, len, ws_rule,
 			      o->file, set, reset, ws);
 	}
-- 
2.18.0.132.g195c49a2227

[PATCH 7/8] diff.c: compute reverse locally in emit_line_0

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index fdad7ffdd77..f565a2c0c2b 100644
--- a/diff.c
+++ b/diff.c
@@ -576,11 +576,12 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
 }
 
 static void emit_line_0(struct diff_options *o,
-			const char *set_sign, const char *set, unsigned reverse, const char *reset,
+			const char *set_sign, const char *set, const char *reset,
 			int first, const char *line, int len)
 {
 	int has_trailing_newline, has_trailing_carriage_return;
 	int nofirst;
+	int reverse = !!set && !!set_sign;
 	FILE *file = o->file;
 
 	fputs(diff_line_prefix(o), file);
@@ -625,7 +626,7 @@ static void emit_line_0(struct diff_options *o,
 static void emit_line(struct diff_options *o, const char *set, const char *reset,
 		      const char *line, int len)
 {
-	emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);
+	emit_line_0(o, set, NULL, reset, line[0], line+1, len-1);
 }
 
 enum diff_symbol {
@@ -998,15 +999,15 @@ static void emit_line_ws_markup(struct diff_options *o,
 	}
 
 	if (!ws && !set_sign)
-		emit_line_0(o, set, NULL, 0, reset, sign, line, len);
+		emit_line_0(o, set, NULL, reset, sign, line, len);
 	else if (!ws) {
-		emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);
+		emit_line_0(o, set_sign, set, reset, sign, line, len);
 	} else if (blank_at_eof)
 		/* Blank line at EOF - paint '+' as well */
-		emit_line_0(o, ws, NULL, 0, reset, sign, line, len);
+		emit_line_0(o, ws, NULL, reset, sign, line, len);
 	else {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0);
+		emit_line_0(o, set_sign, set, reset, sign, "", 0);
 		ws_check_emit(line, len, ws_rule,
 			      o->file, set, reset, ws);
 	}
@@ -1029,7 +1030,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 		context = diff_get_color_opt(o, DIFF_CONTEXT);
 		reset = diff_get_color_opt(o, DIFF_RESET);
 		putc('\n', o->file);
-		emit_line_0(o, context, NULL, 0, reset, '\\',
+		emit_line_0(o, context, NULL, reset, '\\',
 			    nneof, strlen(nneof));
 		break;
 	case DIFF_SYMBOL_SUBMODULE_HEADER:
-- 
2.18.0.132.g195c49a2227

[PATCH 8/8] diff.c: rewrite emit_line_0 more understandably

[Stefan Beller]

emit_line_0 has no nested conditions, but puts out all its arguments
(if set) in order. There is still a slight confusion with set
and set_sign, but let's defer that to a later patch.

'first' used be output always no matter if it was 0, but that got lost
at "diff: add an internal option to dual-color diffs of diffs",
2018-07-21), as there we broadened the meaning of 'first' to also
signal an early return.

The change in 'emit_line' makes sure that 'first' is never content, but
always under our control, a sign or special character in the beginning
of the line (or 0, in which case we ignore it).

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 73 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/diff.c b/diff.c
index f565a2c0c2b..2bd4d3d6839 100644
--- a/diff.c
+++ b/diff.c
@@ -580,43 +580,52 @@ static void emit_line_0(struct diff_options *o,
 			int first, const char *line, int len)
 {
 	int has_trailing_newline, has_trailing_carriage_return;
-	int nofirst;
 	int reverse = !!set && !!set_sign;
+	int needs_reset = 0;
+
 	FILE *file = o->file;
 
 	fputs(diff_line_prefix(o), file);
 
-	if (len == 0) {
-		has_trailing_newline = (first == '\n');
-		has_trailing_carriage_return = (!has_trailing_newline &&
-						(first == '\r'));
-		nofirst = has_trailing_newline || has_trailing_carriage_return;
-	} else {
-		has_trailing_newline = (len > 0 && line[len-1] == '\n');
-		if (has_trailing_newline)
-			len--;
-		has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
-		if (has_trailing_carriage_return)
-			len--;
-		nofirst = 0;
-	}
-
-	if (len || !nofirst) {
-		if (reverse && want_color(o->use_color))
-			fputs(GIT_COLOR_REVERSE, file);
-		if (set_sign || set)
-			fputs(set_sign ? set_sign : set, file);
-		if (first && !nofirst)
-			fputc(first, file);
-		if (len) {
-			if (set_sign && set && set != set_sign)
-				fputs(reset, file);
-			if (set)
-				fputs(set, file);
-			fwrite(line, len, 1, file);
-		}
-		fputs(reset, file);
+	has_trailing_newline = (len > 0 && line[len-1] == '\n');
+	if (has_trailing_newline)
+		len--;
+
+	has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
+	if (has_trailing_carriage_return)
+		len--;
+
+	if (!len && !first)
+		goto end_of_line;
+
+	if (reverse && want_color(o->use_color)) {
+		fputs(GIT_COLOR_REVERSE, file);
+		needs_reset = 1;
+	}
+
+	if (set_sign || set) {
+		fputs(set_sign ? set_sign : set, file);
+		needs_reset = 1;
 	}
+
+	if (first)
+		fputc(first, file);
+
+	if (!len)
+		goto end_of_line;
+
+	if (set) {
+		if (set_sign && set != set_sign)
+			fputs(reset, file);
+		fputs(set, file);
+		needs_reset = 1;
+	}
+	fwrite(line, len, 1, file);
+	needs_reset |= len > 0;
+
+end_of_line:
+	if (needs_reset)
+		fputs(reset, file);
 	if (has_trailing_carriage_return)
 		fputc('\r', file);
 	if (has_trailing_newline)
@@ -626,7 +635,7 @@ static void emit_line_0(struct diff_options *o,
 static void emit_line(struct diff_options *o, const char *set, const char *reset,
 		      const char *line, int len)
 {
-	emit_line_0(o, set, NULL, reset, line[0], line+1, len-1);
+	emit_line_0(o, set, NULL, reset, 0, line, len);
 }
 
 enum diff_symbol {
-- 
2.18.0.132.g195c49a2227

Re: [PATCH 2/8] t3206: add color test for range-diff --dual-color

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The 'expect'ed outcome has been taken by running the 'range-diff | decode'.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t3206-range-diff.sh | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 2237c7f4af9..019724e61a0 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -142,4 +142,43 @@ test_expect_success 'changed message' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'dual-coloring' '
> +	cat >expect <<-\EOF &&

It is a good idea to use something like "sed -e 's/^|//'" instead of
"cat" here; that way allows you to mark the left-edge of the data
with "|", for a test vector like this one that has a line that would
otherwise violate the whitespace style rules.  

An inferiour alternative would be to add .gitaddtibute entry to make
this file exempt from indent-with-tab rule, but even in this 40-line
block there only is one line that requires such a workaround, and it
won't help the initial application of this patch to get modified
when applied with "am --whitespace=fix".

> +	<YELLOW>1:  a4b3333 = 1:  f686024 s/5/A/<RESET>
> +	<RED>2:  f51d370 <RESET><YELLOW>!<RESET><GREEN> 2:  4ab067d<RESET><YELLOW> s/4/A/<RESET>
> +	    <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET>
> +	     <RESET>
> +	         s/4/A/<RESET>
> +	     <RESET>
> +	    <REVERSE><GREEN>+<RESET> <BOLD>   Also a silly comment here!<RESET>
> +	    <REVERSE><GREEN>+<RESET>
> +	     diff --git a/file b/file<RESET>
> +	    <RED> --- a/file<RESET>
> +	    <GREEN> +++ b/file<RESET>
> +	<RED>3:  0559556 <RESET><YELLOW>!<RESET><GREEN> 3:  b9cb956<RESET><YELLOW> s/11/B/<RESET>
> +	    <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET>
> +	      9<RESET>
> +	      10<RESET>
> +	    <RED> -11<RESET>
> +	    <REVERSE><RED>-<RESET><FAINT;GREEN>+BB<RESET>
> +	    <REVERSE><GREEN>+<RESET><BOLD;GREEN>+B<RESET>
> +	      12<RESET>
> +	      13<RESET>
> +	      14<RESET>
> +	<RED>4:  d966c5c <RESET><YELLOW>!<RESET><GREEN> 4:  8add5f1<RESET><YELLOW> s/12/B/<RESET>
> +	    <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET>
> +	    <CYAN> @@<RESET>
> +	      9<RESET>
> +	      10<RESET>
> +	    <REVERSE><RED>-<RESET><FAINT> BB<RESET>
> +	    <REVERSE><GREEN>+<RESET> <BOLD>B<RESET>
> +	    <RED> -12<RESET>
> +	    <GREEN> +B<RESET>
> +	      13<RESET>
> +	EOF
> +	git range-diff changed...changed-message --color --dual-color >actual.raw &&
> +	test_decode_color >actual <actual.raw &&
> +	test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCHv2 0/8] Add color test for range-diff, simplify diff.c

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Stefan Beller (8):
>   test_decode_color: understand FAINT and ITALIC
>   t3206: add color test for range-diff --dual-color
>   diff.c: simplify caller of emit_line_0
>   diff.c: reorder arguments for emit_line_ws_markup
>   diff.c: add set_sign to emit_line_0
>   diff: use emit_line_0 once per line
>   diff.c: compute reverse locally in emit_line_0
>   diff.c: rewrite emit_line_0 more understandably
>
>  diff.c                  | 94 +++++++++++++++++++++++------------------
>  t/t3206-range-diff.sh   | 39 +++++++++++++++++
>  t/test-lib-functions.sh |  2 +
>  3 files changed, 93 insertions(+), 42 deletions(-)

As I cannot merge this as is to 'pu' and keep going, I'll queue the
following to the tip.  I think it can be squashed to "t3206: add
color test" but for now they will remain separate patches.

Subject: [PATCH] fixup! t3206: add color test for range-diff --dual-color

---
 t/t3206-range-diff.sh | 64 +++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 019724e61a..e3b0764b43 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -143,38 +143,38 @@ test_expect_success 'changed message' '
 '
 
 test_expect_success 'dual-coloring' '
-	cat >expect <<-\EOF &&
-	<YELLOW>1:  a4b3333 = 1:  f686024 s/5/A/<RESET>
-	<RED>2:  f51d370 <RESET><YELLOW>!<RESET><GREEN> 2:  4ab067d<RESET><YELLOW> s/4/A/<RESET>
-	    <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET>
-	     <RESET>
-	         s/4/A/<RESET>
-	     <RESET>
-	    <REVERSE><GREEN>+<RESET> <BOLD>   Also a silly comment here!<RESET>
-	    <REVERSE><GREEN>+<RESET>
-	     diff --git a/file b/file<RESET>
-	    <RED> --- a/file<RESET>
-	    <GREEN> +++ b/file<RESET>
-	<RED>3:  0559556 <RESET><YELLOW>!<RESET><GREEN> 3:  b9cb956<RESET><YELLOW> s/11/B/<RESET>
-	    <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET>
-	      9<RESET>
-	      10<RESET>
-	    <RED> -11<RESET>
-	    <REVERSE><RED>-<RESET><FAINT;GREEN>+BB<RESET>
-	    <REVERSE><GREEN>+<RESET><BOLD;GREEN>+B<RESET>
-	      12<RESET>
-	      13<RESET>
-	      14<RESET>
-	<RED>4:  d966c5c <RESET><YELLOW>!<RESET><GREEN> 4:  8add5f1<RESET><YELLOW> s/12/B/<RESET>
-	    <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET>
-	    <CYAN> @@<RESET>
-	      9<RESET>
-	      10<RESET>
-	    <REVERSE><RED>-<RESET><FAINT> BB<RESET>
-	    <REVERSE><GREEN>+<RESET> <BOLD>B<RESET>
-	    <RED> -12<RESET>
-	    <GREEN> +B<RESET>
-	      13<RESET>
+	sed -e "s|^:||" >expect <<-\EOF &&
+	:<YELLOW>1:  a4b3333 = 1:  f686024 s/5/A/<RESET>
+	:<RED>2:  f51d370 <RESET><YELLOW>!<RESET><GREEN> 2:  4ab067d<RESET><YELLOW> s/4/A/<RESET>
+	:    <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET>
+	:     <RESET>
+	:         s/4/A/<RESET>
+	:     <RESET>
+	:    <REVERSE><GREEN>+<RESET> <BOLD>   Also a silly comment here!<RESET>
+	:    <REVERSE><GREEN>+<RESET>
+	:     diff --git a/file b/file<RESET>
+	:    <RED> --- a/file<RESET>
+	:    <GREEN> +++ b/file<RESET>
+	:<RED>3:  0559556 <RESET><YELLOW>!<RESET><GREEN> 3:  b9cb956<RESET><YELLOW> s/11/B/<RESET>
+	:    <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET>
+	:      9<RESET>
+	:      10<RESET>
+	:    <RED> -11<RESET>
+	:    <REVERSE><RED>-<RESET><FAINT;GREEN>+BB<RESET>
+	:    <REVERSE><GREEN>+<RESET><BOLD;GREEN>+B<RESET>
+	:      12<RESET>
+	:      13<RESET>
+	:      14<RESET>
+	:<RED>4:  d966c5c <RESET><YELLOW>!<RESET><GREEN> 4:  8add5f1<RESET><YELLOW> s/12/B/<RESET>
+	:    <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET>
+	:    <CYAN> @@<RESET>
+	:      9<RESET>
+	:      10<RESET>
+	:    <REVERSE><RED>-<RESET><FAINT> BB<RESET>
+	:    <REVERSE><GREEN>+<RESET> <BOLD>B<RESET>
+	:    <RED> -12<RESET>
+	:    <GREEN> +B<RESET>
+	:      13<RESET>
 	EOF
 	git range-diff changed...changed-message --color --dual-color >actual.raw &&
 	test_decode_color >actual <actual.raw &&
-- 
2.18.0-321-gffc6fa0e39

Re: [PATCHv2 0/8] Add color test for range-diff, simplify diff.c

[Stefan Beller]

On Wed, Aug 1, 2018 at 12:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > Stefan Beller (8):
> >   test_decode_color: understand FAINT and ITALIC
> >   t3206: add color test for range-diff --dual-color
> >   diff.c: simplify caller of emit_line_0
> >   diff.c: reorder arguments for emit_line_ws_markup
> >   diff.c: add set_sign to emit_line_0
> >   diff: use emit_line_0 once per line
> >   diff.c: compute reverse locally in emit_line_0
> >   diff.c: rewrite emit_line_0 more understandably
> >
> >  diff.c                  | 94 +++++++++++++++++++++++------------------
> >  t/t3206-range-diff.sh   | 39 +++++++++++++++++
> >  t/test-lib-functions.sh |  2 +
> >  3 files changed, 93 insertions(+), 42 deletions(-)
>
> As I cannot merge this as is to 'pu' and keep going, I'll queue the
> following to the tip.  I think it can be squashed to "t3206: add
> color test" but for now they will remain separate patches.
>
> Subject: [PATCH] fixup! t3206: add color test for range-diff --dual-color

Thanks for dealing with my stubbornness here.

I assumed that the contribution would be a one time hassle
during git-am, not an ongoing problem during the whole time
the patch flows through pu/next/master, which is why I punted
on doing this change and resending in a timely manner.

Further I assumed the sed trick as below is harder to read,
but it turns out it is not. It is still very understandable.

https://public-inbox.org/git/nycvar.QRO.7.76.6.1808011800570.71@tvgsbejvaqbjf.bet/
sounds like DScho wants to incorporate some white space related
stuff that might collide with the later parts of this series, so
I am not sure how easy it will be to carry this through pu,
so feel free to drop it as well.

Thanks!
Stefan

Re: [PATCHv2 0/8] Add color test for range-diff, simplify diff.c

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Wed, Aug 1, 2018 at 12:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Stefan Beller <sbeller@google.com> writes:
>>
>> > Stefan Beller (8):
>> >   test_decode_color: understand FAINT and ITALIC
>> >   t3206: add color test for range-diff --dual-color
>> >   diff.c: simplify caller of emit_line_0
>> >   diff.c: reorder arguments for emit_line_ws_markup
>> >   diff.c: add set_sign to emit_line_0
>> >   diff: use emit_line_0 once per line
>> >   diff.c: compute reverse locally in emit_line_0
>> >   diff.c: rewrite emit_line_0 more understandably
>> >
>> >  diff.c                  | 94 +++++++++++++++++++++++------------------
>> >  t/t3206-range-diff.sh   | 39 +++++++++++++++++
>> >  t/test-lib-functions.sh |  2 +
>> >  3 files changed, 93 insertions(+), 42 deletions(-)
>>
>> As I cannot merge this as is to 'pu' and keep going, I'll queue the
>> following to the tip.  I think it can be squashed to "t3206: add
>> color test" but for now they will remain separate patches.
>>
>> Subject: [PATCH] fixup! t3206: add color test for range-diff --dual-color
>
> Thanks for dealing with my stubbornness here.

I didn't know you were stubborn---I just thought you were busy in
other things.

> I assumed that the contribution would be a one time hassle
> during git-am, not an ongoing problem during the whole time
> the patch flows through pu/next/master, which is why I punted
> on doing this change and resending in a timely manner.

It is a bit worse, in that it won't be at pu/next/master boundary
but each and every time the integration branches are rebuilt, which
happens a few times a day.  Whitespace breakage after a merge is
detected and my automation stops to ask for manual inspection.

[PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup

[Stefan Beller]

The order shall be all colors first, then the content, flags at the end.
The colors are in order.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index f6df18af913..ab6e6a88a56 100644
--- a/diff.c
+++ b/diff.c
@@ -1185,9 +1185,9 @@ static void dim_moved_lines(struct diff_options *o)
 }
 
 static void emit_line_ws_markup(struct diff_options *o,
-				const char *set, const char *reset,
-				const char *line, int len,
-				const char *set_sign, char sign,
+				const char *set_sign, const char *set,
+				const char *reset,
+				char sign, const char *line, int len,
 				unsigned ws_rule, int blank_at_eof)
 {
 	const char *ws = NULL;
@@ -1271,7 +1271,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else if (c == '-')
 				set = diff_get_color_opt(o, DIFF_FILE_OLD);
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, ' ',
+		emit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:
@@ -1314,7 +1314,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 				set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);
 			flags &= ~DIFF_SYMBOL_CONTENT_WS_MASK;
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, '+',
+		emit_line_ws_markup(o, set_sign, set, reset, '+', line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
 		break;
@@ -1357,7 +1357,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else
 				set = diff_get_color_opt(o, DIFF_CONTEXT_DIM);
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, '-',
+		emit_line_ws_markup(o, set_sign, set, reset, '-', line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
 		break;
 	case DIFF_SYMBOL_WORDS_PORCELAIN:
-- 
2.18.0.865.gffc8e1a3cd6-goog

Re: [PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup

[Johannes Schindelin]

Hi Stefan,

On Fri, 10 Aug 2018, Stefan Beller wrote:

> The order shall be all colors first, then the content, flags at the end.

Okay.

> The colors are in order.

In order of what? Of the wavelength?

(I agree that the order now makes more sense, and that the diff is
correct.)

Ciao,
Dscho

> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  diff.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index f6df18af913..ab6e6a88a56 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1185,9 +1185,9 @@ static void dim_moved_lines(struct diff_options *o)
>  }
>  
>  static void emit_line_ws_markup(struct diff_options *o,
> -				const char *set, const char *reset,
> -				const char *line, int len,
> -				const char *set_sign, char sign,
> +				const char *set_sign, const char *set,
> +				const char *reset,
> +				char sign, const char *line, int len,
>  				unsigned ws_rule, int blank_at_eof)
>  {
>  	const char *ws = NULL;
> @@ -1271,7 +1271,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
>  			else if (c == '-')
>  				set = diff_get_color_opt(o, DIFF_FILE_OLD);
>  		}
> -		emit_line_ws_markup(o, set, reset, line, len, set_sign, ' ',
> +		emit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,
>  				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
>  		break;
>  	case DIFF_SYMBOL_PLUS:
> @@ -1314,7 +1314,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
>  				set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);
>  			flags &= ~DIFF_SYMBOL_CONTENT_WS_MASK;
>  		}
> -		emit_line_ws_markup(o, set, reset, line, len, set_sign, '+',
> +		emit_line_ws_markup(o, set_sign, set, reset, '+', line, len,
>  				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
>  				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
>  		break;
> @@ -1357,7 +1357,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
>  			else
>  				set = diff_get_color_opt(o, DIFF_CONTEXT_DIM);
>  		}
> -		emit_line_ws_markup(o, set, reset, line, len, set_sign, '-',
> +		emit_line_ws_markup(o, set_sign, set, reset, '-', line, len,
>  				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
>  		break;
>  	case DIFF_SYMBOL_WORDS_PORCELAIN:
> -- 
> 2.18.0.865.gffc8e1a3cd6-goog
> 
> 

[PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup

[Stefan Beller]

The order shall be all colors first, then the content, flags at the end.
The colors are in the order of occurrence, i.e. first the color for the
sign and then the color for the rest of the line.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index f6df18af913..ab6e6a88a56 100644
--- a/diff.c
+++ b/diff.c
@@ -1185,9 +1185,9 @@ static void dim_moved_lines(struct diff_options *o)
 }
 
 static void emit_line_ws_markup(struct diff_options *o,
-				const char *set, const char *reset,
-				const char *line, int len,
-				const char *set_sign, char sign,
+				const char *set_sign, const char *set,
+				const char *reset,
+				char sign, const char *line, int len,
 				unsigned ws_rule, int blank_at_eof)
 {
 	const char *ws = NULL;
@@ -1271,7 +1271,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else if (c == '-')
 				set = diff_get_color_opt(o, DIFF_FILE_OLD);
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, ' ',
+		emit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:
@@ -1314,7 +1314,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 				set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);
 			flags &= ~DIFF_SYMBOL_CONTENT_WS_MASK;
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, '+',
+		emit_line_ws_markup(o, set_sign, set, reset, '+', line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
 		break;
@@ -1357,7 +1357,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else
 				set = diff_get_color_opt(o, DIFF_CONTEXT_DIM);
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, '-',
+		emit_line_ws_markup(o, set_sign, set, reset, '-', line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
 		break;
 	case DIFF_SYMBOL_WORDS_PORCELAIN:
-- 
2.18.0.865.gffc8e1a3cd6-goog