patch, 4 partsBetter colors in range-diff!

> 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'.
> + */

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

> > +/*
> > + * 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?

> 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 <sbeller@google.com>
> ---
>  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
> 
> 

>
> 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!
>
> > 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 <sbeller@google.com>
> > ---
> >  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()?

> > +#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_*.

> 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 <sbeller@google.com>
> ---
>  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
> 
> 

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

> 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 <sbeller@google.com>
> ---
>  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
> 
> 

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

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

>
> Hi Stefan,
>
> On Mon, 13 Aug 2018, Stefan Beller wrote:
>
> > > > 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.
>

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

> On Tue, Aug 14, 2018 at 11:54 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Mon, 13 Aug 2018, Stefan Beller wrote:
> >
> > > > > 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.
> 
> 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.

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

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

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

>
> Hi Stefan,
>
> On Fri, 17 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.
> >
> > 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.
>
> > 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...
>
> > +             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!

> On Mon, Aug 20, 2018 at 12:31 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Fri, 17 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.
> > >
> > > 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.
> >
> > > 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...
> >
> > > +             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?

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

>
> Stefan Beller <sbeller@google.com> writes:
>
> > This improves colors of the range-diff, see last patch for details.
>
> How does this relate to your other "color with range-diff" topic
> that is still in flight?  This supersedes it?  Builds on it?
> Something else?