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

[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

[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>
Signed-off-by: Junio C Hamano <gitster@pobox.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..7dc7c80a1de 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' '
+	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 &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.18.0.865.gffc8e1a3cd6-goog

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

[Johannes Schindelin]

Hi Stefan,

On Fri, 10 Aug 2018, Stefan Beller wrote:

> +test_expect_success 'dual-coloring' '
> +	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>

That's a neat trick to have an indented here-doc that contains
indentation. I should use this myself.

The rest of the diff looks good, too, thanks!
Dscho

> +	:     <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.865.gffc8e1a3cd6-goog
> 
> 

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

[Stefan Beller]

On Mon, Aug 13, 2018 at 5:05 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Stefan,
>
> On Fri, 10 Aug 2018, Stefan Beller wrote:
>
> > +test_expect_success 'dual-coloring' '
> > +     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>
>
> That's a neat trick to have an indented here-doc that contains
> indentation. I should use this myself.

Thanks to Junio for applying this trick!

[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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index ae131495216..f6df18af913 100644
--- a/diff.c
+++ b/diff.c
@@ -1202,8 +1202,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.865.gffc8e1a3cd6-goog

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

[Johannes Schindelin]

Hi Stefan,

On Fri, 10 Aug 2018, Stefan Beller wrote:

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

I trust your judgement on that, also on how likely this previous condition
is to keep guaranteeing that assumption.

Thank you,
Dscho

> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  diff.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index ae131495216..f6df18af913 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1202,8 +1202,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.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 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 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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index ab6e6a88a56..5eea5edca50 100644
--- a/diff.c
+++ b/diff.c
@@ -622,7 +622,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;
@@ -652,9 +652,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);
 	}
@@ -667,7 +670,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 {
@@ -1199,17 +1202,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);
@@ -1233,7 +1236,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.865.gffc8e1a3cd6-goog

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

[Johannes Schindelin]

Hi Stefan,

On Fri, 10 Aug 2018, Stefan Beller 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' 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.

I'll freely admit that I had to study the diff in order to understand this
paragraph.

My I suggest something along those lines instead?

	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.

	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.

Ciao,
Dscho

> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  diff.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index ab6e6a88a56..5eea5edca50 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -622,7 +622,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;
> @@ -652,9 +652,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);
>  	}
> @@ -667,7 +670,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 {
> @@ -1199,17 +1202,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);
> @@ -1233,7 +1236,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.865.gffc8e1a3cd6-goog
> 
> 

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

[Stefan Beller]

On Mon, Aug 13, 2018 at 5:15 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Stefan,
>
> On Fri, 10 Aug 2018, Stefan Beller 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' 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.
>
> I'll freely admit that I had to study the diff in order to understand this
> paragraph.
>
> My I suggest something along those lines instead?
>
>         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.
>
>         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.
>

That is a wonderful commit message,
I'll just use it as-is.

Thanks,
Stefan

[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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/diff.c b/diff.c
index 5eea5edca50..01095a59d3d 100644
--- a/diff.c
+++ b/diff.c
@@ -629,10 +629,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');
@@ -652,13 +649,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)
@@ -1204,16 +1205,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.865.gffc8e1a3cd6-goog

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

[Johannes Schindelin]

Hi Stefan,

On Fri, 10 Aug 2018, Stefan Beller wrote:

> 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>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  diff.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index 5eea5edca50..01095a59d3d 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -629,10 +629,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);

How about separating this into its own "now that `first` is no longer
`NULL`, skip this" patch?

I found it a bit hard to review this diff, primarily because this hunk
would logically make more sense after the other hunks.


>  	if (len == 0) {
>  		has_trailing_newline = (first == '\n');
> @@ -652,13 +649,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);

Wait, what? Why is `set` all of a sudden also applying to `first`?

I would have expected `set_sign` to apply to `first`, and `set` to the
rest of the line.

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

That sounds as if `set == set_sign` would duplicate the `set`. How about
this instead?

			if (set && set != set_sign) {
				if (set_sign)
					fputs(reset, file);
				fputs(set, file);
			}

The rest looks good to me.

Thank you,
Dscho

> +			fwrite(line, len, 1, file);
> +		}
>  		fputs(reset, file);
>  	}
>  	if (has_trailing_carriage_return)
> @@ -1204,16 +1205,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.865.gffc8e1a3cd6-goog
> 
> 

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

[Stefan Beller]

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

diff --git a/diff.c b/diff.c
index 01095a59d3d..e50cd312755 100644
--- a/diff.c
+++ b/diff.c
@@ -622,11 +622,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);
@@ -671,7 +672,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 {
@@ -1203,15 +1204,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);
 	}
@@ -1234,7 +1235,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.865.gffc8e1a3cd6-goog

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

[Johannes Schindelin]

