threads / patch / 48967

patch, 8 partsAdd color test for range-diff, simplify diff.c

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

## tl;dr

26 messages between Jul 28, 2018 and Aug 2, 2018. Diffs are folded; open one to read it.

replies: 25people: 3as markdown or json

Stefan Beller· Jul 28, 2018, 03:04 UTC · lore
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
Stefan Beller· Jul 28, 2018, 03:04 UTC · re: Stefan Beller · lore

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

Signed-off-by: Stefan Beller <sbeller@google.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.345.g5c9ce644c3-goog
Stefan Beller· Jul 28, 2018, 03:04 UTC · re: Stefan Beller · lore

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

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

-- 
2.18.0.345.g5c9ce644c3-goog
Eric Sunshine· Jul 28, 2018, 06:27 UTC · re: Stefan Beller · lore

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

On Fri, Jul 27, 2018 at 11:05 PM Stefan Beller <sbeller@google.com> wrote:
> The 'expect'ed outcome is taken by running the 'range-diff |decode';
> it is not meant as guidance, rather as a documentation of the current
> situation.
I'm not really sure what this is trying to say. It seems _too_ brief.
Did you want a space after the vertical bar before "decode"?
Show 5 quoted lines
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> +test_expect_success 'simple coloring' '
> +       q_to_tab >expect <<-EOF &&
Why 'q_to_tab'? I don't see any "q"'s in the body.

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

Stefan Beller· Jul 30, 2018, 19:55 UTC · re: Eric Sunshine · lore

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

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

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

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

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

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

Show 11 quoted lines
>
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> > +test_expect_success 'simple coloring' '
> > +       q_to_tab >expect <<-EOF &&
>
> Why 'q_to_tab'? I don't see any "q"'s in the body.
>
> I also don't see any variable interpolation in the body, so maybe you
> want -\EOF instead?
All good suggestions! Thanks for the review, I'll incorporate them.

Thanks, Stefan

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

-- 
2.18.0.345.g5c9ce644c3-goog
Stefan Beller· Jul 28, 2018, 03:04 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 that is setting the color after the sign. Hence, promote any 'set's to set_sign as we want to have color before the sign for now.

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

-- 
2.18.0.345.g5c9ce644c3-goog
Eric Sunshine· Jul 28, 2018, 06:30 UTC · re: Stefan Beller · lore

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

On Fri, Jul 27, 2018 at 11:05 PM Stefan Beller <sbeller@google.com> wrote:
Show 6 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 that is setting
> the color after the sign. Hence, promote any 'set's to set_sign as
> we want to have color before the sign for now.
ECANTPARSE: "and set that is setting"
Stefan Beller· Jul 28, 2018, 03:04 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>
---
 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 a36ed92c54c..fdad7ffdd77 100644
--- a/diff.c
+++ b/diff.c
@@ -583,10 +583,7 @@ static void emit_line_0(struct diff_options *o,
 	int nofirst;
 	FILE *file = o->file;
 
-	if (first)
-		fputs(diff_line_prefix(o), file);
-	else if (!len)
-		return;
+	fputs(diff_line_prefix(o), file);
 
 	if (len == 0) {
 		has_trailing_newline = (first == '\n');
@@ -606,13 +603,17 @@ static void emit_line_0(struct diff_options *o,
 	if (len || !nofirst) {
 		if (reverse && want_color(o->use_color))
 			fputs(GIT_COLOR_REVERSE, file);
-		if (set_sign && set_sign[0])
-			fputs(set_sign, file);
+		if (set_sign || set)
+			fputs(set_sign ? set_sign : set, file);
 		if (first && !nofirst)
 			fputc(first, file);
-		if (set)
-			fputs(set, file);
-		fwrite(line, len, 1, file);
+		if (len) {
+			if (set_sign && set && set != set_sign)
+				fputs(reset, file);
+			if (set)
+				fputs(set, file);
+			fwrite(line, len, 1, file);
+		}
 		fputs(reset, file);
 	}
 	if (has_trailing_carriage_return)
@@ -999,16 +1000,13 @@ static void emit_line_ws_markup(struct diff_options *o,
 	if (!ws && !set_sign)
 		emit_line_0(o, set, NULL, 0, reset, sign, line, len);
 	else if (!ws) {
-		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0);
-		emit_line_0(o, set, NULL, 0, reset, 0, line, len);
+		emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);
 	} else if (blank_at_eof)
 		/* Blank line at EOF - paint '+' as well */
 		emit_line_0(o, ws, NULL, 0, reset, sign, line, len);
 	else {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,
-			    sign, "", 0);
+		emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0);
 		ws_check_emit(line, len, ws_rule,
 			      o->file, set, reset, ws);
 	}

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

