combine-diff: use diff_opts->a_prefix

PATCH In review Salikh Zakirov 2 participants 3 messages Jun 15, 2016

Share & tools

Salikh Zakirov

author

2016-06-15 22:44 UTC #0 lore raw

PATCH 1/1 combine-diff: use diff_opts->a_prefix

The introduction of configurable dir prefix for diff headers in commit eab9a40b 'Teach diff machinery to display other prefixes than "a/" and "b/"' missed combined diff generation.

Signed-off-by Salikh Zakirov <[email protected]>

---

I realize that this fix is ugly, so I am all ears for a suggestion of a better fix.

 combine-diff.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

combine-diff.c +11 -8

index e22db89..5c3b42d 100644
@@ -646,10 +646,11 @@ static void reuse_combine_diff(struct sline *sline, unsigned long cnt,
 	sline->p_lno[i] = sline->p_lno[j];
 }
 
-static void dump_quoted_path(const char *prefix, const char *path,
-			     const char *c_meta, const char *c_reset)
+static void dump_quoted_path(const char *prefix, const char *prefix2,
+                             const char *path, const char *c_meta,
+			     const char *c_reset)
 {
-	printf("%s%s", c_meta, prefix);
+	printf("%s%s%s", c_meta, prefix, prefix2);
 	quote_c_style(path, NULL, stdout, 0);
 	printf("%s\n", c_reset);
 }

@@ -792,7 +793,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 
 		if (rev->loginfo && !rev->no_commit_id)
 			show_log(rev, opt->msg_sep);
-		dump_quoted_path(dense ? "diff --cc " : "diff --combined ",
+		dump_quoted_path(dense ? "diff --cc " : "diff --combined ", "",
 				 elem->path, c_meta, c_reset);
 		printf("%sindex ", c_meta);
 		for (i = 0; i < num_parent; i++) {

@@ -829,13 +830,15 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			printf("%s\n", c_reset);
 		}
 		if (added)
-			dump_quoted_path("--- /dev/", "null", c_meta, c_reset);
+			dump_quoted_path("--- /dev/", "", "null", c_meta, c_reset);
 		else
-			dump_quoted_path("--- a/", elem->path, c_meta, c_reset);
+			dump_quoted_path("--- ", opt->a_prefix, elem->path,
+			                 c_meta, c_reset);
 		if (deleted)
-			dump_quoted_path("+++ /dev/", "null", c_meta, c_reset);
+			dump_quoted_path("+++ /dev/", "", "null", c_meta, c_reset);
 		else
-			dump_quoted_path("+++ b/", elem->path, c_meta, c_reset);
+			dump_quoted_path("+++ ", opt->b_prefix, elem->path,
+			                 c_meta, c_reset);
 		dump_sline(sline, cnt, num_parent, DIFF_OPT_TST(opt, COLOR_DIFF));
 	}
 	free(result);

Signature · 1 line

1.5.3.7.1315.g1b8e7

Junio C Hamano

2016-06-15 22:44 UTC #1 lore raw

Quoted 9 lines · Salikh Zakirov

The introduction of configurable dir prefix for diff headers in commit eab9a40b 'Teach diff machinery to display other prefixes than "a/" and "b/"' missed combined diff generation.
Signed-off-by Salikh Zakirov <[email protected]>
---
I realize that this fix is ugly, so I am all ears for a suggestion of a better fix.

It is not so ugly, but I think the original code is broken wrt its calling of quote_c_style(). It will output "a/" literally and then would spit out the path with quoting. IOW, you would get something like:

	--- a/"foo\tbar"
        +++ b/"foo\tbar"

when it should show:

	--- "a/foo\tbar"
        +++ "b/foo\tbar"

Incidentally, I just noticed that diff.c::emit_rewrite_diff() has the same bug.

Here is a fix to combine-diff.c

-- >8 --
[PATCH] combine-diff: Fix path quoting

Earlier when showing combined diff, the filenames on the ---/+++ header lines were quoted incorrectly. a/ (or b/) prefix was output literally and then the path was output, with c-quoting.

This fixes the quoting logic, and while at it, adjusts the code to use the customizable prefix (a_prefix and b_prefix) introduced recently.

