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 <[email protected]> > Signed-off-by: Junio C Hamano <[email protected]> > --- > 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 > >
