Re: textconv not invoked when viewing merge commit

Jeff King [off-list ref] writes:

On Thu, Apr 14, 2011 at 01:06:19PM -0700, Junio C Hamano wrote:

quoted

Instead, I think we should just use "Binary blob $SHA-1\n" as if that is
the textconv of a binary file without textconv filter.  That would
certainly make the code much simpler, and more importantly, the output
would become more pleasant. We would show something like:

    - Binary blob bc3c57058faba66f6a7a947e1e9642f47053b5bb
     -Binary blob 536e55524db72bd2acf175208aef4f3dfc148d42
    ++Binary blob 67cfeb2016b24df1cb406c18145efd399f6a1792

if we did so.

Yeah, I think that is pretty readable. But it gives me a funny feeling
to encode magic strings inside actual diff output. That is, the output
is indistinguishable from a file which contained the "Binary blob..."
strings.

I can't think of a case where it matters, though, so maybe it is just
paranoia.

We do something similar for textconv, of course, but we always knew that
was a human-only thing, and it isn't enabled for plumbing commands. This
would be.

Yeah, that may be a sensible concern.

If we really cared, I would say that plumbing should keep the current
behaviour (line-by-line even for binaries, and not using textconv unless
it is asked).  If the command line asked for --textconv, we can use that
"Binary blob $SHA-1" string as a fallback textconv result for binary blobs
that do not have any textconv filter configured.  So the additional logic
to convert the final image and parent images (two places to patch) would
become more like:

	if (if we are a Porcelain or --textconv option given) {
		if (path has textconv)
                	use textconv;
		else if (path is binary)
                	use "Binary blob $SHA-1";
	}

Having said all that, I don't think we made -c/--cc available to plumbing
on purpose; rather they happen to be available because we thought people
with common sense wouldn't run things like "diff-tree --c" that are meant
for human consumption and expect the result to be parsable by their
scripts. In other words, making the parser barf only for plumbing was not
worth doing.

On Thu, Apr 14, 2011 at 02:05:07PM -0700, Junio C Hamano wrote:

quoted

Yeah, I think that is pretty readable. But it gives me a funny feeling
to encode magic strings inside actual diff output. That is, the output
is indistinguishable from a file which contained the "Binary blob..."
strings.

[...]

Yeah, that may be a sensible concern.

If we really cared, I would say that plumbing should keep the current
behaviour (line-by-line even for binaries, and not using textconv unless
it is asked).

I disagree. Spewing binary contents in the middle of patch output is
wrong and a bug, and we should fix it. Not to mention that the results
are simply incomprehensible in many cases. Binary data isn't
line-oriented, and treating it that way is just going to produce
confusing and useless results. Not to mention that I wouldn't be
surprised if embedded NULs in the data are not being handled properly by
the diff code.

I would much rather have it say "Binary files differ". It's not that
informative, but at least you don't waste a lot of time trying to figure
out what in the world it means.

Having said all that, I don't think we made -c/--cc available to plumbing
on purpose; rather they happen to be available because we thought people
with common sense wouldn't run things like "diff-tree --c" that are meant
for human consumption and expect the result to be parsable by their
scripts. In other words, making the parser barf only for plumbing was not
worth doing.

Weren't they needed originally for "git rev-list | git diff-tree"? Maybe
they post-date the invention of actual C "git log"; I didn't look. At
any rate, they've been around for a while, and it is not unreasonable
for somebody to want to script around the generation of human-readable
output, so I think they are a good addition.

I think the real argument to be made is that "--cc" was never parseable,
because it can't be applied, and users of the format should know that. I
sort of buy that. Though you could also potentially do other kinds of
analysis on --cc output (e.g., something blame-ish but totally external
to git). And for that you wouldn't want to pretend content was there
that isn't. It's an edge case, certainly, but I don't see any reason not
to be conservative in what we generate. The "Binary files differ" type
of output is not that much harder to generate.

-Peff

Currently, we ignore textconv and binary status for the combined diff
formats (-c, -cc) which was never intended.

Change this so that combined diff uses the same helpers.

Signed-off-by: Michael J Gruber <redacted>
---
So, just so that I don't get the vapor patch award, here's a WIP passing
Jeff's test.

Before looking at free()ing what I've introduced and the binary issue I'll
check whether the whole blob/file read hunk in show_patch_diff() can't be
simply subsumed in the fill_textconv() call. It is almost a copy of
diff_populate_filespec() but not quite.

Also, the situation with worktree is even worse than I thought:

git diff -m produces a combined diff!

Also, my patch does not cure "diff -c" against worktree so far, I'm not
textconv'ing the worktree file yet. But then again, "diff -m" sucks here also.

