Hi Stefan,
On Fri, 10 Aug 2018, Stefan Beller wrote:
> All lines that use emit_line_0 multiple times per line, are combined
> into a single call to emit_line_0, making use of the 'set' argument.
>
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
> diff.c | 26 ++++++++++++--------------
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 5eea5edca50..01095a59d3d 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -629,10 +629,7 @@ static void emit_line_0(struct diff_options *o,
> int nofirst;
> FILE *file = o->file;
>
> - if (first)
> - fputs(diff_line_prefix(o), file);
> - else if (!len)
> - return;
> + fputs(diff_line_prefix(o), file);
How about separating this into its own "now that `first` is no longer
`NULL`, skip this" patch?
I found it a bit hard to review this diff, primarily because this hunk
would logically make more sense after the other hunks.
> if (len == 0) {
> has_trailing_newline = (first == '\n');
> @@ -652,13 +649,17 @@ static void emit_line_0(struct diff_options *o,
> if (len || !nofirst) {
> if (reverse && want_color(o->use_color))
> fputs(GIT_COLOR_REVERSE, file);
> - if (set_sign && set_sign[0])
> - fputs(set_sign, file);
> + if (set_sign || set)
> + fputs(set_sign ? set_sign : set, file);
Wait, what? Why is `set` all of a sudden also applying to `first`?
I would have expected `set_sign` to apply to `first`, and `set` to the
rest of the line.
> if (first && !nofirst)
> fputc(first, file);
> - if (set)
> - fputs(set, file);
> - fwrite(line, len, 1, file);
> + if (len) {
> + if (set_sign && set && set != set_sign)
> + fputs(reset, file);
> + if (set)
> + fputs(set, file);
That sounds as if `set == set_sign` would duplicate the `set`. How about
this instead?
if (set && set != set_sign) {
if (set_sign)
fputs(reset, file);
fputs(set, file);
}
The rest looks good to me.
Thank you,
Dscho
> + fwrite(line, len, 1, file);
> + }
> fputs(reset, file);
> }
> if (has_trailing_carriage_return)
> @@ -1204,16 +1205,13 @@ 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);
> else if (!ws) {
> - /* Emit just the prefix, then the rest. */
> - emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0);
> - emit_line_0(o, set, NULL, 0, reset, 0, line, len);
> + emit_line_0(o, set_sign, set, !!set_sign, 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);
> else {
> /* Emit just the prefix, then the rest. */
> - emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign, reset,
> - sign, "", 0);
> + emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0);
> ws_check_emit(line, len, ws_rule,
> o->file, set, reset, ws);
> }
> --
> 2.18.0.865.gffc8e1a3cd6-goog
>
>
