From: Jonathan Tan Date: Mon, 15 May 2017 22:40:56 GMT Subject: Re: [PATCH 14/19] diff.c: convert word diffing to use emit_line_* Message-ID: In-Reply-To: <20170514040117.25865-15-sbeller@google.com> On 05/13/2017 09:01 PM, Stefan Beller wrote: > In a later patch, I want to propose an option to detect&color > moved lines in a diff, which cannot be done in a one-pass over > the diff. Instead we need to go over the whole diff twice, > because we cannot detect the first line of the two corresponding > lines (+ and -) that got moved. > > So to prepare the diff machinery for two pass algorithms > (i.e. buffer it all up and then operate on the result), > move all emissions to places, such that the only emitting > function is emit_line_0. > > This covers all code related to diffing words. > > Signed-off-by: Stefan Beller > --- > diff.c | 56 ++++++++++++++++++++++++++++---------------------------- > 1 file changed, 28 insertions(+), 28 deletions(-) > > diff --git a/diff.c b/diff.c > index 91dc045a45..07041a35ad 100644 > --- a/diff.c > +++ b/diff.c > @@ -886,37 +886,38 @@ struct diff_words_data { > struct diff_words_style *style; > }; > > -static int fn_out_diff_words_write_helper(FILE *fp, > +static int fn_out_diff_words_write_helper(struct diff_options *o, > struct diff_words_style_elem *st_el, > const char *newline, > size_t count, const char *buf, > const char *line_prefix) > { > - int print = 0; > + struct strbuf sb = STRBUF_INIT; > > while (count) { > char *p = memchr(buf, '\n', count); > - if (print) > - fputs(line_prefix, fp); > + > if (p != buf) { > - if (st_el->color && fputs(st_el->color, fp) < 0) > - return -1; > - if (fputs(st_el->prefix, fp) < 0 || > - fwrite(buf, p ? p - buf : count, 1, fp) != 1 || > - fputs(st_el->suffix, fp) < 0) > - return -1; > - if (st_el->color && *st_el->color > - && fputs(GIT_COLOR_RESET, fp) < 0) > - return -1; > + if (st_el->color) > + strbuf_addstr(&sb, st_el->color); > + strbuf_addstr(&sb, st_el->prefix); > + strbuf_add(&sb, buf, p ? p - buf : count); > + strbuf_addstr(&sb, st_el->suffix); > + if (st_el->color && *st_el->color) > + strbuf_addstr(&sb, GIT_COLOR_RESET); > } > if (!p) > - return 0; > - if (fputs(newline, fp) < 0) > - return -1; > + goto out; > + strbuf_addstr(&sb, newline); > + emit_line(o, NULL, NULL, sb.buf, sb.len); I suspect that this will need to be refactored more thoroughly. Here, for example, emit_line (which prints the prefix) is printed nearly unconditionally, whereas in the original version, "fputs(line_prefix, fp)" is only printed when "print" is true. > + strbuf_reset(&sb); > count -= p + 1 - buf; > buf = p + 1; > - print = 1; > } > +out: > + if (sb.len) > + emit_line(o, NULL, NULL, sb.buf, sb.len); > + strbuf_release(&sb); > return 0; > } > > @@ -994,25 +995,25 @@ static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len) > } else > plus_begin = plus_end = diff_words->plus.orig[plus_first].end; > > - if (color_words_output_graph_prefix(diff_words)) { > - fputs(line_prefix, diff_words->opt->file); > - } > + if (color_words_output_graph_prefix(diff_words)) > + emit_line(diff_words->opt, NULL, NULL, "", 0); > + > if (diff_words->current_plus != plus_begin) { > - fn_out_diff_words_write_helper(diff_words->opt->file, > + fn_out_diff_words_write_helper(diff_words->opt, > &style->ctx, style->newline, > plus_begin - diff_words->current_plus, > diff_words->current_plus, line_prefix); > if (*(plus_begin - 1) == '\n') > - fputs(line_prefix, diff_words->opt->file); > + emit_line(diff_words->opt, NULL, NULL, "", 0); > } > if (minus_begin != minus_end) { > - fn_out_diff_words_write_helper(diff_words->opt->file, > + fn_out_diff_words_write_helper(diff_words->opt, > &style->old, style->newline, > minus_end - minus_begin, minus_begin, > line_prefix); > } > if (plus_begin != plus_end) { > - fn_out_diff_words_write_helper(diff_words->opt->file, > + fn_out_diff_words_write_helper(diff_words->opt, > &style->new, style->newline, > plus_end - plus_begin, plus_begin, > line_prefix); > @@ -1109,8 +1110,7 @@ static void diff_words_show(struct diff_words_data *diff_words) > > /* special case: only removal */ > if (!diff_words->plus.text.size) { > - fputs(line_prefix, diff_words->opt->file); > - fn_out_diff_words_write_helper(diff_words->opt->file, > + fn_out_diff_words_write_helper(diff_words->opt, > &style->old, style->newline, > diff_words->minus.text.size, > diff_words->minus.text.ptr, line_prefix); > @@ -1136,8 +1136,8 @@ static void diff_words_show(struct diff_words_data *diff_words) > if (diff_words->current_plus != diff_words->plus.text.ptr + > diff_words->plus.text.size) { > if (color_words_output_graph_prefix(diff_words)) > - fputs(line_prefix, diff_words->opt->file); > - fn_out_diff_words_write_helper(diff_words->opt->file, > + emit_line(diff_words->opt, NULL, NULL, "", 0); > + fn_out_diff_words_write_helper(diff_words->opt, > &style->ctx, style->newline, > diff_words->plus.text.ptr + diff_words->plus.text.size > - diff_words->current_plus, diff_words->current_plus, >