I'll probably pick this up later today.
---
 combine-diff.c                 |   30 +++++++++---
 diff.h                         |    2 +
 t/t4046-diff-textconv-merge.sh |   97 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+), 8 deletions(-)
 create mode 100755 t/t4046-diff-textconv-merge.sh

diff --git a/combine-diff.c b/combine-diff.c
index 655fa89..8056fc3 100644
--- a/combine-diff.c
+++ b/combine-diff.c

@@ -8,7 +8,7 @@
 #include "log-tree.h"
 #include "refs.h"
 
-static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent)
+static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent, int textconv)
 {
 	struct diff_queue_struct *q = &diff_queued_diff;
 	struct combine_diff_path *p;

@@ -34,9 +34,13 @@ static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr,
 
 			hashcpy(p->sha1, q->queue[i]->two->sha1);
 			p->mode = q->queue[i]->two->mode;
+			if (textconv)
+				p->textconv = get_textconv(q->queue[i]->two);
 			hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
 			p->parent[n].mode = q->queue[i]->one->mode;
 			p->parent[n].status = q->queue[i]->status;
+			if (textconv)
+				p->parent[n].textconv = get_textconv(q->queue[i]->one);
 			*tail = p;
 			tail = &p->next;
 		}

@@ -60,6 +64,8 @@ static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr,
 				hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
 				p->parent[n].mode = q->queue[i]->one->mode;
 				p->parent[n].status = q->queue[i]->status;
+				if (textconv)
+					p->parent[n].textconv = get_textconv(q->queue[i]->one);
 				break;
 			}
 		}

@@ -201,8 +207,8 @@ static void consume_line(void *state_, char *line, unsigned long len)
 	}
 }
 
-static void combine_diff(const unsigned char *parent, unsigned int mode,
-			 mmfile_t *result_file,
+static void combine_diff(const char *path, const unsigned char *parent, unsigned int mode,
+			 struct userdiff_driver *textconv, mmfile_t *result_file,
 			 struct sline *sline, unsigned int cnt, int n,
 			 int num_parent, int result_deleted)
 {

@@ -212,13 +218,13 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,
 	xdemitconf_t xecfg;
 	mmfile_t parent_file;
 	struct combine_diff_state state;
-	unsigned long sz;
+	struct diff_filespec *df = alloc_filespec(path);
 
 	if (result_deleted)
 		return; /* result deleted */
 
-	parent_file.ptr = grab_blob(parent, mode, &sz);
-	parent_file.size = sz;
+	fill_filespec(df, parent, mode);
+	parent_file.size = fill_textconv(textconv, df, &parent_file.ptr);
 	memset(&xpp, 0, sizeof(xpp));
 	xpp.flags = 0;
 	memset(&xecfg, 0, sizeof(xecfg));

@@ -777,6 +783,12 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			close(fd);
 	}
 
+	if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && elem->textconv) {
+		struct diff_filespec *df = alloc_filespec(elem->path);
+		fill_filespec(df, elem->sha1, elem->mode);
+		result_size = fill_textconv(elem->textconv, df, &result);
+	}
+
 	for (cnt = 0, cp = result; cp < result + result_size; cp++) {
 		if (*cp == '\n')
 			cnt++;

@@ -821,8 +833,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			}
 		}
 		if (i <= j)
