[RFC/PATCH 00/17]

[RFC/PATCH 00/17]

[Stefan Beller]

About a week ago, I started experimenting to add a new functionality to the 
diff machinery: We want to color moved lines differently! This will aid developers
to review patches such as the patch "Move libified code from builtin/apply.c to apply.{c,h}"
Or a smaller example:

    git show 11979b9
    

In the original patch series I just used the xdl diff machinery multiple times
to obtain the output for a multi pass algorithm I need for this coloring.
This was one of Junios main concerns (We have to rely on the xdl stuff to 
yield the same output in the two passes).

This brings in another approach:
 * run the xdl stuff only once
 * In case we do not ask for coloring moves, put it out immediately
   (small detour: we do not use fprintf/fputs directly but we channel
   all output through the emit_line_* interface)
 * When asking for coloring moves: buffer all its output for now.
   Feed the line by line into the move detection algorithm.
 * then output it all at once using the information from the move detection
   to recolor moves while outputting.
   
While viewing the output of 11979b9 again, I notice a small bug in the proposal,
but I'd rather be asking for review of the whole design approach.
   
Thanks,
Stefan

Stefan Beller (17):
  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: factor out diff_flush_patch_all_file_pairs
  diff.c: emit_line_0 can handle no color setting
  diff.c: convert fn_out_consume to use emit_line_*
  diff.c: convert builtin_diff to use emit_line_*
  diff.c: convert emit_rewrite_diff to use emit_line_*
  diff.c: convert emit_rewrite_lines to use emit_line_*
  submodule.c: convert show_submodule_summary to use emit_line_fmt
  diff.c: convert emit_binary_diff_body to use emit_line_*
  diff.c: convert show_stats to use emit_line_*
  diff.c: convert word diffing to use emit_line_*
  diff.c: convert diff_flush to use emit_line_*
  diff.c: convert diff_summary to use emit_line_*
  diff: buffer output in emit_line_0
  diff.c: color moved lines differently

 Documentation/config.txt               |  12 +-
 Documentation/diff-options.txt         |   7 +
 contrib/completion/git-completion.bash |   2 +
 diff.c                                 | 666 +++++++++++++++++++++++----------
 diff.h                                 |  35 +-
 submodule.c                            |  42 +--
 submodule.h                            |   3 +-
 t/t4015-diff-whitespace.sh             | 229 ++++++++++++
 8 files changed, 760 insertions(+), 236 deletions(-)

-- 
2.10.0.21.g1da280f.dirty

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

[Stefan Beller]

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 patch moves code that is conceptually part of
emit_hunk_header, but was using output in fn_out_consume,
back to emit_hunk_header.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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;
 	}
 
-- 
2.10.0.21.g1da280f.dirty

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

[Stefan Beller]

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(-)

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.10.0.21.g1da280f.dirty

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

[Stefan Beller]

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(-)

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.10.0.21.g1da280f.dirty

[RFC/PATCH 04/17] diff.c: factor out diff_flush_patch_all_file_pairs

[Stefan Beller]

In a later patch we want to do more things before and after all filepairs
are flushed. So factor flushing out all file pairs into its own function
that the new code can be plugged in easily.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/diff.c b/diff.c
index 9d2e704..b6a40ae 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_all_file_pairs(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(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(p, options);
-		}
+		diff_flush_patch_all_file_pairs(options);
 	}
 
 	if (output_format & DIFF_FORMAT_CALLBACK)
-- 
2.10.0.21.g1da280f.dirty

[RFC/PATCH 05/17] diff.c: emit_line_0 can handle no color setting

[Stefan Beller]

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.

In later patches we may pass lines that are not colored to
the central function emit_line_0, so we
need to emit the color only when it is non-NULL.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index b6a40ae..5d57130 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.10.0.21.g1da280f.dirty

[RFC/PATCH 06/17] diff.c: convert fn_out_consume to use emit_line_*

[Stefan Beller]

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 the remaining parts of fn_out_consume.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index 5d57130..b4f4b75 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;
 
@@ -1229,14 +1241,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 
 	if (ecbdata->label_path[0]) {
 		const char *name_a_tab, *name_b_tab;
-
 		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;
 	}
 
@@ -1277,7 +1287,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		diff_words_flush(ecbdata);
 		if (ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN) {
 			emit_line(o, context, reset, line, len);
-			fputs("~\n", o->file);
+			emit_line(o, 0, 0, "~\n", 2);
 		} else {
 			/*
 			 * Skip the prefix character, if any.  With
-- 
2.10.0.21.g1da280f.dirty

[RFC/PATCH 07/17] diff.c: convert builtin_diff to use emit_line_*

[Stefan Beller]

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 <sbeller@google.com>
---
 diff.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/diff.c b/diff.c
index b4f4b75..cbd4ba7 100644
--- a/diff.c
+++ b/diff.c
@@ -1234,8 +1234,8 @@ 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, 0, 0, ecbdata->header->buf, ecbdata->header->len);
+		strbuf_release(ecbdata->header);
 		ecbdata->header = NULL;
 	}
 
@@ -2333,7 +2333,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);
@@ -2365,7 +2365,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, 0, 0, header.buf, header.len);
 			strbuf_reset(&header);
 			emit_rewrite_diff(name_a, name_b, one, two,
 						textconv_one, textconv_two, o);
@@ -2375,7 +2375,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, 0, 0, header.buf, header.len);
 		strbuf_reset(&header);
 		goto free_ab_and_return;
 	} else if (!DIFF_OPT_TST(o, TEXT) &&
@@ -2385,13 +2386,14 @@ 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, 0, 0, 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, 0, 0, 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)
@@ -2399,17 +2401,18 @@ 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, 0, 0, header.buf, header.len);
 			goto free_ab_and_return;
 		}
-		fprintf(o->file, "%s", header.buf);
+		if (header.len)
+			emit_line(o, 0, 0, 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, 0, 0, "Binary files %s and %s differ\n",
+				      lbl[0], lbl[1]);
 		o->found_changes = 1;
 	} else {
 		/* Crazy xdl interfaces.. */
@@ -2420,8 +2423,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, 0, 0, header.buf, header.len);
 			strbuf_reset(&header);
 		}
 
-- 
2.10.0.21.g1da280f.dirty

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

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/diff.c b/diff.c
index cbd4ba7..ee4c9d4 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.10.0.21.g1da280f.dirty

[RFC/PATCH 09/17] diff.c: convert emit_rewrite_lines to use emit_line_*

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index ee4c9d4..3aa99d1 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.10.0.21.g1da280f.dirty

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

[Stefan Beller]

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 prepares the code for submodules to go through the
emit_line_0 function.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c      |  5 ++---
 diff.h      |  3 +++
 submodule.c | 42 +++++++++++++++++-------------------------
 submodule.h |  3 +--
 4 files changed, 23 insertions(+), 30 deletions(-)

diff --git a/diff.c b/diff.c
index 3aa99d1..1892c2b 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, ...)
 {
@@ -2306,8 +2306,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.10.0.21.g1da280f.dirty

[RFC/PATCH 11/17] diff.c: convert emit_binary_diff_body to use emit_line_*

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/diff.c b/diff.c
index 1892c2b..ff1e829 100644
--- a/diff.c
+++ b/diff.c
@@ -2169,8 +2169,8 @@ static unsigned char *deflate_it(char *data,
 	return deflated;
 }
 
-static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,
-				  const char *prefix)
+static void emit_binary_diff_body(struct diff_options *o,
+				  mmfile_t *one, mmfile_t *two)
 {
 	void *cp;
 	void *delta;
@@ -2199,13 +2199,12 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,
 	}
 
 	if (delta && delta_size < deflate_size) {
-		fprintf(file, "%sdelta %lu\n", prefix, orig_size);
+		emit_line_fmt(o, 0, 0, "delta %lu\n", orig_size);
 		free(deflated);
 		data = delta;
 		data_size = delta_size;
-	}
-	else {
-		fprintf(file, "%sliteral %lu\n", prefix, two->size);
+	} else {
+		emit_line_fmt(o, 0, 0, "literal %lu\n", two->size);
 		free(delta);
 		data = deflated;
 		data_size = deflate_size;
@@ -2214,8 +2213,9 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,
 	/* emit data encoded in base85 */
 	cp = data;
 	while (data_size) {
+		int len;
 		int bytes = (52 < data_size) ? 52 : data_size;
-		char line[70];
+		char line[71];
 		data_size -= bytes;
 		if (bytes <= 26)
 			line[0] = bytes + 'A' - 1;
@@ -2223,20 +2223,25 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,
 			line[0] = bytes - 26 + 'a' - 1;
 		encode_85(line + 1, cp, bytes);
 		cp = (char *) cp + bytes;
-		fprintf(file, "%s", prefix);
-		fputs(line, file);
-		fputc('\n', file);
+
+		len = strlen(line);
+		line[len++] = '\n';
+		line[len] = '\0';
+
+		emit_line(o, 0, 0, line, len);
 	}
-	fprintf(file, "%s\n", prefix);
+	emit_line(o, 0, 0, "\n", 1);
 	free(data);
 }
 
