[PATCH 0/4] Better colors in range-diff!

This improves colors of the range-diff, see last patch for details.

This is a partial resend of
https://public-inbox.org/git/20180804015317.182683-1-sbeller@google.com/
and is also available via

  git fetch https://github.com/stefanbeller/git sb/range-diff-better-colors

It applies on the (just reset) series of sb/range-diff-colors.

Thanks,
Stefan

Stefan Beller (4):
  diff.c: emit_line_0 to take string instead of first sign
  diff.c: add --output-indicator-{new, old, context}
  range-diff: make use of different output indicators
  range-diff: indent special lines as context

 diff.c                | 43 +++++++++++++++++++++++++++++++------------
 diff.h                |  5 +++++
 range-diff.c          | 17 ++++++++++++++++-
 t/t3206-range-diff.sh | 12 ++++++------
 4 files changed, 58 insertions(+), 19 deletions(-)

-- 
2.18.0.865.gffc8e1a3cd6-goog

By providing a string as the first part of the emission we can extend
it later more easily.

While at it, document emit_line_0.

Signed-off-by: Stefan Beller <redacted>
---
 diff.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/diff.c b/diff.c
index af9316c8f91..b3cb73eb69a 100644
--- a/diff.c
+++ b/diff.c

@@ -621,9 +621,15 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
 	ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
 }
 