-			combine_diff(elem->parent[i].sha1,
+			combine_diff(elem->path,
+				     elem->parent[i].sha1,
 				     elem->parent[i].mode,
+				     elem->parent[i].textconv,
 				     &result_file, sline,
 				     cnt, i, num_parent, result_deleted);
 		if (elem->parent[i].mode != elem->mode)

@@ -1001,7 +1015,7 @@ void diff_tree_combined(const unsigned char *sha1,
 			diffopts.output_format = DIFF_FORMAT_NO_OUTPUT;
 		diff_tree_sha1(parent[i], sha1, "", &diffopts);
 		diffcore_std(&diffopts);
-		paths = intersect_paths(paths, i, num_parent);
+		paths = intersect_paths(paths, i, num_parent, DIFF_OPT_TST(opt, ALLOW_TEXTCONV));
 
 		if (show_log_first && i == 0) {
 			show_log(rev);

diff --git a/diff.h b/diff.h
index 007a055..4ca6b84 100644
--- a/diff.h
+++ b/diff.h

@@ -176,10 +176,12 @@ struct combine_diff_path {
 	char *path;
 	unsigned int mode;
 	unsigned char sha1[20];
+	struct userdiff_driver *textconv;
 	struct combine_diff_parent {
 		char status;
 		unsigned int mode;
 		unsigned char sha1[20];
+		struct userdiff_driver *textconv;
 	} parent[FLEX_ARRAY];
 };
 #define combine_diff_path_size(n, l) \

diff --git a/t/t4046-diff-textconv-merge.sh b/t/t4046-diff-textconv-merge.sh
new file mode 100755
index 0000000..8420bb6
--- /dev/null
+++ b/t/t4046-diff-textconv-merge.sh

@@ -0,0 +1,97 @@
+#!/bin/sh
+
+test_description='combined and merge diff uses textconv'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit one file &&
+	test_commit two file &&
+	git checkout -b other HEAD^ &&
+	test_commit three file &&
+	test_must_fail git merge master &&
+	echo resolved >file &&
+	echo "file diff=upcase" >.gitattributes &&
+	git config diff.upcase.textconv "tr a-z A-Z <"
+'
+
+cat >expect <<'EOF'
+diff --combined file
+index 2bdf67a,f719efd..0000000
+--- a/file
++++ b/file
+@@@ -1,1 -1,1 +1,1 @@@
+- THREE
+ -TWO
+++RESOLVED
+EOF
+test_expect_success 'diff -c uses textconv' '
+	git diff -c >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+diff --git a/file b/file
+index 2bdf67a..0000000 100644
+--- a/file
++++ b/file
+@@ -1 +1 @@
+-THREE
++RESOLVED
+
+diff --git a/file b/file
+index f719efd..0000000 100644
+--- a/file
++++ b/file
+@@ -1 +1 @@
+-TWO
++RESOLVED
+EOF
+test_expect_success 'diff -m uses textconv' '
+	git diff -m >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+Merge branch 'master' into other
+
+diff --combined file
+index 2bdf67a,f719efd..2ab19ae
+--- a/file
++++ b/file
+@@@ -1,1 -1,1 +1,1 @@@
+- THREE
+ -TWO
+++RESOLVED
+EOF
+test_expect_success 'show -c uses textconv' '
+	git commit -a &&
+	git show --format=%s -c >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+Merge branch 'master' into other
+
+diff --git a/file b/file
+index 2bdf67a..2ab19ae 100644
+--- a/file
++++ b/file
+@@ -1 +1 @@
+-THREE
++RESOLVED
+Merge branch 'master' into other
+
+diff --git a/file b/file
+index f719efd..2ab19ae 100644
+--- a/file
++++ b/file
+@@ -1 +1 @@
+-TWO
++RESOLVED
+EOF
+test_expect_success 'show -m uses textconv' '
+	git show --format=%s -m >actual &&
+	test_cmp expect actual
+'
+
+test_done

-- 
1.7.5.rc1.312.g1936c

On Fri, Apr 15, 2011 at 05:29:05PM +0200, Michael J Gruber wrote:

Currently, we ignore textconv and binary status for the combined diff
formats (-c, -cc) which was never intended.

Thanks for working on this.

I think it would be simpler to work on the binary half first. Then it
would be clear where the binary codepath diverges, and sticking the
textconv helpers in there would be easier (the helpers were, after all,
written because it was retrofitting existing diff code that already
handled binaries differently).

The whole grab_blob() thing seems like an unnecessary duplication of the
diff_filespec code. I think if we can switch to a more uniform use of
diff_filespec code, the memory management might end up simpler.

+	if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && elem->textconv) {
+		struct diff_filespec *df = alloc_filespec(elem->path);
+		fill_filespec(df, elem->sha1, elem->mode);
+		result_size = fill_textconv(elem->textconv, df, &result);
+	}

The memory management with fill_textconv is kind of ugly. Sometimes it
returns memory which must be freed, and sometimes not. Looking at the
diff.c code, I think in this case it will always need freed (because
elem->textconv is non-NULL). Sorry, that was a mess I created a long
time ago that you now get to deal with. :)

-Peff

On Freitag, 15. April 2011, Michael J Gruber wrote:

Currently, we ignore textconv and binary status for the combined diff
formats (-c, -cc) which was never intended.

Change this so that combined diff uses the same helpers.

Signed-off-by: Michael J Gruber <redacted>
---
So, just so that I don't get the vapor patch award, here's a WIP passing
Jeff's test.

Before looking at free()ing what I've introduced and the binary issue I'll
check whether the whole blob/file read hunk in show_patch_diff() can't be
simply subsumed in the fill_textconv() call. It is almost a copy of
diff_populate_filespec() but not quite.

Also, the situation with worktree is even worse than I thought:

git diff -m produces a combined diff!

Also, my patch does not cure "diff -c" against worktree so far, I'm not
textconv'ing the worktree file yet. But then again, "diff -m" sucks here also.

I'll probably pick this up later today.

Hi,

thanks for working on this.
I tried this patch on my msysgit system and now gitk shows a nice diff
for my merged archives. :-)
For merges of other binary files without textconf filter (jar)
i still get binary output.
(but i expected this from the notes above/discussion)

thanks,
Greetings Peter