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

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

[Stefan Beller]

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

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

[Stefan Beller]

From: Stefan Beller <sbeller@google.com>

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

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

[Stefan Beller]

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

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

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

[Stefan Beller]

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

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

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

[Stefan Beller]

From: Stefan Beller <sbeller@google.com>

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

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

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

[Stefan Beller]

From: Stefan Beller <sbeller@google.com>

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

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

[Stefan Beller]

From: Stefan Beller <sbeller@google.com>

---
 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);
-- 
2.7.4

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

[Stefan Beller]

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

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

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

[Stefan Beller]

From: Stefan Beller <sbeller@google.com>

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

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

[Stefan Beller]

From: Stefan Beller <sbeller@google.com>

---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[Stefan Beller]

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

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

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

[Junio C Hamano]

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.

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

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

[Junio C Hamano]

Stefan Beller <stefanbeller@gmail.com> writes:

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

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Stefan Beller <stefanbeller@gmail.com> writes:

> From: Stefan Beller <sbeller@google.com>
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

The reason being...?

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

[Junio C Hamano]

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.

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

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

[Junio C Hamano]

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.



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

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

[Stefan Beller]

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

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

[Junio C Hamano]

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

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

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

[Stefan Beller]

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

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

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

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

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

with things in parenthesis optional.

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


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

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

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

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

[Stefan Beller]

On Mon, Sep 12, 2016 at 4:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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 ?