Hi,


On Fri, 10 Aug 2018, Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Well, my rationale for having the explicit `reverse` parameter is: this
code is complex enough, introducing some magic "this and that implies
this" makes it much harder to understand.

So I am not at all sure that this is a good thing.

> ---
>  diff.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index 01095a59d3d..e50cd312755 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -622,11 +622,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;

In contrast to, say, Javascript which has this nice feature that you can
write `set || set_sign` to mean `set ? set : set_sign`, I am fairly
certain that `set && set_sign` already evaluates to `0` or `1`. No need
for all those exclamation marks!!!!

:-)

But as I said: I think it is a bit too magic for my liking to say "if
the diff marker color is specified as well as the color for the rest of
the line, then the diff marker will be reversed". That's just making the
code hard to understand, i.e. less readable rather than more.

Ciao,
Dscho

>  	FILE *file = o->file;
>  
>  	fputs(diff_line_prefix(o), file);
> @@ -671,7 +672,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 {
> @@ -1203,15 +1204,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);
>  	}
> @@ -1234,7 +1235,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.865.gffc8e1a3cd6-goog
> 
> 

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

[Stefan Beller]

On Mon, Aug 13, 2018 at 5:26 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi,
>
>
> On Fri, 10 Aug 2018, Stefan Beller wrote:
>
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Well, my rationale for having the explicit `reverse` parameter is: this
> code is complex enough, introducing some magic "this and that implies
> this" makes it much harder to understand.
>
> So I am not at all sure that this is a good thing.

Yeah I am unsure, too. On the other hand the higher level functions
look so much nicer a the complexity is shoved downwards,
such that each function is solving problems in their own domain.

I'll think of an alternative.

[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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 73 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/diff.c b/diff.c
index e50cd312755..af9316c8f91 100644
--- a/diff.c
+++ b/diff.c
@@ -626,43 +626,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)
@@ -672,7 +681,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.865.gffc8e1a3cd6-goog

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

[Johannes Schindelin]

Hi Stefan,

On Fri, 10 Aug 2018, Stefan Beller wrote:

> emit_line_0 has no nested conditions, but puts out all its arguments
> (if set) in order.

Well, currently `emit_line_0()` *has* nested conditions: `first == '\n'`
inside `len == 0`.

And these nested conditions make things hard to read, so resolving that
to a simpler workflow makes a ton of sense. You can sell that better by
starting the commit message with something along the lines that you are
making the overly complex logic easier to follow.

> There is still a slight confusion with set and set_sign, but let's defer
> that to a later patch.

There is no later patch in this here patch series. I would therefore
appreciate it if you could spend the paragraph or two on explaining
yourself here.

> 'first' used be output always no matter if it was 0, but that got lost

s/used be/used to be/

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

Did we? I thought we introduced the possibility of passing *just* a first
character or *just* a "rest of the line". I might misremember, though.

> The change in 'emit_line' makes sure that 'first' is never content, but

<tongue-in-cheek>Awwww, you want to make 'first' sad all the time? That's
not nice of you...</tongue-in-cheek>

Seriously again, the adjective "content" has a different meaning than the
noun and this ambiguity made this sentence very hard for me to parse.

> always under our control, a sign or special character in the beginning
> of the line (or 0, in which case we ignore it).

