Hi,
On Fri, 10 Aug 2018, Stefan Beller wrote:
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
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.
> ---
> 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
> 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
>
>
