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

[Stefan Beller]

On Mon, Sep 12, 2016 at 5:25 PM, Junio C Hamano  wrote:
> Stefan Beller  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(, 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(, "--- %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-stefanbel...@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)

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

[Junio C Hamano]

Stefan Beller  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(, 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(, "--- %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

 * 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.

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

[Stefan Beller]

From: Stefan Beller 

In the coming patches we want to eliminate writes that do not go
though emit_line_* as we later want to hook into the emit_line_*
functions to add a buffered output. To make the conversion easy,
add a function that takes a format string.

Signed-off-by: Stefan Beller 
---
 diff.c | 24 +++-
 1 file changed, 19 insertions(+), 5 deletions(-)

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(, 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);
+
ecbdata->label_path[0] = ecbdata->label_path[1] = NULL;
}
 
-- 
2.7.4