Re: [PATCH 7/8] diff.c: compute reverse locally in emit_line_0
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 13, 2018, 12:26 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1808131422580.71@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20180810223441.30428-8-sbeller@google.com>
Hi,
On Fri, 10 Aug 2018, Stefan Beller wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
Well, my rationale for having the explicit `reverse` parameter is: this code is complex enough, introducing some magic "this and that implies this" makes it much harder to understand.
So I am not at all sure that this is a good thing.
Show 19 quoted lines
> ---
> diff.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 01095a59d3d..e50cd312755 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -622,11 +622,12 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
> }
>
> static void emit_line_0(struct diff_options *o,
> - const char *set_sign, const char *set, unsigned reverse, const char *reset,
> + const char *set_sign, const char *set, const char *reset,
> int first, const char *line, int len)
> {
> int has_trailing_newline, has_trailing_carriage_return;
> int nofirst;
> + int reverse = !!set && !!set_sign;In contrast to, say, Javascript which has this nice feature that you can write `set || set_sign` to mean `set ? set : set_sign`, I am fairly certain that `set && set_sign` already evaluates to `0` or `1`. No need for all those exclamation marks!!!!
:-)
But as I said: I think it is a bit too magic for my liking to say "if the diff marker color is specified as well as the color for the rest of the line, then the diff marker will be reversed". That's just making the code hard to understand, i.e. less readable rather than more.
Ciao, Dscho
Show 45 quoted lines
> FILE *file = o->file;
>
> fputs(diff_line_prefix(o), file);
> @@ -671,7 +672,7 @@ static void emit_line_0(struct diff_options *o,
> static void emit_line(struct diff_options *o, const char *set, const char *reset,
> const char *line, int len)
> {
> - emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);
> + emit_line_0(o, set, NULL, reset, line[0], line+1, len-1);
> }
>
> enum diff_symbol {
> @@ -1203,15 +1204,15 @@ static void emit_line_ws_markup(struct diff_options *o,
> }
>
> if (!ws && !set_sign)
> - emit_line_0(o, set, NULL, 0, reset, sign, line, len);
> + emit_line_0(o, set, NULL, reset, sign, line, len);
> else if (!ws) {
> - emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line, len);
> + emit_line_0(o, set_sign, set, reset, sign, line, len);
> } else if (blank_at_eof)
> /* Blank line at EOF - paint '+' as well */
> - emit_line_0(o, ws, NULL, 0, reset, sign, line, len);
> + emit_line_0(o, ws, NULL, reset, sign, line, len);
> else {
> /* Emit just the prefix, then the rest. */
> - emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0);
> + emit_line_0(o, set_sign, set, reset, sign, "", 0);
> ws_check_emit(line, len, ws_rule,
> o->file, set, reset, ws);
> }
> @@ -1234,7 +1235,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
> context = diff_get_color_opt(o, DIFF_CONTEXT);
> reset = diff_get_color_opt(o, DIFF_RESET);
> putc('\n', o->file);
> - emit_line_0(o, context, NULL, 0, reset, '\\',
> + emit_line_0(o, context, NULL, reset, '\\',
> nneof, strlen(nneof));
> break;
> case DIFF_SYMBOL_SUBMODULE_HEADER:
> --
> 2.18.0.865.gffc8e1a3cd6-goog
>
>