-static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two,
-			     const char *prefix)
+static void emit_binary_diff(struct diff_options *o,
+			     mmfile_t *one, mmfile_t *two)
 {
-	fprintf(file, "%sGIT binary patch\n", prefix);
-	emit_binary_diff_body(file, one, two, prefix);
-	emit_binary_diff_body(file, two, one, prefix);
+	const char *s = "GIT binary patch\n";
+	const int len = strlen(s);
+	emit_line(o, 0, 0, s, len);
+	emit_binary_diff_body(o, one, two);
+	emit_binary_diff_body(o, two, one);
 }
 
 int diff_filespec_is_binary(struct diff_filespec *one)
@@ -2411,7 +2416,7 @@ static void builtin_diff(const char *name_a,
 			emit_line(o, 0, 0, header.buf, header.len);
 		strbuf_reset(&header);
 		if (DIFF_OPT_TST(o, BINARY))
-			emit_binary_diff(o->file, &mf1, &mf2, line_prefix);
+			emit_binary_diff(o, &mf1, &mf2);
 		else
 			emit_line_fmt(o, 0, 0, "Binary files %s and %s differ\n",
 				      lbl[0], lbl[1]);
-- 
2.10.0.21.g1da280f.dirty

[RFC/PATCH 12/17] diff.c: convert show_stats to use emit_line_*

[Stefan Beller]

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.

We call print_stat_summary from builtin/apply, so we still
need the version with a file pointer, so introduce
print_stat_summary_0 that uses emit_line_* machinery and
keep print_stat_summary with the same arguments around.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 87 +++++++++++++++++++++++++++++++++++++-----------------------------
 diff.h |  4 +--
 2 files changed, 51 insertions(+), 40 deletions(-)

diff --git a/diff.c b/diff.c
index ff1e829..00a5a4e 100644
--- a/diff.c
+++ b/diff.c
@@ -1465,20 +1465,19 @@ static int scale_linear(int it, int width, int max_change)
 	return 1 + (it * (width - 1) / max_change);
 }
 
-static void show_name(FILE *file,
+static void show_name(struct strbuf *out,
 		      const char *prefix, const char *name, int len)
 {
-	fprintf(file, " %s%-*s |", prefix, len, name);
+	strbuf_addf(out, " %s%-*s |", prefix, len, name);
 }
 
-static void show_graph(FILE *file, char ch, int cnt, const char *set, const char *reset)
+static void show_graph(struct strbuf *out, char ch, int cnt, const char *set, const char *reset)
 {
 	if (cnt <= 0)
 		return;
-	fprintf(file, "%s", set);
-	while (cnt--)
-		putc(ch, file);
-	fprintf(file, "%s", reset);
+	strbuf_addstr(out, set);
+	strbuf_addchars(out, ch, cnt);
+	strbuf_addstr(out, reset);
 }
 
 static void fill_print_name(struct diffstat_file *file)
@@ -1502,14 +1501,16 @@ static void fill_print_name(struct diffstat_file *file)
 	file->print_name = pname;
 }
 
-int print_stat_summary(FILE *fp, int files, int insertions, int deletions)
+void print_stat_summary_0(struct diff_options *options, int files,
+			  int insertions, int deletions)
 {
 	struct strbuf sb = STRBUF_INIT;
-	int ret;
 
 	if (!files) {
 		assert(insertions == 0 && deletions == 0);
-		return fprintf(fp, "%s\n", " 0 files changed");
+		strbuf_addstr(&sb, " 0 files changed");
+		emit_line_0(options, 0, 0, 0, sb.buf, sb.len);
+		return;
 	}
 
 	strbuf_addf(&sb,
@@ -1536,9 +1537,17 @@ int print_stat_summary(FILE *fp, int files, int insertions, int deletions)
 			    deletions);
 	}
 	strbuf_addch(&sb, '\n');
-	ret = fputs(sb.buf, fp);
+	emit_line_0(options, 0, 0, 0, sb.buf, sb.len);
 	strbuf_release(&sb);
-	return ret;
+}
+
+void print_stat_summary(FILE *fp, int files,
+			int insertions, int deletions)
+{
+	struct diff_options o;
+	memset(&o, 0, sizeof(o));
+	o.file = fp;
+	print_stat_summary_0(&o, files, insertions, deletions);
 }
 
 static void show_stats(struct diffstat_t *data, struct diff_options *options)
@@ -1548,13 +1557,12 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	int total_files = data->nr, count;
 	int width, name_width, graph_width, number_width = 0, bin_width = 0;
 	const char *reset, *add_c, *del_c;
-	const char *line_prefix = "";
 	int extra_shown = 0;
+	struct strbuf out = STRBUF_INIT;
 
 	if (data->nr == 0)
 		return;
 
-	line_prefix = diff_line_prefix(options);
 	count = options->stat_count ? options->stat_count : data->nr;
 
 	reset = diff_get_color_opt(options, DIFF_RESET);
@@ -1708,26 +1716,29 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		}
 
 		if (file->is_binary) {
-			fprintf(options->file, "%s", line_prefix);
-			show_name(options->file, prefix, name, len);
-			fprintf(options->file, " %*s", number_width, "Bin");
+			show_name(&out, prefix, name, len);
+			strbuf_addf(&out, " %*s", number_width, "Bin");
 			if (!added && !deleted) {
-				putc('\n', options->file);
+				strbuf_addch(&out, '\n');
+				emit_line(options, 0, 0, out.buf, out.len);
+				strbuf_reset(&out);
 				continue;
 			}
-			fprintf(options->file, " %s%"PRIuMAX"%s",
+			strbuf_addf(&out, " %s%"PRIuMAX"%s",
 				del_c, deleted, reset);
-			fprintf(options->file, " -> ");
-			fprintf(options->file, "%s%"PRIuMAX"%s",
+			strbuf_addstr(&out, " -> ");
+			strbuf_addf(&out, "%s%"PRIuMAX"%s",
 				add_c, added, reset);
-			fprintf(options->file, " bytes");
-			fprintf(options->file, "\n");
+			strbuf_addstr(&out, " bytes\n");
+			emit_line(options, 0, 0, out.buf, out.len);
+			strbuf_reset(&out);
 			continue;
 		}
 		else if (file->is_unmerged) {
-			fprintf(options->file, "%s", line_prefix);
-			show_name(options->file, prefix, name, len);
-			fprintf(options->file, " Unmerged\n");
+			show_name(&out, prefix, name, len);
+			strbuf_addstr(&out, " Unmerged\n");
+			emit_line(options, 0, 0, out.buf, out.len);
+			strbuf_reset(&out);
 			continue;
 		}
 
@@ -1750,14 +1761,15 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 				add = total - del;
 			}
 		}
-		fprintf(options->file, "%s", line_prefix);
-		show_name(options->file, prefix, name, len);
-		fprintf(options->file, " %*"PRIuMAX"%s",
+		show_name(&out, prefix, name, len);
+		strbuf_addf(&out, " %*"PRIuMAX"%s",
 			number_width, added + deleted,
 			added + deleted ? " " : "");
-		show_graph(options->file, '+', add, add_c, reset);
-		show_graph(options->file, '-', del, del_c, reset);
-		fprintf(options->file, "\n");
+		show_graph(&out, '+', add, add_c, reset);
+		show_graph(&out, '-', del, del_c, reset);
+		strbuf_addch(&out, '\n');
+		emit_line(options, 0, 0, out.buf, out.len);
+		strbuf_reset(&out);
 	}
 
 	for (i = 0; i < data->nr; i++) {
@@ -1778,11 +1790,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		if (i < count)
 			continue;
 		if (!extra_shown)
-			fprintf(options->file, "%s ...\n", line_prefix);
+			emit_line_0(options, 0, 0, 0, " ...\n", strlen(" ...\n"));
 		extra_shown = 1;
 	}
-	fprintf(options->file, "%s", line_prefix);
-	print_stat_summary(options->file, total_files, adds, dels);
+
+	print_stat_summary_0(options, total_files, adds, dels);
 }
 
 static void show_shortstats(struct diffstat_t *data, struct diff_options *options)
@@ -1794,7 +1806,7 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option
 
 	for (i = 0; i < data->nr; i++) {
 		int added = data->files[i]->added;
-		int deleted= data->files[i]->deleted;
+		int deleted = data->files[i]->deleted;
 
 		if (data->files[i]->is_unmerged ||
 		    (!data->files[i]->is_interesting && (added + deleted == 0))) {
@@ -1804,8 +1816,7 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option
 			dels += deleted;
 		}
 	}