Signed-off-by Junio C Hamano <[email protected]>

---
 combine-diff.c |   41 +++++++++++++++++++++++++++++++----------
 1 files changed, 31 insertions(+), 10 deletions(-)

combine-diff.c +31 ~~-10~~

index e22db89..7d71033 100644
@@ -646,12 +646,28 @@ static void reuse_combine_diff(struct sline *sline, unsigned long cnt,
 	sline->p_lno[i] = sline->p_lno[j];
 }
 
-static void dump_quoted_path(const char *prefix, const char *path,
+static void dump_quoted_path(const char *head,
+			     const char *prefix,
+			     const char *path,
 			     const char *c_meta, const char *c_reset)
 {
-	printf("%s%s", c_meta, prefix);
-	quote_c_style(path, NULL, stdout, 0);
-	printf("%s\n", c_reset);
+	static struct strbuf buf = STRBUF_INIT;
+
+	strbuf_reset(&buf);
+	strbuf_addstr(&buf, c_meta);
+	strbuf_addstr(&buf, head);
+	if (quote_c_style(prefix, NULL, NULL, 0) ||
+	    quote_c_style(path, NULL, NULL, 0)) {
+		strbuf_addch(&buf, '"');
+		quote_c_style(prefix, &buf, NULL, 1);
+		quote_c_style(path, &buf, NULL, 1);
+		strbuf_addch(&buf, '"');
+	} else {
+		strbuf_addstr(&buf, prefix);
+		strbuf_addstr(&buf, path);
+	}
+	strbuf_addstr(&buf, c_reset);
+	puts(buf.buf);
 }
 
 static void show_patch_diff(struct combine_diff_path *elem, int num_parent,

@@ -793,7 +809,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 		if (rev->loginfo && !rev->no_commit_id)
 			show_log(rev, opt->msg_sep);
 		dump_quoted_path(dense ? "diff --cc " : "diff --combined ",
-				 elem->path, c_meta, c_reset);
+				 "", elem->path, c_meta, c_reset);
 		printf("%sindex ", c_meta);
 		for (i = 0; i < num_parent; i++) {
 			abb = find_unique_abbrev(elem->parent[i].sha1,

@@ -829,14 +845,19 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			printf("%s\n", c_reset);
 		}
 		if (added)
-			dump_quoted_path("--- /dev/", "null", c_meta, c_reset);
+			dump_quoted_path("--- ", "", "/dev/null",
+					 c_meta, c_reset);
 		else
-			dump_quoted_path("--- a/", elem->path, c_meta, c_reset);
+			dump_quoted_path("--- ", opt->a_prefix, elem->path,
+					 c_meta, c_reset);
 		if (deleted)
-			dump_quoted_path("+++ /dev/", "null", c_meta, c_reset);
+			dump_quoted_path("+++ ", "", "/dev/null",
+					 c_meta, c_reset);
 		else
-			dump_quoted_path("+++ b/", elem->path, c_meta, c_reset);
-		dump_sline(sline, cnt, num_parent, DIFF_OPT_TST(opt, COLOR_DIFF));
+			dump_quoted_path("+++ ", opt->b_prefix, elem->path,
+					 c_meta, c_reset);
+		dump_sline(sline, cnt, num_parent,
+			   DIFF_OPT_TST(opt, COLOR_DIFF));
 	}
 	free(result);

Signature · 1 line

1.5.4.rc1.23.g3a969

Junio C Hamano

2016-06-15 22:44 UTC #2 lore raw

PATCH 1/1 Fix rewrite_diff() name quoting.

This moves the logic to quote two paths (prefix + path) in C-style introduced in the previous commit from the dump_quoted_path() in combine-diff.c to quote.c, and uses it to fix rewrite_diff() that never C-quoted the pathnames correctly.

Signed-off-by Junio C Hamano <[email protected]>

---
 combine-diff.c |   11 +----------
 diff.c         |   12 +++++++++---
 quote.c        |   16 ++++++++++++++++
 quote.h        |    1 +
 4 files changed, 27 insertions(+), 13 deletions(-)

combine-diff.c +1 ~~-10~~

index 7d71033..0e19cba 100644
@@ -656,16 +656,7 @@ static void dump_quoted_path(const char *head,
 	strbuf_reset(&buf);
 	strbuf_addstr(&buf, c_meta);
 	strbuf_addstr(&buf, head);
-	if (quote_c_style(prefix, NULL, NULL, 0) ||
-	    quote_c_style(path, NULL, NULL, 0)) {
-		strbuf_addch(&buf, '"');
-		quote_c_style(prefix, &buf, NULL, 1);
-		quote_c_style(path, &buf, NULL, 1);
-		strbuf_addch(&buf, '"');
-	} else {
-		strbuf_addstr(&buf, prefix);
-		strbuf_addstr(&buf, path);
-	}
+	quote_two_c_style(&buf, prefix, path, 0);
 	strbuf_addstr(&buf, c_reset);
 	puts(buf.buf);
 }

diff.c +9 -3

index 61fd492..5bdc111 100644
@@ -300,19 +300,25 @@ static void emit_rewrite_diff(const char *name_a,
 	const char *old = diff_get_color(color_diff, DIFF_FILE_OLD);
 	const char *new = diff_get_color(color_diff, DIFF_FILE_NEW);
 	const char *reset = diff_get_color(color_diff, DIFF_RESET);
+	static struct strbuf a_name = STRBUF_INIT, b_name = STRBUF_INIT;
 
 	name_a += (*name_a == '/');
 	name_b += (*name_b == '/');
 	name_a_tab = strchr(name_a, ' ') ? "\t" : "";
 	name_b_tab = strchr(name_b, ' ') ? "\t" : "";
 
+	strbuf_reset(&a_name);
+	strbuf_reset(&b_name);
+	quote_two_c_style(&a_name, o->a_prefix, name_a, 0);
+	quote_two_c_style(&b_name, o->b_prefix, name_b, 0);
+
 	diff_populate_filespec(one, 0);
 	diff_populate_filespec(two, 0);
 	lc_a = count_lines(one->data, one->size);
 	lc_b = count_lines(two->data, two->size);
-	printf("%s--- %s%s%s%s\n%s+++ %s%s%s%s\n%s@@ -",
-	       metainfo, o->a_prefix, name_a, name_a_tab, reset,
-	       metainfo, o->b_prefix, name_b, name_b_tab, reset, fraginfo);
+	printf("%s--- %s%s%s\n%s+++ %s%s%s\n%s@@ -",
+	       metainfo, a_name.buf, name_a_tab, reset,
+	       metainfo, b_name.buf, name_b_tab, reset, fraginfo);
 	print_line_count(lc_a);
 	printf(" +");
 	print_line_count(lc_b);

quote.c +16 -0

index 6986b44..d061626 100644
@@ -213,6 +213,22 @@ size_t quote_c_style(const char *name, struct strbuf *sb, FILE *fp, int nodq)
 	return quote_c_style_counted(name, -1, sb, fp, nodq);
 }
 
+void quote_two_c_style(struct strbuf *sb, const char *prefix, const char *path, int nodq)
+{
+	if (quote_c_style(prefix, NULL, NULL, 0) ||
+	    quote_c_style(path, NULL, NULL, 0)) {
+		if (!nodq)
+			strbuf_addch(sb, '"');
+		quote_c_style(prefix, sb, NULL, 1);
+		quote_c_style(path, sb, NULL, 1);
+		if (!nodq)
+			strbuf_addch(sb, '"');
+	} else {
+		strbuf_addstr(sb, prefix);
+		strbuf_addstr(sb, path);
+	}
+}
+
 void write_name_quoted(const char *name, FILE *fp, int terminator)
 {
 	if (terminator) {

quote.h +1 -0

index ab7596f..4da110e 100644
@@ -41,6 +41,7 @@ extern char *sq_dequote(char *);
 
 extern int unquote_c_style(struct strbuf *, const char *quoted, const char **endp);
 extern size_t quote_c_style(const char *name, struct strbuf *, FILE *, int no_dq);
+extern void quote_two_c_style(struct strbuf *, const char *, const char *, int);
 
 extern void write_name_quoted(const char *name, FILE *, int terminator);
 extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,

Signature · 1 line

1.5.4.rc1.23.g3a969