threads / patch / 44055

patch, 10 partsAnother preparatory series for rename detection

Subject: [PATCH 00/10] Another preparatory series for rename detection

## tl;dr

21 messages between Sep 11, 2016 and Sep 16, 2016. Diffs are folded; open one to read it.

replies: 20people: 2as markdown or json

Stefan Beller· Sep 11, 2016, 05:42 UTC · lore
From: Stefan Beller <sbeller@google.com>
This applies on top of sb/diff-cleanup.

The long term strategy is to get the rename detection done, is to run any output through emit_line as there we can either write it to the output file or buffer it internally across file pairs and work on the buffered output.

As I got stuck with the word diff as well as the correct implementation of builtin_diff, I am sending this out as an intermediate state. As the diff code is central to Git, I'd rather send these cleanups early so I do not run into merge conflicts with other people down the road.

Thanks, Stefan

Stefan Beller (10):
  diff: move line ending check into emit_hunk_header
  diff: emit_{add, del, context}_line to increase {pre,post}image line
    count
  diff.c: drop tautologous condition in emit_line_0
  diff.c: rename diff_flush_patch to diff_flush_patch_filepair
  diff.c: reintroduce diff_flush_patch for all files
  diff.c: emit_line_0 can handle no color
  diff.c: convert fn_out_consume to use emit_line_*
  diff.c: convert emit_rewrite_diff to use emit_line_*
  diff.c: convert emit_rewrite_lines to use emit_line_0
  submodule.c: convert show_submodule_summary to use emit_line_fmt
 diff.c      | 114 ++++++++++++++++++++++++++++++++++++------------------------
 diff.h      |   3 ++
 submodule.c |  42 +++++++++-------------
 submodule.h |   3 +-
 4 files changed, 89 insertions(+), 73 deletions(-)
-- 
2.7.4
Stefan Beller· Sep 11, 2016, 05:42 UTC · re: Stefan Beller · lore

[PATCH 01/10] diff: move line ending check into emit_hunk_header

From: Stefan Beller <sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to diff.c +3 −2
diff --git a/diff.c b/diff.c
index cc8e812..aa50b2d 100644
--- a/diff.c
+++ b/diff.c
@@ -610,6 +610,9 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
 	}
 
 	strbuf_add(&msgbuf, line + len, org_len - len);
+	if (line[org_len - 1] != '\n')
+		strbuf_addch(&msgbuf, '\n');
+
 	emit_line(ecbdata->opt, "", "", msgbuf.buf, msgbuf.len);
 	strbuf_release(&msgbuf);
 }
@@ -1247,8 +1250,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		len = sane_truncate_line(ecbdata, line, len);
 		find_lno(line, ecbdata);
 		emit_hunk_header(ecbdata, line, len);
-		if (line[len-1] != '\n')
-			putc('\n', o->file);
 		return;
 	}
 

-- 
2.7.4
Junio C Hamano· Sep 12, 2016, 23:35 UTC · re: Stefan Beller · lore

Re: [PATCH 01/10] diff: move line ending check into emit_hunk_header

Stefan Beller <stefanbeller@gmail.com> writes:
> From: Stefan Beller <sbeller@google.com>
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
The reason being...?

"Doing this would not change any behaviour and would not break anything" may be true, but that is not a reason to do a change.

Hopefully it will become clear why this is needed once we look at a later patch in the series.

Show 25 quoted lines
>  diff.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index cc8e812..aa50b2d 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -610,6 +610,9 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
>  	}
>  
>  	strbuf_add(&msgbuf, line + len, org_len - len);
> +	if (line[org_len - 1] != '\n')
> +		strbuf_addch(&msgbuf, '\n');
> +
>  	emit_line(ecbdata->opt, "", "", msgbuf.buf, msgbuf.len);
>  	strbuf_release(&msgbuf);
>  }
> @@ -1247,8 +1250,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
>  		len = sane_truncate_line(ecbdata, line, len);
>  		find_lno(line, ecbdata);
>  		emit_hunk_header(ecbdata, line, len);
> -		if (line[len-1] != '\n')
> -			putc('\n', o->file);
>  		return;
>  	}
Stefan Beller· Sep 11, 2016, 05:42 UTC · re: Stefan Beller · lore

