From: Jonathan Tan Date: Mon, 15 May 2017 18:42:10 GMT Subject: Re: [PATCH 08/19] diff.c: convert builtin_diff to use emit_line_* Message-ID: In-Reply-To: <20170514040117.25865-9-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 builtin_diff. > > Signed-off-by: Stefan Beller > --- > diff.c | 43 ++++++++++++++++++++++++++----------------- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/diff.c b/diff.c > index 93343a9ccc..8e00010bf4 100644 > --- a/diff.c > +++ b/diff.c > @@ -1293,8 +1293,9 @@ static void fn_out_consume(void *priv, char *line, unsigned long len) > o->found_changes = 1; > > if (ecbdata->header) { > - fprintf(o->file, "%s", ecbdata->header->buf); > - strbuf_reset(ecbdata->header); > + emit_line(o, NULL, NULL, > + ecbdata->header->buf, ecbdata->header->len); > + strbuf_release(ecbdata->header); > ecbdata->header = NULL; > } > > @@ -2407,7 +2408,7 @@ static void builtin_diff(const char *name_a, > b_two = quote_two(b_prefix, name_b + (*name_b == '/')); > lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null"; > lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null"; > - strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, meta, a_one, b_two, reset); > + strbuf_addf(&header, "%sdiff --git %s %s%s\n", meta, a_one, b_two, reset); > if (lbl[0][0] == '/') { > /* /dev/null */ > strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, meta, two->mode, reset); > @@ -2439,7 +2440,7 @@ static void builtin_diff(const char *name_a, > if (complete_rewrite && > (textconv_one || !diff_filespec_is_binary(one)) && > (textconv_two || !diff_filespec_is_binary(two))) { > - fprintf(o->file, "%s", header.buf); > + emit_line(o, NULL, NULL, header.buf, header.len); > strbuf_reset(&header); > emit_rewrite_diff(name_a, name_b, one, two, > textconv_one, textconv_two, o); > @@ -2449,7 +2450,8 @@ static void builtin_diff(const char *name_a, > } > > if (o->irreversible_delete && lbl[1][0] == '/') { > - fprintf(o->file, "%s", header.buf); > + if (header.len) > + emit_line(o, NULL, NULL, header.buf, header.len); This used to unconditionally output diff_line_prefix(0), but now outputs it only if the non-prefix part is blank. Is that expected? (Same comments below.) > strbuf_reset(&header); > goto free_ab_and_return; > } else if (!DIFF_OPT_TST(o, TEXT) && > @@ -2459,13 +2461,16 @@ static void builtin_diff(const char *name_a, > S_ISREG(one->mode) && S_ISREG(two->mode) && > !DIFF_OPT_TST(o, BINARY)) { > if (!oidcmp(&one->oid, &two->oid)) { > - if (must_show_header) > - fprintf(o->file, "%s", header.buf); > + if (must_show_header && header.len) > + emit_line(o, NULL, NULL, > + header.buf, header.len); > goto free_ab_and_return; > } > - fprintf(o->file, "%s", header.buf); > - fprintf(o->file, "%sBinary files %s and %s differ\n", > - line_prefix, lbl[0], lbl[1]); > + if (header.len) > + emit_line(o, NULL, NULL, > + header.buf, header.len); > + emit_line_fmt(o, 0, 0, "Binary files %s and %s differ\n", > + lbl[0], lbl[1]); > goto free_ab_and_return; > } > if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0) > @@ -2473,17 +2478,21 @@ static void builtin_diff(const char *name_a, > /* Quite common confusing case */ > if (mf1.size == mf2.size && > !memcmp(mf1.ptr, mf2.ptr, mf1.size)) { > - if (must_show_header) > - fprintf(o->file, "%s", header.buf); > + if (must_show_header && header.len) > + emit_line(o, NULL, NULL, > + header.buf, header.len); > goto free_ab_and_return; > } > - fprintf(o->file, "%s", header.buf); > + if (header.len) > + emit_line(o, NULL, NULL, > + header.buf, header.len); > strbuf_reset(&header); > if (DIFF_OPT_TST(o, BINARY)) > emit_binary_diff(o->file, &mf1, &mf2, line_prefix); > else > - fprintf(o->file, "%sBinary files %s and %s differ\n", > - line_prefix, lbl[0], lbl[1]); > + emit_line_fmt(o, NULL, NULL, > + "Binary files %s and %s differ\n", > + lbl[0], lbl[1]); > o->found_changes = 1; > } else { > /* Crazy xdl interfaces.. */ > @@ -2494,8 +2503,8 @@ static void builtin_diff(const char *name_a, > struct emit_callback ecbdata; > const struct userdiff_funcname *pe; > > - if (must_show_header) { > - fprintf(o->file, "%s", header.buf); > + if (must_show_header && header.len) { > + emit_line(o, NULL, NULL, header.buf, header.len); > strbuf_reset(&header); > } > >