Re: textconv not invoked when viewing merge commit
On Tue, Apr 12, 2011 at 11:04:43AM +0200, Michael J Gruber wrote:
Show 16 quoted lines
> > 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
Jeff King <peff@peff.net>