[PATCH 02/10] diff: emit_{add, del, context}_line to increase {pre,post}image line count

From: Stefan Beller <sbeller@google.com>

At all call sites of emit_{add, del, context}_line we increment the line count, so move it into the respective functions to make the code at the calling site a bit clearer.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
Show changes to diff.c +6 −9
diff --git a/diff.c b/diff.c
index aa50b2d..156c2aa 100644
--- a/diff.c
+++ b/diff.c
@@ -536,6 +536,7 @@ static void emit_add_line(const char *reset,
 			  struct emit_callback *ecbdata,
 			  const char *line, int len)
 {
+	ecbdata->lno_in_postimage++;
 	emit_line_checked(reset, ecbdata, line, len,
 			  DIFF_FILE_NEW, WSEH_NEW, '+');
 }
@@ -544,6 +545,7 @@ static void emit_del_line(const char *reset,
 			  struct emit_callback *ecbdata,
 			  const char *line, int len)
 {
+	ecbdata->lno_in_preimage++;
 	emit_line_checked(reset, ecbdata, line, len,
 			  DIFF_FILE_OLD, WSEH_OLD, '-');
 }
@@ -552,6 +554,8 @@ static void emit_context_line(const char *reset,
 			      struct emit_callback *ecbdata,
 			      const char *line, int len)
 {
+	ecbdata->lno_in_postimage++;
+	ecbdata->lno_in_preimage++;
 	emit_line_checked(reset, ecbdata, line, len,
 			  DIFF_CONTEXT, WSEH_CONTEXT, ' ');
 }
@@ -662,13 +666,10 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 
 		endp = memchr(data, '\n', size);
 		len = endp ? (endp - data + 1) : size;
-		if (prefix != '+') {
-			ecb->lno_in_preimage++;
+		if (prefix != '+')
 			emit_del_line(reset, ecb, data, len);
-		} else {
-			ecb->lno_in_postimage++;
+		else
 			emit_add_line(reset, ecb, data, len);
-		}
 		size -= len;
 		data += len;
 	}
@@ -1293,16 +1294,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 
 	switch (line[0]) {
 	case '+':
-		ecbdata->lno_in_postimage++;
 		emit_add_line(reset, ecbdata, line + 1, len - 1);
 		break;
 	case '-':
-		ecbdata->lno_in_preimage++;
 		emit_del_line(reset, ecbdata, line + 1, len - 1);
 		break;
 	case ' ':
-		ecbdata->lno_in_postimage++;
-		ecbdata->lno_in_preimage++;
 		emit_context_line(reset, ecbdata, line + 1, len - 1);
 		break;
 	default:

-- 
2.7.4
Junio C Hamano· Sep 12, 2016, 23:47 UTC · re: Stefan Beller · lore

Re: [PATCH 02/10] diff: emit_{add, del, context}_line to increase {pre,post}image line count

Stefan Beller <stefanbeller@gmail.com> writes:
Show 10 quoted lines
> From: Stefan Beller <sbeller@google.com>
>
> At all call sites of emit_{add, del, context}_line we increment the line
> count, so move it into the respective functions to make the code at the
> calling site a bit clearer.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  diff.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)

I am mostly in favor of this change, as the calls to these three functions are always preceded by increment of these fields, but it is "mostly" exactly because the reverse is not true. Namely ...