-	fprintf(options->file, "%s", diff_line_prefix(options));
-	print_stat_summary(options->file, total_files, adds, dels);
+	print_stat_summary_0(options, total_files, adds, dels);
 }
 
 static void show_numstat(struct diffstat_t *data, struct diff_options *options)
diff --git a/diff.h b/diff.h
index 1699d9c..cc5d038 100644
--- a/diff.h
+++ b/diff.h
@@ -379,8 +379,8 @@ extern int parse_rename_score(const char **cp_p);
 
 extern long parse_algorithm_value(const char *value);
 
-extern int print_stat_summary(FILE *fp, int files,
-			      int insertions, int deletions);
+extern void print_stat_summary(FILE *fp, int files,
+			       int insertions, int deletions);
 extern void setup_diff_pager(struct diff_options *);
 
 #endif /* DIFF_H */
-- 
2.10.0.21.g1da280f.dirty

[RFC/PATCH 13/17] diff.c: convert word diffing to use emit_line_*

[Stefan Beller]

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 all code related to diffing words.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 56 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/diff.c b/diff.c
index 00a5a4e..3940f79 100644
--- a/diff.c
+++ b/diff.c
@@ -828,37 +828,38 @@ struct diff_words_data {
 	struct diff_words_style *style;
 };
 
-static int fn_out_diff_words_write_helper(FILE *fp,
+static int fn_out_diff_words_write_helper(struct diff_options *o,
 					  struct diff_words_style_elem *st_el,
 					  const char *newline,
 					  size_t count, const char *buf,
 					  const char *line_prefix)
 {
-	int print = 0;
+	struct strbuf sb = STRBUF_INIT;
 
 	while (count) {
 		char *p = memchr(buf, '\n', count);
-		if (print)
-			fputs(line_prefix, fp);
+
 		if (p != buf) {
-			if (st_el->color && fputs(st_el->color, fp) < 0)
-				return -1;
-			if (fputs(st_el->prefix, fp) < 0 ||
-			    fwrite(buf, p ? p - buf : count, 1, fp) != 1 ||
-			    fputs(st_el->suffix, fp) < 0)
-				return -1;
-			if (st_el->color && *st_el->color
-			    && fputs(GIT_COLOR_RESET, fp) < 0)
-				return -1;
+			if (st_el->color)
+				strbuf_addstr(&sb, st_el->color);
+			strbuf_addstr(&sb, st_el->prefix);
+			strbuf_add(&sb, buf, p ? p - buf : count);
+			strbuf_addstr(&sb, st_el->suffix);
+			if (st_el->color && *st_el->color)
+			    strbuf_addstr(&sb, GIT_COLOR_RESET);
 		}
 		if (!p)
-			return 0;
-		if (fputs(newline, fp) < 0)
-			return -1;
+			goto out;
+		strbuf_addstr(&sb, newline);
+		emit_line_0(o, 0, 0, 0, sb.buf, sb.len);
+		strbuf_reset(&sb);
 		count -= p + 1 - buf;
 		buf = p + 1;
-		print = 1;
 	}
+out:
+	if (sb.len)
+		emit_line_0(o, 0, 0, 0, sb.buf, sb.len);
+	strbuf_release(&sb);
 	return 0;
 }
 
@@ -936,25 +937,25 @@ static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)
 	} else
 		plus_begin = plus_end = diff_words->plus.orig[plus_first].end;
 