It would be good to reword this paragraph to say that from now on, we will
only pass a diff marker as `first`, or 0.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  diff.c | 73 +++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 41 insertions(+), 32 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index e50cd312755..af9316c8f91 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -626,43 +626,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)) {

Since you implied `reverse` to mean that a non-`NULL` `set` *as well as*
`set_sign` were passed in, and since a non-`NULL` `set` *implies* that we
want color, you could drop that `want_color(o->use_color)` here.

But as I stated above, I am not a fan of having such unintuitive
implications in the code.

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

First of all, this should be a `||=`, not a `|=`.

And then, this code is skipped by the `if (!len) goto end_of_line;` part
above, so `len > 0` is *always* 1 at this point.

But then, I wonder why we bother all that much. After all, we want to
reset whenever we used color. So why not simply initialize

	int need_reset = reverse || set_sign || set;

and be done with it?

Thanks,
Dscho

> +
> +end_of_line:
> +	if (needs_reset)
> +		fputs(reset, file);
>  	if (has_trailing_carriage_return)
>  		fputc('\r', file);
>  	if (has_trailing_newline)
> @@ -672,7 +681,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.865.gffc8e1a3cd6-goog
> 
> 

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

[Stefan Beller]

On Mon, Aug 13, 2018 at 5:42 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Stefan,
>
> On Fri, 10 Aug 2018, Stefan Beller wrote:
>
> > emit_line_0 has no nested conditions, but puts out all its arguments
> > (if set) in order.
>
> Well, currently `emit_line_0()` *has* nested conditions: `first == '\n'`
> inside `len == 0`.
>
> And these nested conditions make things hard to read, so resolving that
> to a simpler workflow makes a ton of sense. You can sell that better by
> starting the commit message with something along the lines that you are
> making the overly complex logic easier to follow.
>
> > There is still a slight confusion with set and set_sign, but let's defer
> > that to a later patch.
>
> There is no later patch in this here patch series.

yup, I got overly excited when writing this commit message, as for
what we could try next.

> I would therefore
> appreciate it if you could spend the paragraph or two on explaining
> yourself here.

noted.

>
> > 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.
>
> Did we? I thought we introduced the possibility of passing *just* a first
> character or *just* a "rest of the line". I might misremember, though.
>
> > The change in 'emit_line' makes sure that 'first' is never content, but
>
> <tongue-in-cheek>Awwww, you want to make 'first' sad all the time? That's
> not nice of you...</tongue-in-cheek>
>
> Seriously again, the adjective "content" has a different meaning than the
> noun and this ambiguity made this sentence very hard for me to parse.

So what is a good noun for the stuff that Git stores? ("stuff" is not a
good one)

> > always under our control, a sign or special character in the beginning
> > of the line (or 0, in which case we ignore it).
>
> It would be good to reword this paragraph to say that from now on, we will
> only pass a diff marker as `first`, or 0.

Makes sense.



> > +     if (!len && !first)
> > +             goto end_of_line;
> > +
> > +     if (reverse && want_color(o->use_color)) {
>
> Since you implied `reverse` to mean that a non-`NULL` `set` *as well as*
> `set_sign` were passed in, and since a non-`NULL` `set` *implies* that we
> want color, you could drop that `want_color(o->use_color)` here.
>
> But as I stated above, I am not a fan of having such unintuitive
> implications in the code.

okay. So we'd want to be explicit about reverse again?

> > +     fwrite(line, len, 1, file);
> > +     needs_reset |= len > 0;
>
> First of all, this should be a `||=`, not a `|=`.
>
> And then, this code is skipped by the `if (!len) goto end_of_line;` part
> above, so `len > 0` is *always* 1 at this point.
>
> But then, I wonder why we bother all that much. After all, we want to
> reset whenever we used color. So why not simply initialize
>
>         int need_reset = reverse || set_sign || set;
>
> and be done with it?

I just removed that line and all tests still pass. :/
I could have sworn of a failing test when writing the code
that ensures that content (noun), that contains color codes,
would still look okay by having a reset at the end of the line,
so really we'd need to have

  need_reset = reverse || set_sign || set || (len > 0);

I'll dig into this again.

Thanks,
Stefan

Re: [PATCH 0/8] Resending sb/range-diff-colors

[Stefan Beller]

On Fri, Aug 10, 2018 at 3:34 PM Stefan Beller <sbeller@google.com> wrote:
>
> 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.

I thought about this series a bit, and I think we would want to break
it up into 2:

  * the actual sb/range-diff-colors consisting of the first two patches
  I can resend them or you can just rebase them without conflicts.
  These two patches (test_decode_color: FAINT/ITALIC + t3206: color)
  are essentially just adding the tests and making sure the colored
  range-diff will work correctly in the future

 * The refactoring, which might be titled sb/diff-refactor instead.
    Given Johannes review comments, I will rework all patches
    that are "diff: something" to have less confusion for Johannes
    reviewing them.

And once we have these two we can have a resend of
https://public-inbox.org/git/20180810224923.143625-1-sbeller@google.com/
which might be titled sb/range-diff-adjust-colors.


Thanks,
Stefan

[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

[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>
Signed-off-by: Junio C Hamano <gitster@pobox.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..31f6458f961 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' '
+	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><BOLD><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.865.gffc8e1a3cd6-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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index ae131495216..f6df18af913 100644
--- a/diff.c
+++ b/diff.c
@@ -1202,8 +1202,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.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

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

[Stefan Beller]

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.

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>
---
 diff.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index ab6e6a88a56..4ef66389282 100644
--- a/diff.c
+++ b/diff.c
@@ -622,7 +622,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;
@@ -652,9 +652,15 @@ 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)
+			fputs(set_sign, file);
 		if (first && !nofirst)
 			fputc(first, file);
+		if (set && set != set_sign) {
+			if (set_sign)
+				fputs(reset, file);
+			fputs(set, file);
+		}
 		fwrite(line, len, 1, file);
 		fputs(reset, file);
 	}
@@ -667,7 +673,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 {
@@ -1199,17 +1205,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);
@@ -1233,7 +1239,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.865.gffc8e1a3cd6-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.

We gain a little efficiency here, as we can omit emission of color and
accompanying reset if 'len == 0'.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c                | 16 ++++++++--------
 t/t3206-range-diff.sh |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index 4ef66389282..4f430f44e64 100644
--- a/diff.c
+++ b/diff.c
@@ -656,12 +656,14 @@ static void emit_line_0(struct diff_options *o,
 			fputs(set_sign, file);
 		if (first && !nofirst)
 			fputc(first, file);
-		if (set && set != set_sign) {
-			if (set_sign)
-				fputs(reset, file);
-			fputs(set, file);
+		if (len) {
+			if (set && set != set_sign) {
+				if (set_sign)
+					fputs(reset, file);
+				fputs(set, file);
+			}
+			fwrite(line, len, 1, file);
 		}
-		fwrite(line, len, 1, file);
 		fputs(reset, file);
 	}
 	if (has_trailing_carriage_return)
@@ -1207,9 +1209,7 @@ 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);
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 31f6458f961..7dc7c80a1de 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -151,7 +151,7 @@ test_expect_success 'dual-coloring' '
 	:         s/4/A/<RESET>
 	:     <RESET>
 	:    <REVERSE><GREEN>+<RESET><BOLD>    Also a silly comment here!<RESET>
-	:    <REVERSE><GREEN>+<RESET><BOLD><RESET>
+	:    <REVERSE><GREEN>+<RESET>
 	:     diff --git a/file b/file<RESET>
 	:    <RED> --- a/file<RESET>
 	:    <GREEN> +++ b/file<RESET>
-- 
2.18.0.865.gffc8e1a3cd6-goog

[PATCH 7/8] diff.c: omit check for line prefix in emit_line_0

[Stefan Beller]

As the previous patch made sure we only call emit_line_0 once per line,
we do not need the work around introduced in f7c3b4e2d8c (diff: add an
internal option to dual-color diffs of diffs, 2018-08-13) that would ensure
we'd emit 'diff_line_prefix(o)' just once per line.

By having just one call of emit_line_0 per line, the checks are dead code.

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

diff --git a/diff.c b/diff.c
index 4f430f44e64..7a23adf254d 100644
--- a/diff.c
+++ b/diff.c
@@ -629,10 +629,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');
-- 
2.18.0.865.gffc8e1a3cd6-goog

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

[Stefan Beller]

Rewrite emit_line_0 to have fewer (nested) conditions.

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>
---
 diff.c | 73 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 40 insertions(+), 33 deletions(-)

diff --git a/diff.c b/diff.c
index 7a23adf254d..c5c7739ce34 100644
--- a/diff.c
+++ b/diff.c
@@ -626,43 +626,50 @@ 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 needs_reset = 0; /* at the end of the line */
 	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)
-			fputs(set_sign, file);
-		if (first && !nofirst)
-			fputc(first, file);
-		if (len) {
-			if (set && set != set_sign) {
-				if (set_sign)
-					fputs(reset, file);
-				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) {
+		fputs(set_sign, 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 = 1; /* 'line' may contain color codes. */
+
+end_of_line:
+	if (needs_reset)
+		fputs(reset, file);
 	if (has_trailing_carriage_return)
 		fputc('\r', file);
 	if (has_trailing_newline)
@@ -672,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, 0, reset, line[0], line+1, len-1);
+	emit_line_0(o, set, NULL, 0, reset, 0, line, len);
 }
 
 enum diff_symbol {
-- 
2.18.0.865.gffc8e1a3cd6-goog

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

[Johannes Schindelin]

Hi Stefan,

On Mon, 13 Aug 2018, Stefan Beller wrote:

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

If by semantic conflict you mean...

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

... this, then I do not understand. This looks like a straight-up change
in your commit. v1 of this patch did not append <BOLD><RESET> to the end,
while v2 apparently does.

And it looks a bit funny, indeed.

In any case, from what I see you indeed addressed all my comments (the
need_reset was done differently than I suggested, but still okay).

Ciao,
Dscho

>      +	:     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 {
> 

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

[Stefan Beller]

On Tue, Aug 14, 2018 at 8:05 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Stefan,
>
> On Mon, 13 Aug 2018, Stefan Beller wrote:
>
> > 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?)
>
> If by semantic conflict you mean...
>
> > 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>
>
> ... this, then I do not understand. This looks like a straight-up change
> in your commit. v1 of this patch did not append <BOLD><RESET> to the end,
> while v2 apparently does.

Yes, I did mean this. Both before and now the test passes on top of
 js/range-diff (which was resend), so I concluded that the logic in
range-diff itself
has changed. However this is changed in a later patch to not appear any more.

>
> In any case, from what I see you indeed addressed all my comments (the
> need_reset was done differently than I suggested, but still okay).

Thanks.

So unless I hear from other people I consider this series done.
(just like range-diff is not going to be resend but built on top now)

Thanks,
Stefan

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

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