Show 17 quoted lines
> @@ -1293,16 +1294,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
>  
>  	switch (line[0]) {
>  	case '+':
> -		ecbdata->lno_in_postimage++;
>  		emit_add_line(reset, ecbdata, line + 1, len - 1);
>  		break;
>  	case '-':
> -		ecbdata->lno_in_preimage++;
>  		emit_del_line(reset, ecbdata, line + 1, len - 1);
>  		break;
>  	case ' ':
> -		ecbdata->lno_in_postimage++;
> -		ecbdata->lno_in_preimage++;
>  		emit_context_line(reset, ecbdata, line + 1, len - 1);
>  		break;
>  	default:

... there still needs an increment in the context lines, not shown in the patch, just after this "default:". I think the patch is OK as the comment after this "default:" (also not shown in the patch) makes it clear what is going on.

Thanks.
Stefan Beller· Sep 11, 2016, 05:42 UTC · re: Stefan Beller · lore

[PATCH 03/10] diff.c: drop tautologous condition in emit_line_0

From: Stefan Beller <sbeller@google.com>

When `first` equals '\r', then it cannot equal '\n', which makes `!has_trailing_newline` always true if `first` is '\r'. Drop that check.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Show changes to diff.c +1 −2
diff --git a/diff.c b/diff.c
index 156c2aa..9d2e704 100644
--- a/diff.c
+++ b/diff.c
@@ -460,8 +460,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
 
 	if (len == 0) {
 		has_trailing_newline = (first == '\n');
-		has_trailing_carriage_return = (!has_trailing_newline &&
-						(first == '\r'));
+		has_trailing_carriage_return = (first == '\r');
 		nofirst = has_trailing_newline || has_trailing_carriage_return;
 	} else {
 		has_trailing_newline = (len > 0 && line[len-1] == '\n');

-- 
2.7.4
Junio C Hamano· Sep 12, 2016, 23:53 UTC · re: Stefan Beller · lore

Re: [PATCH 03/10] diff.c: drop tautologous condition in emit_line_0

Stefan Beller <stefanbeller@gmail.com> writes:
Show 14 quoted lines
> diff --git a/diff.c b/diff.c
> index 156c2aa..9d2e704 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -460,8 +460,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
>  
>  	if (len == 0) {
>  		has_trailing_newline = (first == '\n');
> -		has_trailing_carriage_return = (!has_trailing_newline &&
> -						(first == '\r'));
> +		has_trailing_carriage_return = (first == '\r');
>  		nofirst = has_trailing_newline || has_trailing_carriage_return;
>  	} else {
>  		has_trailing_newline = (len > 0 && line[len-1] == '\n');
Interesting.

This may be a mis-conversion at 250f7993 ("diff.c: split emit_line() from the first char and the rest of the line", 2009-09-14), I suspect. The original took line[] with length and peeked for '\n', and when it saw one, it decremented length before checking line[len-1] for '\r'.

But of course if there is only one byte on the line (i.e. len == 0 after first is stripped off), it cannot be both '\n' or '\r' at the same time.

Thanks for spotting.
Stefan Beller· Sep 16, 2016, 23:04 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/10] diff.c: drop tautologous condition in emit_line_0

On Mon, Sep 12, 2016 at 4:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 30 quoted lines
> Stefan Beller <stefanbeller@gmail.com> writes:
>
>> diff --git a/diff.c b/diff.c
>> index 156c2aa..9d2e704 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -460,8 +460,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
>>
>>       if (len == 0) {
>>               has_trailing_newline = (first == '\n');
>> -             has_trailing_carriage_return = (!has_trailing_newline &&
>> -                                             (first == '\r'));
>> +             has_trailing_carriage_return = (first == '\r');
>>               nofirst = has_trailing_newline || has_trailing_carriage_return;
>>       } else {
>>               has_trailing_newline = (len > 0 && line[len-1] == '\n');
>
> Interesting.
>
> This may be a mis-conversion at 250f7993 ("diff.c: split emit_line()
> from the first char and the rest of the line", 2009-09-14), I
> suspect.  The original took line[] with length and peeked for '\n',
> and when it saw one, it decremented length before checking
> line[len-1] for '\r'.
>
> But of course if there is only one byte on the line (i.e. len == 0
> after first is stripped off), it cannot be both '\n' or '\r' at the
> same time.
>
> Thanks for spotting.

Oh, right, it used to be possible to remove \r\n completely and that information was then kept as has_trailing_newline = has_trailing_carriage_return = 1; and the resulting line is kept completely without ending line.

After some thought I don't think I can use this mis-conversion to trigger a bug though, because the len=0 can only ever happen if first is '\n' alone essentially.

Another thing I noticed when playing around with diffs:
    $ printf "\r\n" >crlf
    $ git commit crlf -m "add file crlf, empty line"
    $ printf "non zero length\r\n" >crlf
    $ diff --git a/crlf b/crlf
    $ index d3f5a12..ece7140 100644
    --- a/crlf
    +++ b/crlf
    @@ -1 +1 @@
    -
    +non zero length^M
    $ # The - line is missing a ^M ?
Stefan Beller· Sep 11, 2016, 05:42 UTC · re: Stefan Beller · lore

[PATCH 05/10] diff.c: reintroduce diff_flush_patch for all files

From: Stefan Beller <sbeller@google.com>
---
 diff.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
Show changes to diff.c +12 −5
diff --git a/diff.c b/diff.c
index 85fb887..87b1bb2 100644
--- a/diff.c
+++ b/diff.c
@@ -4608,6 +4608,17 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
 		warning(rename_limit_advice, varname, needed);
 }
 
+static void diff_flush_patch(struct diff_options *o)
+{
+	int i;
+	struct diff_queue_struct *q = &diff_queued_diff;
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p = q->queue[i];
+		if (check_pair_status(p))
+			diff_flush_patch_filepair(p, o);
+	}
+}
+
 void diff_flush(struct diff_options *options)
 {
 	struct diff_queue_struct *q = &diff_queued_diff;
@@ -4702,11 +4713,7 @@ void diff_flush(struct diff_options *options)
 			}
 		}
 
-		for (i = 0; i < q->nr; i++) {
-			struct diff_filepair *p = q->queue[i];
-			if (check_pair_status(p))
-				diff_flush_patch_filepair(p, options);
-		}
+		diff_flush_patch(options);
 	}
 
 	if (output_format & DIFF_FORMAT_CALLBACK)

