[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

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

[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

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"

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

[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

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

[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: [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 0/8] Resending sb/range-diff-colors

[Stefan Beller]

This is also avaliable as
git fetch https://github.com/stefanbeller/git sb/range-diff-colors

and is a resend of sb/range-diff-colors.

It is rebased on the version of range-diff that Johannes just sent out
(pr-1/dscho/branch-diff-v5 in GGG repo), and squashes the fisup commit
(which had to be adapted slightly in one line, too)

range-diff below.

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 \
  5bf616af71afe1c4c36da7f21077662febf28cbe..c1b144ea695514cfe185fe70089198621c38d01c \
  ccf8c1bb2459d33c7dc97098c08c47ca7d77ed3e..4dc97b54a35c60c25ab7634441d60711ead0e84e \
  >>0000-cover-letter.patch

 1:  7f88339e03e =  1:  0fedd4c0a20 test_decode_color: understand FAINT and ITALIC
 2:  13e8528be69 <  -:  ----------- t3206: add color test for range-diff --dual-color
 -:  ----------- >  2:  6a1c7698c68 t3206: add color test for range-diff --dual-color
 3:  2f80811b319 =  3:  7e12ece1d34 diff.c: simplify caller of emit_line_0
 4:  15af0d378c8 !  4:  74dabd6d36f diff.c: reorder arguments for emit_line_ws_markup
    @@ -35,7 +35,7 @@
      	case DIFF_SYMBOL_PLUS:
     @@
      				set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);
    - 			flags |= WS_IGNORE_FIRST_SPACE;
    + 			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,
 5:  dce49bb58fd =  5:  e304e15aa6b diff.c: add set_sign to emit_line_0
 6:  7581e16d63f =  6:  8d935d5212c diff: use emit_line_0 once per line
 7:  d070d393f73 =  7:  2344aac787a diff.c: compute reverse locally in emit_line_0
 8:  669e7516e03 =  8:  4dc97b54a35 diff.c: rewrite emit_line_0 more understandably
 9:  c1b144ea695 <  -:  ----------- fixup! t3206: add color test for range-diff --dual-color

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

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.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.865.gffc8e1a3cd6-goog

[PATCHv2 0/8] Resending sb/range-diff-colors

[Stefan Beller]

This is also avaliable as
git fetch https://github.com/stefanbeller/git sb/range-diff-colors

This applies on top of js/range-diff (a7be92acd9600), this version 
* resolves a semantic conflict in the test
  (Did range-diff change its output slightly?)
* addressed Johannes feedback, such as
 -> keeping the reverse computation out of emit_line_0
 -> better commit messages.
 -> Split "use emit_line_0 once per line" to have an additional
    "diff.c: omit check for line prefix in emit_line_0" as having
    these two things in the same patch is confusing

The interdiff seems more useful than the range-diff here,
both attached below.

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: omit check for line prefix in emit_line_0
  diff.c: rewrite emit_line_0 more understandably

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

(interdiff seems to be more useful)
git diff 4dc97b54a35..058e03a1601

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index ef3ba22e297..f52d45d9d61 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -53,6 +53,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		else
 			i += c;
 	}
+	while (i < argc)
+		argv[j++] = argv[i++];
 	argc = j;
 	diff_setup_done(&diffopt);
 
diff --git a/diff.c b/diff.c
index af9316c8f91..c5c7739ce34 100644
--- a/diff.c
+++ b/diff.c
@@ -622,13 +622,11 @@ 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, 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;
-	int reverse = !!set && !!set_sign;
-	int needs_reset = 0;
-
+	int needs_reset = 0; /* at the end of the line */
 	FILE *file = o->file;
 
 	fputs(diff_line_prefix(o), file);
@@ -649,8 +647,8 @@ static void emit_line_0(struct diff_options *o,
 		needs_reset = 1;
 	}
 
-	if (set_sign || set) {
-		fputs(set_sign ? set_sign : set, file);
+	if (set_sign) {
+		fputs(set_sign, file);
 		needs_reset = 1;
 	}
 
@@ -667,7 +665,7 @@ static void emit_line_0(struct diff_options *o,
 		needs_reset = 1;
 	}
 	fwrite(line, len, 1, file);
-	needs_reset |= len > 0;
+	needs_reset = 1; /* 'line' may contain color codes. */
 
 end_of_line:
 	if (needs_reset)
@@ -681,7 +679,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, 0, line, len);
+	emit_line_0(o, set, NULL, 0, reset, 0, line, len);
 }
 
 enum diff_symbol {
@@ -1213,15 +1211,16 @@ static void emit_line_ws_markup(struct diff_options *o,
 	}
 
 	if (!ws && !set_sign)
-		emit_line_0(o, set, NULL, reset, sign, line, len);
+		emit_line_0(o, set, NULL, 0, reset, sign, line, len);
 	else if (!ws) {
-		emit_line_0(o, set_sign, set, reset, sign, 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, 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, reset, sign, "", 0);
+		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);
 	}
@@ -1244,7 +1243,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, reset, '\\',
+		emit_line_0(o, context, NULL, 0, reset, '\\',
 			    nneof, strlen(nneof));
 		break;
 	case DIFF_SYMBOL_SUBMODULE_HEADER:




./git-range-diff github/sb/range-diff-colors... below:

22:  0fedd4c0a20 = 22:  dd772035ac9 test_decode_color: understand FAINT and ITALIC
23:  6a1c7698c68 ! 23:  5701745379b t3206: add color test for range-diff --dual-color
    @@ -23,7 +23,7 @@
     +	:         s/4/A/<RESET>
     +	:     <RESET>
     +	:    <REVERSE><GREEN>+<RESET><BOLD>    Also a silly comment here!<RESET>
    -+	:    <REVERSE><GREEN>+<RESET>
    ++	:    <REVERSE><GREEN>+<RESET><BOLD><RESET>
     +	:     diff --git a/file b/file<RESET>
     +	:    <RED> --- a/file<RESET>
     +	:    <GREEN> +++ b/file<RESET>
24:  7e12ece1d34 = 24:  4e56f63a5f5 diff.c: simplify caller of emit_line_0
25:  74dabd6d36f ! 25:  cf126556940 diff.c: reorder arguments for emit_line_ws_markup
    @@ -3,7 +3,8 @@
         diff.c: reorder arguments for emit_line_ws_markup
     
         The order shall be all colors first, then the content, flags at the end.
    -    The colors are in order.
    +    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>
26:  e304e15aa6b ! 26:  69008364cb8 diff.c: add set_sign to emit_line_0
    @@ -2,14 +2,17 @@
     
         diff.c: add set_sign to emit_line_0
     
    -    For now just change the signature, we'll reason about the actual
    -    change in a follow up patch.
    +    Split the meaning of the `set` parameter that is passed to
    +    emit_line_0()` to separate between the color of the "sign" (i.e.
    +    the diff marker '+', '-' or ' ' that is passed in as the `first`
    +    parameter) and the color of the rest of the line.
     
    -    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.
    +    This changes the meaning of the `set` parameter to no longer refer
    +    to the color of the diff marker, but instead to refer to the color
    +    of the rest of the line. A value of `NULL` indicates that the rest
    +    of the line wants to be colored the same as the diff marker.
     
    +    Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
         Signed-off-by: Stefan Beller <sbeller@google.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    @@ -30,12 +33,15 @@
      		if (reverse && want_color(o->use_color))
      			fputs(GIT_COLOR_REVERSE, file);
     -		fputs(set, file);
    -+		if (set_sign && set_sign[0])
    ++		if (set_sign)
     +			fputs(set_sign, file);
      		if (first && !nofirst)
      			fputc(first, file);
    -+		if (set)
    ++		if (set && set != set_sign) {
    ++			if (set_sign)
    ++				fputs(reset, file);
     +			fputs(set, file);
    ++		}
      		fwrite(line, len, 1, file);
      		fputs(reset, file);
      	}
27:  8d935d5212c <  -:  ----------- diff: use emit_line_0 once per line
28:  2344aac787a <  -:  ----------- diff.c: compute reverse locally in emit_line_0
 -:  ----------- > 27:  5d2629281a1 diff: use emit_line_0 once per line
 -:  ----------- > 28:  5240e94a69a diff.c: omit check for line prefix in emit_line_0
29:  4dc97b54a35 ! 29:  058e03a1601 diff.c: rewrite emit_line_0 more understandably
    @@ -2,21 +2,15 @@
     
         diff.c: rewrite emit_line_0 more understandably
     
    -    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.
    +    Rewrite emit_line_0 to have fewer (nested) conditions.
     
    -    '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).
    +    The change in 'emit_line' makes sure that 'first' is never user data,
    +    but always under our control, a sign or special character in the
    +    beginning of the line (or 0, in which case we ignore it).
    +    So from now on, let's pass only a diff marker or 0 as the 'first'
    +    character of the line.
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
     diff --git a/diff.c b/diff.c
     --- a/diff.c
    @@ -26,9 +20,7 @@
      {
      	int has_trailing_newline, has_trailing_carriage_return;
     -	int nofirst;
    - 	int reverse = !!set && !!set_sign;
    -+	int needs_reset = 0;
    -+
    ++	int needs_reset = 0; /* at the end of the line */
      	FILE *file = o->file;
      
      	fputs(diff_line_prefix(o), file);
    @@ -51,15 +43,16 @@
     -	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 (set_sign)
    +-			fputs(set_sign, file);
     -		if (first && !nofirst)
     -			fputc(first, file);
     -		if (len) {
    --			if (set_sign && set && set != set_sign)
    --				fputs(reset, file);
    --			if (set)
    +-			if (set && set != set_sign) {
    +-				if (set_sign)
    +-					fputs(reset, file);
     -				fputs(set, file);
    +-			}
     -			fwrite(line, len, 1, file);
     -		}
     -		fputs(reset, file);
    @@ -79,8 +72,8 @@
     +		needs_reset = 1;
     +	}
     +
    -+	if (set_sign || set) {
    -+		fputs(set_sign ? set_sign : set, file);
    ++	if (set_sign) {
    ++		fputs(set_sign, file);
     +		needs_reset = 1;
      	}
     +
    @@ -97,7 +90,7 @@
     +		needs_reset = 1;
     +	}
     +	fwrite(line, len, 1, file);
    -+	needs_reset |= len > 0;
    ++	needs_reset = 1; /* 'line' may contain color codes. */
     +
     +end_of_line:
     +	if (needs_reset)
    @@ -109,8 +102,8 @@
      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);
    +-	emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);
    ++	emit_line_0(o, set, NULL, 0, reset, 0, line, len);
      }
      
      enum diff_symbol {

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

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.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.865.gffc8e1a3cd6-goog