+/*
+ * Emits
+ * <set_sign> <first> <reset> <set> <second> <reset> LF
+ * if they are present. 'first' is a NULL terminated string,
+ * 'second' is a buffer of length 'len'.
+ */
 static void emit_line_0(struct diff_options *o,
 			const char *set_sign, const char *set, const char *reset,
-			int first, const char *line, int len)
+			const char *first, const char *second, int len)
 {
 	int has_trailing_newline, has_trailing_carriage_return;
 	int reverse = !!set && !!set_sign;

@@ -633,11 +639,11 @@ static void emit_line_0(struct diff_options *o,
 
 	fputs(diff_line_prefix(o), file);
 
-	has_trailing_newline = (len > 0 && line[len-1] == '\n');
+	has_trailing_newline = (len > 0 && second[len-1] == '\n');
 	if (has_trailing_newline)
 		len--;
 
-	has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
+	has_trailing_carriage_return = (len > 0 && second[len-1] == '\r');
 	if (has_trailing_carriage_return)
 		len--;
 

@@ -655,7 +661,7 @@ static void emit_line_0(struct diff_options *o,
 	}
 
 	if (first)
-		fputc(first, file);
+		fputs(first, file);
 
 	if (!len)
 		goto end_of_line;

@@ -666,7 +672,7 @@ static void emit_line_0(struct diff_options *o,
 		fputs(set, file);
 		needs_reset = 1;
 	}
-	fwrite(line, len, 1, file);
+	fwrite(second, len, 1, file);
 	needs_reset |= len > 0;
 
 end_of_line:

@@ -681,7 +687,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, reset, NULL, line, len);
 }
 
 enum diff_symbol {

@@ -1201,7 +1207,7 @@ static void dim_moved_lines(struct diff_options *o)
 static void emit_line_ws_markup(struct diff_options *o,
 				const char *set_sign, const char *set,
 				const char *reset,
-				char sign, const char *line, int len,
+				const char *sign, const char *line, int len,
 				unsigned ws_rule, int blank_at_eof)
 {
 	const char *ws = NULL;

@@ -1244,7 +1250,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, reset, "\\",
 			    nneof, strlen(nneof));
 		break;
 	case DIFF_SYMBOL_SUBMODULE_HEADER:

@@ -1282,7 +1288,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_sign, set, reset, ' ', line, len,
+		emit_line_ws_markup(o, set_sign, set, reset, " ", line, len,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:

@@ -1325,7 +1331,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_sign, set, reset, '+', line, len,
+		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;

@@ -1368,7 +1374,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_sign, set, reset, '-', line, len,
+		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

This will prove useful in range-diff in a later patch as we will be able to
differentiate between adding a new file (that line is starting with +++
and then the file name) and regular new lines.

It could also be useful for experimentation in new patch formats, i.e.
we could teach git to emit moved lines with lines other than +/-.

Signed-off-by: Stefan Beller <redacted>
---
 diff.c | 21 +++++++++++++++++----
 diff.h |  5 +++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index b3cb73eb69a..b75eb085cb3 100644
--- a/diff.c
+++ b/diff.c

@@ -1237,7 +1237,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 					 struct emitted_diff_symbol *eds)
 {
 	static const char *nneof = " No newline at end of file\n";
-	const char *context, *reset, *set, *set_sign, *meta, *fraginfo;
+	const char *context, *reset, *set, *set_sign, *meta, *fraginfo, *first;
 	struct strbuf sb = STRBUF_INIT;
 
 	enum diff_symbol s = eds->s;

@@ -1288,7 +1288,9 @@ 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_sign, set, reset, " ", line, len,
+		first = o->output_indicators[OI_CONTEXT] ?
+			o->output_indicators[OI_CONTEXT] : " ";
+		emit_line_ws_markup(o, set_sign, set, reset, first, line, len,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:

@@ -1331,7 +1333,10 @@ 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_sign, set, reset, "+", line, len,
+
+		first = o->output_indicators[OI_NEW] ?
+			o->output_indicators[OI_NEW] : "+";
+		emit_line_ws_markup(o, set_sign, set, reset, first, line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
 		break;

@@ -1374,7 +1379,9 @@ 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_sign, set, reset, "-", line, len,
+		first = o->output_indicators[OI_OLD] ?
+			o->output_indicators[OI_OLD] : "-";
+		emit_line_ws_markup(o, set_sign, set, reset, first, line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
 		break;
 	case DIFF_SYMBOL_WORDS_PORCELAIN:

@@ -4876,6 +4883,12 @@ int diff_opt_parse(struct diff_options *options,
 		 options->output_format |= DIFF_FORMAT_DIFFSTAT;
 	} else if (!strcmp(arg, "--no-compact-summary"))
 		 options->flags.stat_with_summary = 0;
+	else if (skip_prefix(arg, "--output-indicator-new=", &arg))
+		options->output_indicators[OI_NEW] = arg;
+	else if (skip_prefix(arg, "--output-indicator-old=", &arg))
+		options->output_indicators[OI_OLD] = arg;
+	else if (skip_prefix(arg, "--output-indicator-context=", &arg))
+		options->output_indicators[OI_CONTEXT] = arg;
 
 	/* renames options */
 	else if (starts_with(arg, "-B") ||

diff --git a/diff.h b/diff.h
index e1e54256c18..2d4097df1c7 100644
--- a/diff.h
+++ b/diff.h

@@ -194,6 +194,11 @@ struct diff_options {
 	FILE *file;
 	int close_file;
 
+#define OI_NEW 0
+#define OI_OLD 1
+#define OI_CONTEXT 2
+	const char *output_indicators[3];
+
 	struct pathspec pathspec;
 	pathchange_fn_t pathchange;
 	change_fn_t change;

-- 
2.18.0.865.gffc8e1a3cd6-goog

This change itself only changes the internal communication and should
have no visible effect to the user. We instruct the diff code that produces
the inner diffs to use X, Y, Z instead of the usual markers for new, old
and context lines

Signed-off-by: Stefan Beller <redacted>
---
 range-diff.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/range-diff.c b/range-diff.c
index b6b9abac266..6e4192c8e79 100644
--- a/range-diff.c
+++ b/range-diff.c

@@ -38,6 +38,9 @@ static int read_patches(const char *range, struct string_list *list)
 
 	argv_array_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
 			"--reverse", "--date-order", "--decorate=no",
+			"--output-indicator-new=X",
+			"--output-indicator-old=Y",
+			"--output-indicator-context=Z",
 			"--no-abbrev-commit", range,
 			NULL);
 	cp.out = -1;

@@ -108,8 +111,18 @@ static int read_patches(const char *range, struct string_list *list)
 			 * we are not interested.
 			 */
 			continue;
-		else
+		else if (line.buf[0] == 'X') {
+			strbuf_addch(&buf, '+');
+			strbuf_add(&buf, line.buf + 1, line.len - 1);
+		} else if (line.buf[0] == 'Y') {
+			strbuf_addch(&buf, '-');
+			strbuf_add(&buf, line.buf + 1, line.len - 1);
+		} else if (line.buf[0] == 'Z') {
+			strbuf_addch(&buf, ' ');
+			strbuf_add(&buf, line.buf + 1, line.len - 1);
+		} else {
 			strbuf_addbuf(&buf, &line);
+		}
 
 		strbuf_addch(&buf, '\n');
 		util->diffsize++;

-- 
2.18.0.865.gffc8e1a3cd6-goog

The range-diff coloring is a bit fuzzy when it comes to special lines of
a diff, such as indicating new and old files with +++ and ---, as it
would pickup the first character and interpret it for its coloring, which
seems annoying as in regular diffs, these lines are colored bold via
DIFF_METAINFO.

By indenting these lines by a white space, they will be treated as context
which is much more useful, an example [1] on the range diff series itself:

[...]
    + diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
    + new file mode 100644
    + --- /dev/null
    + +++ b/Documentation/git-range-diff.txt
    +@@
    ++git-range-diff(1)
[...]
    +
      diff --git a/Makefile b/Makefile
      --- a/Makefile
      +++ b/Makefile
[...]

The first lines that introduce the new file for the man page will have the
'+' sign colored and the rest of the line will be bold.

The later lines that indicate a change to the Makefile will be treated as
context both in the outer and inner diff, such that those lines stay
regular color.

[1] ./git-range-diff pr-1/dscho/branch-diff-v3...pr-1/dscho/branch-diff-v4
    These tags are found at https://github.com/gitgitgadget/git

Signed-off-by: Stefan Beller <redacted>
---
 range-diff.c          |  2 ++
 t/t3206-range-diff.sh | 12 ++++++------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index 6e4192c8e79..a8db7c4f8d3 100644
--- a/range-diff.c
+++ b/range-diff.c

@@ -85,6 +85,7 @@ static int read_patches(const char *range, struct string_list *list)
 			strbuf_addch(&buf, '\n');
 			if (!util->diff_offset)
 				util->diff_offset = buf.len;
+			strbuf_addch(&buf, ' ');
 			strbuf_addbuf(&buf, &line);
 		} else if (in_header) {
 			if (starts_with(line.buf, "Author: ")) {

@@ -121,6 +122,7 @@ static int read_patches(const char *range, struct string_list *list)
 			strbuf_addch(&buf, ' ');
 			strbuf_add(&buf, line.buf + 1, line.len - 1);
 		} else {
+			strbuf_addch(&buf, ' ');
 			strbuf_addbuf(&buf, &line);
 		}
 

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 7dc7c80a1de..c94f9bf5ee1 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh

@@ -133,9 +133,9 @@ test_expect_success 'changed message' '
 	    Z
 	    +    Also a silly comment here!
 	    +
-	    Zdiff --git a/file b/file
-	    Z--- a/file
-	    Z+++ b/file
+	    Z diff --git a/file b/file
+	    Z --- a/file
+	    Z +++ b/file
 	3:  147e64e = 3:  b9cb956 s/11/B/
 	4:  a63e992 = 4:  8add5f1 s/12/B/
 	EOF

@@ -152,9 +152,9 @@ test_expect_success 'dual-coloring' '
 	:     <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>
+	:      diff --git a/file b/file<RESET>
+	:      --- a/file<RESET>
+	:      +++ 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>

-- 
2.18.0.865.gffc8e1a3cd6-goog

Hi Stefan,

On Fri, 10 Aug 2018, Stefan Beller wrote:

By providing a string as the first part of the emission we can extend
it later more easily.

Thank you for working on this!

While at it, document emit_line_0.

[...]
 
+/*
+ * Emits
+ * <set_sign> <first> <reset> <set> <second> <reset> LF
+ * if they are present. 'first' is a NULL terminated string,
+ * 'second' is a buffer of length 'len'.
+ */

That does not make it clear what the role of `first` or `second` is. Could
you clarify that?

(TBH I am not so sure myself what roles they serve. Previously, it was
kind of obvious to me that `first` tried to specify the diff marker, if
any. But now...?)

The rest looks good to me.

Thanks,
Dscho

Hi Steafn,

On Fri, 10 Aug 2018, Stefan Beller wrote:

This will prove useful in range-diff in a later patch as we will be able to
differentiate between adding a new file (that line is starting with +++
and then the file name) and regular new lines.

Very good!

Instead of doing this over and over again, how about

1) setting o->output_indicators to " " in diff_setup()?

2) passing OI_CONTEXT to emit_line_ws_markup() instead of `first`? I.e.
   change it to the index in the output_indicators, with -1 indicating
   "none"?

I could imagine that OI_* is too generic a prefix, and that we would want
to have a prefix that is less prone to collide with other global
constants, such as OUTPUT_INDICATOR_*.

Ciao,
Dscho

+	const char *output_indicators[3];
+
 	struct pathspec pathspec;
 	pathchange_fn_t pathchange;
 	change_fn_t change;
-- 
2.18.0.865.gffc8e1a3cd6-goog

Hi Stefan,

On Fri, 10 Aug 2018, Stefan Beller wrote:

My preliminary reading (I sadly lack the time to pull your branch and play
with it) suggests that this works, although I have to admit that X/Y/Z
would confuse me in 6 months from now, as they do not really read like
diff markers but like plain text. I could imagine that '>', '<' and '#'
would not impart that confusion on me.

Ciao,
Dscho

 
 		strbuf_addch(&buf, '\n');
 		util->diffsize++;
-- 
2.18.0.865.gffc8e1a3cd6-goog

Hi Stefan,

On Fri, 10 Aug 2018, Stefan Beller wrote:

The range-diff coloring is a bit fuzzy when it comes to special lines of
a diff, such as indicating new and old files with +++ and ---, as it
would pickup the first character and interpret it for its coloring, which
seems annoying as in regular diffs, these lines are colored bold via
DIFF_METAINFO.

By indenting these lines by a white space, they will be treated as context
which is much more useful, an example [1] on the range diff series itself:

[...]
    + diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
    + new file mode 100644
    + --- /dev/null
    + +++ b/Documentation/git-range-diff.txt
    +@@
    ++git-range-diff(1)
[...]
    +
      diff --git a/Makefile b/Makefile
      --- a/Makefile
      +++ b/Makefile
[...]

The first lines that introduce the new file for the man page will have the
'+' sign colored and the rest of the line will be bold.

The later lines that indicate a change to the Makefile will be treated as
context both in the outer and inner diff, such that those lines stay
regular color.

While I am a fan of having those lines colored correctly, I have to admit
that I am not exactly enthusiastic about that extra indentation...

Otherwise, this looks good to me.

Thanks,
Dscho

On Mon, Aug 13, 2018 at 4:42 AM Johannes Schindelin
[off-list ref] wrote:

quoted

+/*
+ * Emits
+ * <set_sign> <first> <reset> <set> <second> <reset> LF
+ * if they are present. 'first' is a NULL terminated string,
+ * 'second' is a buffer of length 'len'.
+ */

That does not make it clear what the role of `first` or `second` is. Could
you clarify that?

For now it is just "first string" and "second string", where the first is
used for signs and indicators, and the second is allowed to have '\0'
in it as we give the length as a parameter. This doc tried to be
neutral w.r.t. the purpose of the first/second string.

(TBH I am not so sure myself what roles they serve. Previously, it was
kind of obvious to me that `first` tried to specify the diff marker, if
any. But now...?)

Originally I thought we'd split the line into

  "++" "line"

for a range diff, but this is not the case.

As the next patch introduces configurable strings, we'd need this patch.
I consider changing the next patch to allow only giving a single character
instead, such that we can keep 'first', which indicates the sign character.
(So maybe I'd even call it 'sign').

On Mon, Aug 13, 2018 at 4:47 AM Johannes Schindelin
[off-list ref] wrote:

Hi Steafn,

On Fri, 10 Aug 2018, Stefan Beller wrote:

quoted

This will prove useful in range-diff in a later patch as we will be able to
differentiate between adding a new file (that line is starting with +++
and then the file name) and regular new lines.

Very good!

quoted

It could also be useful for experimentation in new patch formats, i.e.
we could teach git to emit moved lines with lines other than +/-.

Signed-off-by: Stefan Beller <redacted>
---
 diff.c | 21 +++++++++++++++++----
 diff.h |  5 +++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index b3cb73eb69a..b75eb085cb3 100644
--- a/diff.c
+++ b/diff.c

@@ -1237,7 +1237,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
                                       struct emitted_diff_symbol *eds)
 {
      static const char *nneof = " No newline at end of file\n";
-     const char *context, *reset, *set, *set_sign, *meta, *fraginfo;
+     const char *context, *reset, *set, *set_sign, *meta, *fraginfo, *first;
      struct strbuf sb = STRBUF_INIT;

      enum diff_symbol s = eds->s;

@@ -1288,7 +1288,9 @@ 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_sign, set, reset, " ", line, len,
+             first = o->output_indicators[OI_CONTEXT] ?
+                     o->output_indicators[OI_CONTEXT] : " ";
+             emit_line_ws_markup(o, set_sign, set, reset, first, line, len,

Instead of doing this over and over again, how about

1) setting o->output_indicators to " " in diff_setup()?

... and when parsing the command line options we could already overwrite it
  in place.

2) passing OI_CONTEXT to emit_line_ws_markup() instead of `first`? I.e.
   change it to the index in the output_indicators, with -1 indicating
   "none"?

That sounds like an elegant design, as then it is super clear that 'first'
can only ever be a sign (or character that we chose), but
giving -1 for "none" sounds cumbersome. I'll take a look into that.

quoted

+#define OI_NEW 0
+#define OI_OLD 1
+#define OI_CONTEXT 2

I could imagine that OI_* is too generic a prefix, and that we would want
to have a prefix that is less prone to collide with other global
constants, such as OUTPUT_INDICATOR_*.

I agree on that; will take the suggestion of
OUTPUT_INDICATOR_*.

quoted

                      strbuf_addbuf(&buf, &line);
+             }

My preliminary reading (I sadly lack the time to pull your branch and play
with it) suggests that this works, although I have to admit that X/Y/Z
would confuse me in 6 months from now, as they do not really read like
diff markers but like plain text. I could imagine that '>', '<' and '#'
would not impart that confusion on me.

Thanks for that suggestion! (I'll change it and add a comment)

quoted

The later lines that indicate a change to the Makefile will be treated as
context both in the outer and inner diff, such that those lines stay
regular color.

While I am a fan of having those lines colored correctly, I have to admit
that I am not exactly enthusiastic about that extra indentation...

Otherwise, this looks good to me.

Can you explain what makes you less enthused about the indentation?

Advantage:
* allows easy coloring (easy implementation)
Disadvantage:
* formats change, but the range diff is still in its early design phase,
  so we're not breaking things, yet?
  (Do we ever plan on sending range-diff patches that can be applied to
   rewrite history? I am very uncertain on such a feature request.
   It sounds cool, though)

Hi Stefan,

On Mon, 13 Aug 2018, Stefan Beller wrote:

quoted

quoted

The later lines that indicate a change to the Makefile will be treated as
context both in the outer and inner diff, such that those lines stay
regular color.

While I am a fan of having those lines colored correctly, I have to admit
that I am not exactly enthusiastic about that extra indentation...

Otherwise, this looks good to me.

Can you explain what makes you less enthused about the indentation?

Advantage:
* allows easy coloring (easy implementation)
Disadvantage:
* formats change,

This is it. It breaks my visual flow.

but the range diff is still in its early design phase, so we're not
breaking things, yet?

Indeed. We're not breaking things. If you feel strongly about it, we can
have that indentation, I *can* get used to it.

  (Do we ever plan on sending range-diff patches that can be applied to
  rewrite history? I am very uncertain on such a feature request.  It
  sounds cool, though)

I remember that I heard you discussing this internally. I am not too big a
fan of this idea, I have to admit. The range diff seems more designed to
explain how a patch series evolved, rather than providing machine-readable
data that allows to recreate said evolution. For example, the committer
information as well as the date are missing, which would preclude a
faithful reconstruction.

And that is not all: if you wanted to "apply" a range diff, you would need
to know more about the base(s) of the two commit ranges. You would need to
know that they are at least very similar to the base onto which you want
to apply this.

And quite seriously, this would be the wrong way to go in my mind. We have
a very efficient data format to transport all of that information: the Git
bundle.

Let's not overload the range diff format with multiple, partially
contradicting purposes. Think "separation of concerns". It's the same
issue, really, as trying to send highly structured data such as bug
reports or code contributions via a medium meant to send unstructured
plain or formatted text back and forth between human beings.

Ciao,
Dscho

On Tue, Aug 14, 2018 at 11:54 AM Johannes Schindelin
[off-list ref] wrote:

Hi Stefan,

On Mon, 13 Aug 2018, Stefan Beller wrote:

quoted

quoted

quoted

The later lines that indicate a change to the Makefile will be treated as
context both in the outer and inner diff, such that those lines stay
regular color.

While I am a fan of having those lines colored correctly, I have to admit
that I am not exactly enthusiastic about that extra indentation...

Otherwise, this looks good to me.

Can you explain what makes you less enthused about the indentation?

Advantage:
* allows easy coloring (easy implementation)
Disadvantage:
* formats change,

This is it. It breaks my visual flow.

quoted

but the range diff is still in its early design phase, so we're not
breaking things, yet?

Indeed. We're not breaking things. If you feel strongly about it, we can
have that indentation, I *can* get used to it.

I only feel strongly about it now as that is the *easiest* way to make
the colors
look like I want them to look. And I really value colors in the range-diff.
Earlier you said that color-less range-diff is nearly useless for you and I
thought it was hyperbole, but by now I realize how much truth you spoke.
So getting the colors fixed to not markup files (+++/ --- lines of the inner
diff) is a high priority for me. So high that I would compromise on the
indentation/flow of these corner case areas.

quoted

  (Do we ever plan on sending range-diff patches that can be applied to
  rewrite history? I am very uncertain on such a feature request.  It
  sounds cool, though)

I remember that I heard you discussing this internally. I am not too big a
fan of this idea, I have to admit. The range diff seems more designed to
explain how a patch series evolved, rather than providing machine-readable
data that allows to recreate said evolution. For example, the committer
information as well as the date are missing, which would preclude a
faithful reconstruction.

Ah! good point. Though we could just work around that and use the email
date for the new author dates. ;-)

And that is not all: if you wanted to "apply" a range diff, you would need
to know more about the base(s) of the two commit ranges. You would need to
know that they are at least very similar to the base onto which you want
to apply this.

You would say so in the cover letter "This is a resend of sb/range-diff-colors"
and by the knowledge of that tip only and the range-diff you would
know how the new series would look like, even if it was rebased.

And quite seriously, this would be the wrong way to go in my mind. We have
a very efficient data format to transport all of that information: the Git
bundle.

The bundle format is very efficient for machine transport, but I thought the
whole point of the mailing list was easy human readable parts, i.e. you can
point out things in a diff, which you could also do in a range-diff to some
extend. We would loose some of the "fresh eyes" as you'd only see the
changed part of the series. :-/ So yeah even for the workflow this seems
a net-negative. I just thought it would be cool.

Let's not overload the range diff format with multiple, partially
contradicting purposes. Think "separation of concerns". It's the same
issue, really, as trying to send highly structured data such as bug
reports or code contributions via a medium meant to send unstructured
plain or formatted text back and forth between human beings.

ok.

Thanks,
Stefan

Hi Stefan,

On Tue, 14 Aug 2018, Stefan Beller wrote:

On Tue, Aug 14, 2018 at 11:54 AM Johannes Schindelin
[off-list ref] wrote:

quoted

On Mon, 13 Aug 2018, Stefan Beller wrote:

quoted

quoted

quoted

The later lines that indicate a change to the Makefile will be
treated as context both in the outer and inner diff, such that
those lines stay regular color.

While I am a fan of having those lines colored correctly, I have
to admit that I am not exactly enthusiastic about that extra
indentation...

Otherwise, this looks good to me.

Can you explain what makes you less enthused about the indentation?

Advantage:
* allows easy coloring (easy implementation)
Disadvantage:
* formats change,

This is it. It breaks my visual flow.

quoted

but the range diff is still in its early design phase, so we're not
breaking things, yet?

Indeed. We're not breaking things. If you feel strongly about it, we
can have that indentation, I *can* get used to it.

I only feel strongly about it now as that is the *easiest* way to make
the colors look like I want them to look. And I really value colors in
the range-diff.  Earlier you said that color-less range-diff is nearly
useless for you and I thought it was hyperbole, but by now I realize how
much truth you spoke.  So getting the colors fixed to not markup files
(+++/ --- lines of the inner diff) is a high priority for me. So high
that I would compromise on the indentation/flow of these corner case
areas.

Okay, let's go with your indentation, then.

Ciao,
Dscho

This improves colors of the range-diff, see last patch for details.
it is also available via

  git fetch https://github.com/stefanbeller/git sb/range-diff-better-colors

Thanks,
Stefan

Stefan Beller (3):
  diff.c: add --output-indicator-{new, old, context}
  range-diff: make use of different output indicators
  range-diff: indent special lines as context

 diff.c                | 21 ++++++++++++++++++---
 diff.h                |  5 +++++
 range-diff.c          | 22 +++++++++++++++++++++-
 t/t3206-range-diff.sh | 12 ++++++------
 4 files changed, 50 insertions(+), 10 deletions(-)

-- 
2.18.0.265.g16de1b435c9.dirty

This will prove useful in range-diff in a later patch as we will be able to
differentiate between adding a new file (that line is starting with +++
and then the file name) and regular new lines.

It could also be useful for experimentation in new patch formats, i.e.
we could teach git to emit moved lines with lines other than +/-.

Signed-off-by: Stefan Beller <redacted>
---
 diff.c | 21 ++++++++++++++++++---
 diff.h |  5 +++++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index c5c7739ce34..03486c35b75 100644
--- a/diff.c
+++ b/diff.c

@@ -1281,7 +1281,9 @@ 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_sign, set, reset, ' ', line, len,
+		emit_line_ws_markup(o, set_sign, set, reset,
+				    o->output_indicators[OUTPUT_INDICATOR_CONTEXT],
+				    line, len,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:

@@ -1324,7 +1326,9 @@ 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_sign, set, reset, '+', line, len,
+		emit_line_ws_markup(o, set_sign, set, reset,
+				    o->output_indicators[OUTPUT_INDICATOR_NEW],
+				    line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
 		break;

@@ -1367,7 +1371,9 @@ 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_sign, set, reset, '-', line, len,
+		emit_line_ws_markup(o, set_sign, set, reset,
+				    o->output_indicators[OUTPUT_INDICATOR_OLD],
+				    line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
 		break;
 	case DIFF_SYMBOL_WORDS_PORCELAIN:

@@ -4382,6 +4388,9 @@ void diff_setup(struct diff_options *options)
 
 	options->file = stdout;
 
+	options->output_indicators[OUTPUT_INDICATOR_NEW] = '+';
+	options->output_indicators[OUTPUT_INDICATOR_OLD] = '-';
+	options->output_indicators[OUTPUT_INDICATOR_CONTEXT] = ' ';
 	options->abbrev = DEFAULT_ABBREV;
 	options->line_termination = '\n';
 	options->break_opt = -1;

@@ -4869,6 +4878,12 @@ int diff_opt_parse(struct diff_options *options,
 		 options->output_format |= DIFF_FORMAT_DIFFSTAT;
 	} else if (!strcmp(arg, "--no-compact-summary"))
 		 options->flags.stat_with_summary = 0;
+	else if (skip_prefix(arg, "--output-indicator-new=", &arg))
+		options->output_indicators[OUTPUT_INDICATOR_NEW] = arg[0];
+	else if (skip_prefix(arg, "--output-indicator-old=", &arg))
+		options->output_indicators[OUTPUT_INDICATOR_OLD] = arg[0];
+	else if (skip_prefix(arg, "--output-indicator-context=", &arg))
+		options->output_indicators[OUTPUT_INDICATOR_CONTEXT] = arg[0];
 
 	/* renames options */
 	else if (starts_with(arg, "-B") ||

diff --git a/diff.h b/diff.h
index e1e54256c18..d7edc64454a 100644
--- a/diff.h
+++ b/diff.h

@@ -194,6 +194,11 @@ struct diff_options {
 	FILE *file;
 	int close_file;
 
+#define OUTPUT_INDICATOR_NEW 0
+#define OUTPUT_INDICATOR_OLD 1
+#define OUTPUT_INDICATOR_CONTEXT 2
+	char output_indicators[3];
+
 	struct pathspec pathspec;
 	pathchange_fn_t pathchange;
 	change_fn_t change;

-- 
2.18.0.265.g16de1b435c9.dirty

This change itself only changes the internal communication and should
have no visible effect to the user. We instruct the diff code that
produces the inner diffs to use other markers instead of the
usual markers for new, old and context lines.

Signed-off-by: Stefan Beller <redacted>
---
 range-diff.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/range-diff.c b/range-diff.c
index b6b9abac266..a906d25f139 100644
--- a/range-diff.c
+++ b/range-diff.c

@@ -38,6 +38,14 @@ static int read_patches(const char *range, struct string_list *list)
 
 	argv_array_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
 			"--reverse", "--date-order", "--decorate=no",
+			/*
+			 * Choose indicators that are not used anywhere
+			 * else in diffs, but still look reasonable
+			 * (e.g. will not be confusing when debugging)
+			 */
+			"--output-indicator-new=>",
+			"--output-indicator-old=<",
+			"--output-indicator-context=#",
 			"--no-abbrev-commit", range,
 			NULL);
 	cp.out = -1;

@@ -108,8 +116,18 @@ static int read_patches(const char *range, struct string_list *list)
 			 * we are not interested.
 			 */
 			continue;
-		else
+		else if (line.buf[0] == '>') {
+			strbuf_addch(&buf, '+');
+			strbuf_add(&buf, line.buf + 1, line.len - 1);
+		} else if (line.buf[0] == '<') {
+			strbuf_addch(&buf, '-');
+			strbuf_add(&buf, line.buf + 1, line.len - 1);
+		} else if (line.buf[0] == '#') {
+			strbuf_addch(&buf, ' ');
+			strbuf_add(&buf, line.buf + 1, line.len - 1);
+		} else {
 			strbuf_addbuf(&buf, &line);
+		}
 
 		strbuf_addch(&buf, '\n');
 		util->diffsize++;

-- 
2.18.0.265.g16de1b435c9.dirty

The range-diff coloring is a bit fuzzy when it comes to special lines of
a diff, such as indicating new and old files with +++ and ---, as it
would pickup the first character and interpret it for its coloring, which
seems annoying as in regular diffs, these lines are colored bold via
DIFF_METAINFO.

By indenting these lines by a white space, they will be treated as context
which is much more useful, an example [1] on the range diff series itself:

[...]
    + diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
    + new file mode 100644
    + --- /dev/null
    + +++ b/Documentation/git-range-diff.txt
    +@@
    ++git-range-diff(1)
[...]
    +
      diff --git a/Makefile b/Makefile
      --- a/Makefile
      +++ b/Makefile
[...]

The first lines that introduce the new file for the man page will have the
'+' sign colored and the rest of the line will be bold.

The later lines that indicate a change to the Makefile will be treated as
context both in the outer and inner diff, such that those lines stay
regular color.

[1] ./git-range-diff pr-1/dscho/branch-diff-v3...pr-1/dscho/branch-diff-v4
    These tags are found at https://github.com/gitgitgadget/git

Signed-off-by: Stefan Beller <redacted>
---
 range-diff.c          |  2 ++
 t/t3206-range-diff.sh | 12 ++++++------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index a906d25f139..3e9b9844012 100644
--- a/range-diff.c
+++ b/range-diff.c

@@ -90,6 +90,7 @@ static int read_patches(const char *range, struct string_list *list)
 			strbuf_addch(&buf, '\n');
 			if (!util->diff_offset)
 				util->diff_offset = buf.len;
+			strbuf_addch(&buf, ' ');
 			strbuf_addbuf(&buf, &line);
 		} else if (in_header) {
 			if (starts_with(line.buf, "Author: ")) {

@@ -126,6 +127,7 @@ static int read_patches(const char *range, struct string_list *list)
 			strbuf_addch(&buf, ' ');
 			strbuf_add(&buf, line.buf + 1, line.len - 1);
 		} else {
+			strbuf_addch(&buf, ' ');
 			strbuf_addbuf(&buf, &line);
 		}
 

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 7dc7c80a1de..c94f9bf5ee1 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh

@@ -133,9 +133,9 @@ test_expect_success 'changed message' '
 	    Z
 	    +    Also a silly comment here!
 	    +
-	    Zdiff --git a/file b/file
-	    Z--- a/file
-	    Z+++ b/file
+	    Z diff --git a/file b/file
+	    Z --- a/file
+	    Z +++ b/file
 	3:  147e64e = 3:  b9cb956 s/11/B/
 	4:  a63e992 = 4:  8add5f1 s/12/B/
 	EOF

@@ -152,9 +152,9 @@ test_expect_success 'dual-coloring' '
 	:     <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>
+	:      diff --git a/file b/file<RESET>
+	:      --- a/file<RESET>
+	:      +++ 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>

-- 
2.18.0.265.g16de1b435c9.dirty

Hi Stefan,

On Fri, 17 Aug 2018, Stefan Beller wrote:

Thanks.

                                    ^
Here we already pass `o`... so...

+		emit_line_ws_markup(o, set_sign, set, reset,
+				    o->output_indicators[OUTPUT_INDICATOR_CONTEXT],

                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
... here, we could simply pass `OUTPUT_INDICATOR_CONTEXT` and let the
callee look it up in`o->output_indicators[]`...

I read all three patches and did not see a reason why we could not
simplify the code that way.

Other than that: great!

Thank you,
Dscho

On Mon, Aug 20, 2018 at 12:31 PM Johannes Schindelin
[off-list ref] wrote:

Hi Stefan,

On Fri, 17 Aug 2018, Stefan Beller wrote:

quoted

This will prove useful in range-diff in a later patch as we will be able
to differentiate between adding a new file (that line is starting with

+++ and then the file name) and regular new lines.

It could also be useful for experimentation in new patch formats, i.e.
we could teach git to emit moved lines with lines other than +/-.

Thanks.

quoted

diff --git a/diff.c b/diff.c
index c5c7739ce34..03486c35b75 100644
--- a/diff.c
+++ b/diff.c

@@ -1281,7 +1281,9 @@ 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_sign, set, reset, ' ', line, len,

                                    ^
Here we already pass `o`... so...

quoted

+             emit_line_ws_markup(o, set_sign, set, reset,
+                                 o->output_indicators[OUTPUT_INDICATOR_CONTEXT],

                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
... here, we could simply pass `OUTPUT_INDICATOR_CONTEXT` and let the
callee look it up in`o->output_indicators[]`...

I read all three patches and did not see a reason why we could not
simplify the code that way.

Other than that: great!

Thanks!

I considered it, but was put off by the (small) effort of yet another
diff refactoring.

I'll include it in a resend if a resend is needed, otherwise
I would suggest to make it a patch on top?

Thanks,
Stefan

Hi Stefan,

On Mon, 20 Aug 2018, Stefan Beller wrote:

On Mon, Aug 20, 2018 at 12:31 PM Johannes Schindelin
[off-list ref] wrote:

quoted

On Fri, 17 Aug 2018, Stefan Beller wrote:

quoted

This will prove useful in range-diff in a later patch as we will be able
to differentiate between adding a new file (that line is starting with

+++ and then the file name) and regular new lines.

It could also be useful for experimentation in new patch formats, i.e.
we could teach git to emit moved lines with lines other than +/-.

Thanks.

quoted

diff --git a/diff.c b/diff.c
index c5c7739ce34..03486c35b75 100644
--- a/diff.c
+++ b/diff.c

@@ -1281,7 +1281,9 @@ 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_sign, set, reset, ' ', line, len,

                                    ^
Here we already pass `o`... so...

quoted

+             emit_line_ws_markup(o, set_sign, set, reset,
+                                 o->output_indicators[OUTPUT_INDICATOR_CONTEXT],

                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
... here, we could simply pass `OUTPUT_INDICATOR_CONTEXT` and let the
callee look it up in`o->output_indicators[]`...

I read all three patches and did not see a reason why we could not
simplify the code that way.

Other than that: great!

Thanks!

I considered it, but was put off by the (small) effort of yet another
diff refactoring.

I'll include it in a resend if a resend is needed, otherwise
I would suggest to make it a patch on top?

Sounds good!
Dscho

Instead of passing the sign directly to emit_line_ws_markup, pass only the
index to lookup the sign in diff_options->output_indicators.

Signed-off-by: Stefan Beller <redacted>
---
 diff.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
 
So something like this on top of sb/range-diff-colors ?
If a resend is needed I'll squash this in (or carry it as a cleanup patch early
in the series), otherwise we could put this on top.

Thanks,
Stefan
 

diff --git a/diff.c b/diff.c
index 03486c35b75..17e33d506a1 100644
--- a/diff.c
+++ b/diff.c

@@ -1199,10 +1199,11 @@ static void dim_moved_lines(struct diff_options *o)
 static void emit_line_ws_markup(struct diff_options *o,
 				const char *set_sign, const char *set,
 				const char *reset,
-				char sign, const char *line, int len,
+				int sign_index, const char *line, int len,
 				unsigned ws_rule, int blank_at_eof)
 {
 	const char *ws = NULL;
+	int sign = o->output_indicators[sign_index];
 
 	if (o->ws_error_highlight & ws_rule) {
 		ws = diff_get_color_opt(o, DIFF_WHITESPACE);

@@ -1282,8 +1283,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 				set = diff_get_color_opt(o, DIFF_FILE_OLD);
 		}
 		emit_line_ws_markup(o, set_sign, set, reset,
-				    o->output_indicators[OUTPUT_INDICATOR_CONTEXT],
-				    line, len,
+				    OUTPUT_INDICATOR_CONTEXT, line, len,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:

@@ -1327,8 +1327,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			flags &= ~DIFF_SYMBOL_CONTENT_WS_MASK;
 		}
 		emit_line_ws_markup(o, set_sign, set, reset,
-				    o->output_indicators[OUTPUT_INDICATOR_NEW],
-				    line, len,
+				    OUTPUT_INDICATOR_NEW, line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
 		break;

@@ -1372,8 +1371,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 				set = diff_get_color_opt(o, DIFF_CONTEXT_DIM);
 		}
 		emit_line_ws_markup(o, set_sign, set, reset,
-				    o->output_indicators[OUTPUT_INDICATOR_OLD],
-				    line, len,
+				    OUTPUT_INDICATOR_OLD, line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
 		break;
 	case DIFF_SYMBOL_WORDS_PORCELAIN:

-- 
2.18.0.265.g16de1b435c9.dirty

Hi Stefan,

On Wed, 22 Aug 2018, Stefan Beller wrote:

Instead of passing the sign directly to emit_line_ws_markup, pass only the
index to lookup the sign in diff_options->output_indicators.

Signed-off-by: Stefan Beller <redacted>

Looks good to me!

---
 diff.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
 
So something like this on top of sb/range-diff-colors ?  If a resend is
needed I'll squash this in (or carry it as a cleanup patch early in the
series), otherwise we could put this on top.

I'd leave this as a separate commit.

Ciao,
Dscho