-- 
2.7.4
Junio C Hamano· Sep 13, 2016, 00:05 UTC · re: Stefan Beller · lore

Re: [PATCH 05/10] diff.c: reintroduce diff_flush_patch for all files

Stefan Beller <stefanbeller@gmail.com> writes:
> From: Stefan Beller <sbeller@google.com>
>
> ---

This shows why 04/10 should have had the overall plan for these two steps. We want a short-and-sweet name "diff-flush-patch" to mean "flush all the queued diff elements" so rename the singleton one from diff-flush-patch to diff-flush-patch-filepair to make room in 04/10 and then introduce the "diff-flush-patch-all" in 05/10.

I just said with the above explanation the changes in 04/10 and 05/10 become undertandable, which is a bit different from being justifiable. "diff_flush_raw()", "diff_flush_stat()", etc. are _all_ about a single filepair. I'd rather see diff_flush_patch() to be also about a single filepair.

It may be helpful to have a helper that calls diff_flush_patch() for all filepairs in the queue, but can't that function get a new name instead? By definition, it will be called much less often than a pair-wise one, so it can afford to have a longer name.

diff_flush_patch_queue() or something, perhaps? I am not sure if it should be diff_flush_queue_patch(), or even

    diff_flush_queue(struct diff_options *o, diff_flush_fn fn);
where diff_flush_fn is 
    typedef void (*diff_flush_fn)(struct diff_filepair *p,
    			struct diff_options *o, void *other_data)

that can be used to flush the queue by calling any of these filepair-wise flush functions like diff_flush_{raw,stat,checkdiff,patch}.

This last approach might be overkill, but if you want to try it, you'd need a preparatory step to give an unused "void *other" to diff_flush_{raw,patch,checkdiff} as diff_flush_stat() is an oddball that needs an extra "accumulator" pointer. I actually wonder if that "diffstat" pointer should become part of "struct diff_options", though. Anyway.

Show 38 quoted lines
>  diff.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 85fb887..87b1bb2 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4608,6 +4608,17 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
>  		warning(rename_limit_advice, varname, needed);
>  }
>  
> +static void diff_flush_patch(struct diff_options *o)
> +{
> +	int i;
> +	struct diff_queue_struct *q = &diff_queued_diff;
> +	for (i = 0; i < q->nr; i++) {
> +		struct diff_filepair *p = q->queue[i];
> +		if (check_pair_status(p))
> +			diff_flush_patch_filepair(p, o);
> +	}
> +}
> +
>  void diff_flush(struct diff_options *options)
>  {
>  	struct diff_queue_struct *q = &diff_queued_diff;
> @@ -4702,11 +4713,7 @@ void diff_flush(struct diff_options *options)
>  			}
>  		}
>  
> -		for (i = 0; i < q->nr; i++) {
> -			struct diff_filepair *p = q->queue[i];
> -			if (check_pair_status(p))
> -				diff_flush_patch_filepair(p, options);
> -		}
> +		diff_flush_patch(options);
>  	}
>  
>  	if (output_format & DIFF_FORMAT_CALLBACK)
Stefan Beller· Sep 11, 2016, 05:42 UTC · re: Stefan Beller · lore

