Re: [PATCH 07/10] diff.c: convert fn_out_consume to use emit_line_*

[Stefan Beller <sbeller@google.com>]

On Mon, Sep 12, 2016 at 5:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <stefanbeller@gmail.com> writes:
>
>> diff --git a/diff.c b/diff.c
>> index 2aefd0f..7dcef73 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -493,6 +493,19 @@ static void emit_line(struct diff_options *o, const char *set, const char *reset
>>       emit_line_0(o, set, reset, line[0], line+1, len-1);
>>  }
>>
>> +static void emit_line_fmt(struct diff_options *o,
>> +                       const char *set, const char *reset,
>> +                       const char *fmt, ...)
>> +{
>> +     struct strbuf sb = STRBUF_INIT;
>> +     va_list ap;
>> +     va_start(ap, fmt);
>> +     strbuf_vaddf(&sb, fmt, ap);
>> +     va_end(ap);
>> +
>> +     emit_line(o, set, reset, sb.buf, sb.len);
>> +}
>> +
>>  static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char *line, int len)
>>  {
>>       if (!((ecbdata->ws_rule & WS_BLANK_AT_EOF) &&
>> @@ -1217,7 +1230,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
>>       const char *context = diff_get_color(ecbdata->color_diff, DIFF_CONTEXT);
>>       const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
>>       struct diff_options *o = ecbdata->opt;
>> -     const char *line_prefix = diff_line_prefix(o);
>>
>>       o->found_changes = 1;
>>
>> @@ -1233,10 +1245,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
>>               name_a_tab = strchr(ecbdata->label_path[0], ' ') ? "\t" : "";
>>               name_b_tab = strchr(ecbdata->label_path[1], ' ') ? "\t" : "";
>>
>> -             fprintf(o->file, "%s%s--- %s%s%s\n",
>> -                     line_prefix, meta, ecbdata->label_path[0], reset, name_a_tab);
>> -             fprintf(o->file, "%s%s+++ %s%s%s\n",
>> -                     line_prefix, meta, ecbdata->label_path[1], reset, name_b_tab);
>> +             emit_line_fmt(o, meta, reset, "--- %s%s\n",
>> +                           ecbdata->label_path[0], name_a_tab);
>> +
>> +             emit_line_fmt(o, meta, reset, "+++ %s%s\n",
>> +                           ecbdata->label_path[1], name_b_tab);
>
> Hmph, the original showed the following for the name-a line:
>
>         diff_line_prefix(o) META "--- " label_path RESET name_a_tab LF
>
> The updated one calls emit_line_fmt() with o, meta, reset, fmt and
> args, and then
>
>  * strbuf_vaddf(&buf, "--- %s%s\n", label_path, name_a_tab) creates
>    a string "--- " + label_path + LF
>
>  * emit_line() is called on the whole thing with META and RESET

Oh right, that is a real issue, i.e. name_b_tab is not colored. I'll
have to think about that.

>
>  * which is emit_line_0() that encloses the whole thing between META
>    and RESET but knows the trailing LF should come after RESET.
>
> So the coloring seems to be correct, but I am not sure where the
> line-prefix went.

emit_line_0 puts the line_prefix by default in front, i.e.
it emits

    line_prefix (SET) (first char) line (RESET) (CR/LF)

with things in parenthesis optional.

As said in 6/10 I think the emit_line_0 function is a great starting point
to build up a buffer when we do a two passes output. For that I want to channel
all output through emit_line_*.


---
I am done converting all diff output to emit_line for a rought first series,
and now all tests pass when asking for a buffered output.

So I started building the --color-moved on top of that, which seems
to work as well. The diff stat is  ~700 insertions and ~300 deletions
compared to 2.10, however we call out to the xdl stuff only once.

Comparing that to the original quick-hack-patch
https://public-inbox.org/git/20160906070151.15163-1-stefanbeller@gmail.com/
which has just 280 additions, 10 deletions;
we seem to need about ~350 refactor lines and
slightly slower emissions in the non-color-moved case
(all the calls to emit_line instead of directly using fprintf/fputs/fputc)