{"thread":{"id":"27062","subject":"textconv not invoked when viewing merge commit","startedAt":"2011-04-11T17:12:47Z","lastAt":"2011-04-21T16:08:05Z","messageCount":25,"participants":["Peter Oberndorfer","Michael J Gruber","Jeff King","Junio C Hamano","Matthieu Moy","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"165630","messageId":"201104111912.47547.kumbayo84@arcor.de","threadId":"27062","inReplyTo":null,"subject":"textconv not invoked when viewing merge commit","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2011-04-11T17:12:47Z","receivedAt":"2011-04-11T17:12:47Z","isPatch":false,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"Hi,\n\ni currently use a textconv filter to show contents of a zip like archive in a user readable format(as a list of contained files).\n\nThis works fine, except for merge commits.\nFor merge commits i see the diff of the binary contents of the file.\n\nIs this intentional?\ngit help gitattributes mentions no such limitation.\nAnywhere else(gitk(on non merge commit), git gui blame) i see the the filtered textual representation of the file.\n\nI tried 1.7.4 msysgit and current master\n\nGreetings Peter\n"},{"id":"165675","messageId":"4DA415AB.9020008@drmicha.warpmail.net","threadId":"27062","inReplyTo":"201104111912.47547.kumbayo84@arcor.de","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-04-12T09:04:43Z","receivedAt":"2011-04-12T09:04:43Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Peter Oberndorfer venit, vidit, dixit 11.04.2011 19:12:\n> Hi,\n> \n> i currently use a textconv filter to show contents of a zip like archive in a user readable format(as a list of contained files).\n> \n> This works fine, except for merge commits.\n> For merge commits i see the diff of the binary contents of the file.\n> \n> Is this intentional?\n> git help gitattributes mentions no such limitation.\n> Anywhere else(gitk(on non merge commit), git gui blame) i see the the filtered textual representation of the file.\n> \n> I tried 1.7.4 msysgit and current master\n> \n> Greetings Peter\n\ntextconv is applied for \"diff -m\" but not for combined diffs (-c, --cc)\nat the moment. They go through a completely different codepath, so it is\nexpected code-wise (not a bug per se) but not ui-wise.\n\nLooking at the code and trying to dig something up atm...\n\nMichael\n"},{"id":"165826","messageId":"20110414190901.GA1184@sigill.intra.peff.net","threadId":"27062","inReplyTo":"4DA415AB.9020008@drmicha.warpmail.net","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T19:09:01Z","receivedAt":"2011-04-14T19:09:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 12, 2011 at 11:04:43AM +0200, Michael J Gruber wrote:\n\n> > This works fine, except for merge commits.\n> > For merge commits i see the diff of the binary contents of the file.\n> > \n> > Is this intentional?\n> > git help gitattributes mentions no such limitation.\n> > Anywhere else(gitk(on non merge commit), git gui blame) i see the the filtered textual representation of the file.\n> > \n> > I tried 1.7.4 msysgit and current master\n> > \n> > Greetings Peter\n> \n> textconv is applied for \"diff -m\" but not for combined diffs (-c, --cc)\n> at the moment. They go through a completely different codepath, so it is\n> expected code-wise (not a bug per se) but not ui-wise.\n> \n> Looking at the code and trying to dig something up atm...\n\nIck. I started with this test:\n\ndiff --git a/t/t4046-diff-textconv-merge.sh b/t/t4046-diff-textconv-merge.sh\nnew file mode 100755\nindex 0000000..8643330\n--- /dev/null\n+++ b/t/t4046-diff-textconv-merge.sh\n@@ -0,0 +1,35 @@\n+#!/bin/sh\n+\n+test_description='combined diff uses textconv'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit one file &&\n+\ttest_commit two file &&\n+\tgit checkout -b other HEAD^ &&\n+\ttest_commit three file &&\n+\ttest_must_fail git merge master &&\n+\techo resolved >file &&\n+\tgit commit -a &&\n+\techo \"file diff=upcase\" >.gitattributes &&\n+\tgit config diff.upcase.textconv \"tr a-z A-Z <\"\n+'\n+\n+cat >expect <<'EOF'\n+Merge branch 'master' into other\n+\n+diff --combined file\n+index 2bdf67a,f719efd..2ab19ae\n+--- a/file\n++++ b/file\n+@@@ -1,1 -1,1 +1,1 @@@\n+- THREE\n+ -TWO\n+++RESOLVED\n+EOF\n+test_expect_success 'diff -c uses textconv' '\n+\tgit show --format=%s -c >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n\nbut after looking at the codepath, it is must worse than just textconv.\nTry this:\n\n  git init repo &&\n  cd repo &&\n  openssl rand 64 >file.bin &&\n  git add file.bin &&\n  git commit -m one &&\n  openssl rand 64 >file.bin &&\n  git commit -a -m two &&\n  git checkout -b other HEAD^\n  openssl rand 64 >file.bin &&\n  git commit -a -m three &&\n  (git merge master || true) &&\n  openssl rand 64 >file.bin &&\n  git commit -a -m resolved &&\n  git show\n\nWe just dump the binary goo all over the terminal. So I think the whole\ncombined-diff code path needs to learn how to handle binaries properly.\n\nUnfortunately, it seems to be totally distinct from the regular diff\ncode path. It doesn't even use diff_filespecs, so our usual is_binary\nand textconv code won't work. So the best way forward may involve\nsignificant refactoring.\n\nAnd of course we have to figure out sane semantics. The textconv case is\neasy; just use the textconv blobs instead of the regular ones. But what\nshould the true binary case (as in the rand example above) show?\n\n-Peff\n"},{"id":"165828","messageId":"20110414191522.GA4862@sigill.intra.peff.net","threadId":"27062","inReplyTo":"20110414190901.GA1184@sigill.intra.peff.net","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T19:15:22Z","receivedAt":"2011-04-14T19:15:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2011 at 03:09:01PM -0400, Jeff King wrote:\n\n> but after looking at the codepath, it is must worse than just textconv.\n> Try this:\n\nUgh, that should be \"much worse\" of course.\n\n-Peff\n"},{"id":"165829","messageId":"7vipughbxh.fsf@alter.siamese.dyndns.org","threadId":"27062","inReplyTo":"20110414190901.GA1184@sigill.intra.peff.net","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-14T19:26:18Z","receivedAt":"2011-04-14T19:26:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We just dump the binary goo all over the terminal. So I think the whole\n> combined-diff code path needs to learn how to handle binaries properly.\n\nHow would you show multi-way diffs for binary files?\n\nIt would probably be sufficient to say \"binary files differ\" at the\nbeginning of the patch-combining codepath of the combined diff, which\nwould at least keep the --raw -c/--cc output working.\n"},{"id":"165830","messageId":"20110414192839.GA6001@sigill.intra.peff.net","threadId":"27062","inReplyTo":"7vipughbxh.fsf@alter.siamese.dyndns.org","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T19:28:39Z","receivedAt":"2011-04-14T19:28:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2011 at 12:26:18PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > We just dump the binary goo all over the terminal. So I think the whole\n> > combined-diff code path needs to learn how to handle binaries properly.\n> \n> How would you show multi-way diffs for binary files?\n\nNo clue. But anything would be better than pretending it's line oriented\nand dumping binary goo to the terminal.\n\n> It would probably be sufficient to say \"binary files differ\" at the\n> beginning of the patch-combining codepath of the combined diff, which\n> would at least keep the --raw -c/--cc output working.\n\nYeah, something like \"binary files differ\" would probably be OK for\n\"-c\". I think for \"--cc\", that is probably the best we can do, too. It\nis about condensing uninteresting hunks, but we don't even have the\nconcept of hunks.\n\n-Peff\n"},{"id":"165832","messageId":"4DA74C8C.509@drmicha.warpmail.net","threadId":"27062","inReplyTo":"20110414192839.GA6001@sigill.intra.peff.net","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-04-14T19:35:40Z","receivedAt":"2011-04-14T19:35:40Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 14.04.2011 21:28:\n> On Thu, Apr 14, 2011 at 12:26:18PM -0700, Junio C Hamano wrote:\n> \n>> Jeff King <peff@peff.net> writes:\n>>\n>>> We just dump the binary goo all over the terminal. So I think the whole\n>>> combined-diff code path needs to learn how to handle binaries properly.\n>>\n>> How would you show multi-way diffs for binary files?\n> \n> No clue. But anything would be better than pretending it's line oriented\n> and dumping binary goo to the terminal.\n> \n>> It would probably be sufficient to say \"binary files differ\" at the\n>> beginning of the patch-combining codepath of the combined diff, which\n>> would at least keep the --raw -c/--cc output working.\n> \n> Yeah, something like \"binary files differ\" would probably be OK for\n> \"-c\". I think for \"--cc\", that is probably the best we can do, too. It\n> is about condensing uninteresting hunks, but we don't even have the\n> concept of hunks.\n> \n> -Peff\n\nI have \"one half\" of a partial fix cooking which applies textconv. Good\nto see that I'm not the only who is surprised by the disjointness of\ncodepaths. That should give me some freedom to go wild in\ncombine-diff.c... But it's rc-time, we're devoting all git time to\nregression fixes and cleanups, right?\n\nMichael\n"},{"id":"165834","messageId":"7vd3kohb5n.fsf@alter.siamese.dyndns.org","threadId":"27062","inReplyTo":"7vipughbxh.fsf@alter.siamese.dyndns.org","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-14T19:43:00Z","receivedAt":"2011-04-14T19:43:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> We just dump the binary goo all over the terminal. So I think the whole\n>> combined-diff code path needs to learn how to handle binaries properly.\n>\n> How would you show multi-way diffs for binary files?\n>\n> It would probably be sufficient to say \"binary files differ\" at the\n> beginning of the patch-combining codepath of the combined diff, which\n> would at least keep the --raw -c/--cc output working.\n\nIn other words, I suspect that the only places you need to touch in the\nexisting codepath would be these places.\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 655fa89..9c96f1f 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -201,7 +201,7 @@ static void consume_line(void *state_, char *line, unsigned long len)\n \t}\n }\n \n-static void combine_diff(const unsigned char *parent, unsigned int mode,\n+static int combine_diff(const unsigned char *parent, unsigned int mode,\n \t\t\t mmfile_t *result_file,\n \t\t\t struct sline *sline, unsigned int cnt, int n,\n \t\t\t int num_parent, int result_deleted)\n@@ -215,10 +215,17 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,\n \tunsigned long sz;\n \n \tif (result_deleted)\n-\t\treturn; /* result deleted */\n+\t\treturn 0; /* result deleted */\n \n \tparent_file.ptr = grab_blob(parent, mode, &sz);\n \tparent_file.size = sz;\n+\n+\tif (path has textconv) {\n+\t\tparent_file.{ptr,size} = textconv of parent_file;\n+\t} else if (path is binary) {\n+\t\treturn -1;\n+\t}\n+\n \tmemset(&xpp, 0, sizeof(xpp));\n \txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n@@ -255,6 +262,7 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,\n \t\t\tp_lno++; /* no '+' means parent had it */\n \t}\n \tsline[lno].p_lno[n] = p_lno; /* trailer */\n+\treturn 0;\n }\n \n static unsigned long context = 3;\n@@ -777,6 +785,12 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tclose(fd);\n \t}\n \n+\tif (path has textconv) {\n+\t\tresult, result_size = textconv of result;\n+\t} else if (path is binary) {\n+\t\tgoto exit_binary;\n+\t}\n+\n \tfor (cnt = 0, cp = result; cp < result + result_size; cp++) {\n \t\tif (*cp == '\\n')\n \t\t\tcnt++;\n@@ -820,11 +834,13 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n-\t\tif (i <= j)\n-\t\t\tcombine_diff(elem->parent[i].sha1,\n-\t\t\t\t     elem->parent[i].mode,\n-\t\t\t\t     &result_file, sline,\n-\t\t\t\t     cnt, i, num_parent, result_deleted);\n+\t\tif (i <= j) {\n+\t\t\tif (combine_diff(elem->parent[i].sha1,\n+\t\t\t\t\t elem->parent[i].mode,\n+\t\t\t\t\t &result_file, sline,\n+\t\t\t\t\t cnt, i, num_parent, result_deleted))\n+\t\t\t\tgoto exit_binary;\n+\t\t}\n \t\tif (elem->parent[i].mode != elem->mode)\n \t\t\tmode_differs = 1;\n \t}\n@@ -892,6 +908,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\tdump_sline(sline, cnt, num_parent,\n \t\t\t   DIFF_OPT_TST(opt, COLOR_DIFF), result_deleted);\n \t}\n+\n+free_exit:\n \tfree(result);\n \n \tfor (lno = 0; lno < cnt; lno++) {\n@@ -906,6 +924,11 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t}\n \tfree(sline[0].p_lno);\n \tfree(sline);\n+\treturn;\n+\n+exit_binary:\n+\tprintf(\"path '%s' is binary\\n\", elem->path);\n+\tgoto free_exit;\n }\n \n #define COLONS \"::::::::::::::::::::::::::::::::\"\n"},{"id":"165835","messageId":"7v8vvcha2s.fsf@alter.siamese.dyndns.org","threadId":"27062","inReplyTo":"7vd3kohb5n.fsf@alter.siamese.dyndns.org","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-14T20:06:19Z","receivedAt":"2011-04-14T20:06:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Jeff King <peff@peff.net> writes:\n>>\n>>> We just dump the binary goo all over the terminal. So I think the whole\n>>> combined-diff code path needs to learn how to handle binaries properly.\n>>\n>> How would you show multi-way diffs for binary files?\n>>\n>> It would probably be sufficient to say \"binary files differ\" at the\n>> beginning of the patch-combining codepath of the combined diff, which\n>> would at least keep the --raw -c/--cc output working.\n>\n> In other words, I suspect that the only places you need to touch in the\n> existing codepath would be these places.\n\n\nIn the \"here are the places\" patch, I changed combine_diff() to return \"is\nthis binary?\" and made show_patch_diff() to give just a single \"path is\nbinary\", but I suspect that it would be simpler not try to be too nice\nabout binary like that.\n\nInstead, I think we should just use \"Binary blob $SHA-1\\n\" as if that is\nthe textconv of a binary file without textconv filter.  That would\ncertainly make the code much simpler, and more importantly, the output\nwould become more pleasant. We would show something like:\n\n    - Binary blob bc3c57058faba66f6a7a947e1e9642f47053b5bb\n     -Binary blob 536e55524db72bd2acf175208aef4f3dfc148d42\n    ++Binary blob 67cfeb2016b24df1cb406c18145efd399f6a1792\n\nif we did so.\n\nWhen showing the working-tree version, we obviously do not have the blob\nobject name yet, so in such a case, we can say \"Binary blob\", or just\n\"Binary\".\n"},{"id":"165838","messageId":"20110414202356.GB6525@sigill.intra.peff.net","threadId":"27062","inReplyTo":"7v8vvcha2s.fsf@alter.siamese.dyndns.org","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T20:23:56Z","receivedAt":"2011-04-14T20:23:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2011 at 01:06:19PM -0700, Junio C Hamano wrote:\n\n> Instead, I think we should just use \"Binary blob $SHA-1\\n\" as if that is\n> the textconv of a binary file without textconv filter.  That would\n> certainly make the code much simpler, and more importantly, the output\n> would become more pleasant. We would show something like:\n> \n>     - Binary blob bc3c57058faba66f6a7a947e1e9642f47053b5bb\n>      -Binary blob 536e55524db72bd2acf175208aef4f3dfc148d42\n>     ++Binary blob 67cfeb2016b24df1cb406c18145efd399f6a1792\n> \n> if we did so.\n\nYeah, I think that is pretty readable. But it gives me a funny feeling\nto encode magic strings inside actual diff output. That is, the output\nis indistinguishable from a file which contained the \"Binary blob...\"\nstrings.\n\nI can't think of a case where it matters, though, so maybe it is just\nparanoia.\n\nWe do something similar for textconv, of course, but we always knew that\nwas a human-only thing, and it isn't enabled for plumbing commands. This\nwould be.\n\n-Peff\n"},{"id":"165843","messageId":"7vwriwfssc.fsf@alter.siamese.dyndns.org","threadId":"27062","inReplyTo":"20110414202356.GB6525@sigill.intra.peff.net","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-14T21:05:07Z","receivedAt":"2011-04-14T21:05:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Apr 14, 2011 at 01:06:19PM -0700, Junio C Hamano wrote:\n>\n>> Instead, I think we should just use \"Binary blob $SHA-1\\n\" as if that is\n>> the textconv of a binary file without textconv filter.  That would\n>> certainly make the code much simpler, and more importantly, the output\n>> would become more pleasant. We would show something like:\n>> \n>>     - Binary blob bc3c57058faba66f6a7a947e1e9642f47053b5bb\n>>      -Binary blob 536e55524db72bd2acf175208aef4f3dfc148d42\n>>     ++Binary blob 67cfeb2016b24df1cb406c18145efd399f6a1792\n>> \n>> if we did so.\n>\n> Yeah, I think that is pretty readable. But it gives me a funny feeling\n> to encode magic strings inside actual diff output. That is, the output\n> is indistinguishable from a file which contained the \"Binary blob...\"\n> strings.\n>\n> I can't think of a case where it matters, though, so maybe it is just\n> paranoia.\n>\n> We do something similar for textconv, of course, but we always knew that\n> was a human-only thing, and it isn't enabled for plumbing commands. This\n> would be.\n\nYeah, that may be a sensible concern.\n\nIf we really cared, I would say that plumbing should keep the current\nbehaviour (line-by-line even for binaries, and not using textconv unless\nit is asked).  If the command line asked for --textconv, we can use that\n\"Binary blob $SHA-1\" string as a fallback textconv result for binary blobs\nthat do not have any textconv filter configured.  So the additional logic\nto convert the final image and parent images (two places to patch) would\nbecome more like:\n\n\tif (if we are a Porcelain or --textconv option given) {\n\t\tif (path has textconv)\n                \tuse textconv;\n\t\telse if (path is binary)\n                \tuse \"Binary blob $SHA-1\";\n\t}\n\nHaving said all that, I don't think we made -c/--cc available to plumbing\non purpose; rather they happen to be available because we thought people\nwith common sense wouldn't run things like \"diff-tree --c\" that are meant\nfor human consumption and expect the result to be parsable by their\nscripts. In other words, making the parser barf only for plumbing was not\nworth doing.\n"},{"id":"165852","messageId":"20110414213006.GA7709@sigill.intra.peff.net","threadId":"27062","inReplyTo":"7vwriwfssc.fsf@alter.siamese.dyndns.org","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T21:30:06Z","receivedAt":"2011-04-14T21:30:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2011 at 02:05:07PM -0700, Junio C Hamano wrote:\n\n> > Yeah, I think that is pretty readable. But it gives me a funny feeling\n> > to encode magic strings inside actual diff output. That is, the output\n> > is indistinguishable from a file which contained the \"Binary blob...\"\n> > strings.\n>[...]\n> \n> Yeah, that may be a sensible concern.\n> \n> If we really cared, I would say that plumbing should keep the current\n> behaviour (line-by-line even for binaries, and not using textconv unless\n> it is asked).\n\nI disagree. Spewing binary contents in the middle of patch output is\nwrong and a bug, and we should fix it. Not to mention that the results\nare simply incomprehensible in many cases. Binary data isn't\nline-oriented, and treating it that way is just going to produce\nconfusing and useless results. Not to mention that I wouldn't be\nsurprised if embedded NULs in the data are not being handled properly by\nthe diff code.\n\nI would much rather have it say \"Binary files differ\". It's not that\ninformative, but at least you don't waste a lot of time trying to figure\nout what in the world it means.\n\n> Having said all that, I don't think we made -c/--cc available to plumbing\n> on purpose; rather they happen to be available because we thought people\n> with common sense wouldn't run things like \"diff-tree --c\" that are meant\n> for human consumption and expect the result to be parsable by their\n> scripts. In other words, making the parser barf only for plumbing was not\n> worth doing.\n\nWeren't they needed originally for \"git rev-list | git diff-tree\"? Maybe\nthey post-date the invention of actual C \"git log\"; I didn't look. At\nany rate, they've been around for a while, and it is not unreasonable\nfor somebody to want to script around the generation of human-readable\noutput, so I think they are a good addition.\n\nI think the real argument to be made is that \"--cc\" was never parseable,\nbecause it can't be applied, and users of the format should know that. I\nsort of buy that. Though you could also potentially do other kinds of\nanalysis on --cc output (e.g., something blame-ish but totally external\nto git). And for that you wouldn't want to pretend content was there\nthat isn't. It's an edge case, certainly, but I don't see any reason not\nto be conservative in what we generate. The \"Binary files differ\" type\nof output is not that much harder to generate.\n\n-Peff\n"},{"id":"165882","messageId":"vpq62qg3sxy.fsf@bauges.imag.fr","threadId":"27062","inReplyTo":"20110414202356.GB6525@sigill.intra.peff.net","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-04-15T06:54:49Z","receivedAt":"2011-04-15T06:54:49Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Apr 14, 2011 at 01:06:19PM -0700, Junio C Hamano wrote:\n>\n>> Instead, I think we should just use \"Binary blob $SHA-1\\n\" as if that is\n>> the textconv of a binary file without textconv filter.  That would\n>> certainly make the code much simpler, and more importantly, the output\n>> would become more pleasant. We would show something like:\n>> \n>>     - Binary blob bc3c57058faba66f6a7a947e1e9642f47053b5bb\n>>      -Binary blob 536e55524db72bd2acf175208aef4f3dfc148d42\n>>     ++Binary blob 67cfeb2016b24df1cb406c18145efd399f6a1792\n>> \n>> if we did so.\n>\n> Yeah, I think that is pretty readable. But it gives me a funny feeling\n> to encode magic strings inside actual diff output. That is, the output\n> is indistinguishable from a file which contained the \"Binary blob...\"\n> strings.\n>\n> I can't think of a case where it matters, though, so maybe it is just\n> paranoia.\n\nA line-counting, statistics tool would think that 1 line has been\nremoved from both branches, and one new added by the merge.\n\nWell, I know no tool parsing combined diff actually, so it's indeed a\nhypothetical case.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"165904","messageId":"36a715a966a22207135f60532e723f6d87dd1ffb.1302881295.git.git@drmicha.warpmail.net","threadId":"27062","inReplyTo":"20110414213006.GA7709@sigill.intra.peff.net","subject":"[PATCH] combine-diff: use textconv for combined diff format","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-04-15T15:29:05Z","receivedAt":"2011-04-15T15:29:05Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, we ignore textconv and binary status for the combined diff\nformats (-c, -cc) which was never intended.\n\nChange this so that combined diff uses the same helpers.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\nSo, just so that I don't get the vapor patch award, here's a WIP passing\nJeff's test.\n\nBefore looking at free()ing what I've introduced and the binary issue I'll\ncheck whether the whole blob/file read hunk in show_patch_diff() can't be\nsimply subsumed in the fill_textconv() call. It is almost a copy of\ndiff_populate_filespec() but not quite.\n\nAlso, the situation with worktree is even worse than I thought:\n\ngit diff -m produces a combined diff!\n\nAlso, my patch does not cure \"diff -c\" against worktree so far, I'm not\ntextconv'ing the worktree file yet. But then again, \"diff -m\" sucks here also.\n\nI'll probably pick this up later today.\n---\n combine-diff.c                 |   30 +++++++++---\n diff.h                         |    2 +\n t/t4046-diff-textconv-merge.sh |   97 ++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 121 insertions(+), 8 deletions(-)\n create mode 100755 t/t4046-diff-textconv-merge.sh\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 655fa89..8056fc3 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -8,7 +8,7 @@\n #include \"log-tree.h\"\n #include \"refs.h\"\n \n-static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent)\n+static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent, int textconv)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tstruct combine_diff_path *p;\n@@ -34,9 +34,13 @@ static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr,\n \n \t\t\thashcpy(p->sha1, q->queue[i]->two->sha1);\n \t\t\tp->mode = q->queue[i]->two->mode;\n+\t\t\tif (textconv)\n+\t\t\t\tp->textconv = get_textconv(q->queue[i]->two);\n \t\t\thashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);\n \t\t\tp->parent[n].mode = q->queue[i]->one->mode;\n \t\t\tp->parent[n].status = q->queue[i]->status;\n+\t\t\tif (textconv)\n+\t\t\t\tp->parent[n].textconv = get_textconv(q->queue[i]->one);\n \t\t\t*tail = p;\n \t\t\ttail = &p->next;\n \t\t}\n@@ -60,6 +64,8 @@ static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr,\n \t\t\t\thashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);\n \t\t\t\tp->parent[n].mode = q->queue[i]->one->mode;\n \t\t\t\tp->parent[n].status = q->queue[i]->status;\n+\t\t\t\tif (textconv)\n+\t\t\t\t\tp->parent[n].textconv = get_textconv(q->queue[i]->one);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n@@ -201,8 +207,8 @@ static void consume_line(void *state_, char *line, unsigned long len)\n \t}\n }\n \n-static void combine_diff(const unsigned char *parent, unsigned int mode,\n-\t\t\t mmfile_t *result_file,\n+static void combine_diff(const char *path, const unsigned char *parent, unsigned int mode,\n+\t\t\t struct userdiff_driver *textconv, mmfile_t *result_file,\n \t\t\t struct sline *sline, unsigned int cnt, int n,\n \t\t\t int num_parent, int result_deleted)\n {\n@@ -212,13 +218,13 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,\n \txdemitconf_t xecfg;\n \tmmfile_t parent_file;\n \tstruct combine_diff_state state;\n-\tunsigned long sz;\n+\tstruct diff_filespec *df = alloc_filespec(path);\n \n \tif (result_deleted)\n \t\treturn; /* result deleted */\n \n-\tparent_file.ptr = grab_blob(parent, mode, &sz);\n-\tparent_file.size = sz;\n+\tfill_filespec(df, parent, mode);\n+\tparent_file.size = fill_textconv(textconv, df, &parent_file.ptr);\n \tmemset(&xpp, 0, sizeof(xpp));\n \txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n@@ -777,6 +783,12 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tclose(fd);\n \t}\n \n+\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && elem->textconv) {\n+\t\tstruct diff_filespec *df = alloc_filespec(elem->path);\n+\t\tfill_filespec(df, elem->sha1, elem->mode);\n+\t\tresult_size = fill_textconv(elem->textconv, df, &result);\n+\t}\n+\n \tfor (cnt = 0, cp = result; cp < result + result_size; cp++) {\n \t\tif (*cp == '\\n')\n \t\t\tcnt++;\n@@ -821,8 +833,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t}\n \t\t}\n \t\tif (i <= j)\n-\t\t\tcombine_diff(elem->parent[i].sha1,\n+\t\t\tcombine_diff(elem->path,\n+\t\t\t\t     elem->parent[i].sha1,\n \t\t\t\t     elem->parent[i].mode,\n+\t\t\t\t     elem->parent[i].textconv,\n \t\t\t\t     &result_file, sline,\n \t\t\t\t     cnt, i, num_parent, result_deleted);\n \t\tif (elem->parent[i].mode != elem->mode)\n@@ -1001,7 +1015,7 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\t\tdiffopts.output_format = DIFF_FORMAT_NO_OUTPUT;\n \t\tdiff_tree_sha1(parent[i], sha1, \"\", &diffopts);\n \t\tdiffcore_std(&diffopts);\n-\t\tpaths = intersect_paths(paths, i, num_parent);\n+\t\tpaths = intersect_paths(paths, i, num_parent, DIFF_OPT_TST(opt, ALLOW_TEXTCONV));\n \n \t\tif (show_log_first && i == 0) {\n \t\t\tshow_log(rev);\ndiff --git a/diff.h b/diff.h\nindex 007a055..4ca6b84 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -176,10 +176,12 @@ struct combine_diff_path {\n \tchar *path;\n \tunsigned int mode;\n \tunsigned char sha1[20];\n+\tstruct userdiff_driver *textconv;\n \tstruct combine_diff_parent {\n \t\tchar status;\n \t\tunsigned int mode;\n \t\tunsigned char sha1[20];\n+\t\tstruct userdiff_driver *textconv;\n \t} parent[FLEX_ARRAY];\n };\n #define combine_diff_path_size(n, l) \\\ndiff --git a/t/t4046-diff-textconv-merge.sh b/t/t4046-diff-textconv-merge.sh\nnew file mode 100755\nindex 0000000..8420bb6\n--- /dev/null\n+++ b/t/t4046-diff-textconv-merge.sh\n@@ -0,0 +1,97 @@\n+#!/bin/sh\n+\n+test_description='combined and merge diff uses textconv'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit one file &&\n+\ttest_commit two file &&\n+\tgit checkout -b other HEAD^ &&\n+\ttest_commit three file &&\n+\ttest_must_fail git merge master &&\n+\techo resolved >file &&\n+\techo \"file diff=upcase\" >.gitattributes &&\n+\tgit config diff.upcase.textconv \"tr a-z A-Z <\"\n+'\n+\n+cat >expect <<'EOF'\n+diff --combined file\n+index 2bdf67a,f719efd..0000000\n+--- a/file\n++++ b/file\n+@@@ -1,1 -1,1 +1,1 @@@\n+- THREE\n+ -TWO\n+++RESOLVED\n+EOF\n+test_expect_success 'diff -c uses textconv' '\n+\tgit diff -c >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+diff --git a/file b/file\n+index 2bdf67a..0000000 100644\n+--- a/file\n++++ b/file\n+@@ -1 +1 @@\n+-THREE\n++RESOLVED\n+\n+diff --git a/file b/file\n+index f719efd..0000000 100644\n+--- a/file\n++++ b/file\n+@@ -1 +1 @@\n+-TWO\n++RESOLVED\n+EOF\n+test_expect_success 'diff -m uses textconv' '\n+\tgit diff -m >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+Merge branch 'master' into other\n+\n+diff --combined file\n+index 2bdf67a,f719efd..2ab19ae\n+--- a/file\n++++ b/file\n+@@@ -1,1 -1,1 +1,1 @@@\n+- THREE\n+ -TWO\n+++RESOLVED\n+EOF\n+test_expect_success 'show -c uses textconv' '\n+\tgit commit -a &&\n+\tgit show --format=%s -c >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+Merge branch 'master' into other\n+\n+diff --git a/file b/file\n+index 2bdf67a..2ab19ae 100644\n+--- a/file\n++++ b/file\n+@@ -1 +1 @@\n+-THREE\n++RESOLVED\n+Merge branch 'master' into other\n+\n+diff --git a/file b/file\n+index f719efd..2ab19ae 100644\n+--- a/file\n++++ b/file\n+@@ -1 +1 @@\n+-TWO\n++RESOLVED\n+EOF\n+test_expect_success 'show -m uses textconv' '\n+\tgit show --format=%s -m >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.7.5.rc1.312.g1936c\n"},{"id":"165919","messageId":"7voc47cqj0.fsf@alter.siamese.dyndns.org","threadId":"27062","inReplyTo":"36a715a966a22207135f60532e723f6d87dd1ffb.1302881295.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] combine-diff: use textconv for combined diff format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-15T18:34:27Z","receivedAt":"2011-04-15T18:34:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> git diff -m produces a combined diff!\n\nHmm, what is the rest of your command line?  I thought -m was a way to ask\npairwise diff with each parent.\n\n> +static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent, int textconv)\n>  {\n>  \tstruct diff_queue_struct *q = &diff_queued_diff;\n>  \tstruct combine_diff_path *p;\n> @@ -34,9 +34,13 @@ static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr,\n>  \n>  \t\t\thashcpy(p->sha1, q->queue[i]->two->sha1);\n>  \t\t\tp->mode = q->queue[i]->two->mode;\n> +\t\t\tif (textconv)\n> +\t\t\t\tp->textconv = get_textconv(q->queue[i]->two);\n>  \t\t\thashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);\n>  \t\t\tp->parent[n].mode = q->queue[i]->one->mode;\n>  \t\t\tp->parent[n].status = q->queue[i]->status;\n> +\t\t\tif (textconv)\n> +\t\t\t\tp->parent[n].textconv = get_textconv(q->queue[i]->one);\n\nThis code attempts to handle different textconv set for each different\nparents.  But I have to wonder if that is really worth it.\n\nThe attribute to decide the content type of the blob is read from the same\nset of .gitattributes files, regardless of which parent you are looking at\n(and this is not likely to change---the exact procedure that is applied\ncomes from .git/config that is not even versioned, so there is not much\npoint in reading from the .gitattributes from the parent tree, trying to\nbe \"precise\").\n\nIf q->queue[i] is not a rename, p->textconv and p->parent[n].textconv\nwould be the same because one and two came from the same path.  If it is a\nrename, they by definition consist of similar contents, and the user would\nwant the same textconv conversion applied to them to make them comparable.\nEven though using p->parent[n].textconv to convert q->queue[i]->one->sha1\nblob and using p->textconv to convert q->queue[i]->two->sha1 blob might be\nthe right thing to do in theory, doing so wouldn't make a difference in\npractice.  More importantly, even if the two textconvs specify different\nconversions, it is likely that it is an user error (e.g. the preimage had\n\"img4433.jqg\" that was renamed to img4433.jpg\" in the postimage, and the\nattributes mechanism does not say \".jqg\" is a JPEG that wants to get\n\"exif\" run to be texualized for the purpose of diffing, or something).\n\nBesides, if you really want to support \"left hand side and right hand\nside, depending on which parent we are talking about, may use different\ntextconv\", you would need to defeat the optimization in show_patch_diff()\nthat calls reuse_combine_diff() when sha1 are the same from other parent\nwe have already compared---the parent we are looking at may be using a\ndifferent textconv procedure.  Even worse, if parent and child have the\nsame sha1, the result of running parent textconv on the parent blob may be\ndifferent from that of the child, which you would never even see in this\ncodepath.\n\nSo I suspect that using only one textconv per \"struct combine_diff_path\"\nwould make both the code simpler, and more importantly would make the\nresult more correct from the end user's point of view.\n\n> @@ -777,6 +783,12 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n>  \t\t\tclose(fd);\n>  \t}\n>  \n> +\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && elem->textconv) {\n> +\t\tstruct diff_filespec *df = alloc_filespec(elem->path);\n> +\t\tfill_filespec(df, elem->sha1, elem->mode);\n> +\t\tresult_size = fill_textconv(elem->textconv, df, &result);\n> +\t}\n\nI suspect that these three lines have to become a small helper function to\nbe used to convert the final blob (done here), and parent blob (done in\ncombine_diff).  With the \"binary\" support, it would eventually need to be\nenhanced to something like:\n\n\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV)) {\n        \tif (textconv) {\n                \tdo these three lines;\n\t\t} else if (is binary) {\n                \t\"Binary blob $SHA-1\";\n\t\t}\n\t}\n\nand having a small helper function early in the series would help that\nprocess.\n\n> +\t\tpaths = intersect_paths(paths, i, num_parent, DIFF_OPT_TST(opt, ALLOW_TEXTCONV));\n\nAs an internal API within this file, I would rather see \"opt\" as a whole\npassed to intersect_paths().  We may probably want to determine if the\nblob is binary in that function depending on other \"opt\" fields.\n"},{"id":"165932","messageId":"7v7havckgg.fsf@alter.siamese.dyndns.org","threadId":"27062","inReplyTo":"vpq62qg3sxy.fsf@bauges.imag.fr","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-15T20:45:35Z","receivedAt":"2011-04-15T20:45:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n>> I can't think of a case where it matters, though, so maybe it is just\n>> paranoia.\n>\n> A line-counting, statistics tool would think that 1 line has been\n> removed from both branches, and one new added by the merge.\n>\n> Well, I know no tool parsing combined diff actually, so it's indeed a\n> hypothetical case.\n\nAnd the ones that have been parsing cdiff wouldn't have done anything good\nbefore this change on such a binary blob anyway, no?\n"},{"id":"165939","messageId":"20110415235628.GA9334@sigill.intra.peff.net","threadId":"27062","inReplyTo":"36a715a966a22207135f60532e723f6d87dd1ffb.1302881295.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] combine-diff: use textconv for combined diff format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-15T23:56:28Z","receivedAt":"2011-04-15T23:56:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 15, 2011 at 05:29:05PM +0200, Michael J Gruber wrote:\n\n> Currently, we ignore textconv and binary status for the combined diff\n> formats (-c, -cc) which was never intended.\n\nThanks for working on this.\n\nI think it would be simpler to work on the binary half first. Then it\nwould be clear where the binary codepath diverges, and sticking the\ntextconv helpers in there would be easier (the helpers were, after all,\nwritten because it was retrofitting existing diff code that already\nhandled binaries differently).\n\nThe whole grab_blob() thing seems like an unnecessary duplication of the\ndiff_filespec code. I think if we can switch to a more uniform use of\ndiff_filespec code, the memory management might end up simpler.\n\n> +\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && elem->textconv) {\n> +\t\tstruct diff_filespec *df = alloc_filespec(elem->path);\n> +\t\tfill_filespec(df, elem->sha1, elem->mode);\n> +\t\tresult_size = fill_textconv(elem->textconv, df, &result);\n> +\t}\n\nThe memory management with fill_textconv is kind of ugly. Sometimes it\nreturns memory which must be freed, and sometimes not. Looking at the\ndiff.c code, I think in this case it will always need freed (because\nelem->textconv is non-NULL). Sorry, that was a mess I created a long\ntime ago that you now get to deal with. :)\n\n-Peff\n"},{"id":"165945","messageId":"20110416014758.GB23306@sigill.intra.peff.net","threadId":"27062","inReplyTo":"7v7havckgg.fsf@alter.siamese.dyndns.org","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-16T01:47:59Z","receivedAt":"2011-04-16T01:47:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 15, 2011 at 01:45:35PM -0700, Junio C Hamano wrote:\n\n> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n> \n> >> I can't think of a case where it matters, though, so maybe it is just\n> >> paranoia.\n> >\n> > A line-counting, statistics tool would think that 1 line has been\n> > removed from both branches, and one new added by the merge.\n> >\n> > Well, I know no tool parsing combined diff actually, so it's indeed a\n> > hypothetical case.\n> \n> And the ones that have been parsing cdiff wouldn't have done anything good\n> before this change on such a binary blob anyway, no?\n\nNo, but we can view the proposed change as fixing a bug for such a tool\nWhereas turning it into:\n\n  --Binary blob XXX\n  + Binary blob YYY\n   +Binary blob ZZZ\n\nis codifying ambiguous output, and making the tool forever broken.\n\n-Peff\n"},{"id":"165949","messageId":"7v39lid8uz.fsf@alter.siamese.dyndns.org","threadId":"27062","inReplyTo":"20110416014758.GB23306@sigill.intra.peff.net","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-16T06:10:44Z","receivedAt":"2011-04-16T06:10:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> > Well, I know no tool parsing combined diff actually, so it's indeed a\n>> > hypothetical case.\n>> \n>> And the ones that have been parsing cdiff wouldn't have done anything good\n>> before this change on such a binary blob anyway, no?\n>\n> No, but we can view the proposed change as fixing a bug for such a tool\n> Whereas turning it into:\n>\n>   --Binary blob XXX\n>   + Binary blob YYY\n>    +Binary blob ZZZ\n>\n> is codifying ambiguous output, and making the tool forever broken.\n\nOf course, if we did this for a plumbing command and when the user did not\nask for --textconv, I would agree with your argument.  Such an output\nmakes it impossible to tell between the text files that had these lines\nand binary files.\n\nWhat I am suggesting is to make any binary file use a fallback textconv\n\"Binary blob $SHA-1\", when the --textconv option is given from the command\nline and no textconv filter is configured for the path, in any textconv\naware commands consistently, not limited to -c/--cc under discussion.\n\nWith the current codebase, such a change *would* break a bog-standard,\ntwo-way \"git diff\" for a binary file; we do want to see the traditional\n\"Binary files differ\" by not using the fallback textconv, but we cannot\ntell if the --textconv option was explicitly given from the command line\nwith the test used in Michael's patch (i.e. ALLOW_TEXTCONV), because we\nset the bit by default for Porcelain commands.  And showing \"-Binary X\"\nfollowed by \"-Binary Y\" is simply wrong and ambiguous, of course, in such\na case.  We need to be able to tell if an explicit --textconv was given or\nwe have ALLOW_TEXTCONV merely because we are running a Porcelain.\n\nBut I suspect that isn't something we cannot fix---we can just use another\nbit to record that in the command line parser.\n\nOnce that is fixed, I don't think giving \"Binary files differ\" when the\nline-counter script reads from a plumbing command that was invoked\nexplicitly with the --textconv command is any better than giving the above\nthree lines.  For a two-way merge, it does not matter much, but when\nviewing a merge with three or more parents, -c/--cc output that shows\nwhich sets of parents had the same blobs would be useful for humans (and\ntools) than a single \"Binary files differ\" output that does not tell any\ndetails.  The line-counter script would be counting \"forever broken\" data\nwhen you feed your JPEG collection with exif extracting textconv filter\nanyway, and I do not necessarily think it would make things worse to give\na fallback textconv filter to binary files that do not have one defined.\n"},{"id":"165954","messageId":"20110416063353.GB28853@sigill.intra.peff.net","threadId":"27062","inReplyTo":"7v39lid8uz.fsf@alter.siamese.dyndns.org","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-16T06:33:53Z","receivedAt":"2011-04-16T06:33:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 15, 2011 at 11:10:44PM -0700, Junio C Hamano wrote:\n\n> >> And the ones that have been parsing cdiff wouldn't have done anything good\n> >> before this change on such a binary blob anyway, no?\n> >\n> > No, but we can view the proposed change as fixing a bug for such a tool\n> > Whereas turning it into:\n> >\n> >   --Binary blob XXX\n> >   + Binary blob YYY\n> >    +Binary blob ZZZ\n> >\n> > is codifying ambiguous output, and making the tool forever broken.\n> \n> Of course, if we did this for a plumbing command and when the user did not\n> ask for --textconv, I would agree with your argument.  Such an output\n> makes it impossible to tell between the text files that had these lines\n> and binary files.\n\nOK, but what do you intend to do for a plumbing command _without_\n--textconv? I think what it is doing now (pretending that lines in the\nbinary file are relevant, and either truncating output on NUL or spewing\nNULs to the output stream) is just wrong.\n\nThe only reasonable thing I see there is inventing some combined-diff\nform of the \"Binary files differ\" message.\n\n> What I am suggesting is to make any binary file use a fallback textconv\n> \"Binary blob $SHA-1\", when the --textconv option is given from the command\n> line and no textconv filter is configured for the path, in any textconv\n> aware commands consistently, not limited to -c/--cc under discussion.\n\nIck, why? That pseudo-diff contains no additional interesting\ninformation that is not already there (since the \"index\" line already\ncontains the blob sha1s). I suppose one could argue that it's more\nreadable, but I don't find it so; I actually think it is less readable,\nbecause it makes you (even as a human, not a parsing script) think you\nare looking at a meaningful text diff.\n\nAnd then on top of that is the fact that what we do now is consistent\nwith other diff implementations, so people expect it.\n\n> With the current codebase, such a change *would* break a bog-standard,\n> two-way \"git diff\" for a binary file; we do want to see the traditional\n> \"Binary files differ\" by not using the fallback textconv, but we cannot\n> tell if the --textconv option was explicitly given from the command line\n> with the test used in Michael's patch (i.e. ALLOW_TEXTCONV), because we\n> set the bit by default for Porcelain commands.  And showing \"-Binary X\"\n> followed by \"-Binary Y\" is simply wrong and ambiguous, of course, in such\n> a case.  We need to be able to tell if an explicit --textconv was given or\n> we have ALLOW_TEXTCONV merely because we are running a Porcelain.\n> \n> But I suspect that isn't something we cannot fix---we can just use another\n> bit to record that in the command line parser.\n\nSure, it would take some code tweaking, but it wouldn't be hard to get\nthe behavior you are mentioning.\n\n> Once that is fixed, I don't think giving \"Binary files differ\" when the\n> line-counter script reads from a plumbing command that was invoked\n> explicitly with the --textconv command is any better than giving the above\n> three lines.  For a two-way merge, it does not matter much, but when\n> viewing a merge with three or more parents, -c/--cc output that shows\n> which sets of parents had the same blobs would be useful for humans (and\n> tools) than a single \"Binary files differ\" output that does not tell any\n> details.\n\nOh, sure. I am not proposing that \"-c\" should just say exactly \"Binary\nfiles X and Y differ\", only that we need a message _like_ that.  I think\nit would be fine to represent which parents had which sha1, either in\nsome structured format or even as text. I just think that making it look\nexactly like a text diff (even though, yes, that is a convenient\nstructured format that we already have) is unnecessarily confusing to\nboth humans and scripts.\n\n-Peff\n"},{"id":"165958","messageId":"4DA96E48.3050008@drmicha.warpmail.net","threadId":"27062","inReplyTo":"7voc47cqj0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] combine-diff: use textconv for combined diff format","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-04-16T10:24:08Z","receivedAt":"2011-04-16T10:24:08Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 15.04.2011 20:34:\n> Michael J Gruber <git@drmicha.warpmail.net> writes:\n> \n>> git diff -m produces a combined diff!\n> \n> Hmm, what is the rest of your command line?  I thought -m was a way to ask\n> pairwise diff with each parent.\n\nSure, but it does not always work like that. Just look at the test from\nmy patch, or do any \"git merge --no-commit\" and then \"git diff -m\". I\nwould expect that to compare the worktree to each parent, but in fact it\nruns \"diff --cc\".\n\nAt least I thought that's the only way how combine-diff would ever have\nto deal with a merge result in the worktree as opposed to a blob. And it\nseems that \"diff -m\" does not handle this but relays to \"diff --cc\" for\ncurrent git. I have not checked the \"-m\" codepath.\n\n>> +static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent, int textconv)\n>>  {\n>>  \tstruct diff_queue_struct *q = &diff_queued_diff;\n>>  \tstruct combine_diff_path *p;\n>> @@ -34,9 +34,13 @@ static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr,\n>>  \n>>  \t\t\thashcpy(p->sha1, q->queue[i]->two->sha1);\n>>  \t\t\tp->mode = q->queue[i]->two->mode;\n>> +\t\t\tif (textconv)\n>> +\t\t\t\tp->textconv = get_textconv(q->queue[i]->two);\n>>  \t\t\thashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);\n>>  \t\t\tp->parent[n].mode = q->queue[i]->one->mode;\n>>  \t\t\tp->parent[n].status = q->queue[i]->status;\n>> +\t\t\tif (textconv)\n>> +\t\t\t\tp->parent[n].textconv = get_textconv(q->queue[i]->one);\n> \n> This code attempts to handle different textconv set for each different\n> parents.  But I have to wonder if that is really worth it.\n> \n> The attribute to decide the content type of the blob is read from the same\n> set of .gitattributes files, regardless of which parent you are looking at\n> (and this is not likely to change---the exact procedure that is applied\n> comes from .git/config that is not even versioned, so there is not much\n> point in reading from the .gitattributes from the parent tree, trying to\n> be \"precise\").\n> \n> If q->queue[i] is not a rename, p->textconv and p->parent[n].textconv\n> would be the same because one and two came from the same path.  If it is a\n> rename, they by definition consist of similar contents, and the user would\n> want the same textconv conversion applied to them to make them comparable.\n> Even though using p->parent[n].textconv to convert q->queue[i]->one->sha1\n> blob and using p->textconv to convert q->queue[i]->two->sha1 blob might be\n> the right thing to do in theory, doing so wouldn't make a difference in\n> practice.  More importantly, even if the two textconvs specify different\n> conversions, it is likely that it is an user error (e.g. the preimage had\n> \"img4433.jqg\" that was renamed to img4433.jpg\" in the postimage, and the\n> attributes mechanism does not say \".jqg\" is a JPEG that wants to get\n> \"exif\" run to be texualized for the purpose of diffing, or something).\n> \n> Besides, if you really want to support \"left hand side and right hand\n> side, depending on which parent we are talking about, may use different\n> textconv\", you would need to defeat the optimization in show_patch_diff()\n> that calls reuse_combine_diff() when sha1 are the same from other parent\n> we have already compared---the parent we are looking at may be using a\n> different textconv procedure.  Even worse, if parent and child have the\n> same sha1, the result of running parent textconv on the parent blob may be\n> different from that of the child, which you would never even see in this\n> codepath.\n> \n> So I suspect that using only one textconv per \"struct combine_diff_path\"\n> would make both the code simpler, and more importantly would make the\n> result more correct from the end user's point of view.\n\nI'd be happy to take the simpler approach. While I still think the other\none is \"more correct\" (modulo the reuse issue) it should not matter in\nmost cases.\n\n> \n>> @@ -777,6 +783,12 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n>>  \t\t\tclose(fd);\n>>  \t}\n>>  \n>> +\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && elem->textconv) {\n>> +\t\tstruct diff_filespec *df = alloc_filespec(elem->path);\n>> +\t\tfill_filespec(df, elem->sha1, elem->mode);\n>> +\t\tresult_size = fill_textconv(elem->textconv, df, &result);\n>> +\t}\n> \n> I suspect that these three lines have to become a small helper function to\n> be used to convert the final blob (done here), and parent blob (done in\n> combine_diff).  With the \"binary\" support, it would eventually need to be\n> enhanced to something like:\n> \n> \tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV)) {\n>         \tif (textconv) {\n>                 \tdo these three lines;\n> \t\t} else if (is binary) {\n>                 \t\"Binary blob $SHA-1\";\n> \t\t}\n> \t}\n> \n\n\"diff -m --oneline\" says something like\n\naa01ae1 (from 64c0923) Merge branch 'master' into somebranch\ndiff --git a/a b/a\nindex 72594ed..d8323da 100644\nBinary files a/a and b/a differ\naa01ae1 (from e85049e) Merge branch 'master' into somebranch\ndiff --git a/a b/a\nindex 86e041d..d8323da 100644\nBinary files a/a and b/a differ\n\n\nso I'm wondering whether we shouldn't stay closer to that with \"--cc\nalso\", e.g.:\n\naa01ae1 Merge branch 'master' into somebranch\ndiff --cc a\nindex 72594ed,86e041d..d8323da\nBinary files a/a and b/a differ\n\nBTW: Currently, \"--cc --oneline\" produces an extra newline before the\ndiff line, and also note how the diff lines differ (\"a/a b/a\" vs. \"a\").\nBut those are different issues.\n\n> and having a small helper function early in the series would help that\n> process.\n> \n>> +\t\tpaths = intersect_paths(paths, i, num_parent, DIFF_OPT_TST(opt, ALLOW_TEXTCONV));\n> \n> As an internal API within this file, I would rather see \"opt\" as a whole\n> passed to intersect_paths().  We may probably want to determine if the\n> blob is binary in that function depending on other \"opt\" fields.\n\nYep.\nMichael\n\n[Resent today, sorry. Couldn't get myself to reboot that box yesterday\nafter a disk gave up.]\n"},{"id":"165966","messageId":"7vtydyb1xi.fsf@alter.siamese.dyndns.org","threadId":"27062","inReplyTo":"20110416063353.GB28853@sigill.intra.peff.net","subject":"Re: textconv not invoked when viewing merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-16T16:23:21Z","receivedAt":"2011-04-16T16:23:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> OK, but what do you intend to do for a plumbing command _without_\n> --textconv? I think what it is doing now (pretending that lines in the\n> binary file are relevant, and either truncating output on NUL or spewing\n> NULs to the output stream) is just wrong.\n\nOh, no question about it.  \"Binary files differ\" codepath needs to be\nadded, and independent of if we want to add a fallback textconv.\n\n> Ick, why? That pseudo-diff contains no additional interesting\n> information that is not already there (since the \"index\" line already\n> contains the blob sha1s).\n\nTrue enough.\n\nThe only case that might make a difference would be if one side was binary\nand the other side and the result was text, in which case the user can not\njust see but read the result, but I don't think it is worth caring about.\n\nAlso unlike my weatherbaloon patch, Michael's approach (if it is updated\nto pass the whole diff options structure instead of just one \"do we care\nabout the textconv\" bit to intersect_paths() function) will let us\ndetermine if the combined path should say \"Binary files differ\" a lot\nearly, so there is no need to worry about what to do on binary files in\nthe places we would be adding textconv anymore.\n"},{"id":"165974","messageId":"7vei52azbf.fsf@alter.siamese.dyndns.org","threadId":"27062","inReplyTo":"4DA96E48.3050008@drmicha.warpmail.net","subject":"Re: [PATCH] combine-diff: use textconv for combined diff format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-16T17:19:48Z","receivedAt":"2011-04-16T17:19:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> Junio C Hamano venit, vidit, dixit 15.04.2011 20:34:\n>> Michael J Gruber <git@drmicha.warpmail.net> writes:\n>> \n>>> git diff -m produces a combined diff!\n>> \n>> Hmm, what is the rest of your command line?  I thought -m was a way to ask\n>> pairwise diff with each parent.\n>\n> Sure, but it does not always work like that. Just look at the test from\n> my patch, or do any \"git merge --no-commit\" and then \"git diff -m\". I\n> would expect that to compare the worktree to each parent, but in fact it\n> runs \"diff --cc\".\n\nThanks; it wasn't clear you are comparing stages with the working tree.\nAsking for the rest of the command line paid off ;-)\n\nAnd yes, comparing multiple entries with the worktree files is done in\nrun_diff_files() defined in diff-lib.c; it is unaware of the -m option\nthat was originally defined for diff-tree to show pairwise diff.  That\ncodepath never cared about -m before nor after -c/--cc was invented for\ndiff-tree, and it only learned about -c/--cc when it was introduced.  I\nthink diff-index is unaware of the -m option from the same historical\nbackground (read: not \"for the same reason or justification\").\n\nWe may want to change that, but I am personally not very interested.  We\ncan ask to diff against a specific stage, I know that is what I do, and I\nthink that is what most people do [*1*].\n\n> \"diff -m --oneline\" says something like\n>\n> aa01ae1 (from 64c0923) Merge branch 'master' into somebranch\n> diff --git a/a b/a\n> index 72594ed..d8323da 100644\n> Binary files a/a and b/a differ\n> aa01ae1 (from e85049e) Merge branch 'master' into somebranch\n> diff --git a/a b/a\n> index 86e041d..d8323da 100644\n> Binary files a/a and b/a differ\n\nYes this is the case for diff-tree running pair-wise comparison.\n\n> so I'm wondering whether we shouldn't stay closer to that with \"--cc\n> also\", e.g.:\n>\n> aa01ae1 Merge branch 'master' into somebranch\n> diff --cc a\n> index 72594ed,86e041d..d8323da\n> Binary files a/a and b/a differ\n\nThe -c/--cc options are about presenting the pairwise -m output in a\ndifferent way by combining and condensing.  So in that sense, if we really\nwant to combine and condense information, one possibility is to do:\n\n Binary files a/a and c/a differ, b/a and c/a differ.\n\nnaming each parent as 'a', 'b', ... and giving the highest letter (in the\ntwo-parent merge case, 'c') to the final result.  By doing so, you can\nexpress where in its tree each parent had the content when you are viewing\na renaming merge.\n\nBu that opens an old can of worms we should have opened and closed four\nyears ago.\n\nThe header shows \"diff --cc a\" followed by \"--- a/a\" followed by \"+++ b/a\"\nbefore the hunk for a two-way merge.  But if we are to \"combine and\ncondense\", another possibility is to show:\n\n    diff --cc a/a b/a c/a\n    index bf7c788,fa9d23a,5d24d9f..cc69134\n    --- a/a\n    --- b/a\n    +++ c/a\n    @@@@ -74,26 -74,6 -74,29 +74,50 @@@@\n    ...\n\nto keep the paths information.  I do not think anybody cared so far, and\nperhaps we should have done it when we introduced -c/--cc, but it is not\nat all worth changing now.\n\nThat means that we are not all that worried about losing the rename\ninformation when showing such a diff in --cc/-c form.  After all, the\n\"diff --cc a\" header is not \"diff --cc a/a b/a c/a\" that mentions all\npaths, and \"--- a/a\" lines are not repeated for each parent.  So while\nshowing the names like you suggested may be a possibility, I think an\napproach that is more in line with the current output would be:\n\n \"Binary files a in different versions differ\"\n\nor something without naming them with a/, b/, ...\n\nIn short, it all depends on how much we condense when running -c/--cc.  We\nare inconsistent by showing both \"--- a/a\" and \"+++ b/a\" lines, but modulo\nthat we condense away the renamed path information in our current output.\nAnd the final alternative in the previous paragraph would be more in line\nwith that design.\n\n\n[Footnote]\n\n*1* I also often use \n\n diff HEAD...MERGE_HEAD $path\n diff HEAD $path\n diff MERGE_HEAD $path\n\nduring a conflicted merge when it is hard to read in the --cc form.\n"},{"id":"165986","messageId":"m31v11yj37.fsf@localhost.localdomain","threadId":"27062","inReplyTo":"7vei52azbf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] combine-diff: use textconv for combined diff format","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-04-16T21:37:30Z","receivedAt":"2011-04-16T21:37:30Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> But that opens an old can of worms we should have opened and closed four\n> years ago.\n> \n> The header shows \"diff --cc a\" followed by \"--- a/a\" followed by \"+++ b/a\"\n> before the hunk for a two-way merge.  But if we are to \"combine and\n> condense\", another possibility is to show:\n> \n>     diff --cc a/a b/a c/a\n>     index bf7c788,fa9d23a,5d24d9f..cc69134\n>     --- a/a\n>     --- b/a\n>     +++ c/a\n>     @@@@ -74,26 -74,6 -74,29 +74,50 @@@@\n>     ...\n> \n> to keep the paths information.  I do not think anybody cared so far, and\n> perhaps we should have done it when we introduced -c/--cc, but it is not\n> at all worth changing now.\n\nSuch feature would greatly simplify gitweb code for dealing with\ncombined diff (for a merge commit).  It wouldn't have to jump through\nhoops[1] to get pre-image names to have correct link to pre-image...\n\nThis affects gitweb performance... in those rare case where we have\nrename in merge commit (gitweb is smart enough to do this dance only\nif there is rename in a merge).\n\nNote that tree-diff doesn't help either - we have only post-image\nname.\n\n[1]: fill_from_file_info subroutine, which in turn uses\n     git_get_path_by_hash once per parent, which uses git-ls-tree\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"166195","messageId":"201104211808.06400.kumbayo84@arcor.de","threadId":"27062","inReplyTo":"36a715a966a22207135f60532e723f6d87dd1ffb.1302881295.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] combine-diff: use textconv for combined diff format","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2011-04-21T16:08:05Z","receivedAt":"2011-04-21T16:08:05Z","isPatch":true,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"On Freitag, 15. April 2011, Michael J Gruber wrote:\n> Currently, we ignore textconv and binary status for the combined diff\n> formats (-c, -cc) which was never intended.\n> \n> Change this so that combined diff uses the same helpers.\n> \n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> ---\n> So, just so that I don't get the vapor patch award, here's a WIP passing\n> Jeff's test.\n> \n> Before looking at free()ing what I've introduced and the binary issue I'll\n> check whether the whole blob/file read hunk in show_patch_diff() can't be\n> simply subsumed in the fill_textconv() call. It is almost a copy of\n> diff_populate_filespec() but not quite.\n> \n> Also, the situation with worktree is even worse than I thought:\n> \n> git diff -m produces a combined diff!\n> \n> Also, my patch does not cure \"diff -c\" against worktree so far, I'm not\n> textconv'ing the worktree file yet. But then again, \"diff -m\" sucks here also.\n> \n> I'll probably pick this up later today.\n\nHi,\n\nthanks for working on this.\nI tried this patch on my msysgit system and now gitk shows a nice diff\nfor my merged archives. :-)\nFor merges of other binary files without textconf filter (jar)\ni still get binary output.\n(but i expected this from the notes above/discussion)\n\nthanks,\nGreetings Peter\n"}]}