On Mon, Sep 12, 2016 at 5:25 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <stefanbeller@xxxxxxxxx> 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@xxxxxxxxx/
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)