[PATCH 04/10] diff.c: rename diff_flush_patch to diff_flush_patch_filepair

From: Stefan Beller <sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to diff.c +4 −4
diff --git a/diff.c b/diff.c
index 9d2e704..85fb887 100644
--- a/diff.c
+++ b/diff.c
@@ -4176,7 +4176,7 @@ int diff_unmodified_pair(struct diff_filepair *p)
 	return 0;
 }
 
-static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)
+static void diff_flush_patch_filepair(struct diff_filepair *p, struct diff_options *o)
 {
 	if (diff_unmodified_pair(p))
 		return;
@@ -4672,7 +4672,7 @@ void diff_flush(struct diff_options *options)
 	    DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&
 	    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
 		/*
-		 * run diff_flush_patch for the exit status. setting
+		 * run diff_flush_patch_filepair for the exit status. setting
 		 * options->file to /dev/null should be safe, because we
 		 * aren't supposed to produce any output anyway.
 		 */
@@ -4685,7 +4685,7 @@ void diff_flush(struct diff_options *options)
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))
-				diff_flush_patch(p, options);
+				diff_flush_patch_filepair(p, options);
 			if (options->found_changes)
 				break;
 		}
@@ -4705,7 +4705,7 @@ void diff_flush(struct diff_options *options)
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))
-				diff_flush_patch(p, options);
+				diff_flush_patch_filepair(p, options);
 		}
 	}
 

-- 
2.7.4
Junio C Hamano· Sep 12, 2016, 23:53 UTC · re: Stefan Beller · lore

Re: [PATCH 04/10] diff.c: rename diff_flush_patch to diff_flush_patch_filepair

Stefan Beller <stefanbeller@gmail.com> writes:
> From: Stefan Beller <sbeller@google.com>
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
The reason being...?
Stefan Beller· Sep 11, 2016, 05:42 UTC · re: Stefan Beller · lore

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

From: Stefan Beller <sbeller@google.com>

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 <stefanbeller@gmail.com>
---
 diff.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)
Show changes to diff.c +19 −5
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);
+
 		ecbdata->label_path[0] = ecbdata->label_path[1] = NULL;
 	}
 

-- 
2.7.4
Junio C Hamano· Sep 13, 2016, 00:25 UTC · re: Stefan Beller · lore

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

Stefan Beller <stefanbeller@gmail.com> writes:
Show 45 quoted lines
> 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
 * 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.

Stefan Beller· Sep 13, 2016, 00:41 UTC · re: Junio C Hamano · lore

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

On Mon, Sep 12, 2016 at 5:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 59 quoted lines
> 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.

Show 6 quoted lines
>
>  * 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)

Stefan Beller· Sep 11, 2016, 05:42 UTC · re: Stefan Beller · lore

[PATCH 10/10] submodule.c: convert show_submodule_summary to use emit_line_fmt

From: Stefan Beller <sbeller@google.com>
---
 diff.c      |  5 ++---
 diff.h      |  3 +++
 submodule.c | 42 +++++++++++++++++-------------------------
 submodule.h |  3 +--
 4 files changed, 23 insertions(+), 30 deletions(-)
Show changes to 4 files +23 −30

diff.c, diff.h, submodule.c, submodule.h

