Re: textconv not invoked when viewing merge commit

[Jeff King <peff@peff.net>]

On Tue, Apr 12, 2011 at 11:04:43AM +0200, Michael J Gruber wrote:

> > This works fine, except for merge commits.
> > For merge commits i see the diff of the binary contents of the file.
> > 
> > Is this intentional?
> > git help gitattributes mentions no such limitation.
> > Anywhere else(gitk(on non merge commit), git gui blame) i see the the filtered textual representation of the file.
> > 
> > I tried 1.7.4 msysgit and current master
> > 
> > Greetings Peter
> 
> textconv is applied for "diff -m" but not for combined diffs (-c, --cc)
> at the moment. They go through a completely different codepath, so it is
> expected code-wise (not a bug per se) but not ui-wise.
> 
> Looking at the code and trying to dig something up atm...

Ick. I started with this test:

diff --git a/t/t4046-diff-textconv-merge.sh b/t/t4046-diff-textconv-merge.sh
new file mode 100755
index 0000000..8643330
--- /dev/null
+++ b/t/t4046-diff-textconv-merge.sh
@@ -0,0 +1,35 @@
+#!/bin/sh
+
+test_description='combined 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 &&
+	git commit -a &&
+	echo "file diff=upcase" >.gitattributes &&
+	git config diff.upcase.textconv "tr a-z A-Z <"
+'
+
+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 'diff -c uses textconv' '
+	git show --format=%s -c >actual &&
+	test_cmp expect actual
+'
+
+test_done

but after looking at the codepath, it is must worse than just textconv.
Try this:

  git init repo &&
  cd repo &&
  openssl rand 64 >file.bin &&
  git add file.bin &&
  git commit -m one &&
  openssl rand 64 >file.bin &&
  git commit -a -m two &&
  git checkout -b other HEAD^
  openssl rand 64 >file.bin &&
  git commit -a -m three &&
  (git merge master || true) &&
  openssl rand 64 >file.bin &&
  git commit -a -m resolved &&
  git show

We just dump the binary goo all over the terminal. So I think the whole
combined-diff code path needs to learn how to handle binaries properly.

Unfortunately, it seems to be totally distinct from the regular diff
code path. It doesn't even use diff_filespecs, so our usual is_binary
and textconv code won't work. So the best way forward may involve
significant refactoring.

And of course we have to figure out sane semantics. The textconv case is
easy; just use the textconv blobs instead of the regular ones. But what
should the true binary case (as in the rand example above) show?

-Peff