Re: [PATCH 1/3] diff.c: add --output-indicator-{new, old, context}

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH 1/3] diff.c: add --output-indicator-{new, old, context}
Date: Mon, 20 Aug 2018 21:31:43 +0200 (DST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1808202129420.73@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20180817204354.108625-2-sbeller@google.com>

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!

Thank you,
Dscho

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