diff --git a/diff.c b/diff.c
index 255fbf3..c00306b 100644
--- a/diff.c
+++ b/diff.c
@@ -493,7 +493,7 @@ 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,
+void emit_line_fmt(struct diff_options *o,
 			  const char *set, const char *reset,
 			  const char *fmt, ...)
 {
@@ -2310,8 +2310,7 @@ static void builtin_diff(const char *name_a,
 			(!two->mode || S_ISGITLINK(two->mode))) {
 		const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
 		const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
-		show_submodule_summary(o->file, one->path ? one->path : two->path,
-				line_prefix,
+		show_submodule_summary(o, one->path ? one->path : two->path,
 				one->oid.hash, two->oid.hash,
 				two->dirty_submodule,
 				meta, del, add, reset);
diff --git a/diff.h b/diff.h
index 7883729..1699d9c 100644
--- a/diff.h
+++ b/diff.h
@@ -180,6 +180,9 @@ struct diff_options {
 	int diff_path_counter;
 };
 
+void emit_line_fmt(struct diff_options *o, const char *set, const char *reset,
+		   const char *fmt, ...);
+
 enum color_diff {
 	DIFF_RESET = 0,
 	DIFF_CONTEXT = 1,
diff --git a/submodule.c b/submodule.c
index 1b5cdfb..c817b46 100644
--- a/submodule.c
+++ b/submodule.c
@@ -304,8 +304,8 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,
 	return prepare_revision_walk(rev);
 }
 
-static void print_submodule_summary(struct rev_info *rev, FILE *f,
-		const char *line_prefix,
+static void print_submodule_summary(struct rev_info *rev,
+		struct diff_options *o,
 		const char *del, const char *add, const char *reset)
 {
 	static const char format[] = "  %m %s";
@@ -317,24 +317,16 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,
 		ctx.date_mode = rev->date_mode;
 		ctx.output_encoding = get_log_output_encoding();
 		strbuf_setlen(&sb, 0);
-		strbuf_addstr(&sb, line_prefix);
-		if (commit->object.flags & SYMMETRIC_LEFT) {
-			if (del)
-				strbuf_addstr(&sb, del);
-		}
-		else if (add)
-			strbuf_addstr(&sb, add);
 		format_commit_message(commit, format, &sb, &ctx);
-		if (reset)
-			strbuf_addstr(&sb, reset);
-		strbuf_addch(&sb, '\n');
-		fprintf(f, "%s", sb.buf);
+		if (commit->object.flags & SYMMETRIC_LEFT)
+			emit_line_fmt(o, del, reset, "%s\n", sb.buf);
+		else if (add)
+			emit_line_fmt(o, add, reset, "%s\n", sb.buf);
 	}
 	strbuf_release(&sb);
 }
 
-void show_submodule_summary(FILE *f, const char *path,
-		const char *line_prefix,
+void show_submodule_summary(struct diff_options *o, const char *path,
 		unsigned char one[20], unsigned char two[20],
 		unsigned dirty_submodule, const char *meta,
 		const char *del, const char *add, const char *reset)
@@ -359,30 +351,30 @@ void show_submodule_summary(FILE *f, const char *path,
 		message = "(revision walker failed)";
 
 	if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
-		fprintf(f, "%sSubmodule %s contains untracked content\n",
-			line_prefix, path);
+		emit_line_fmt(o, 0, 0,
+			      "Submodule %s contains untracked content\n", path);
 	if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-		fprintf(f, "%sSubmodule %s contains modified content\n",
-			line_prefix, path);
+		emit_line_fmt(o, 0, 0,
+			      "Submodule %s contains modified content\n", path);
 
 	if (!hashcmp(one, two)) {
 		strbuf_release(&sb);
 		return;
 	}
 
-	strbuf_addf(&sb, "%s%sSubmodule %s %s..", line_prefix, meta, path,
-			find_unique_abbrev(one, DEFAULT_ABBREV));
+	strbuf_addf(&sb, "Submodule %s %s..", path,
+		    find_unique_abbrev(one, DEFAULT_ABBREV));
 	if (!fast_backward && !fast_forward)
 		strbuf_addch(&sb, '.');
 	strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
 	if (message)
-		strbuf_addf(&sb, " %s%s\n", message, reset);
+		strbuf_addf(&sb, " %s\n", message);
 	else
-		strbuf_addf(&sb, "%s:%s\n", fast_backward ? " (rewind)" : "", reset);
-	fwrite(sb.buf, sb.len, 1, f);
+		strbuf_addf(&sb, "%s:\n", fast_backward ? " (rewind)" : "");
+	emit_line_fmt(o, meta, reset, "%s", sb.buf);
 
 	if (!message) /* only NULL if we succeeded in setting up the walk */
-		print_submodule_summary(&rev, f, line_prefix, del, add, reset);
+		print_submodule_summary(&rev, o, del, add, reset);
 	if (left)
 		clear_commit_marks(left, ~0);
 	if (right)
diff --git a/submodule.h b/submodule.h
index 2af9390..fefc0ab 100644
--- a/submodule.h
+++ b/submodule.h
@@ -41,8 +41,7 @@ int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst);
 const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
 void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
-void show_submodule_summary(FILE *f, const char *path,
-		const char *line_prefix,
+void show_submodule_summary(struct diff_options *o, const char *path,
 		unsigned char one[20], unsigned char two[20],
 		unsigned dirty_submodule, const char *meta,
 		const char *del, const char *add, const char *reset);

-- 
2.7.4
Stefan Beller· Sep 11, 2016, 05:42 UTC · re: Stefan Beller · lore

[PATCH 09/10] diff.c: convert emit_rewrite_lines to use emit_line_0

From: Stefan Beller <sbeller@google.com>
---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to diff.c +1 −1
diff --git a/diff.c b/diff.c
index 98d6655..255fbf3 100644
--- a/diff.c
+++ b/diff.c
@@ -690,7 +690,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 	if (!endp) {
 		const char *context = diff_get_color(ecb->color_diff,
 						     DIFF_CONTEXT);
-		putc('\n', ecb->opt->file);
+		emit_line_0(ecb->opt, 0, 0, 0, "\n", 1);
 		emit_line_0(ecb->opt, context, reset, '\\',
 			    nneof, strlen(nneof));
 	}

-- 
2.7.4
Stefan Beller· Sep 11, 2016, 05:42 UTC · re: Stefan Beller · lore

[PATCH 08/10] diff.c: convert emit_rewrite_diff to use emit_line_*

From: Stefan Beller <sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)
Show changes to diff.c +18 −15
diff --git a/diff.c b/diff.c
index 7dcef73..98d6655 100644
--- a/diff.c
+++ b/diff.c
@@ -653,17 +653,17 @@ static void remove_tempfile(void)
 	}
 }
 
-static void print_line_count(FILE *file, int count)
+static void add_line_count(struct strbuf *out, int count)
 {
 	switch (count) {
 	case 0:
-		fprintf(file, "0,0");
+		strbuf_addstr(out, "0,0");
 		break;
 	case 1:
-		fprintf(file, "1");
+		strbuf_addstr(out, "1");
 		break;
 	default:
-		fprintf(file, "1,%d", count);
+		strbuf_addf(out, "1,%d", count);
 		break;
 	}
 }
@@ -714,7 +714,7 @@ static void emit_rewrite_diff(const char *name_a,
 	char *data_one, *data_two;
 	size_t size_one, size_two;
 	struct emit_callback ecbdata;
-	const char *line_prefix = diff_line_prefix(o);
+	struct strbuf out = STRBUF_INIT;
 
 	if (diff_mnemonic_prefix && DIFF_OPT_TST(o, REVERSE_DIFF)) {
 		a_prefix = o->b_prefix;
@@ -752,20 +752,23 @@ static void emit_rewrite_diff(const char *name_a,
 	ecbdata.lno_in_preimage = 1;
 	ecbdata.lno_in_postimage = 1;
 
+	emit_line_fmt(o, metainfo, reset, "--- %s%s\n", a_name.buf, name_a_tab);
+	emit_line_fmt(o, metainfo, reset, "+++ %s%s\n", b_name.buf, name_b_tab);
+
 	lc_a = count_lines(data_one, size_one);
 	lc_b = count_lines(data_two, size_two);
-	fprintf(o->file,
-		"%s%s--- %s%s%s\n%s%s+++ %s%s%s\n%s%s@@ -",
-		line_prefix, metainfo, a_name.buf, name_a_tab, reset,
-		line_prefix, metainfo, b_name.buf, name_b_tab, reset,
-		line_prefix, fraginfo);
+
+	strbuf_addstr(&out, "@@ -");
 	if (!o->irreversible_delete)
-		print_line_count(o->file, lc_a);
+		add_line_count(&out, lc_a);
 	else
-		fprintf(o->file, "?,?");
-	fprintf(o->file, " +");
-	print_line_count(o->file, lc_b);
-	fprintf(o->file, " @@%s\n", reset);
+		strbuf_addstr(&out, "?,?");
+	strbuf_addstr(&out, " +");
+	add_line_count(&out, lc_b);
+	strbuf_addstr(&out, " @@\n");
+	emit_line(o, fraginfo, reset, out.buf, out.len);
+	strbuf_release(&out);
+
 	if (lc_a && !o->irreversible_delete)
 		emit_rewrite_lines(&ecbdata, '-', data_one, size_one);
 	if (lc_b)

-- 
2.7.4
Stefan Beller· Sep 11, 2016, 05:42 UTC · re: Stefan Beller · lore

[PATCH 06/10] diff.c: emit_line_0 can handle no color

From: Stefan Beller <sbeller@google.com>
---
 diff.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Show changes to diff.c +4 −2
diff --git a/diff.c b/diff.c
index 87b1bb2..2aefd0f 100644
--- a/diff.c
+++ b/diff.c
@@ -473,11 +473,13 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
 	}
 
 	if (len || !nofirst) {
-		fputs(set, file);
+		if (set)
+			fputs(set, file);
 		if (!nofirst)
 			fputc(first, file);
 		fwrite(line, len, 1, file);
-		fputs(reset, file);
+		if (reset)
+			fputs(reset, file);
 	}
 	if (has_trailing_carriage_return)
 		fputc('\r', file);

-- 
2.7.4
Junio C Hamano· Sep 13, 2016, 00:11 UTC · re: Stefan Beller · lore

Re: [PATCH 06/10] diff.c: emit_line_0 can handle no color

Stefan Beller <stefanbeller@gmail.com> writes:
> From: Stefan Beller <sbeller@google.com>
>
> ---

"X can do Y" can be taken as a statement of fact (to which "so what?" is an appropriate response), a desire (to which "then please say 'make X do Y' instead" is an appropriate response), or a report of a bug (to which "please explain why X should be forbidden from doing Y" is an appropriate response).

This is way under-explained. I think this is "make X do Y" kind, and if so, please say so and possibly why it is a good idea to teach X how to do Y.

Thanks.
Show 23 quoted lines
>  diff.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 87b1bb2..2aefd0f 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -473,11 +473,13 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
>  	}
>  
>  	if (len || !nofirst) {
> -		fputs(set, file);
> +		if (set)
> +			fputs(set, file);
>  		if (!nofirst)
>  			fputc(first, file);
>  		fwrite(line, len, 1, file);
> -		fputs(reset, file);
> +		if (reset)
> +			fputs(reset, file);
>  	}
>  	if (has_trailing_carriage_return)
>  		fputc('\r', file);
Stefan Beller· Sep 13, 2016, 00:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 06/10] diff.c: emit_line_0 can handle no color

On Mon, Sep 12, 2016 at 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> Stefan Beller <stefanbeller@gmail.com> writes:
>
>> From: Stefan Beller <sbeller@google.com>
>>
>> ---
>
> "X can do Y" can be taken as a statement of fact (to which "so
> what?"  is an appropriate response), a desire (to which "then please
> say 'make X do Y' instead" is an appropriate response), or a report
> of a bug (to which "please explain why X should be forbidden from
> doing Y" is an appropriate response).
>
> This is way under-explained.  I think this is "make X do Y" kind,
> and if so, please say so and possibly why it is a good idea to teach
> X how to do Y.
>
> Thanks.
Ok, I see the general pattern of your answers: Add more explanations.

Answering for patch 01/10 as well as this one here:

    I want to propose an option to detect&color moved lines in a patch,
    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 a
    line pair that got moved in the first pass. So I aim for
    * collecting all output into a buffer as a first pass,
    * as the second pass output the buffer.

So in a later patch I will split up the emit_line_* machinery to either emitting to options->file or buffering if we do the 2 pass thing.

To make sure the 2 passes work correctly, we need to make sure all output is routed through the emit_line functions, and there will be no direct writes.

Now that we will be using the emit_lines functions for non colored output as well, we want to pass in "no color" which I think is best done via NULL and then not calling the output of the color writes.

← back to recent threads