threads / patch / 49095

patch, 8 partsResending sb/range-diff-colors

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

## tl;dr

32 messages between Aug 10, 2018 and Aug 14, 2018. Diffs are folded; open one to read it.

replies: 31people: 2as markdown or json

Stefan Beller· Aug 10, 2018, 22:34 UTC · lore

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
Stefan Beller· Aug 10, 2018, 22:34 UTC · re: Stefan Beller · lore

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

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(+)
Show changes to t/test-lib-functions.sh +2 −0
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
Stefan Beller· Aug 10, 2018, 22:34 UTC · re: Stefan Beller · lore

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

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(+)
Show changes to t/t3206-range-diff.sh +39 −0
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
Johannes Schindelin· Aug 13, 2018, 12:05 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Fri, 10 Aug 2018, Stefan Beller wrote:
Show 5 quoted lines
> +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

Show 39 quoted lines
> +	:     <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
> 
> 
Stefan Beller· Aug 13, 2018, 18:30 UTC · re: Johannes Schindelin · lore

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

On Mon, Aug 13, 2018 at 5:05 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 13 quoted lines
>
> 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!
Stefan Beller· Aug 10, 2018, 22:34 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to diff.c +1 −2
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
Johannes Schindelin· Aug 13, 2018, 12:07 UTC · re: Stefan Beller · lore

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

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

Show 24 quoted lines
> 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
> 
> 
Stefan Beller· Aug 10, 2018, 22:34 UTC · re: Stefan Beller · lore

[PATCH 4/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.

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(-)
Show changes to diff.c +6 −6
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
Johannes Schindelin· Aug 13, 2018, 12:09 UTC · re: Stefan Beller · lore

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

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

Show 55 quoted lines
> 
> 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
> 
> 
Stefan Beller· Aug 10, 2018, 22:34 UTC · re: Stefan Beller · lore

[PATCH 5/8] 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.

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(-)
Show changes to diff.c +12 −9
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
Johannes Schindelin· Aug 13, 2018, 12:16 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Fri, 10 Aug 2018, Stefan Beller wrote:
Show 7 quoted lines
> 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

Show 79 quoted lines
> 
> 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
> 
> 
Stefan Beller· Aug 13, 2018, 23:42 UTC · re: Johannes Schindelin · lore

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

On Mon, Aug 13, 2018 at 5:15 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 28 quoted lines
>
> 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

Stefan Beller· Aug 10, 2018, 22:34 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to diff.c +12 −14
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
Johannes Schindelin· Aug 13, 2018, 12:22 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Fri, 10 Aug 2018, Stefan Beller wrote:
Show 22 quoted lines
> 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.

Show 10 quoted lines
>  	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.

Show 10 quoted lines
>  		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

Show 28 quoted lines
> +			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
> 
> 
Stefan Beller· Aug 10, 2018, 22:34 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to diff.c +8 −7
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
Johannes Schindelin· Aug 13, 2018, 12:26 UTC · re: Stefan Beller · lore

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

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.
Show 19 quoted lines
> ---
>  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

Show 45 quoted lines
>  	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
> 
> 
Stefan Beller· Aug 13, 2018, 19:00 UTC · re: Johannes Schindelin · lore

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

On Mon, Aug 13, 2018 at 5:26 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 14 quoted lines
>
> 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.
Stefan Beller· Aug 10, 2018, 22:34 UTC · re: Stefan Beller · lore

[PATCH 8/8] 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.

'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(-)
Show changes to diff.c +41 −32
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
Johannes Schindelin· Aug 13, 2018, 12:42 UTC · re: Stefan Beller · lore

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

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.

Show 64 quoted lines
> 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.

Show 23 quoted lines
> +		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

Show 20 quoted lines
> +
> +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
> 
> 
Stefan Beller· Aug 13, 2018, 18:57 UTC · re: Johannes Schindelin · lore

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

On Mon, Aug 13, 2018 at 5:42 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 20 quoted lines
>
> 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.
Show 15 quoted lines
>
> > 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)

Show 5 quoted lines
> > 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.
Show 11 quoted lines
> > +     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?
Show 14 quoted lines
> > +     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

Stefan Beller· Aug 13, 2018, 23:36 UTC · re: Stefan Beller · lore

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

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

Stefan Beller· Aug 14, 2018, 01:41 UTC · re: Stefan Beller · lore

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

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

Show changes to 2 files +14 −13

builtin/range-diff.c, diff.c

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 {
Stefan Beller· Aug 14, 2018, 01:41 UTC · re: Stefan Beller · lore

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

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(+)
Show changes to t/test-lib-functions.sh +2 −0
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
Stefan Beller· Aug 14, 2018, 01:41 UTC · re: Stefan Beller · lore

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

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(+)
Show changes to t/t3206-range-diff.sh +39 −0
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
Stefan Beller· Aug 14, 2018, 01:41 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to diff.c +1 −2
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
Stefan Beller· Aug 14, 2018, 01:41 UTC · re: Stefan Beller · lore

[PATCH 4/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 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(-)
Show changes to diff.c +6 −6
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
Stefan Beller· Aug 14, 2018, 01:41 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to diff.c +15 −9
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
Stefan Beller· Aug 14, 2018, 01:41 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 2 files +9 −9

diff.c, t/t3206-range-diff.sh

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
Stefan Beller· Aug 14, 2018, 01:41 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to diff.c +1 −4
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
Stefan Beller· Aug 14, 2018, 01:41 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to diff.c +40 −33
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
Johannes Schindelin· Aug 14, 2018, 15:05 UTC · re: Stefan Beller · lore

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

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...
Show 7 quoted lines
> 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

Show 156 quoted lines
>      +	:     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 {
> 
Stefan Beller· Aug 14, 2018, 16:45 UTC · re: Johannes Schindelin · lore

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

On Tue, Aug 14, 2018 at 8:05 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 22 quoted lines
>
> 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

← back to recent threads