-- 
2.18.0.345.g5c9ce644c3-goog
Stefan Beller· Jul 28, 2018, 03:04 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 got lost at e8c285c4f9c (diff: add an internal option to dual-color diffs of diffs, 2018-07-21), as there we broadened the meaning of 'first' to also signal an early return.

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 73 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 32 deletions(-)
Show changes to diff.c +41 −32
diff --git a/diff.c b/diff.c
index f565a2c0c2b..2bd4d3d6839 100644
--- a/diff.c
+++ b/diff.c
@@ -580,43 +580,52 @@ static void emit_line_0(struct diff_options *o,
 			int first, const char *line, int len)
 {
 	int has_trailing_newline, has_trailing_carriage_return;
-	int nofirst;
 	int reverse = !!set && !!set_sign;
+	int needs_reset = 0;
+
 	FILE *file = o->file;
 
 	fputs(diff_line_prefix(o), file);
 
-	if (len == 0) {
-		has_trailing_newline = (first == '\n');
-		has_trailing_carriage_return = (!has_trailing_newline &&
-						(first == '\r'));
-		nofirst = has_trailing_newline || has_trailing_carriage_return;
-	} else {
-		has_trailing_newline = (len > 0 && line[len-1] == '\n');
-		if (has_trailing_newline)
-			len--;
-		has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
-		if (has_trailing_carriage_return)
-			len--;
-		nofirst = 0;
-	}
-
-	if (len || !nofirst) {
-		if (reverse && want_color(o->use_color))
-			fputs(GIT_COLOR_REVERSE, file);
-		if (set_sign || set)
-			fputs(set_sign ? set_sign : set, file);
-		if (first && !nofirst)
-			fputc(first, file);
-		if (len) {
-			if (set_sign && set && set != set_sign)
-				fputs(reset, file);
-			if (set)
-				fputs(set, file);
-			fwrite(line, len, 1, file);
-		}
-		fputs(reset, file);
+	has_trailing_newline = (len > 0 && line[len-1] == '\n');
+	if (has_trailing_newline)
+		len--;
+
+	has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
+	if (has_trailing_carriage_return)
+		len--;
+
+	if (!len && !first)
+		goto end_of_line;
+
+	if (reverse && want_color(o->use_color)) {
+		fputs(GIT_COLOR_REVERSE, file);
+		needs_reset = 1;
+	}
+
+	if (set_sign || set) {
+		fputs(set_sign ? set_sign : set, file);
+		needs_reset = 1;
 	}
+
+	if (first)
+		fputc(first, file);
+
+	if (!len)
+		goto end_of_line;
+
+	if (set) {
+		if (set_sign && set != set_sign)
+			fputs(reset, file);
+		fputs(set, file);
+		needs_reset = 1;
+	}
+	fwrite(line, len, 1, file);
+	needs_reset |= len > 0;
+
+end_of_line:
+	if (needs_reset)
+		fputs(reset, file);
 	if (has_trailing_carriage_return)
 		fputc('\r', file);
 	if (has_trailing_newline)
@@ -626,7 +635,7 @@ static void emit_line_0(struct diff_options *o,
 static void emit_line(struct diff_options *o, const char *set, const char *reset,
 		      const char *line, int len)
 {
-	emit_line_0(o, set, NULL, reset, line[0], line+1, len-1);
+	emit_line_0(o, set, NULL, reset, 0, line, len);
 }
 
 enum diff_symbol {

-- 
2.18.0.345.g5c9ce644c3-goog
Eric Sunshine· Jul 28, 2018, 06:33 UTC · re: Stefan Beller · lore

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

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

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

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
Stefan Beller· Jul 31, 2018, 00:31 UTC · re: Stefan Beller · lore

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

Signed-off-by: Stefan Beller <sbeller@google.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.132.g195c49a2227
Stefan Beller· Jul 31, 2018, 00:31 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>
---
 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..019724e61a0 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -142,4 +142,43 @@ test_expect_success 'changed message' '
 	test_cmp expected actual
 '
 
+test_expect_success 'dual-coloring' '
+	cat >expect <<-\EOF &&
+	<YELLOW>1:  a4b3333 = 1:  f686024 s/5/A/<RESET>
+	<RED>2:  f51d370 <RESET><YELLOW>!<RESET><GREEN> 2:  4ab067d<RESET><YELLOW> s/4/A/<RESET>
+	    <REVERSE><CYAN>@@ -2,6 +2,8 @@<RESET>
+	     <RESET>
+	         s/4/A/<RESET>
+	     <RESET>
+	    <REVERSE><GREEN>+<RESET> <BOLD>   Also a silly comment here!<RESET>
+	    <REVERSE><GREEN>+<RESET>
+	     diff --git a/file b/file<RESET>
+	    <RED> --- a/file<RESET>
+	    <GREEN> +++ b/file<RESET>
+	<RED>3:  0559556 <RESET><YELLOW>!<RESET><GREEN> 3:  b9cb956<RESET><YELLOW> s/11/B/<RESET>
+	    <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET>
+	      9<RESET>
+	      10<RESET>
+	    <RED> -11<RESET>
+	    <REVERSE><RED>-<RESET><FAINT;GREEN>+BB<RESET>
+	    <REVERSE><GREEN>+<RESET><BOLD;GREEN>+B<RESET>
+	      12<RESET>
+	      13<RESET>
+	      14<RESET>
+	<RED>4:  d966c5c <RESET><YELLOW>!<RESET><GREEN> 4:  8add5f1<RESET><YELLOW> s/12/B/<RESET>
+	    <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET>
+	    <CYAN> @@<RESET>
+	      9<RESET>
+	      10<RESET>
+	    <REVERSE><RED>-<RESET><FAINT> BB<RESET>
+	    <REVERSE><GREEN>+<RESET> <BOLD>B<RESET>
+	    <RED> -12<RESET>
+	    <GREEN> +B<RESET>
+	      13<RESET>
+	EOF
+	git range-diff changed...changed-message --color --dual-color >actual.raw &&
+	test_decode_color >actual <actual.raw &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.18.0.132.g195c49a2227
Junio C Hamano· Jul 31, 2018, 20:51 UTC · re: Stefan Beller · lore

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

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

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

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

Show 38 quoted lines
> +	<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
Stefan Beller· Jul 31, 2018, 00:31 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>
---
 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 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
Stefan Beller· Jul 31, 2018, 00:31 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>
---
 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 f7251c89cbb..8fd2171d808 100644
--- a/diff.c
+++ b/diff.c
@@ -980,9 +980,9 @@ static void dim_moved_lines(struct diff_options *o)
 }
 
 static void emit_line_ws_markup(struct diff_options *o,
-				const char *set, const char *reset,
-				const char *line, int len,
-				const char *set_sign, char sign,
+				const char *set_sign, const char *set,
+				const char *reset,
+				char sign, const char *line, int len,
 				unsigned ws_rule, int blank_at_eof)
 {
 	const char *ws = NULL;
@@ -1066,7 +1066,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else if (c == '-')
 				set = diff_get_color_opt(o, DIFF_FILE_OLD);
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, ' ',
+		emit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:
@@ -1109,7 +1109,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 				set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);
 			flags |= WS_IGNORE_FIRST_SPACE;
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, '+',
+		emit_line_ws_markup(o, set_sign, set, reset, '+', line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
 		break;
@@ -1152,7 +1152,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else
 				set = diff_get_color_opt(o, DIFF_CONTEXT_DIM);
 		}
-		emit_line_ws_markup(o, set, reset, line, len, set_sign, '-',
+		emit_line_ws_markup(o, set_sign, set, reset, '-', line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
 		break;
 	case DIFF_SYMBOL_WORDS_PORCELAIN:

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

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

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

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

-- 
2.18.0.132.g195c49a2227
Junio C Hamano· Aug 1, 2018, 19:13 UTC · re: Stefan Beller · lore

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

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

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

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

-- 
2.18.0-321-gffc6fa0e39
Stefan Beller· Aug 1, 2018, 19:46 UTC · re: Junio C Hamano · lore

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

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

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

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

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

Thanks! Stefan

Junio C Hamano· Aug 2, 2018, 15:48 UTC · re: Stefan Beller · lore

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

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

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

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

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

← back to recent threads