-	if (color_words_output_graph_prefix(diff_words)) {
-		fputs(line_prefix, diff_words->opt->file);
-	}
+	if (color_words_output_graph_prefix(diff_words))
+		emit_line_0(diff_words->opt, 0, 0, 0, "", 0);
+
 	if (diff_words->current_plus != plus_begin) {
-		fn_out_diff_words_write_helper(diff_words->opt->file,
+		fn_out_diff_words_write_helper(diff_words->opt,
 				&style->ctx, style->newline,
 				plus_begin - diff_words->current_plus,
 				diff_words->current_plus, line_prefix);
 		if (*(plus_begin - 1) == '\n')
-			fputs(line_prefix, diff_words->opt->file);
+			emit_line_0(diff_words->opt, 0, 0, 0, "", 0);
 	}
 	if (minus_begin != minus_end) {
-		fn_out_diff_words_write_helper(diff_words->opt->file,
+		fn_out_diff_words_write_helper(diff_words->opt,
 				&style->old, style->newline,
 				minus_end - minus_begin, minus_begin,
 				line_prefix);
 	}
 	if (plus_begin != plus_end) {
-		fn_out_diff_words_write_helper(diff_words->opt->file,
+		fn_out_diff_words_write_helper(diff_words->opt,
 				&style->new, style->newline,
 				plus_end - plus_begin, plus_begin,
 				line_prefix);
@@ -1050,8 +1051,7 @@ static void diff_words_show(struct diff_words_data *diff_words)
 
 	/* special case: only removal */
 	if (!diff_words->plus.text.size) {
-		fputs(line_prefix, diff_words->opt->file);
-		fn_out_diff_words_write_helper(diff_words->opt->file,
+		fn_out_diff_words_write_helper(diff_words->opt,
 			&style->old, style->newline,
 			diff_words->minus.text.size,
 			diff_words->minus.text.ptr, line_prefix);
@@ -1077,8 +1077,8 @@ static void diff_words_show(struct diff_words_data *diff_words)
 	if (diff_words->current_plus != diff_words->plus.text.ptr +
 			diff_words->plus.text.size) {
 		if (color_words_output_graph_prefix(diff_words))
-			fputs(line_prefix, diff_words->opt->file);
-		fn_out_diff_words_write_helper(diff_words->opt->file,
+			emit_line_0(diff_words->opt, 0, 0, 0, "", 0);
+		fn_out_diff_words_write_helper(diff_words->opt,
 			&style->ctx, style->newline,
 			diff_words->plus.text.ptr + diff_words->plus.text.size
 			- diff_words->current_plus, diff_words->current_plus,
-- 
2.10.0.21.g1da280f.dirty

[RFC/PATCH 14/17] diff.c: convert diff_flush to use emit_line_*

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 3940f79..2b1ebcc 100644
--- a/diff.c
+++ b/diff.c
@@ -4737,12 +4737,13 @@ void diff_flush(struct diff_options *options)
 
 	if (output_format & DIFF_FORMAT_PATCH) {
 		if (separator) {
-			fprintf(options->file, "%s%c",
-				diff_line_prefix(options),
-				options->line_termination);
+			emit_line_0(options, NULL, NULL,
+				    options->line_termination, "", 0);
 			if (options->stat_sep) {
 				/* attach patch instead of inline */
-				fputs(options->stat_sep, options->file);
+				emit_line_0(options, NULL, NULL, 0,
+					    options->stat_sep,
+					    strlen(options->stat_sep));
 			}
 		}
 
-- 
2.10.0.21.g1da280f.dirty

[RFC/PATCH 15/17] diff.c: convert diff_summary to use emit_line_*

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 62 +++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/diff.c b/diff.c
index 2b1ebcc..68da135 100644
--- a/diff.c
+++ b/diff.c
@@ -4382,67 +4382,71 @@ static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
 	}
 }
 
-static void show_file_mode_name(FILE *file, const char *newdelete, struct diff_filespec *fs)
+static void show_file_mode_name(struct diff_options *opt, const char *newdelete, struct diff_filespec *fs)
 {
+	struct strbuf sb = STRBUF_INIT;
 	if (fs->mode)
-		fprintf(file, " %s mode %06o ", newdelete, fs->mode);
+		strbuf_addf(&sb, " %s mode %06o ", newdelete, fs->mode);
 	else
-		fprintf(file, " %s ", newdelete);
-	write_name_quoted(fs->path, file, '\n');
+		strbuf_addf(&sb, " %s ", newdelete);
+
+	quote_c_style(fs->path, &sb, NULL, 0);
+	strbuf_addch(&sb, '\n');
+	emit_line(opt, NULL, NULL, sb.buf, sb.len);
+	strbuf_release(&sb);
 }
 
 
-static void show_mode_change(FILE *file, struct diff_filepair *p, int show_name,
-		const char *line_prefix)
+static void show_mode_change(struct diff_options *opt, struct diff_filepair *p,
+		int show_name)
 {
 	if (p->one->mode && p->two->mode && p->one->mode != p->two->mode) {
-		fprintf(file, "%s mode change %06o => %06o%c", line_prefix, p->one->mode,
-			p->two->mode, show_name ? ' ' : '\n');
+		struct strbuf sb = STRBUF_INIT;
 		if (show_name) {
-			write_name_quoted(p->two->path, file, '\n');
+			strbuf_addch(&sb, ' ');
+			quote_c_style(p->two->path, &sb, NULL, 0);
 		}
+		emit_line_fmt(opt, NULL, NULL,
+			      " mode change %06o => %06o%s\n",
+			      p->one->mode, p->two->mode,
+			      show_name ? sb.buf : "");
+		strbuf_release(&sb);
 	}
 }
 
-static void show_rename_copy(FILE *file, const char *renamecopy, struct diff_filepair *p,
-			const char *line_prefix)
+static void show_rename_copy(struct diff_options *opt, const char *renamecopy,
+		struct diff_filepair *p)
 {
 	char *names = pprint_rename(p->one->path, p->two->path);
-
-	fprintf(file, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
+	emit_line_fmt(opt, NULL, NULL, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
 	free(names);
-	show_mode_change(file, p, 0, line_prefix);
+	show_mode_change(opt, p, 0);
 }
 
 static void diff_summary(struct diff_options *opt, struct diff_filepair *p)
 {
-	FILE *file = opt->file;
-	const char *line_prefix = diff_line_prefix(opt);
-
 	switch(p->status) {
 	case DIFF_STATUS_DELETED:
-		fputs(line_prefix, file);
-		show_file_mode_name(file, "delete", p->one);
+		show_file_mode_name(opt, "delete", p->one);
 		break;
 	case DIFF_STATUS_ADDED:
-		fputs(line_prefix, file);
-		show_file_mode_name(file, "create", p->two);
+		show_file_mode_name(opt, "create", p->two);
 		break;
 	case DIFF_STATUS_COPIED:
-		fputs(line_prefix, file);
-		show_rename_copy(file, "copy", p, line_prefix);
+		show_rename_copy(opt, "copy", p);
 		break;
 	case DIFF_STATUS_RENAMED:
-		fputs(line_prefix, file);
-		show_rename_copy(file, "rename", p, line_prefix);
+		show_rename_copy(opt, "rename", p);
 		break;
 	default:
 		if (p->score) {
-			fprintf(file, "%s rewrite ", line_prefix);
-			write_name_quoted(p->two->path, file, ' ');
-			fprintf(file, "(%d%%)\n", similarity_index(p));
+			struct strbuf sb = STRBUF_INIT;
+			strbuf_addstr(&sb, " rewrite ");
+			quote_c_style(p->two->path, &sb, NULL, 0);
+			strbuf_addf(&sb, " (%d%%)\n", similarity_index(p));
+			emit_line(opt, NULL, NULL, sb.buf, sb.len);
 		}
-		show_mode_change(file, p, !p->score, line_prefix);
+		show_mode_change(opt, p, !p->score);
 		break;
 	}
 }
-- 
2.10.0.21.g1da280f.dirty

[RFC/PATCH 16/17] diff: buffer output in emit_line_0

[Stefan Beller]

emit_line_0 factors out the emission part into emit_line_emission,
and depending on the diff_options->use_buffer the emission
will be performed directly when calling emit_line_0 or after the
whole process is done, i.e. by buffering we have add the possibility
for a second pass over the whole output before doing the actual
output.

In 6440d34 (2012-03-14, diff: tweak a _copy_ of diff_options with
word-diff) we introduced a duplicate diff options struct for word
emissions as we may have different regex settings in there.

When buffering the output, we need to operate on just one buffer,
so we have to copy back the emissions of the word buffer into the main
buffer.

Unconditionally enable output via buffer in this patch as it yields
a great opportunity for testing, i.e. all the diff tests from the
test suite pass without having reordering issues (i.e. only parts
of the output got buffered, and we forgot to buffer other parts).
The test suite passes, which gives confidence that we converted all
functions to use emit_line_* for output.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++------------
 diff.h |  18 ++++++++
 2 files changed, 142 insertions(+), 27 deletions(-)

diff --git a/diff.c b/diff.c
index 68da135..e240e89 100644
--- a/diff.c
+++ b/diff.c
@@ -449,42 +449,86 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
 	ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
 }
 
-static void emit_line_0(struct diff_options *o, const char *set, const char *reset,
-			int first, const char *line, int len)
+static void emit_line_emission(struct diff_options *o, struct line_emission *e)
 {
-	int has_trailing_newline, has_trailing_carriage_return;
-	int nofirst;
 	FILE *file = o->file;
 
 	fputs(diff_line_prefix(o), file);
 
+	if (e->len || e->first) {
+		if (e->set)
+			fputs(e->set, file);
+		if (e->first)
+			fputc(e->first, file);
+		if (e->whitespace_check) {
+			if (e->reset)
+				fputs(e->reset, file);
+			ws_check_emit(e->line, e->len, e->ws_rule,
+				      file, e->set, e->reset, e->ws);
+		} else {
+			fwrite(e->line, e->len, 1, file);
+			if (e->reset)
+				fputs(e->reset, file);
+		}
+	}
+	if (e->has_trailing_carriage_return)
+		fputc('\r', file);
+	if (e->has_trailing_newline)
+		fputc('\n', file);
+}
+
+static void free_line_emission(struct line_emission *e)
+{
+	/*
+	 * No need to free set, reset, ws as they always point to the
+	 * diff_colors[] static array. We don't own that memory.
+	 */
+	free((char*)e->line);
+}
+
+static void append_line_emission_to_buffer(struct diff_options *o,
+			     struct line_emission *e)
+{
+	struct line_emission *f;
+	ALLOC_GROW(o->line_buffer,
+		   o->line_buffer_nr + 1,
+		   o->line_buffer_alloc);
+	f = &o->line_buffer[o->line_buffer_nr++];
+	memcpy(f, e, sizeof(*e));
+
+	/* duplicate the line for now as it is not a stable pointer */
+	f->line = xmemdupz(e->line, e->len);
+}
+
+static void emit_line_0(struct diff_options *o, const char *set,
+			const char *reset, int first, const char *line, int len)
+{
+	int nofirst;
+	struct line_emission e = LINE_EMISSION_INIT;
+
 	if (len == 0) {
-		has_trailing_newline = (first == '\n');
-		has_trailing_carriage_return = (first == '\r');
-		nofirst = has_trailing_newline || has_trailing_carriage_return;
+		e.has_trailing_newline = (first == '\n');
+		e.has_trailing_carriage_return = (first == '\r');
+		nofirst = e.has_trailing_newline || e.has_trailing_carriage_return;
 	} else {
-		has_trailing_newline = (len > 0 && line[len-1] == '\n');
-		if (has_trailing_newline)
+		e.has_trailing_newline = (len > 0 && line[len-1] == '\n');
+		if (e.has_trailing_newline)
 			len--;
-		has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
-		if (has_trailing_carriage_return)
+		e.has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
+		if (e.has_trailing_carriage_return)
 			len--;
 		nofirst = 0;
 	}
 
-	if (len || !nofirst) {
-		if (set)
-			fputs(set, file);
-		if (!nofirst)
-			fputc(first, file);
-		fwrite(line, len, 1, file);
-		if (reset)
-			fputs(reset, file);
-	}
-	if (has_trailing_carriage_return)
-		fputc('\r', file);
-	if (has_trailing_newline)
-		fputc('\n', file);
+	e.set = set;
+	e.first = !nofirst ? first : 0;
+	e.line = line;
+	e.len = len;
+	e.reset = reset;
+	if (!o->use_buffer)
+		emit_line_emission(o, &e);
+	else
+		append_line_emission_to_buffer(o, &e);
 }
 
 static void emit_line(struct diff_options *o, const char *set, const char *reset,
@@ -540,9 +584,22 @@ static void emit_line_checked(const char *reset,
 		emit_line_0(ecbdata->opt, ws, reset, sign, line, len);
 	else {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(ecbdata->opt, set, reset, sign, "", 0);
-		ws_check_emit(line, len, ecbdata->ws_rule,
-			      ecbdata->opt->file, set, reset, ws);
+		if (ecbdata->opt->use_buffer) {
+			struct line_emission e;
+			e.line = line;
+			e.len = len;
+			e.ws_rule = ecbdata->ws_rule;
+			e.set = set;
+			e.reset = reset;
+			e.ws = ws;
+			e.whitespace_check = 1;
+			e.first = sign;
+			append_line_emission_to_buffer(ecbdata->opt, &e);
+		} else {
+			emit_line_0(ecbdata->opt, set, reset, sign, "", 0);
+			ws_check_emit(line, len, ecbdata->ws_rule,
+				      ecbdata->opt->file, set, reset, ws);
+		}
 	}
 }
 
@@ -1093,6 +1150,22 @@ static void diff_words_flush(struct emit_callback *ecbdata)
 	if (ecbdata->diff_words->minus.text.size ||
 	    ecbdata->diff_words->plus.text.size)
 		diff_words_show(ecbdata->diff_words);
+
+	if (ecbdata->diff_words->opt->line_buffer_nr) {
+		int i;
+		for (i = 0; i < ecbdata->diff_words->opt->line_buffer_nr; i++) {
+			struct line_emission *e =
+				&ecbdata->diff_words->opt->line_buffer[i];
+			ALLOC_GROW(ecbdata->opt->line_buffer,
+				   ecbdata->opt->line_buffer_nr + 1,
+				   ecbdata->opt->line_buffer_alloc);
+			memcpy(&ecbdata->opt->line_buffer
+					[ecbdata->opt->line_buffer_nr],
+				e, sizeof(*e));
+			ecbdata->opt->line_buffer_nr++;
+		}
+		ecbdata->diff_words->opt->line_buffer_nr = 0;
+	}
 }
 
 static void diff_filespec_load_driver(struct diff_filespec *one)
@@ -1128,6 +1201,12 @@ static void init_diff_words_data(struct emit_callback *ecbdata,
 		xcalloc(1, sizeof(struct diff_words_data));
 	ecbdata->diff_words->type = o->word_diff;
 	ecbdata->diff_words->opt = o;
+
+	/* Create our own buffer if needed. */
+	o->line_buffer = NULL;
+	o->line_buffer_nr = 0;
+	o->line_buffer_alloc = 0;
+
 	if (!o->word_regex)
 		o->word_regex = userdiff_word_regex(one);
 	if (!o->word_regex)
@@ -4649,11 +4728,29 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 {
 	int i;
 	struct diff_queue_struct *q = &diff_queued_diff;
+	/*
+	 * TODO:
+	 * For testing purposes we want to make sure the diff machinery
+	 * works with the buffer. If there is anything emitted outside the
+	 * emit_line_emission, then the order is screwed up and the tests
+	 * will fail.
+	 *
+	 * We'll unset this flag in a later patch.
+	 */
+	o->use_buffer = 1;
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		if (check_pair_status(p))
 			diff_flush_patch(p, o);
 	}
+
+	if (o->use_buffer) {
+		for (i = 0; i < o->line_buffer_nr; i++) {
+			emit_line_emission(o, &o->line_buffer[i]);
+			free_line_emission(&o->line_buffer[i]);
+		}
+		o->line_buffer_nr = 0;
+	}
 }
 
 void diff_flush(struct diff_options *options)
diff --git a/diff.h b/diff.h
index cc5d038..4df6aa5 100644
--- a/diff.h
+++ b/diff.h
@@ -110,6 +110,20 @@ enum diff_words_type {
 	DIFF_WORDS_COLOR
 };
 
+struct line_emission {
+	const char *set;
+	const char *line;
+	const char *ws;
+	const char *reset;
+	int first;
+	int len;
+	int whitespace_check;
+	unsigned ws_rule;
+	int has_trailing_carriage_return;
+	int has_trailing_newline;
+};
+#define LINE_EMISSION_INIT {NULL, NULL, NULL, NULL, 0, 0, 0, 0, 0, 0 }
+
 struct diff_options {
 	const char *orderfile;
 	const char *pickaxe;
@@ -178,6 +192,10 @@ struct diff_options {
 	void *output_prefix_data;
 
 	int diff_path_counter;
+
+	int use_buffer;
+	struct line_emission *line_buffer;
+	int line_buffer_nr, line_buffer_alloc;
 };
 
 void emit_line_fmt(struct diff_options *o, const char *set, const char *reset,
-- 
2.10.0.21.g1da280f.dirty

[RFC/PATCH 17/17] diff.c: color moved lines differently

[Stefan Beller]

When there is a lot of code moved around such as in 11979b9 (2005-11-18,
"http.c: reorder to avoid compilation failure.") for example, the review
process is quite hard, as it is not mentally challenging. It is rather a
tedious process, that gets boring quickly. However you still need to read
through all of the code to make sure the moved lines are there as supposed.

While it is trivial to color up a patch like the following

    $ git show -p
    commit 95b1fc60907fb9224bd785111ccd16e7e0aec4d1
    Author: Stefan Beller <sbeller@google.com>
    Date:   Mon Sep 12 20:02:46 2016 -0700

    move secure_foo from test.c to file2.c

    diff --git a/file2.c b/file2.c
    index 9163a0f..8e66dc0 100644
    --- a/file2.c
    +++ b/file2.c
    @@ -3,13 +3,6 @@ void *xmemdupz(const void *data, size_t len)
            return memcpy(xmallocz(len), data, len);
     }

    -int secure_foo(struct user *u)
    -{
    -       if (!u->is_allowed_foo)
    -               return;
    -       foo(u);
    -}
    -
     char *xstrndup(const char *str, size_t len)
     {
            char *p = memchr(str, '\0', len);
    diff --git a/test.c b/test.c
    index a95e6fe..81eb0eb 100644
    --- a/test.c
    +++ b/test.c
    @@ -18,6 +18,13 @@ ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)
            return total;
     }

    +int secure_foo(struct user *u)
    +{
    +       if (!u->is_allowed_foo)
    +               return;
    +       foo(u);
    +}
    +
     int xdup(int fd)
     {
            int ret = dup(fd);

as in this patch all lines that add or remove lines
should be colored in the new color that indicates moved
lines.

However the intention of this patch is to aid reviewers
to spotting permutations in the moved code. So consider the
following malicious move:

    diff --git a/file2.c b/file2.c
    index 9163a0f..8e66dc0 100644
    --- a/file2.c
    +++ b/file2.c
    @@ -3,13 +3,6 @@ void *xmemdupz(const void *data, size_t len)
            return memcpy(xmallocz(len), data, len);
     }

    -int secure_foo(struct user *u)
    -{
    -       if (!u->is_allowed_foo)
    -               return;
    -       foo(u);
    -}
    -
     char *xstrndup(const char *str, size_t len)
     {
            char *p = memchr(str, '\0', len);
    diff --git a/test.c b/test.c
    index a95e6fe..a679c40 100644
    --- a/test.c
    +++ b/test.c
    @@ -18,6 +18,13 @@ ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)
            return total;
     }

    +int secure_foo(struct user *u)
    +{
    +       foo(u);
    +       if (!u->is_allowed_foo)
    +               return;
    +}
    +
     int xdup(int fd)
     {
            int ret = dup(fd);

If the moved code is larger, it is easier to hide some permutation in the
code, which is why we would not want to color all lines as "moved" in this
case. So additionally to coloring lines differently that are added and
removed in the same diff, we need to tweak the algorithm a bit more.

As the reviewers attention should be brought to the places, where the
difference is introduced to the moved code, we will not color a moved
line if the previous line is a different moved line, i.e. in the code
above the line `foo()` is still colored as a regular added line, because
the line before the call to `foo` changed from `return` to `{`. The line
with the condition is also not colored as moved but as a new addition.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/config.txt               |  12 +-
 Documentation/diff-options.txt         |   7 +
 contrib/completion/git-completion.bash |   2 +
 diff.c                                 | 171 ++++++++++++++++++++----
 diff.h                                 |  10 +-
 t/t4015-diff-whitespace.sh             | 229 +++++++++++++++++++++++++++++++++
 6 files changed, 402 insertions(+), 29 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..e0c7b91 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the
 'git-diff-{asterisk}' plumbing commands.  Can be overridden on the
 command line with the `--color[=<when>]` option.
 
+color.moved::
+	A boolean value, whether a diff should color moved lines
+	differently. The moved lines are searched for in the diff only.
+	Duplicated lines from somewhere in the project that are not
+	part of the diff are not colored as moved.
+	Defaults to true.
+
 color.diff.<slot>::
 	Use customized color for diff colorization.  `<slot>` specifies
 	which part of the patch to use the specified color, and is one
 	of `context` (context text - `plain` is a historical synonym),
 	`meta` (metainformation), `frag`
 	(hunk header), 'func' (function in hunk header), `old` (removed lines),
-	`new` (added lines), `commit` (commit headers), or `whitespace`
-	(highlighting whitespace errors).
+	`new` (added lines), `commit` (commit headers), `whitespace`
+	(highlighting whitespace errors), `movedFrom` (removed lines that
+	reappear), `movedTo` (added lines that were removed elsewhere).
 
 color.decorate.<slot>::
 	Use customized color for 'git log --decorate' output.  `<slot>` is one
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 705a873..13b6a2a 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -234,6 +234,13 @@ ifdef::git-diff[]
 endif::git-diff[]
 	It is the same as `--color=never`.
 
+--[no-]color-moved::
+	Show moved blocks in a different color.
+ifdef::git-diff[]
+	It can be changed by the `diff.ui` and `color.diff`
+	configuration settings.
+endif::git-diff[]
+
 --word-diff[=<mode>]::
 	Show a word diff, using the <mode> to delimit changed words.
 	By default, words are delimited by whitespace; see
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9c8f738..85032dd 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2115,6 +2115,8 @@ _git_config ()
 		color.diff.old
 		color.diff.plain
 		color.diff.whitespace
+		color.diff.movedFrom
+		color.diff.movedTo
 		color.grep
 		color.grep.context
 		color.grep.filename
diff --git a/diff.c b/diff.c
index e240e89..f41cdcf 100644
--- a/diff.c
+++ b/diff.c
@@ -30,6 +30,7 @@ static int diff_compaction_heuristic; /* experimental */
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
+static int diff_color_moved_default;
 static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
@@ -52,6 +53,8 @@ static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_YELLOW,	/* COMMIT */
 	GIT_COLOR_BG_RED,	/* WHITESPACE */
 	GIT_COLOR_NORMAL,	/* FUNCINFO */
+	GIT_COLOR_BLUE,		/* MOVED FROM */
+	GIT_COLOR_MAGENTA,	/* MOVED TO */
 };
 
 static int parse_diff_color_slot(const char *var)
@@ -72,6 +75,10 @@ static int parse_diff_color_slot(const char *var)
 		return DIFF_WHITESPACE;
 	if (!strcasecmp(var, "func"))
 		return DIFF_FUNCINFO;
+	if (!strcasecmp(var, "movedfrom"))
+		return DIFF_FILE_MOVED_FROM;
+	if (!strcasecmp(var, "movedto"))
+		return DIFF_FILE_MOVED_TO;
 	return -1;
 }
 
@@ -180,6 +187,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		diff_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "color.moved")) {
+		diff_color_moved_default = git_config_bool(var, value);
+		return 0;
+	}
 	if (!strcmp(var, "diff.context")) {
 		diff_context_default = git_config_int(var, value);
 		if (diff_context_default < 0)
@@ -287,6 +298,41 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
+struct moved_entry {
+	struct hashmap_entry ent;
+	char *line;
+	int hash_prev;
+};
+
+static int moved_entry_cmp(const struct moved_entry *a,
+			   const struct moved_entry *b,
+			   const void *unused)
+{
+	return strcmp(a->line, b->line) &&
+	       a->hash_prev == b->hash_prev;
+}
+
+static struct moved_entry *prepare_entry(const char *line,
+					 unsigned long len,
+					 int ws_rule,
+					 int hash_prev)
+{
+	int i;
+	struct strbuf sb = STRBUF_INIT;
+	struct moved_entry *ret = xmalloc(sizeof(*ret));
+
+	for (i = 0; i < len; i++) {
+		if (ws_rule && isspace(line[i]))
+			continue;
+		strbuf_addch(&sb, line[i]);
+	}
+
+	ret->ent.hash = memhash(sb.buf, sb.len);
+	ret->line = xmemdupz(line, len);
+	ret->hash_prev = hash_prev;
+	return ret;
+}
+
 static char *quote_two(const char *one, const char *two)
 {
 	int need_one = quote_c_style(one, NULL, NULL, 1);
@@ -449,12 +495,49 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
 	ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
 }
 
+static int should_color_as_moved(struct diff_options *o,
+				 struct line_emission *e,
+				 struct moved_entry *keydata,
+				 struct hashmap *lookup)
+{
+	struct moved_entry *me;
+
+	if (!o->color_moved)
+		return 0;
+	if (e->first != '+' && e->first != '-')
+		return 0;
+
+	me = hashmap_get(lookup, keydata, keydata);
+
+	if (!me)
+		return 0;
+
+	if (me->hash_prev == 0 || o->hash_prev == 0)
+		return 1;
+
+	return (me->hash_prev == o->hash_prev);
+}
+
 static void emit_line_emission(struct diff_options *o, struct line_emission *e)
 {
 	FILE *file = o->file;
 
 	fputs(diff_line_prefix(o), file);
 
+	if (o->color_moved && (e->first == '+' || e->first == '-')) {
+		struct moved_entry *keydata;
+		keydata = prepare_entry(e->line, e->len,
+					DIFF_XDL_TST(o, IGNORE_WHITESPACE), 0);
+		if (e->first == '-' &&
+		    should_color_as_moved(o, e, keydata, o->added_lines))
+			e->set = diff_get_color_opt(o, DIFF_FILE_MOVED_FROM);
+		if (e->first == '+' &&
+		    should_color_as_moved(o, e, keydata, o->deleted_lines))
+			e->set = diff_get_color_opt(o, DIFF_FILE_MOVED_TO);
+
+		o->hash_prev = keydata->ent.hash;
+	}
+
 	if (e->len || e->first) {
 		if (e->set)
 			fputs(e->set, file);
@@ -568,67 +651,95 @@ static void emit_line_checked(const char *reset,
 			      unsigned ws_error_highlight,
 			      char sign)
 {
+	int i;
+	struct line_emission *e;
 	const char *set = diff_get_color(ecbdata->color_diff, color);
 	const char *ws = NULL;
+	struct diff_options *o = ecbdata->opt;
 
-	if (ecbdata->opt->ws_error_highlight & ws_error_highlight) {
+	if (o->ws_error_highlight & ws_error_highlight) {
 		ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
 		if (!*ws)
 			ws = NULL;
 	}
 
 	if (!ws)
-		emit_line_0(ecbdata->opt, set, reset, sign, line, len);
+		emit_line_0(o, set, reset, sign, line, len);
 	else if (sign == '+' && new_blank_line_at_eof(ecbdata, line, len))
 		/* Blank line at EOF - paint '+' as well */
-		emit_line_0(ecbdata->opt, ws, reset, sign, line, len);
+		emit_line_0(o, ws, reset, sign, line, len);
 	else {
 		/* Emit just the prefix, then the rest. */
-		if (ecbdata->opt->use_buffer) {
-			struct line_emission e;
-			e.line = line;
-			e.len = len;
-			e.ws_rule = ecbdata->ws_rule;
-			e.set = set;
-			e.reset = reset;
-			e.ws = ws;
-			e.whitespace_check = 1;
-			e.first = sign;
-			append_line_emission_to_buffer(ecbdata->opt, &e);
+		if (o->use_buffer) {
+			emit_line_0(o, set, reset, sign, line, len);
+			i = o->line_buffer_nr - 1;
+			e = &o->line_buffer[i];
+			e->whitespace_check = 1;
+			e->ws_rule = ecbdata->ws_rule;
+			e->ws = ws;
 		} else {
-			emit_line_0(ecbdata->opt, set, reset, sign, "", 0);
+			emit_line_0(o, set, reset, sign, "", 0);
 			ws_check_emit(line, len, ecbdata->ws_rule,
-				      ecbdata->opt->file, set, reset, ws);
+				      o->file, set, reset, ws);
 		}
 	}
+	if (o->color_moved) {
+		i = o->line_buffer_nr - 1;
+		e = &o->line_buffer[i];
+	}
 }
 
 static void emit_add_line(const char *reset,
 			  struct emit_callback *ecbdata,
 			  const char *line, int len)
 {
+	struct diff_options *o = ecbdata->opt;
 	ecbdata->lno_in_postimage++;
 	emit_line_checked(reset, ecbdata, line, len,
 			  DIFF_FILE_NEW, WSEH_NEW, '+');
+	if (o->color_moved) {
+		struct line_emission *l = &o->line_buffer[o->line_buffer_nr - 1];
+		struct moved_entry *keydata = prepare_entry(l->line, l->len,
+					DIFF_XDL_TST(o, IGNORE_WHITESPACE),
+					o->hash_prev);
+		l = &o->line_buffer[o->line_buffer_nr - 2];
+		if (l->first == '+')
+			keydata->hash_prev = o->hash_prev;
+		hashmap_add(o->added_lines, keydata);
+		o->hash_prev = keydata->ent.hash;
+	}
 }
 
 static void emit_del_line(const char *reset,
 			  struct emit_callback *ecbdata,
 			  const char *line, int len)
 {
+	struct diff_options *o = ecbdata->opt;
 	ecbdata->lno_in_preimage++;
 	emit_line_checked(reset, ecbdata, line, len,
 			  DIFF_FILE_OLD, WSEH_OLD, '-');
+	if (o->color_moved) {
+		struct line_emission *l = &o->line_buffer[o->line_buffer_nr - 1];
+		struct moved_entry *keydata = prepare_entry(l->line, l->len,
+					DIFF_XDL_TST(o, IGNORE_WHITESPACE), 0);
+		l = &o->line_buffer[o->line_buffer_nr - 2];
+		if (l->first == '-')
+			keydata->hash_prev = o->hash_prev;
+		hashmap_add(o->deleted_lines, keydata);
+		o->hash_prev = keydata->ent.hash;
+	}
 }
 
 static void emit_context_line(const char *reset,
 			      struct emit_callback *ecbdata,
 			      const char *line, int len)
 {
+	struct diff_options *o = ecbdata->opt;
 	ecbdata->lno_in_postimage++;
 	ecbdata->lno_in_preimage++;
 	emit_line_checked(reset, ecbdata, line, len,
 			  DIFF_CONTEXT, WSEH_CONTEXT, ' ');
+	o->hash_prev = 0;
 }
 
 static void emit_hunk_header(struct emit_callback *ecbdata,
@@ -3389,6 +3500,7 @@ void diff_setup(struct diff_options *options)
 	options->change = diff_change;
 	options->add_remove = diff_addremove;
 	options->use_color = diff_use_color_default;
+	options->color_moved = diff_color_moved_default;
 	options->detect_rename = diff_detect_rename_default;
 	options->xdl_opts |= diff_algorithm;
 	if (diff_compaction_heuristic)
@@ -3511,6 +3623,9 @@ void diff_setup_done(struct diff_options *options)
 
 	if (DIFF_OPT_TST(options, FOLLOW_RENAMES) && options->pathspec.nr != 1)
 		die(_("--follow requires exactly one pathspec"));
+
+	if (!options->use_color || external_diff())
+		options->color_moved = 0;
 }
 
 static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
@@ -3961,6 +4076,10 @@ int diff_opt_parse(struct diff_options *options,
 	}
 	else if (!strcmp(arg, "--no-color"))
 		options->use_color = 0;
+	else if (!strcmp(arg, "--color-moved"))
+		options->color_moved = 1;
+	else if (!strcmp(arg, "--no-color-moved"))
+		options->color_moved = 0;
 	else if (!strcmp(arg, "--color-words")) {
 		options->use_color = 1;
 		options->word_diff = DIFF_WORDS_COLOR;
@@ -4728,16 +4847,15 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 {
 	int i;
 	struct diff_queue_struct *q = &diff_queued_diff;
-	/*
-	 * TODO:
-	 * For testing purposes we want to make sure the diff machinery
-	 * works with the buffer. If there is anything emitted outside the
-	 * emit_line_emission, then the order is screwed up and the tests
-	 * will fail.
-	 *
-	 * We'll unset this flag in a later patch.
-	 */
-	o->use_buffer = 1;
+
+	if (o->color_moved) {
+		o->use_buffer = 1;
+		o->deleted_lines = xmallocz(sizeof(*o->deleted_lines));
+		o->added_lines = xmallocz(sizeof(*o->added_lines));
+		hashmap_init(o->deleted_lines, (hashmap_cmp_fn)moved_entry_cmp, 0);
+		hashmap_init(o->added_lines, (hashmap_cmp_fn)moved_entry_cmp, 0);
+	}
+
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		if (check_pair_status(p))
@@ -4827,6 +4945,7 @@ void diff_flush(struct diff_options *options)
 		if (!options->file)
 			die_errno("Could not open /dev/null");
 		options->close_file = 1;
+		options->color_moved = 0;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))
diff --git a/diff.h b/diff.h
index 4df6aa5..8c3f8f7 100644
--- a/diff.h
+++ b/diff.h
@@ -7,6 +7,7 @@
 #include "tree-walk.h"
 #include "pathspec.h"
 #include "object.h"
+#include "hashmap.h"
 
 struct rev_info;
 struct diff_options;
@@ -193,9 +194,14 @@ struct diff_options {
 
 	int diff_path_counter;
 
+	int color_moved;
 	int use_buffer;
 	struct line_emission *line_buffer;
 	int line_buffer_nr, line_buffer_alloc;
+
+	struct hashmap *deleted_lines;
+	struct hashmap *added_lines;
+	int hash_prev;
 };
 
 void emit_line_fmt(struct diff_options *o, const char *set, const char *reset,
@@ -210,7 +216,9 @@ enum color_diff {
 	DIFF_FILE_NEW = 5,
 	DIFF_COMMIT = 6,
 	DIFF_WHITESPACE = 7,
-	DIFF_FUNCINFO = 8
+	DIFF_FUNCINFO = 8,
+	DIFF_FILE_MOVED_TO = 9,
+	DIFF_FILE_MOVED_FROM = 10
 };
 const char *diff_get_color(int diff_use_color, enum color_diff ix);
 #define diff_get_color_opt(o, ix) \
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 2434157..2e3d7c9 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -934,4 +934,233 @@ test_expect_success 'the same with --ws-error-highlight' '
 	test_cmp expected current
 '
 
+test_expect_success 'detect moved code, complete file' '
+	git reset --hard &&
+	cat <<-\EOF >test.c &&
+	#include<stdio.h>
+	main()
+	{
+	printf("Hello World");
+	}
+	EOF
+	git add test.c &&
+	git commit -m "add main function" &&
+	git mv test.c main.c &&
+	git diff HEAD --color-moved --no-renames | test_decode_color >actual &&
+	cat >expected <<-\EOF &&
+	<BOLD>diff --git a/main.c b/main.c<RESET>
+	<BOLD>new file mode 100644<RESET>
+	<BOLD>index 0000000..a986c57<RESET>
+	<BOLD>--- /dev/null<RESET>
+	<BOLD>+++ b/main.c<RESET>
+	<CYAN>@@ -0,0 +1,5 @@<RESET>
+	<BLUE>+<RESET><BLUE>#include<stdio.h><RESET>
+	<BLUE>+<RESET><BLUE>main()<RESET>
+	<BLUE>+<RESET><BLUE>{<RESET>
+	<BLUE>+<RESET><BLUE>printf("Hello World");<RESET>
+	<BLUE>+<RESET><BLUE>}<RESET>
+	<BOLD>diff --git a/test.c b/test.c<RESET>
+	<BOLD>deleted file mode 100644<RESET>
+	<BOLD>index a986c57..0000000<RESET>
+	<BOLD>--- a/test.c<RESET>
+	<BOLD>+++ /dev/null<RESET>
+	<CYAN>@@ -1,5 +0,0 @@<RESET>
+	<MAGENTA>-#include<stdio.h><RESET>
+	<MAGENTA>-main()<RESET>
+	<MAGENTA>-{<RESET>
+	<MAGENTA>-printf("Hello World");<RESET>
+	<MAGENTA>-}<RESET>
+	EOF
+
+	test_cmp expected actual
+'
+
+test_expect_success 'detect moved code, inside file' '
+	git reset --hard &&
+	cat <<-\EOF >main.c &&
+		#include<stdio.h>
+		int stuff()
+		{
+			printf("Hello ");
+			printf("World\n");
+		}
+
+		int secure_foo(struct user *u)
+		{
+			if (!u->is_allowed_foo)
+				return;
+			foo(u);
+		}
+
+		int main()
+		{
+			foo();
+		}
+	EOF
+	cat <<-\EOF >test.c &&
+		#include<stdio.h>
+		int bar()
+		{
+			printf("Hello World, but different\n");
+		}
+
+		int another_function()
+		{
+			bar();
+		}
+	EOF
+	git add main.c test.c &&
+	git commit -m "add main and test file" &&
+	cat <<-\EOF >main.c &&
+		#include<stdio.h>
+		int stuff()
+		{
+			printf("Hello ");
+			printf("World\n");
+		}
+
+		int main()
+		{
+			foo();
+		}
+	EOF
+	cat <<-\EOF >test.c &&
+		#include<stdio.h>
+		int bar()
+		{
+			printf("Hello World, but different\n");
+		}
+
+		int secure_foo(struct user *u)
+		{
+			if (!u->is_allowed_foo)
+				return;
+			foo(u);
+		}
+
+		int another_function()
+		{
+			bar();
+		}
+	EOF
+	git diff HEAD --no-renames --color-moved| test_decode_color >actual &&
+	cat <<-\EOF >expected &&
+	<BOLD>diff --git a/main.c b/main.c<RESET>
+	<BOLD>index 27a619c..7cf9336 100644<RESET>
+	<BOLD>--- a/main.c<RESET>
+	<BOLD>+++ b/main.c<RESET>
+	<CYAN>@@ -5,13 +5,6 @@<RESET> <RESET>printf("Hello ");<RESET>
+	 printf("World\n");<RESET>
+	 }<RESET>
+	 <RESET>
+	<MAGENTA>-int secure_foo(struct user *u)<RESET>
+	<MAGENTA>-{<RESET>
+	<MAGENTA>-if (!u->is_allowed_foo)<RESET>
+	<MAGENTA>-return;<RESET>
+	<MAGENTA>-foo(u);<RESET>
+	<MAGENTA>-}<RESET>
+	<MAGENTA>-<RESET>
+	 int main()<RESET>
+	 {<RESET>
+	 foo();<RESET>
+	<BOLD>diff --git a/test.c b/test.c<RESET>
+	<BOLD>index 1dc1d85..e34eb69 100644<RESET>
+	<BOLD>--- a/test.c<RESET>
+	<BOLD>+++ b/test.c<RESET>
+	<CYAN>@@ -4,6 +4,13 @@<RESET> <RESET>int bar()<RESET>
+	 printf("Hello World, but different\n");<RESET>
+	 }<RESET>
+	 <RESET>
+	<BLUE>+<RESET><BLUE>int secure_foo(struct user *u)<RESET>
+	<BLUE>+<RESET><BLUE>{<RESET>
+	<BLUE>+<RESET><BLUE>if (!u->is_allowed_foo)<RESET>
+	<BLUE>+<RESET><BLUE>return;<RESET>
+	<BLUE>+<RESET><BLUE>foo(u);<RESET>
+	<BLUE>+<RESET><BLUE>}<RESET>
+	<BLUE>+<RESET>
+	 int another_function()<RESET>
+	 {<RESET>
+	 bar();<RESET>
+	EOF
+
+	test_cmp expected actual
+'
+
+test_expect_success 'detect permutations inside moved code, ' '
+	# reusing the move example from last test:
+	cat <<-\EOF >main.c &&
+		#include<stdio.h>
+		int stuff()
+		{
+			printf("Hello ");
+			printf("World\n");
+		}
+
+		int main()
+		{
+			foo();
+		}
+	EOF
+	cat <<-\EOF >test.c &&
+		#include<stdio.h>
+		int bar()
+		{
+			printf("Hello World, but different\n");
+		}
+
+		int secure_foo(struct user *u)
+		{
+			foo(u);
+			if (!u->is_allowed_foo)
+				return;
+		}
+
+		int another_function()
+		{
+			bar();
+		}
+	EOF
+	git diff HEAD --no-renames --color-moved| test_decode_color >actual &&
+	cat <<-\EOF >expected &&
+	<BOLD>diff --git a/main.c b/main.c<RESET>
+	<BOLD>index 27a619c..7cf9336 100644<RESET>
+	<BOLD>--- a/main.c<RESET>
+	<BOLD>+++ b/main.c<RESET>
+	<CYAN>@@ -5,13 +5,6 @@<RESET> <RESET>printf("Hello ");<RESET>
+	 printf("World\n");<RESET>
+	 }<RESET>
+	 <RESET>
+	<MAGENTA>-int secure_foo(struct user *u)<RESET>
+	<MAGENTA>-{<RESET>
+	<RED>-if (!u->is_allowed_foo)<RESET>
+	<MAGENTA>-return;<RESET>
+	<RED>-foo(u);<RESET>
+	<RED>-}<RESET>
+	<MAGENTA>-<RESET>
+	 int main()<RESET>
+	 {<RESET>
+	 foo();<RESET>
+	<BOLD>diff --git a/test.c b/test.c<RESET>
+	<BOLD>index 1dc1d85..2bedec9 100644<RESET>
+	<BOLD>--- a/test.c<RESET>
+	<BOLD>+++ b/test.c<RESET>
+	<CYAN>@@ -4,6 +4,13 @@<RESET> <RESET>int bar()<RESET>
+	 printf("Hello World, but different\n");<RESET>
+	 }<RESET>
+	 <RESET>
+	<BLUE>+<RESET><BLUE>int secure_foo(struct user *u)<RESET>
+	<BLUE>+<RESET><BLUE>{<RESET>
+	<GREEN>+<RESET><GREEN>foo(u);<RESET>
+	<GREEN>+<RESET><GREEN>if (!u->is_allowed_foo)<RESET>
+	<BLUE>+<RESET><BLUE>return;<RESET>
+	<GREEN>+<RESET><GREEN>}<RESET>
+	<BLUE>+<RESET>
+	 int another_function()<RESET>
+	 {<RESET>
+	 bar();<RESET>
+	EOF
+
+	test_cmp expected actual
+'
+
 test_done
-- 
2.10.0.21.g1da280f.dirty

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

[René Scharfe]

Am 13.09.2016 um 06:45 schrieb Stefan Beller:
> 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 patch moves code that is conceptually part of
> emit_hunk_header, but was using output in fn_out_consume,
> back to emit_hunk_header.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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');
> +

Using strbuf_complete_line() would be nicer.

>  	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;
>  	}
>
>

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

[Stefan Beller]

On Tue, Sep 13, 2016 at 7:42 AM, René Scharfe <l.s.r@web.de> wrote:

>>
>>         strbuf_add(&msgbuf, line + len, org_len - len);
>> +       if (line[org_len - 1] != '\n')
>> +               strbuf_addch(&msgbuf, '\n');
>> +
>
>
> Using strbuf_complete_line() would be nicer.

That makes sense!

Thanks,
Stefan

Re: [RFC/PATCH 05/17] diff.c: emit_line_0 can handle no color setting

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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.
>
> In later patches we may pass lines that are not colored to
> the central function emit_line_0, so we
> need to emit the color only when it is non-NULL.

Explained this way, a reader would find that this step is here
before the underlying code is ready--we are not even buffering
at this step yet.

But that is OK.  It used to be that passing "" as set/reset was the
way to get a --no-color output.  Now you can pass NULL instead of
empty strings.  That would be an alternative explanation why this is
an acceptable change (as your later step probably has a good reason
why it cannot pass "" instead of NULL).

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  diff.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index b6a40ae..5d57130 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);

Re: [RFC/PATCH 06/17] diff.c: convert fn_out_consume to use emit_line_*

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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 the remaining parts of fn_out_consume.

name_x_tab are colored as before, which you are already aware of and
we'd need to find a way to handle, but other than that, this is a
no-op conversion, getting us closer to the goal of making everything
go through a single funnel.

>  		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;
>  	}

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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 prepares the code for submodules to go through the
> emit_line_0 function.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

I wonder how this interacts with the jk/diff-submodule-diff-inline
topic by Jacob that has graduated recently to 'master'.  IIRC, it
just lets a separate "git diff" instance that is spawned in the
submodule directory emit its findings to the output of the driving
"git diff" in the superproject.

Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +struct line_emission {
> +	const char *set;
> +	const char *line;
> +	const char *ws;
> +	const char *reset;
> +	int first;
> +	int len;
> +	int whitespace_check;
> +	unsigned ws_rule;
> +	int has_trailing_carriage_return;
> +	int has_trailing_newline;
> +};

It is somewhat strange to see whitespace things are per-line here.
I'd understand it if it were per-path, though.

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

[Stefan Beller]

On Tue, Sep 13, 2016 at 4:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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 prepares the code for submodules to go through the
>> emit_line_0 function.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> I wonder how this interacts with the jk/diff-submodule-diff-inline
> topic by Jacob that has graduated recently to 'master'.  IIRC, it
> just lets a separate "git diff" instance that is spawned in the
> submodule directory emit its findings to the output of the driving
> "git diff" in the superproject.
>

The easiest way to find out is to merge HEAD^ of this patch series
(i.e. "diff: buffer output in emit_line_0") with whatever we suspect can
cause breakage for the goal of channeling everything though emit_line_*
functions. Looking at that series, I think I'll have to redo
this (maybe even including sb/diff-cleanup, to have it all in one series)
to capture all output there.

I suspected a breakage, but as the patch series grew larger and larger,
I first wanted to get into a working state before paying attention to solving
conflicts as resolving conflicts is easier when I know where this series is
headed.

Thanks!
Stefan

Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0

[Stefan Beller]

On Tue, Sep 13, 2016 at 4:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +struct line_emission {
>> +     const char *set;
>> +     const char *line;
>> +     const char *ws;
>> +     const char *reset;
>> +     int first;
>> +     int len;
>> +     int whitespace_check;
>> +     unsigned ws_rule;
>> +     int has_trailing_carriage_return;
>> +     int has_trailing_newline;
>> +};
>
> It is somewhat strange to see whitespace things are per-line here.
> I'd understand it if it were per-path, though.

Yeah we have to have it at least per path as that is the granularity
the user can configure it.

So would we rather want to keep the ecbdata around for each file pair and
just reference that? I thought we deliberately want to avoid ecbdata, so maybe
we rather want to have another struct that keeps path related information
around (pointer to the blob and white space information).

Thanks,
Stefan

Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So would we rather want to keep the ecbdata around for each file pair and
> just reference that? I thought we deliberately want to avoid ecbdata, so maybe
> we rather want to have another struct that keeps path related information
> around (pointer to the blob and white space information).

I would expect that there would be two structs, one per path
"struct buffered_patch" that has the per-path thing, and another per
line "struct buffered_patch_line" that describes what each line is,
and has a pointer to the former.


	

Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0

[Stefan Beller]

On Tue, Sep 13, 2016 at 4:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So would we rather want to keep the ecbdata around for each file pair and
>> just reference that? I thought we deliberately want to avoid ecbdata, so maybe
>> we rather want to have another struct that keeps path related information
>> around (pointer to the blob and white space information).
>
> I would expect that there would be two structs, one per path
> "struct buffered_patch" that has the per-path thing, and another per
> line "struct buffered_patch_line" that describes what each line is,
> and has a pointer to the former.
>

Heh, I was trying to come up with a clever thing to save that pointer,
as we would need to have that pointer once per line, so in large patches
that would save a bit of space, but probably I should not try to be too
smart about it.

So I'd split up the struct line_emission into the two proposed
buffered_patch_line as well as buffered_patch.

However the naming is a bit off than I would expect. Historically you
had one patch per file, so it was natural to name a change of multiple
files a "patchset" (c.f. a commit in Gerrit is called "patchset"/revision)

Today as Git is quite successful, one "patch" is easily understood
as the equivalent of one patch, i.e. what format-patch produced.

So I'd prefer to go with buffer_filepair and buffer_line maybe?