{"thread":{"id":"27428","subject":"combined diff does not detect binary files and ignores -diff attribute","startedAt":"2011-05-22T20:12:53Z","lastAt":"2011-05-31T22:42:47Z","messageCount":37,"participants":["Jay Soffian","Michael J Gruber","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"168456","messageId":"BANLkTi=FtkiUjwAa7e3KAC5FF3GNxWzd3Q@mail.gmail.com","threadId":"27428","inReplyTo":null,"subject":"combined diff does not detect binary files and ignores -diff attribute","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-05-22T20:12:53Z","receivedAt":"2011-05-22T20:12:53Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"In a merge commit, I added some PNGs (so they were new to both\nparents). I was surprised when I did a \"git show\" on the commit and\nhad the binary bits spewed to the terminal.\n\nI thought it was just git not detecting the PNG as a binary file, but\nadding \"*.png -diff\" to .git/info/attributes made no difference.\n\nJust reporting this for now, as I don't have time to investigate.\n\nj.\n"},{"id":"168498","messageId":"4DDA618E.4030604@drmicha.warpmail.net","threadId":"27428","inReplyTo":"BANLkTi=FtkiUjwAa7e3KAC5FF3GNxWzd3Q@mail.gmail.com","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-23T13:30:54Z","receivedAt":"2011-05-23T13:30:54Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jay Soffian venit, vidit, dixit 22.05.2011 22:12:\n> In a merge commit, I added some PNGs (so they were new to both\n> parents). I was surprised when I did a \"git show\" on the commit and\n> had the binary bits spewed to the terminal.\n> \n> I thought it was just git not detecting the PNG as a binary file, but\n> adding \"*.png -diff\" to .git/info/attributes made no difference.\n> \n> Just reporting this for now, as I don't have time to investigate.\n> \n> j.\n\nWe know, but we somehow got stuck, see:\n\nhttp://permalink.gmane.org/gmane.comp.version-control.git/171613\n\nI don't have the time to follow up on this currently, it got out of\nproportion.\n\nMichael\n"},{"id":"168503","messageId":"BANLkTinu3AbTmtswn6DLQKAWdLL=gBvAqA@mail.gmail.com","threadId":"27428","inReplyTo":"4DDA618E.4030604@drmicha.warpmail.net","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-05-23T15:17:14Z","receivedAt":"2011-05-23T15:17:14Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, May 23, 2011 at 9:30 AM, Michael J Gruber\n wrote:\n> Jay Soffian venit, vidit, dixit 22.05.2011 22:12:\n>> In a merge commit, I added some PNGs (so they were new to both\n>> parents). I was surprised when I did a \"git show\" on the commit and\n>> had the binary bits spewed to the terminal.\n>>\n>> I thought it was just git not detecting the PNG as a binary file, but\n>> adding \"*.png -diff\" to .git/info/attributes made no difference.\n>>\n>> Just reporting this for now, as I don't have time to investigate.\n>>\n>> j.\n>\n> We know, but we somehow got stuck, see:\n>\n> http://permalink.gmane.org/gmane.comp.version-control.git/171613\n>\n> I don't have the time to follow up on this currently, it got out of\n> proportion.\n\nDrat, seems like the perfect being the enemy of the good there. I\nwonder if Junio's patch in that thread isn't good enough for now.\n\nThanks,\n\nj.\n"},{"id":"168509","messageId":"7vzkmduz0e.fsf@alter.siamese.dyndns.org","threadId":"27428","inReplyTo":"BANLkTinu3AbTmtswn6DLQKAWdLL=gBvAqA@mail.gmail.com","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-23T17:07:13Z","receivedAt":"2011-05-23T17:07:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here is a quick summary of what I have in the \"talked about, sounded\ninteresting, and never got completed\" list.\n\n\ngmane=http://thread.gmane.org/gmane.comp.version-control.git/\n\n* Teach pack protocol to transfer estimated pack size and history\n depth to allow receiving end make more intelligent decision between\n unpack-objects and index-pack.\n\n $gmane/173610\n\n* The \"combined\" diff always assumes it deals with text files. Teach it\n to punt on binary and also use the textconv.\n\n $gname/171613\n\n* Audit use of symbolic-ref without -m in our scripts and for each\n case decide if leaving a reflog entry for the HEAD is desirable.\n If so, add them.\n\n $gmane/172516\n\n* \"git status\" on intent-to-add index entries (say \"I\" in the first\n column instead of \"A\" for short status, add \"(needs 'git add')\" at the\n end of \"new file: $path \" in long status).\n\n $gmane/170658\n\n* synopsys: use {} instead of () for grouping alternatives (Jari Aalto)\n $gmane/72243\n\n* \"[alias] st = status\" and \"cd .git && git st\" (Jeff King)\n $gmane/72327\n"},{"id":"168515","messageId":"20110523181147.GA26035@sigill.intra.peff.net","threadId":"27428","inReplyTo":"BANLkTinu3AbTmtswn6DLQKAWdLL=gBvAqA@mail.gmail.com","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-23T18:11:47Z","receivedAt":"2011-05-23T18:11:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 23, 2011 at 11:17:14AM -0400, Jay Soffian wrote:\n\n> > We know, but we somehow got stuck, see:\n> >\n> > http://permalink.gmane.org/gmane.comp.version-control.git/171613\n> >\n> > I don't have the time to follow up on this currently, it got out of\n> > proportion.\n> \n> Drat, seems like the perfect being the enemy of the good there. I\n> wonder if Junio's patch in that thread isn't good enough for now.\n\nI'll have a patch series in a few minutes that at least handles the\nbinary case. I'll see how painful the textconv bit is on top of that.\n\n-Peff\n"},{"id":"168522","messageId":"20110523201529.GA6281@sigill.intra.peff.net","threadId":"27428","inReplyTo":"20110523181147.GA26035@sigill.intra.peff.net","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-23T20:15:29Z","receivedAt":"2011-05-23T20:15:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 23, 2011 at 02:11:47PM -0400, Jeff King wrote:\n\n> On Mon, May 23, 2011 at 11:17:14AM -0400, Jay Soffian wrote:\n> \n> > > We know, but we somehow got stuck, see:\n> > >\n> > > http://permalink.gmane.org/gmane.comp.version-control.git/171613\n> > >\n> > > I don't have the time to follow up on this currently, it got out of\n> > > proportion.\n> > \n> > Drat, seems like the perfect being the enemy of the good there. I\n> > wonder if Junio's patch in that thread isn't good enough for now.\n> \n> I'll have a patch series in a few minutes that at least handles the\n> binary case. I'll see how painful the textconv bit is on top of that.\n\nIt turned out not too bad:\n\n [1/5]: combine-diff: split header printing into its own function\n [2/5]: combine-diff: calculate mode_differs earlier\n [3/5]: combine-diff: handle binary files as binary\n [4/5]: refactor get_textconv to not require diff_filespec\n [5/5]: combine-diff: respect textconv attributes\n\n-Peff\n"},{"id":"168523","messageId":"20110523201641.GA6298@sigill.intra.peff.net","threadId":"27428","inReplyTo":"20110523201529.GA6281@sigill.intra.peff.net","subject":"[PATCH 1/5] combine-diff: split header printing into its own function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-23T20:16:41Z","receivedAt":"2011-05-23T20:16:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a pretty big logical chunk, so it makes the function\na bit more readable to have it split out. In addition, it\nwill make it easier to add an alternate code path for binary\ndiffs in a future patch.\n\nSigned-off-by: Jeff King \n---\n combine-diff.c | 135 ++++++++++++++++++++++++++++++-------------------------\n 1 files changed, 74 insertions(+), 61 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 655fa89..309dc6c 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -681,6 +681,78 @@ static void dump_quoted_path(const char *head,\n \tputs(buf.buf);\n }\n \n+static void show_combined_header(struct combine_diff_path *elem,\n+\t\t\t\t int num_parent,\n+\t\t\t\t int dense,\n+\t\t\t\t struct rev_info *rev,\n+\t\t\t\t int mode_differs)\n+{\n+\tstruct diff_options *opt = &rev->diffopt;\n+\tint abbrev = DIFF_OPT_TST(opt, FULL_INDEX) ? 40 : DEFAULT_ABBREV;\n+\tconst char *a_prefix = opt->a_prefix ? opt->a_prefix : \"a/\";\n+\tconst char *b_prefix = opt->b_prefix ? opt->b_prefix : \"b/\";\n+\tint use_color = DIFF_OPT_TST(opt, COLOR_DIFF);\n+\tconst char *c_meta = diff_get_color(use_color, DIFF_METAINFO);\n+\tconst char *c_reset = diff_get_color(use_color, DIFF_RESET);\n+\tconst char *abb;\n+\tint added = 0;\n+\tint deleted = 0;\n+\tint i;\n+\n+\tif (rev->loginfo && !rev->no_commit_id)\n+\t\tshow_log(rev);\n+\n+\tdump_quoted_path(dense ? \"diff --cc \" : \"diff --combined \",\n+\t\t\t \"\", elem->path, c_meta, c_reset);\n+\tprintf(\"%sindex \", c_meta);\n+\tfor (i = 0; i < num_parent; i++) {\n+\t\tabb = find_unique_abbrev(elem->parent[i].sha1,\n+\t\t\t\t\t abbrev);\n+\t\tprintf(\"%s%s\", i ? \",\" : \"\", abb);\n+\t}\n+\tabb = find_unique_abbrev(elem->sha1, abbrev);\n+\tprintf(\"..%s%s\\n\", abb, c_reset);\n+\n+\tif (mode_differs) {\n+\t\tdeleted = !elem->mode;\n+\n+\t\t/* We say it was added if nobody had it */\n+\t\tadded = !deleted;\n+\t\tfor (i = 0; added && i < num_parent; i++)\n+\t\t\tif (elem->parent[i].status !=\n+\t\t\t DIFF_STATUS_ADDED)\n+\t\t\t\tadded = 0;\n+\t\tif (added)\n+\t\t\tprintf(\"%snew file mode %06o\",\n+\t\t\t c_meta, elem->mode);\n+\t\telse {\n+\t\t\tif (deleted)\n+\t\t\t\tprintf(\"%sdeleted file \", c_meta);\n+\t\t\tprintf(\"mode \");\n+\t\t\tfor (i = 0; i < num_parent; i++) {\n+\t\t\t\tprintf(\"%s%06o\", i ? \",\" : \"\",\n+\t\t\t\t elem->parent[i].mode);\n+\t\t\t}\n+\t\t\tif (elem->mode)\n+\t\t\t\tprintf(\"..%06o\", elem->mode);\n+\t\t}\n+\t\tprintf(\"%s\\n\", c_reset);\n+\t}\n+\n+\tif (added)\n+\t\tdump_quoted_path(\"--- \", \"\", \"/dev/null\",\n+\t\t\t\t c_meta, c_reset);\n+\telse\n+\t\tdump_quoted_path(\"--- \", a_prefix, elem->path,\n+\t\t\t\t c_meta, c_reset);\n+\tif (deleted)\n+\t\tdump_quoted_path(\"+++ \", \"\", \"/dev/null\",\n+\t\t\t\t c_meta, c_reset);\n+\telse\n+\t\tdump_quoted_path(\"+++ \", b_prefix, elem->path,\n+\t\t\t\t c_meta, c_reset);\n+}\n+\n static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t int dense, struct rev_info *rev)\n {\n@@ -692,13 +764,9 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tint mode_differs = 0;\n \tint i, show_hunks;\n \tint working_tree_file = is_null_sha1(elem->sha1);\n-\tint abbrev = DIFF_OPT_TST(opt, FULL_INDEX) ? 40 : DEFAULT_ABBREV;\n-\tconst char *a_prefix, *b_prefix;\n \tmmfile_t result_file;\n \n \tcontext = opt->context;\n-\ta_prefix = opt->a_prefix ? opt->a_prefix : \"a/\";\n-\tb_prefix = opt->b_prefix ? opt->b_prefix : \"b/\";\n \n \t/* Read the result of merge first */\n \tif (!working_tree_file)\n@@ -832,63 +900,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tshow_hunks = make_hunks(sline, cnt, num_parent, dense);\n \n \tif (show_hunks || mode_differs || working_tree_file) {\n-\t\tconst char *abb;\n-\t\tint use_color = DIFF_OPT_TST(opt, COLOR_DIFF);\n-\t\tconst char *c_meta = diff_get_color(use_color, DIFF_METAINFO);\n-\t\tconst char *c_reset = diff_get_color(use_color, DIFF_RESET);\n-\t\tint added = 0;\n-\t\tint deleted = 0;\n-\n-\t\tif (rev->loginfo && !rev->no_commit_id)\n-\t\t\tshow_log(rev);\n-\t\tdump_quoted_path(dense ? \"diff --cc \" : \"diff --combined \",\n-\t\t\t\t \"\", elem->path, c_meta, c_reset);\n-\t\tprintf(\"%sindex \", c_meta);\n-\t\tfor (i = 0; i < num_parent; i++) {\n-\t\t\tabb = find_unique_abbrev(elem->parent[i].sha1,\n-\t\t\t\t\t\t abbrev);\n-\t\t\tprintf(\"%s%s\", i ? \",\" : \"\", abb);\n-\t\t}\n-\t\tabb = find_unique_abbrev(elem->sha1, abbrev);\n-\t\tprintf(\"..%s%s\\n\", abb, c_reset);\n-\n-\t\tif (mode_differs) {\n-\t\t\tdeleted = !elem->mode;\n-\n-\t\t\t/* We say it was added if nobody had it */\n-\t\t\tadded = !deleted;\n-\t\t\tfor (i = 0; added && i < num_parent; i++)\n-\t\t\t\tif (elem->parent[i].status !=\n-\t\t\t\t DIFF_STATUS_ADDED)\n-\t\t\t\t\tadded = 0;\n-\t\t\tif (added)\n-\t\t\t\tprintf(\"%snew file mode %06o\",\n-\t\t\t\t c_meta, elem->mode);\n-\t\t\telse {\n-\t\t\t\tif (deleted)\n-\t\t\t\t\tprintf(\"%sdeleted file \", c_meta);\n-\t\t\t\tprintf(\"mode \");\n-\t\t\t\tfor (i = 0; i < num_parent; i++) {\n-\t\t\t\t\tprintf(\"%s%06o\", i ? \",\" : \"\",\n-\t\t\t\t\t elem->parent[i].mode);\n-\t\t\t\t}\n-\t\t\t\tif (elem->mode)\n-\t\t\t\t\tprintf(\"..%06o\", elem->mode);\n-\t\t\t}\n-\t\t\tprintf(\"%s\\n\", c_reset);\n-\t\t}\n-\t\tif (added)\n-\t\t\tdump_quoted_path(\"--- \", \"\", \"/dev/null\",\n-\t\t\t\t\t c_meta, c_reset);\n-\t\telse\n-\t\t\tdump_quoted_path(\"--- \", a_prefix, elem->path,\n-\t\t\t\t\t c_meta, c_reset);\n-\t\tif (deleted)\n-\t\t\tdump_quoted_path(\"+++ \", \"\", \"/dev/null\",\n-\t\t\t\t\t c_meta, c_reset);\n-\t\telse\n-\t\t\tdump_quoted_path(\"+++ \", b_prefix, elem->path,\n-\t\t\t\t\t c_meta, c_reset);\n+\t\tshow_combined_header(elem, num_parent, dense, rev,\n+\t\t\t\t mode_differs);\n \t\tdump_sline(sline, cnt, num_parent,\n \t\t\t DIFF_OPT_TST(opt, COLOR_DIFF), result_deleted);\n \t}\n-- \n1.7.5.2.4.g43415\n"},{"id":"168524","messageId":"20110523201659.GB6298@sigill.intra.peff.net","threadId":"27428","inReplyTo":"20110523201529.GA6281@sigill.intra.peff.net","subject":"[PATCH 2/5] combine-diff: calculate mode_differs earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-23T20:16:59Z","receivedAt":"2011-05-23T20:16:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One loop combined both the patch generation and checking\nwhether there was any mode change to report. Let's factor\nthat into two separate loops, as we may care about the mode\nchange even if we are not generating patches (e.g., because\nwe are showing a binary diff, which will come in a future\npatch).\n\nSigned-off-by: Jeff King \n---\n combine-diff.c | 9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 309dc6c..2183184 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -845,6 +845,13 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tclose(fd);\n \t}\n \n+\tfor (i = 0; i < num_parent; i++) {\n+\t\tif (elem->parent[i].mode != elem->mode) {\n+\t\t\tmode_differs = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \tfor (cnt = 0, cp = result; cp < result + result_size; cp++) {\n \t\tif (*cp == '\\n')\n \t\t\tcnt++;\n@@ -893,8 +900,6 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\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 (elem->parent[i].mode != elem->mode)\n-\t\t\tmode_differs = 1;\n \t}\n \n \tshow_hunks = make_hunks(sline, cnt, num_parent, dense);\n-- \n1.7.5.2.4.g43415\n"},{"id":"168525","messageId":"20110523202734.GC6298@sigill.intra.peff.net","threadId":"27428","inReplyTo":"20110523201529.GA6281@sigill.intra.peff.net","subject":"[PATCH 3/5] combine-diff: handle binary files as binary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-23T20:27:34Z","receivedAt":"2011-05-23T20:27:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The combined diff code path is totally different from the\nregular diff code path, and didn't handle binary files at\nall. The results of a combined diff on a binary file could\nrange from annoying (since we spewed binary garbage,\npossibly upsetting the user's terminal), to wrong (embedded\nNULs caused us to show incorrect diffs, with lines truncated\nat the NUL character), to potential security problems\n(embedded NULs could interfere with \"-z\" output, possibly\ndefeating policy hooks which parse diff output).\n\nInstead, we consider a combined diff to be binary if any of\nthe input blobs is binary. To show a binary combined diff,\nwe indicate \"Binary blobs differ\"; the \"index\" meta line\nwill show which parents had which blob.\n\nSigned-off-by: Jeff King \n---\nThe big question here is what we want the output to look like. I chose\nthis:\n\n diff --combined foo\n index 7ea6ded,6197570..9563691\n Binary files differ\n\nwhich contains all of the information we have: for file \"foo\" there were\ntwo parents (at 7ea6ded and 6197570 respectively), and the resolution\nwas 9563691. If you want to see full sha1s, you can use --full-index.\nThis format maps very closely to the non-combined case, which looks\nlike:\n\n diff --git a/foo b/foo\n index 6197570..9563691 100644\n Binary files a/foo and b/foo differ\n\nTwo other obvious options for format would be:\n\n 1. A combined diff of fake text, like:\n\n - Binary blob 7ea6ded\n -Binary blob 6197570\n ++Binary blob 9563691\n\n But this contains no additional information over the index line, and\n I worry that it will be less obvious to readers what is going on.\n\n 2. The --raw format, which looks like:\n\n ::100644 100644 100644 7ea6ded... 6197570... 9563691... MM binary\n\n but again, there's really no extra information there (the modes\n are there, but in the case of changed modes, we already produce a\n separate mode line like \"mode 100644,100755 100755\". And I\n personally find it way less readable (it's more compact, which is\n good for actual --raw mode, but in patch mode we are already\n spending quite a few lines per file).\n\nSo I think what I have is succint, complete, and matches the\nnon-combined case as well as we can.\n\n combine-diff.c | 37 ++++++++++++-\n t/t4048-diff-combined-binary.sh | 113 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 148 insertions(+), 2 deletions(-)\n create mode 100755 t/t4048-diff-combined-binary.sh\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 2183184..94a207f 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -7,6 +7,7 @@\n #include \"xdiff-interface.h\"\n #include \"log-tree.h\"\n #include \"refs.h\"\n+#include \"userdiff.h\"\n \n static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent)\n {\n@@ -685,7 +686,8 @@ static void show_combined_header(struct combine_diff_path *elem,\n \t\t\t\t int num_parent,\n \t\t\t\t int dense,\n \t\t\t\t struct rev_info *rev,\n-\t\t\t\t int mode_differs)\n+\t\t\t\t int mode_differs,\n+\t\t\t\t int show_file_header)\n {\n \tstruct diff_options *opt = &rev->diffopt;\n \tint abbrev = DIFF_OPT_TST(opt, FULL_INDEX) ? 40 : DEFAULT_ABBREV;\n@@ -739,6 +741,9 @@ static void show_combined_header(struct combine_diff_path *elem,\n \t\tprintf(\"%s\\n\", c_reset);\n \t}\n \n+\tif (!show_file_header)\n+\t\treturn;\n+\n \tif (added)\n \t\tdump_quoted_path(\"--- \", \"\", \"/dev/null\",\n \t\t\t\t c_meta, c_reset);\n@@ -765,8 +770,13 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tint i, show_hunks;\n \tint working_tree_file = is_null_sha1(elem->sha1);\n \tmmfile_t result_file;\n+\tstruct userdiff_driver *userdiff;\n+\tint is_binary;\n \n \tcontext = opt->context;\n+\tuserdiff = userdiff_find_by_path(elem->path);\n+\tif (!userdiff)\n+\t\tuserdiff = userdiff_find_by_name(\"default\");\n \n \t/* Read the result of merge first */\n \tif (!working_tree_file)\n@@ -852,6 +862,29 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t}\n \t}\n \n+\tif (userdiff->binary != -1)\n+\t\tis_binary = userdiff->binary;\n+\telse {\n+\t\tis_binary = buffer_is_binary(result, result_size);\n+\t\tfor (i = 0; !is_binary && i < num_parent; i++) {\n+\t\t\tchar *buf;\n+\t\t\tunsigned long size;\n+\t\t\tbuf = grab_blob(elem->parent[i].sha1,\n+\t\t\t\t\telem->parent[i].mode,\n+\t\t\t\t\t&size);\n+\t\t\tif (buffer_is_binary(buf, size))\n+\t\t\t\tis_binary = 1;\n+\t\t\tfree(buf);\n+\t\t}\n+\t}\n+\tif (is_binary) {\n+\t\tshow_combined_header(elem, num_parent, dense, rev,\n+\t\t\t\t mode_differs, 0);\n+\t\tprintf(\"Binary files differ\\n\");\n+\t\tfree(result);\n+\t\treturn;\n+\t}\n+\n \tfor (cnt = 0, cp = result; cp < result + result_size; cp++) {\n \t\tif (*cp == '\\n')\n \t\t\tcnt++;\n@@ -906,7 +939,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \n \tif (show_hunks || mode_differs || working_tree_file) {\n \t\tshow_combined_header(elem, num_parent, dense, rev,\n-\t\t\t\t mode_differs);\n+\t\t\t\t mode_differs, 1);\n \t\tdump_sline(sline, cnt, num_parent,\n \t\t\t DIFF_OPT_TST(opt, COLOR_DIFF), result_deleted);\n \t}\ndiff --git a/t/t4048-diff-combined-binary.sh b/t/t4048-diff-combined-binary.sh\nnew file mode 100755\nindex 0000000..a943994\n--- /dev/null\n+++ b/t/t4048-diff-combined-binary.sh\n@@ -0,0 +1,113 @@\n+#!/bin/sh\n+\n+test_description='combined and merge diff handle binary files and textconv'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup binary merge conflict' '\n+\techo oneQ1 | q_to_nul >binary &&\n+\tgit add binary &&\n+\tgit commit -m one &&\n+\techo twoQ2 | q_to_nul >binary &&\n+\tgit commit -a -m two &&\n+\tgit checkout -b branch-binary HEAD^ &&\n+\techo threeQ3 | q_to_nul >binary &&\n+\tgit commit -a -m three &&\n+\ttest_must_fail git merge master &&\n+\techo resolvedQhooray | q_to_nul >binary &&\n+\tgit commit -a -m resolved\n+'\n+\n+cat >expect <<'EOF'\n+resolved\n+\n+diff --git a/binary b/binary\n+index 7ea6ded..9563691 100644\n+Binary files a/binary and b/binary differ\n+resolved\n+\n+diff --git a/binary b/binary\n+index 6197570..9563691 100644\n+Binary files a/binary and b/binary differ\n+EOF\n+test_expect_success 'diff -m indicates binary-ness' '\n+\tgit show --format=%s -m >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+resolved\n+\n+diff --combined binary\n+index 7ea6ded,6197570..9563691\n+Binary files differ\n+EOF\n+test_expect_success 'diff -c indicates binary-ness' '\n+\tgit show --format=%s -c >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+resolved\n+\n+diff --cc binary\n+index 7ea6ded,6197570..9563691\n+Binary files differ\n+EOF\n+test_expect_success 'diff --cc indicates binary-ness' '\n+\tgit show --format=%s --cc >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'setup non-binary with binary attribute' '\n+\tgit checkout master &&\n+\ttest_commit one text &&\n+\ttest_commit two text &&\n+\tgit checkout -b branch-text HEAD^ &&\n+\ttest_commit three text &&\n+\ttest_must_fail git merge master &&\n+\ttest_commit resolved text &&\n+\techo text -diff >.gitattributes\n+'\n+\n+cat >expect <<'EOF'\n+resolved\n+\n+diff --git a/text b/text\n+index 2bdf67a..2ab19ae 100644\n+Binary files a/text and b/text differ\n+resolved\n+\n+diff --git a/text b/text\n+index f719efd..2ab19ae 100644\n+Binary files a/text and b/text differ\n+EOF\n+test_expect_success 'diff -m respects binary attribute' '\n+\tgit show --format=%s -m >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+resolved\n+\n+diff --combined text\n+index 2bdf67a,f719efd..2ab19ae\n+Binary files differ\n+EOF\n+test_expect_success 'diff -c respects binary attribute' '\n+\tgit show --format=%s -c >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+resolved\n+\n+diff --cc text\n+index 2bdf67a,f719efd..2ab19ae\n+Binary files differ\n+EOF\n+test_expect_success 'diff --cc respects binary attribute' '\n+\tgit show --format=%s --cc >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.7.5.2.4.g43415\n"},{"id":"168526","messageId":"20110523203014.GD6298@sigill.intra.peff.net","threadId":"27428","inReplyTo":"20110523201529.GA6281@sigill.intra.peff.net","subject":"[PATCH 4/5] refactor get_textconv to not require diff_filespec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-23T20:30:14Z","receivedAt":"2011-05-23T20:30:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function actually does two things:\n\n 1. Load the userdiff driver for the filespec.\n\n 2. Decide whether the driver has a textconv component, and\n initialize the textconv cache if applicable.\n\nOnly part (1) requires the filespec object, and some callers\nmay not have a filespec at all. So let's split them it into\ntwo functions, and put part (2) with the userdiff code,\nwhich is a better fit.\n\nSigned-off-by: Jeff King \n---\nIt's a little funny to have \"userdiff_get_textconv(foo)\" return either\nNULL or \"foo\"; you are not really getting the textconv but rather seeing\nif there is a textconv available, and if so, preparing it. But that is\nnot a new thing; get_textconv was already like that.\n\nOne possible refactoring would be that the textconv (including the\ncache) should be a sub-structure of the userdiff driver. I chose not to\ndo that refactoring because of the amount of noise it would introduce in\ndiff.c. But I can do it if we care.\n\n diff.c | 14 +-------------\n userdiff.c | 17 +++++++++++++++++\n userdiff.h | 2 ++\n 3 files changed, 20 insertions(+), 13 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ba5f7aa..3f538f5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1976,19 +1976,7 @@ struct userdiff_driver *get_textconv(struct diff_filespec *one)\n \t\treturn NULL;\n \n \tdiff_filespec_load_driver(one);\n-\tif (!one->driver->textconv)\n-\t\treturn NULL;\n-\n-\tif (one->driver->textconv_want_cache && !one->driver->textconv_cache) {\n-\t\tstruct notes_cache *c = xmalloc(sizeof(*c));\n-\t\tstruct strbuf name = STRBUF_INIT;\n-\n-\t\tstrbuf_addf(&name, \"textconv/%s\", one->driver->name);\n-\t\tnotes_cache_init(c, name.buf, one->driver->textconv);\n-\t\tone->driver->textconv_cache = c;\n-\t}\n-\n-\treturn one->driver;\n+\treturn userdiff_get_textconv(one->driver);\n }\n \n static void builtin_diff(const char *name_a,\ndiff --git a/userdiff.c b/userdiff.c\nindex 1ff4797..5d62e79 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -267,3 +267,20 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)\n \t\treturn NULL;\n \treturn userdiff_find_by_name(check.value);\n }\n+\n+struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)\n+{\n+\tif (!driver->textconv)\n+\t\treturn NULL;\n+\n+\tif (driver->textconv_want_cache && !driver->textconv_cache) {\n+\t\tstruct notes_cache *c = xmalloc(sizeof(*c));\n+\t\tstruct strbuf name = STRBUF_INIT;\n+\n+\t\tstrbuf_addf(&name, \"textconv/%s\", driver->name);\n+\t\tnotes_cache_init(c, name.buf, driver->textconv);\n+\t\tdriver->textconv_cache = c;\n+\t}\n+\n+\treturn driver;\n+}\ndiff --git a/userdiff.h b/userdiff.h\nindex 942d594..4a7e78f 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -23,4 +23,6 @@ int userdiff_config(const char *k, const char *v);\n struct userdiff_driver *userdiff_find_by_name(const char *name);\n struct userdiff_driver *userdiff_find_by_path(const char *path);\n \n+struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver);\n+\n #endif /* USERDIFF */\n-- \n1.7.5.2.4.g43415\n"},{"id":"168527","messageId":"20110523203105.GE6298@sigill.intra.peff.net","threadId":"27428","inReplyTo":"20110523201529.GA6281@sigill.intra.peff.net","subject":"[PATCH 5/5] combine-diff: respect textconv attributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-23T20:31:05Z","receivedAt":"2011-05-23T20:31:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When doing a combined diff, we did not respect textconv\nattributes at all. This generally lead to us printing\n\"Binary files differ\" when we could show a combined diff of\nthe converted text.\n\nThis patch converts file contents according to textconv\nattributes. The implementation is slightly ugly; because the\ntextconv code is tightly linked with the diff_filespec code,\nwe temporarily create a diff_filespec during conversion. In\npractice, though, this should not create a performance\nproblem.\n\nSigned-off-by: Jeff King \n---\n combine-diff.c | 41 +++++++++++++----\n t/t4048-diff-combined-binary.sh | 99 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 131 insertions(+), 9 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 94a207f..fbed374 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -93,7 +93,8 @@ struct sline {\n \tunsigned long *p_lno;\n };\n \n-static char *grab_blob(const unsigned char *sha1, unsigned int mode, unsigned long *size)\n+static char *grab_blob(const unsigned char *sha1, unsigned int mode,\n+\t\t unsigned long *size, struct userdiff_driver *textconv)\n {\n \tchar *blob;\n \tenum object_type type;\n@@ -106,6 +107,13 @@ static char *grab_blob(const unsigned char *sha1, unsigned int mode, unsigned lo\n \t\t/* deleted blob */\n \t\t*size = 0;\n \t\treturn xcalloc(1, 1);\n+\t} else if (textconv) {\n+\t\t/* yuck, the textconv code is linked heavily with\n+\t\t * filespecs */\n+\t\tstruct diff_filespec *df = alloc_filespec(\"\");\n+\t\tfill_filespec(df, sha1, mode);\n+\t\t*size = fill_textconv(textconv, df, &blob);\n+\t\tfree_filespec(df);\n \t} else {\n \t\tblob = read_sha1_file(sha1, &type, size);\n \t\tif (type != OBJ_BLOB)\n@@ -205,7 +213,8 @@ static void consume_line(void *state_, char *line, unsigned long len)\n static void 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+\t\t\t int num_parent, int result_deleted,\n+\t\t\t struct userdiff_driver *textconv)\n {\n \tunsigned int p_lno, lno;\n \tunsigned long nmask = (1UL << n);\n@@ -218,7 +227,7 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,\n \tif (result_deleted)\n \t\treturn; /* result deleted */\n \n-\tparent_file.ptr = grab_blob(parent, mode, &sz);\n+\tparent_file.ptr = grab_blob(parent, mode, &sz, textconv);\n \tparent_file.size = sz;\n \tmemset(&xpp, 0, sizeof(xpp));\n \txpp.flags = 0;\n@@ -771,16 +780,20 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tint working_tree_file = is_null_sha1(elem->sha1);\n \tmmfile_t result_file;\n \tstruct userdiff_driver *userdiff;\n+\tstruct userdiff_driver *textconv = NULL;\n \tint is_binary;\n \n \tcontext = opt->context;\n \tuserdiff = userdiff_find_by_path(elem->path);\n \tif (!userdiff)\n \t\tuserdiff = userdiff_find_by_name(\"default\");\n+\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV))\n+\t\ttextconv = userdiff_get_textconv(userdiff);\n \n \t/* Read the result of merge first */\n \tif (!working_tree_file)\n-\t\tresult = grab_blob(elem->sha1, elem->mode, &result_size);\n+\t\tresult = grab_blob(elem->sha1, elem->mode, &result_size,\n+\t\t\t\t textconv);\n \telse {\n \t\t/* Used by diff-tree to read from the working tree */\n \t\tstruct stat st;\n@@ -803,9 +816,16 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t} else if (S_ISDIR(st.st_mode)) {\n \t\t\tunsigned char sha1[20];\n \t\t\tif (resolve_gitlink_ref(elem->path, \"HEAD\", sha1) < 0)\n-\t\t\t\tresult = grab_blob(elem->sha1, elem->mode, &result_size);\n+\t\t\t\tresult = grab_blob(elem->sha1, elem->mode,\n+\t\t\t\t\t\t &result_size, NULL);\n \t\t\telse\n-\t\t\t\tresult = grab_blob(sha1, elem->mode, &result_size);\n+\t\t\t\tresult = grab_blob(sha1, elem->mode,\n+\t\t\t\t\t\t &result_size, NULL);\n+\t\t} else if (textconv) {\n+\t\t\tstruct diff_filespec *df = alloc_filespec(elem->path);\n+\t\t\tfill_filespec(df, null_sha1, st.st_mode);\n+\t\t\tresult_size = fill_textconv(textconv, df, &result);\n+\t\t\tfree_filespec(df);\n \t\t} else if (0 <= (fd = open(elem->path, O_RDONLY))) {\n \t\t\tsize_t len = xsize_t(st.st_size);\n \t\t\tssize_t done;\n@@ -862,7 +882,9 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t}\n \t}\n \n-\tif (userdiff->binary != -1)\n+\tif (textconv)\n+\t\tis_binary = 0;\n+\telse if (userdiff->binary != -1)\n \t\tis_binary = userdiff->binary;\n \telse {\n \t\tis_binary = buffer_is_binary(result, result_size);\n@@ -871,7 +893,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tunsigned long size;\n \t\t\tbuf = grab_blob(elem->parent[i].sha1,\n \t\t\t\t\telem->parent[i].mode,\n-\t\t\t\t\t&size);\n+\t\t\t\t\t&size, NULL);\n \t\t\tif (buffer_is_binary(buf, size))\n \t\t\t\tis_binary = 1;\n \t\t\tfree(buf);\n@@ -932,7 +954,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\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\t\t\t cnt, i, num_parent, result_deleted,\n+\t\t\t\t textconv);\n \t}\n \n \tshow_hunks = make_hunks(sline, cnt, num_parent, dense);\ndiff --git a/t/t4048-diff-combined-binary.sh b/t/t4048-diff-combined-binary.sh\nindex a943994..87a8949 100755\n--- a/t/t4048-diff-combined-binary.sh\n+++ b/t/t4048-diff-combined-binary.sh\n@@ -110,4 +110,103 @@ test_expect_success 'diff --cc respects binary attribute' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup textconv attribute' '\n+\techo \"text diff=upcase\" >.gitattributes &&\n+\tgit config diff.upcase.textconv \"tr a-z A-Z <\"\n+'\n+\n+cat >expect <<'EOF'\n+resolved\n+\n+diff --git a/text b/text\n+index 2bdf67a..2ab19ae 100644\n+--- a/text\n++++ b/text\n+@@ -1 +1 @@\n+-THREE\n++RESOLVED\n+resolved\n+\n+diff --git a/text b/text\n+index f719efd..2ab19ae 100644\n+--- a/text\n++++ b/text\n+@@ -1 +1 @@\n+-TWO\n++RESOLVED\n+EOF\n+test_expect_success 'diff -m respects textconv attribute' '\n+\tgit show --format=%s -m >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+resolved\n+\n+diff --combined text\n+index 2bdf67a,f719efd..2ab19ae\n+--- a/text\n++++ b/text\n+@@@ -1,1 -1,1 +1,1 @@@\n+- THREE\n+ -TWO\n+++RESOLVED\n+EOF\n+test_expect_success 'diff -c respects textconv attribute' '\n+\tgit show --format=%s -c >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+resolved\n+\n+diff --cc text\n+index 2bdf67a,f719efd..2ab19ae\n+--- a/text\n++++ b/text\n+@@@ -1,1 -1,1 +1,1 @@@\n+- THREE\n+ -TWO\n+++RESOLVED\n+EOF\n+test_expect_success 'diff --cc respects textconv attribute' '\n+\tgit show --format=%s --cc >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+diff --combined text\n+index 2bdf67a,f719efd..2ab19ae\n+--- a/text\n++++ b/text\n+@@@ -1,1 -1,1 +1,1 @@@\n+- three\n+ -two\n+++resolved\n+EOF\n+test_expect_success 'diff-tree plumbing does not respect textconv' '\n+\tgit diff-tree HEAD -c -p >full &&\n+\ttail -n +2 full >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+diff --cc text\n+index 2bdf67a,f719efd..0000000\n+--- a/text\n++++ b/text\n+@@@ -1,1 -1,1 +1,5 @@@\n+++<<<<<<< HEAD\n+ +THREE\n+++=======\n++ TWO\n+++>>>>>>> MASTER\n+EOF\n+test_expect_success 'diff --cc respects textconv on worktree file' '\n+\tgit reset --hard HEAD^ &&\n+\ttest_must_fail git merge master &&\n+\tgit diff >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.5.2.4.g43415\n"},{"id":"168540","messageId":"7vwrhht4oj.fsf@alter.siamese.dyndns.org","threadId":"27428","inReplyTo":"20110523203105.GE6298@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] combine-diff: respect textconv attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-23T22:47:40Z","receivedAt":"2011-05-23T22:47:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +\t} else if (textconv) {\n> +\t\t/* yuck, the textconv code is linked heavily with\n> +\t\t * filespecs */\n> +\t\tstruct diff_filespec *df = alloc_filespec(\"\");\n> +\t\tfill_filespec(df, sha1, mode);\n> +\t\t*size = fill_textconv(textconv, df, &blob);\n\nI thought your 4/5 talked something about not requiring filespec for\ntextconv handling...? Is it still yuck?\n"},{"id":"168541","messageId":"BANLkTikvPjO=bbhAoPCftdXzGSYtryNvzw@mail.gmail.com","threadId":"27428","inReplyTo":"20110523201529.GA6281@sigill.intra.peff.net","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-05-23T22:55:07Z","receivedAt":"2011-05-23T22:55:07Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, May 23, 2011 at 4:15 PM, Jeff King wrote:\n> It turned out not too bad:\n>\n>  [1/5]: combine-diff: split header printing into its own function\n>  [2/5]: combine-diff: calculate mode_differs earlier\n>  [3/5]: combine-diff: handle binary files as binary\n\nTested-by: Jay Soffian \n\nIn a real-world merge, png's were correctly shown as \"Binary files\ndiffer\". I also tested with \"*.xib -diff\" and that worked as expected.\n\nHowever, custom diff drivers (still) don't work. :-)\n\nAlso read the patches and they LGTM.\n\n>  [4/5]: refactor get_textconv to not require diff_filespec\n>  [5/5]: combine-diff: respect textconv attributes\n\nDidn't test explicitly, but don't see anything obviously wrong.\n\nThanks Peff,\n\nj.\n"},{"id":"168543","messageId":"7vk4dht3z1.fsf@alter.siamese.dyndns.org","threadId":"27428","inReplyTo":"20110523202734.GC6298@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] combine-diff: handle binary files as binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-23T23:02:58Z","receivedAt":"2011-05-23T23:02:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The big question here is what we want the output to look like. I chose\n> this:\n>\n> diff --combined foo\n> index 7ea6ded,6197570..9563691\n> Binary files differ\n>\n> which contains all of the information we have: for file \"foo\" there were\n> two parents (at 7ea6ded and 6197570 respectively), and the resolution\n> was 9563691.\n\nYeah, I think the above is sane and matches more-or-less the conclusion of\nthe previous discussion Michael quoted.\n\nI've only read up to this patch (I wanted to conclude the first round of\ntoday's integration round before 17:00 local) and they all looked good;\nwill queue the whole thing to 'pu' in the meantime.\n"},{"id":"168547","messageId":"BANLkTikAS63wZ=VP9s6LE44MBd5T1X3gMA@mail.gmail.com","threadId":"27428","inReplyTo":"BANLkTikvPjO=bbhAoPCftdXzGSYtryNvzw@mail.gmail.com","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-05-23T23:31:02Z","receivedAt":"2011-05-23T23:31:02Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, May 23, 2011 at 6:55 PM, Jay Soffian wrote:\n>>  [4/5]: refactor get_textconv to not require diff_filespec\n>>  [5/5]: combine-diff: respect textconv attributes\n>\n> Didn't test explicitly, but don't see anything obviously wrong.\n\nOkay, tested these now. I noticed two tiny issues unrelated to this\nseries, but rather since this is the first time I've used textconv.\n\n1. The temporary path git generates loses the original file extension.\nAt least in the case of Mac OS X, ibtool gets angry when the \".xib\"\nisn' there. I worked around this with a wrapper, but maybe git could\npreserve the extension.\n\n2. It's not really clear to me when one should use textconv vs a\ncustom diff driver. (Also, --no-textconv isn't documented...).\n\nThanks,\n\nj.\n"},{"id":"168548","messageId":"20110523233916.GA10488@sigill.intra.peff.net","threadId":"27428","inReplyTo":"7vwrhht4oj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 5/5] combine-diff: respect textconv attributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-23T23:39:16Z","receivedAt":"2011-05-23T23:39:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 23, 2011 at 03:47:40PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > +\t} else if (textconv) {\n> > +\t\t/* yuck, the textconv code is linked heavily with\n> > +\t\t * filespecs */\n> > +\t\tstruct diff_filespec *df = alloc_filespec(\"\");\n> > +\t\tfill_filespec(df, sha1, mode);\n> > +\t\t*size = fill_textconv(textconv, df, &blob);\n> \n> I thought your 4/5 talked something about not requiring filespec for\n> textconv handling...? Is it still yuck?\n\nYeah, I removed the requirement for get_textconv to need a filespec.\nWhich is nice, because it means the non-textconv codepath doesn't deal\nwith this at all.\n\nBut removing the filespec from fill_textconv is much nastier, because it\nrelies on diff.c's prepare_temp_file, which also uses the filespec.\n\nSo it's still yuck, but limited to this little conditional. It would be\nnice if we used filespecs all the way through the combine-diff code\npath, but that was a bit more refactoring than I was up for today.\n\n-Peff\n"},{"id":"168550","messageId":"20110523234131.GB10488@sigill.intra.peff.net","threadId":"27428","inReplyTo":"BANLkTikvPjO=bbhAoPCftdXzGSYtryNvzw@mail.gmail.com","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-23T23:41:31Z","receivedAt":"2011-05-23T23:41:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 23, 2011 at 06:55:07PM -0400, Jay Soffian wrote:\n\n> On Mon, May 23, 2011 at 4:15 PM, Jeff King wrote:\n> > It turned out not too bad:\n> >\n> >  [1/5]: combine-diff: split header printing into its own function\n> >  [2/5]: combine-diff: calculate mode_differs earlier\n> >  [3/5]: combine-diff: handle binary files as binary\n> \n> Tested-by: Jay Soffian \n> \n> In a real-world merge, png's were correctly shown as \"Binary files\n> differ\". I also tested with \"*.xib -diff\" and that worked as expected.\n> \n> However, custom diff drivers (still) don't work. :-)\n\nYeah, I didn't add any support for that. I'm not sure what it should do;\ncustom diff drivers don't know how to handle combined diff, do they?\n\nIf you write me a test case that explains what _should_ happen, I'll see\nwhat I can do. :)\n\n-Peff\n"},{"id":"168551","messageId":"20110523234934.GC10488@sigill.intra.peff.net","threadId":"27428","inReplyTo":"BANLkTikAS63wZ=VP9s6LE44MBd5T1X3gMA@mail.gmail.com","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-23T23:49:34Z","receivedAt":"2011-05-23T23:49:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 23, 2011 at 07:31:02PM -0400, Jay Soffian wrote:\n\n> 1. The temporary path git generates loses the original file extension.\n> At least in the case of Mac OS X, ibtool gets angry when the \".xib\"\n> isn' there. I worked around this with a wrapper, but maybe git could\n> preserve the extension.\n\nHmm, I thought we did preserve the extension. I think it may actually be\nrelated to me making a fake filespec and not passing the path in.\n\nDoes the (totally untested) patch below fix it for you?\n\n> 2. It's not really clear to me when one should use textconv vs a\n> custom diff driver. (Also, --no-textconv isn't documented...).\n\nThe motivation for textconv was originally a combination of \"when you\nare too lazy to write a full diff driver\" and \"when you like how git\nformats the diff with pretty colors and word-diff\". But now I think we\ncan add to that:\n\n 1. When you want something fancy and git-ish like combined diff.\n\n 2. When you want free caching to speed up repeated log viewing (try\n setting diff.*.cachetextconv to true).\n\nMaybe it is worth adding an \"advantages of textconv\" section to\ngitattributes(5) (the advantage of a full external diff command is that\nit can be more flexible, or even graphical in nature).\n\nAnyway, here's the patch.\n\n---\ndiff --git a/combine-diff.c b/combine-diff.c\nindex fbed374..a4f2ff5 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -94,7 +94,8 @@ struct sline {\n };\n \n static char *grab_blob(const unsigned char *sha1, unsigned int mode,\n-\t\t unsigned long *size, struct userdiff_driver *textconv)\n+\t\t unsigned long *size, struct userdiff_driver *textconv,\n+\t\t const char *path)\n {\n \tchar *blob;\n \tenum object_type type;\n@@ -110,7 +111,7 @@ static char *grab_blob(const unsigned char *sha1, unsigned int mode,\n \t} else if (textconv) {\n \t\t/* yuck, the textconv code is linked heavily with\n \t\t * filespecs */\n-\t\tstruct diff_filespec *df = alloc_filespec(\"\");\n+\t\tstruct diff_filespec *df = alloc_filespec(path);\n \t\tfill_filespec(df, sha1, mode);\n \t\t*size = fill_textconv(textconv, df, &blob);\n \t\tfree_filespec(df);\n@@ -214,7 +215,8 @@ static void 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-\t\t\t struct userdiff_driver *textconv)\n+\t\t\t struct userdiff_driver *textconv,\n+\t\t\t const char *path)\n {\n \tunsigned int p_lno, lno;\n \tunsigned long nmask = (1UL << n);\n@@ -227,7 +229,7 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,\n \tif (result_deleted)\n \t\treturn; /* result deleted */\n \n-\tparent_file.ptr = grab_blob(parent, mode, &sz, textconv);\n+\tparent_file.ptr = grab_blob(parent, mode, &sz, textconv, path);\n \tparent_file.size = sz;\n \tmemset(&xpp, 0, sizeof(xpp));\n \txpp.flags = 0;\n@@ -793,7 +795,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t/* Read the result of merge first */\n \tif (!working_tree_file)\n \t\tresult = grab_blob(elem->sha1, elem->mode, &result_size,\n-\t\t\t\t textconv);\n+\t\t\t\t textconv, elem->path);\n \telse {\n \t\t/* Used by diff-tree to read from the working tree */\n \t\tstruct stat st;\n@@ -817,10 +819,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tunsigned char sha1[20];\n \t\t\tif (resolve_gitlink_ref(elem->path, \"HEAD\", sha1) < 0)\n \t\t\t\tresult = grab_blob(elem->sha1, elem->mode,\n-\t\t\t\t\t\t &result_size, NULL);\n+\t\t\t\t\t\t &result_size, NULL, NULL);\n \t\t\telse\n \t\t\t\tresult = grab_blob(sha1, elem->mode,\n-\t\t\t\t\t\t &result_size, NULL);\n+\t\t\t\t\t\t &result_size, NULL, NULL);\n \t\t} else if (textconv) {\n \t\t\tstruct diff_filespec *df = alloc_filespec(elem->path);\n \t\t\tfill_filespec(df, null_sha1, st.st_mode);\n@@ -893,7 +895,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tunsigned long size;\n \t\t\tbuf = grab_blob(elem->parent[i].sha1,\n \t\t\t\t\telem->parent[i].mode,\n-\t\t\t\t\t&size, NULL);\n+\t\t\t\t\t&size, NULL, NULL);\n \t\t\tif (buffer_is_binary(buf, size))\n \t\t\t\tis_binary = 1;\n \t\t\tfree(buf);\n@@ -955,7 +957,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\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\t\t\t textconv);\n+\t\t\t\t textconv, elem->path);\n \t}\n \n \tshow_hunks = make_hunks(sline, cnt, num_parent, dense);\n"},{"id":"168552","messageId":"20110523235035.GD10488@sigill.intra.peff.net","threadId":"27428","inReplyTo":"7vk4dht3z1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] combine-diff: handle binary files as binary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-23T23:50:35Z","receivedAt":"2011-05-23T23:50:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 23, 2011 at 04:02:58PM -0700, Junio C Hamano wrote:\n\n> Yeah, I think the above is sane and matches more-or-less the conclusion of\n> the previous discussion Michael quoted.\n> \n> I've only read up to this patch (I wanted to conclude the first round of\n> today's integration round before 17:00 local) and they all looked good;\n> will queue the whole thing to 'pu' in the meantime.\n\nThanks. I think there is a minor tweak to 5/5 that I posted elsewhere in\nthe thread. I'll do more testing with it and post tomorrow. I'm done\ngitting for today.\n\n-Peff\n"},{"id":"168560","messageId":"BANLkTi=nTZABRPLwq30Khs8KJHRt+jzNbA@mail.gmail.com","threadId":"27428","inReplyTo":"20110523234934.GC10488@sigill.intra.peff.net","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-05-24T00:59:51Z","receivedAt":"2011-05-24T00:59:51Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, May 23, 2011 at 7:49 PM, Jeff King wrote:\n> Hmm, I thought we did preserve the extension. I think it may actually be\n> related to me making a fake filespec and not passing the path in.\n>\n> Does the (totally untested) patch below fix it for you?\n\nYes.\n\n>> 2. It's not really clear to me when one should use textconv vs a\n>> custom diff driver. (Also, --no-textconv isn't documented...).\n>\n> The motivation for textconv was originally a combination of \"when you\n> are too lazy to write a full diff driver\" and \"when you like how git\n> formats the diff with pretty colors and word-diff\". But now I think we\n> can add to that:\n>\n>  1. When you want something fancy and git-ish like combined diff.\n>\n>  2. When you want free caching to speed up repeated log viewing (try\n>     setting diff.*.cachetextconv to true).\n\nGotcha.\n\n> Maybe it is worth adding an \"advantages of textconv\" section to\n> gitattributes(5) (the advantage of a full external diff command is that\n> it can be more flexible, or even graphical in nature).\n\nOkay.\n\nj.\n"},{"id":"168569","messageId":"7v39k4aeos.fsf@alter.siamese.dyndns.org","threadId":"27428","inReplyTo":"20110523234131.GB10488@sigill.intra.peff.net","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-24T04:46:27Z","receivedAt":"2011-05-24T04:46:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> However, custom diff drivers (still) don't work. :-)\n>\n> Yeah, I didn't add any support for that. I'm not sure what it should do;\n> custom diff drivers don't know how to handle combined diff, do they?\n>\n> If you write me a test case that explains what _should_ happen, I'll see\n> what I can do. :)\n\nI do not think it is sensible to expect anybody to come up with a sane\nsemantics for combined diff to work with GIT_EXTERNAL_DIFF (and external\ndiff driver that can be specified via the attributes mechanism) in any\nmeaningful way.\n\nThe whole point of the external diff mechanism is that an external command\ncan take _two_ files and represent the change between them in a way that\nis more suited for the need of the user than the patch form. The output\nfrom such an external command does not have any obligation to even follow\nthe convention used by the patch output, namely:\n\n @@ from here to there things have changed @@\n this is common\n -this was the removed content\n +this is the new content\n\nas the _whole_ point of the external diff mechanism is to give something\nthat is _different_ from the patch form, in the hope that it is in a more\nappropriate form for whoever consumes the output.\n\nOn the other hand, combined diff is all about combining multiple patches\nshow them side-by-side in a combined fashion. Without the above four kinds\nof cues, there is no way to even _align_ the change outputs from two\nparents, let alone _combining_ them.\n\nAnybody interested can check \"compare-cooking.perl\" in the todo branch,\nwhich is used as an external diff driver to view the differences between\n\"What's cooking\" postings via these:\n\n [diff \"whatscooking\"]\n xfuncname = \"^\\\\[(.*)\\\\]$\"\n command = ./compare-cooking.perl\n\nin the .git/config file, together with\n\n whats-cooking.txt diff=whatscooking\n\nin the .gitattributes file. Running\n\n $ git log -p --ext-diff todo -- whats-cooking.txt\n\nwould give a sample output.\n\nIt is conceivable that we _could_ newly define a \"combined external diff\ndriver\" that would take 3 or more files, and compute and show the combined\nresult by itself, but that will certainly not go through the codepath you\ntouched with the textconv patch. Calling out to such a new type of\nexternal diff driver would have to happen at the level where we have 1+N\nblob object names for a N-parent commit, namely, at the beginning of\nshow_patch_diff(), bypassing the entire contents of that function and\ninstead letting the new n-way external diff driver do everything.\n\nI however highly doubt that such an interface would make sense. For\nexample, what would be the desirable format to compare three versions of\n\"What's cooking\" postings, and how would the updated compare-cooking.perl\nscript would look like?\n"},{"id":"168571","messageId":"4DDB5C0F.1080102@drmicha.warpmail.net","threadId":"27428","inReplyTo":"7v39k4aeos.fsf@alter.siamese.dyndns.org","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-24T07:19:43Z","receivedAt":"2011-05-24T07:19:43Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"First of all:\n\nJeff, thanks a bunch for taking this up again! That's a great\nimprovement. (I'm not sure I can devote enough time to reviewing, but\nI'll see.)\n\nJunio C Hamano venit, vidit, dixit 24.05.2011 06:46:\n> Jeff King writes:\n> \n>>> However, custom diff drivers (still) don't work. :-)\n>>\n>> Yeah, I didn't add any support for that. I'm not sure what it should do;\n>> custom diff drivers don't know how to handle combined diff, do they?\n>>\n>> If you write me a test case that explains what _should_ happen, I'll see\n>> what I can do. :)\n> \n> I do not think it is sensible to expect anybody to come up with a sane\n> semantics for combined diff to work with GIT_EXTERNAL_DIFF (and external\n> diff driver that can be specified via the attributes mechanism) in any\n> meaningful way.\n> \n> The whole point of the external diff mechanism is that an external command\n> can take _two_ files and represent the change between them in a way that\n> is more suited for the need of the user than the patch form. The output\n> from such an external command does not have any obligation to even follow\n> the convention used by the patch output, namely:\n> \n> @@ from here to there things have changed @@\n> this is common\n> -this was the removed content\n> +this is the new content\n> \n> as the _whole_ point of the external diff mechanism is to give something\n> that is _different_ from the patch form, in the hope that it is in a more\n> appropriate form for whoever consumes the output.\n> \n> On the other hand, combined diff is all about combining multiple patches\n> show them side-by-side in a combined fashion. Without the above four kinds\n> of cues, there is no way to even _align_ the change outputs from two\n> parents, let alone _combining_ them.\n> \n> Anybody interested can check \"compare-cooking.perl\" in the todo branch,\n> which is used as an external diff driver to view the differences between\n> \"What's cooking\" postings via these:\n> \n> [diff \"whatscooking\"]\n> xfuncname = \"^\\\\[(.*)\\\\]$\"\n> command = ./compare-cooking.perl\n> \n> in the .git/config file, together with\n> \n> whats-cooking.txt diff=whatscooking\n> \n> in the .gitattributes file. Running\n> \n> $ git log -p --ext-diff todo -- whats-cooking.txt\n> \n> would give a sample output.\n> \n> It is conceivable that we _could_ newly define a \"combined external diff\n> driver\" that would take 3 or more files, and compute and show the combined\n> result by itself, but that will certainly not go through the codepath you\n> touched with the textconv patch. Calling out to such a new type of\n> external diff driver would have to happen at the level where we have 1+N\n> blob object names for a N-parent commit, namely, at the beginning of\n> show_patch_diff(), bypassing the entire contents of that function and\n> instead letting the new n-way external diff driver do everything.\n> \n> I however highly doubt that such an interface would make sense. For\n> example, what would be the desirable format to compare three versions of\n> \"What's cooking\" postings, and how would the updated compare-cooking.perl\n> script would look like?\n\nYeah, currently --cc with external makes no sense, but there are several\nexternal tools which could present a 3-way diff in a useful way (or even\nn-way with n>3), e.g. vimdiff, kdiff3, meld.\n\nWhen the --cc/textconv issue came up I looked into this, and maybe\ndifftool is a place where one could plug this in first in the sense of\nrefactoring that even more and providing a diff3tool or such to view a\nmerge commit (or compare any 3 versions), or/and provide \"git diff3 A B\nC\" which creates a fake merge (A+B -> C). Now, imagine we also have\nINDEX and WORKTREE pseudorevs and can do\n\ngit diff3[tool] HEAD INDEX WORKTREE\n\n:)\n\nMichael\n"},{"id":"168573","messageId":"BANLkTikxcnR_f6UEATfKTDZcq51Cv8V0TQ@mail.gmail.com","threadId":"27428","inReplyTo":"7v39k4aeos.fsf@alter.siamese.dyndns.org","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-05-24T14:40:34Z","receivedAt":"2011-05-24T14:40:34Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Tue, May 24, 2011 at 12:46 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>>> However, custom diff drivers (still) don't work. :-)\n>>\n>> Yeah, I didn't add any support for that. I'm not sure what it should do;\n>> custom diff drivers don't know how to handle combined diff, do they?\n>>\n>> If you write me a test case that explains what _should_ happen, I'll see\n>> what I can do. :)\n>\n> I do not think it is sensible to expect anybody to come up with a sane\n> semantics for combined diff to work with GIT_EXTERNAL_DIFF (and external\n> diff driver that can be specified via the attributes mechanism) in any\n> meaningful way.\n\nIndeed, it was a thinko that I even considered it. It turns out I had\nbeen using an external diff command where I should have been using\ntextconv all along.\n\nj.\n"},{"id":"168576","messageId":"7vsjs48616.fsf@alter.siamese.dyndns.org","threadId":"27428","inReplyTo":"4DDB5C0F.1080102@drmicha.warpmail.net","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-24T15:36:21Z","receivedAt":"2011-05-24T15:36:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Jeff, thanks a bunch for taking this up again! That's a great\n> improvement. (I'm not sure I can devote enough time to reviewing, but\n> I'll see.)\n\nThanks. I've given a cursory look at the rest of the series (including the\naddendum to be squashed in) and they seemed Ok. Will give it another round\nof eyeballing before merging to \"next\".\n\n>> I however highly doubt that such an interface would make sense. For\n>> example, what would be the desirable format to compare three versions of\n>> \"What's cooking\" postings, and how would the updated compare-cooking.perl\n>> script would look like?\n>\n> Yeah, currently --cc with external makes no sense, but there are several\n> external tools which could present a 3-way diff in a useful way (or even\n> n-way with n>3), e.g. vimdiff, kdiff3, meld.\n\nWith an external command that can make 3-way diffs of arbitrary three\ndirectories, you can trivially do something like:\n\n\t#!/bin/sh\n\tmkdir tmp-head tmp-index &&\n ( cd tmp-head &&\n GIT_DIR=../.git GIT_INDEX_FILE=$GIT_DIR/.index-head &&\n git checkout -f HEAD\n\t) &&\n ( cd tmp-index &&\n GIT_DIR=../.git &&\n git checkout -f .\n\t) &&\n $ext_diff3_cmd tmp-head/ tmp-index/ . &\n wait\n rm -fr tmp-index tmp-head\n\nBut that seems totally offtopic and has nothing to do with the \"combined\ndiff\" discussion, no?\n\nIf you want to plug in an external command that can make n-way diff of n\nfiles when some paths are still shown using the usual --cc codepath, then\nyou would need an interface totally different from the diff..cmd\ninterface for two-way diff to the external diff. I pointed at where to\nplug such a thing, but I do not think it would be of much use unless you\nare handing the whole n-trees to the external command (which essentially\nis what the above outline does). How would the user read the output that\ncomes out mixed from different codepaths, some from our own --cc while\nothers come from the external command, possibly opening separate windows\nand even worse grabbing control and getting the caller stuck until the\nuser closes that window?\n\n\tSide note: about getting stuck, will we see an update to the\n\tdiffstat count series by the end of this cycle? I do not mind\n\tcarrying it over to the next cycle at all, but I'd rather see\n\tsomething already started gets finished.\n\n> When the --cc/textconv issue came up I looked into this, and maybe\n> difftool is a place where one could plug this in first in the sense of\n> refactoring that even more and providing a diff3tool or such to view a\n> merge commit (or compare any 3 versions), or/and provide \"git diff3 A B\n> C\" which creates a fake merge (A+B -> C).\n\nYou do not need \"git diff3 A B C\" for a fake merge.\n\n\t$ git diff 61d7503d 2d22086 5bf29b9\n\nalready is a way to show you how the commit 61d7503d was created by\nmerging the other two (the merge result comes first and then its parents).\nYou could put the index into the mix by doing something like:\n\n\t$ git diff next master $(git write-tree)\n\nTrying to show combined diff to merge the index and the working tree into\nthe current HEAD (which may be an example that does not make much sense)\nwould look like this:\n\n\t$ git diff HEAD $(git write-tree) $(\n\t\tgit read-tree --index-output=.tmp-index HEAD &&\n\t\tGIT_INDEX_FILE=.tmp-index git add -A :/ &&\n GIT_INDEX_FILE=.tmp-index git write-tree\n\t)\n\nBut for the \"working tree\" set, which paths should be included? The same\nset as what is in the index? Or would we use the set that is the union of\nother tree-like things that are being compared, including the ones that\nare not in the index? Or everything in the working tree, as we are\ninterested in what the user _could_ add? That is one of the reasons why I\ndo not think it makes much sense trying to throw the working tree into the\npicture, as it would have to open a large can of worms.\n"},{"id":"168580","messageId":"7vk4dg83yy.fsf@alter.siamese.dyndns.org","threadId":"27428","inReplyTo":"20110523203105.GE6298@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] combine-diff: respect textconv attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-24T16:20:53Z","receivedAt":"2011-05-24T16:20:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This patch converts file contents according to textconv attributes. The\n> implementation is slightly ugly; because the textconv code is tightly\n> linked with the diff_filespec code, we temporarily create a\n> diff_filespec during conversion.\n\nAfter reading this patch again, I think this aversion to diff_filespec is\nprobably unjustified. It is primarily a structure that records what path\nhas which blob by recording its object name and what mode, and also\nin-core data when we make the contents available. There are small diff\nspecific data associated with it, but that could be separated out if you\nreally wanted to, perhaps like:\n\n\tstruct diff_filespec {\n \tstruct filespec {\n\t\t\t... the generic and essential part ...\n } spec;\n /* diff specific part follows */\n const char *funcname_pattern_ident;\n int xfrm_flags;\n int rename_used;\n unsigned dirty_submodule :2;\n struct userdiff_driver *driver;\n int is_binary;\n\t};\n\nand let most users use \"struct filespec\".\n\nIf anything else, we should be using the type in _more_ codepaths that are\nnot diff related but want to represent a path with its contents in the git\nnamespace (be it from working tree, index or a tree), not less, and in the\nlonger term weaken functions like fill_textconv() that take diff_filespec\nto take filespec so that they can be made more reusable.\n"},{"id":"168583","messageId":"4DDBDF0C.2040708@drmicha.warpmail.net","threadId":"27428","inReplyTo":"7vsjs48616.fsf@alter.siamese.dyndns.org","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-24T16:38:36Z","receivedAt":"2011-05-24T16:38:36Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 24.05.2011 17:36:\n> Michael J Gruber writes:\n> \n>> Jeff, thanks a bunch for taking this up again! That's a great\n>> improvement. (I'm not sure I can devote enough time to reviewing, but\n>> I'll see.)\n> \n> Thanks. I've given a cursory look at the rest of the series (including the\n> addendum to be squashed in) and they seemed Ok. Will give it another round\n> of eyeballing before merging to \"next\".\n> \n>>> I however highly doubt that such an interface would make sense. For\n>>> example, what would be the desirable format to compare three versions of\n>>> \"What's cooking\" postings, and how would the updated compare-cooking.perl\n>>> script would look like?\n>>\n>> Yeah, currently --cc with external makes no sense, but there are several\n>> external tools which could present a 3-way diff in a useful way (or even\n>> n-way with n>3), e.g. vimdiff, kdiff3, meld.\n> \n> With an external command that can make 3-way diffs of arbitrary three\n> directories, you can trivially do something like:\n> \n> \t#!/bin/sh\n> \tmkdir tmp-head tmp-index &&\n> ( cd tmp-head &&\n> GIT_DIR=../.git GIT_INDEX_FILE=$GIT_DIR/.index-head &&\n> git checkout -f HEAD\n> \t) &&\n> ( cd tmp-index &&\n> GIT_DIR=../.git &&\n> git checkout -f .\n> \t) &&\n> $ext_diff3_cmd tmp-head/ tmp-index/ . &\n> wait\n> rm -fr tmp-index tmp-head\n> \n\nand the fancy version of that is \"difftool\" in a sense, yes.\n\n> But that seems totally offtopic and has nothing to do with the \"combined\n> diff\" discussion, no?\n\nWell, it seemed that Jay wanted an external tool for merge diffs. \"-m\"\ndoes a series of simple 2way diffs, but \"--cc\" is special in that it\nlooks at all diffs at once, so a corresponding external tool would have\nto do the same.\n\n> If you want to plug in an external command that can make n-way diff of n\n> files when some paths are still shown using the usual --cc codepath, then\n> you would need an interface totally different from the diff..cmd\n> interface for two-way diff to the external diff. I pointed at where to\n> plug such a thing, but I do not think it would be of much use unless you\n> are handing the whole n-trees to the external command (which essentially\n> is what the above outline does). How would the user read the output that\n> comes out mixed from different codepaths, some from our own --cc while\n> others come from the external command, possibly opening separate windows\n> and even worse grabbing control and getting the caller stuck until the\n> user closes that window?\n\nI don't quite follow, I think I/we/? am/are mixing external diff drivers\nand difftools. I agree that is a side track/off-topic.\n\n> \n> \tSide note: about getting stuck, will we see an update to the\n> \tdiffstat count series by the end of this cycle? I do not mind\n> \tcarrying it over to the next cycle at all, but I'd rather see\n> \tsomething already started gets finished.\n\nYes, on my list. End of month you said, right?\n\n>> When the --cc/textconv issue came up I looked into this, and maybe\n>> difftool is a place where one could plug this in first in the sense of\n>> refactoring that even more and providing a diff3tool or such to view a\n>> merge commit (or compare any 3 versions), or/and provide \"git diff3 A B\n>> C\" which creates a fake merge (A+B -> C).\n> \n> You do not need \"git diff3 A B C\" for a fake merge.\n> \n> \t$ git diff 61d7503d 2d22086 5bf29b9\n> \n> already is a way to show you how the commit 61d7503d was created by\n> merging the other two (the merge result comes first and then its parents).\n\nYes. (I came across this when I investigated a bit back then but failed\nto mention it.)\n\n> You could put the index into the mix by doing something like:\n> \n> \t$ git diff next master $(git write-tree)\n> \n> Trying to show combined diff to merge the index and the working tree into\n> the current HEAD (which may be an example that does not make much sense)\n> would look like this:\n> \n> \t$ git diff HEAD $(git write-tree) $(\n> \t\tgit read-tree --index-output=.tmp-index HEAD &&\n> \t\tGIT_INDEX_FILE=.tmp-index git add -A :/ &&\n> GIT_INDEX_FILE=.tmp-index git write-tree\n> \t)\n> \n> But for the \"working tree\" set, which paths should be included? The same\n> set as what is in the index? Or would we use the set that is the union of\n> other tree-like things that are being compared, including the ones that\n> are not in the index? Or everything in the working tree, as we are\n> interested in what the user _could_ add? That is one of the reasons why I\n> do not think it makes much sense trying to throw the working tree into the\n> picture, as it would have to open a large can of worms.\n\nI've simply been drooling too much over vim-fugitive and was wondering\nwhich aspects would fit into our ui. I was thinking of a 3-way-diff\nversion of git status, so to say. Next would be a 3-way-merge interface\nwhich does all your add/reset/checkout -p from 3 vim bufs in diff mode,\nwhich even vim-fugitive doesn't do. We can all dream, can't we?\n\nMichael\n"},{"id":"168585","messageId":"7v7h9g82xx.fsf@alter.siamese.dyndns.org","threadId":"27428","inReplyTo":"4DDBDF0C.2040708@drmicha.warpmail.net","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-24T16:43:06Z","receivedAt":"2011-05-24T16:43:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n>> \tSide note: about getting stuck, will we see an update to the\n>> \tdiffstat count series by the end of this cycle? I do not mind\n>> \tcarrying it over to the next cycle at all, but I'd rather see\n>> \tsomething already started gets finished.\n>\n> Yes, on my list. End of month you said, right?\n\nYes, but if you post anything on May 31st, it will never have enough time\nto land in 'master' before the feature freeze ;-).\n\n> I've simply been drooling too much over vim-fugitive and was wondering\n> which aspects would fit into our ui.\n\nHeh.\n"},{"id":"168587","messageId":"BANLkTi=XbiZ0CVdwi8upnxVG26HyB9pmrg@mail.gmail.com","threadId":"27428","inReplyTo":"4DDBDF0C.2040708@drmicha.warpmail.net","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-05-24T16:52:01Z","receivedAt":"2011-05-24T16:52:01Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Tue, May 24, 2011 at 12:38 PM, Michael J Gruber\n wrote:\n> Well, it seemed that Jay wanted an external tool for merge diffs.\n\nOnly because I wasn't using textconv, but I should have been.\n\nj.\n"},{"id":"168589","messageId":"20110524185246.GA584@sigill.intra.peff.net","threadId":"27428","inReplyTo":"7vk4dg83yy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 5/5] combine-diff: respect textconv attributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-24T18:52:46Z","receivedAt":"2011-05-24T18:52:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 24, 2011 at 09:20:53AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > This patch converts file contents according to textconv attributes. The\n> > implementation is slightly ugly; because the textconv code is tightly\n> > linked with the diff_filespec code, we temporarily create a\n> > diff_filespec during conversion.\n> \n> After reading this patch again, I think this aversion to diff_filespec is\n> probably unjustified.\n\nIt's not an aversion to diff_filespec; it's an aversion to only half\nbuying-into diff_filespec. I think the best thing would be rewriting all\nof combine-diff in terms of diff_filespec; I just started to do it and\nit looked big and ugly for not much gain.\n\nIt's a little ugly to have to manually do the conversion into another\ndata structure just to call a function like fill_textconv, but I can\nlive with it. What I worry about more is that the ad-hoc use of a\nstructure like diff_filespec means we are violating some assumption that\nother code has about the data structure (e.g., the bug Jay already\nuncovered that we must have a valid name in the \"path\" field).\n\nI think what is there now is correct; it's just that as a general rule,\nswitching data formats or abstractions in the middle of code makes me\nfeel I'm doing something wrong (or that the code interfaces need to be\nrefactored).\n\n> If anything else, we should be using the type in _more_ codepaths that are\n> not diff related but want to represent a path with its contents in the git\n> namespace (be it from working tree, index or a tree), not less, and in the\n> longer term weaken functions like fill_textconv() that take diff_filespec\n> to take filespec so that they can be made more reusable.\n\nYeah, I would agree with that.\n\n-Peff\n"},{"id":"168590","messageId":"20110524191337.GB584@sigill.intra.peff.net","threadId":"27428","inReplyTo":"4DDB5C0F.1080102@drmicha.warpmail.net","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-24T19:13:37Z","receivedAt":"2011-05-24T19:13:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 24, 2011 at 09:19:43AM +0200, Michael J Gruber wrote:\n\n> > It is conceivable that we _could_ newly define a \"combined external diff\n> > driver\" that would take 3 or more files, and compute and show the combined\n> > result by itself, but that will certainly not go through the codepath you\n> > touched with the textconv patch. Calling out to such a new type of\n> > external diff driver would have to happen at the level where we have 1+N\n> > blob object names for a N-parent commit, namely, at the beginning of\n> > show_patch_diff(), bypassing the entire contents of that function and\n> > instead letting the new n-way external diff driver do everything.\n> > \n> > I however highly doubt that such an interface would make sense. For\n> > example, what would be the desirable format to compare three versions of\n> > \"What's cooking\" postings, and how would the updated compare-cooking.perl\n> > script would look like?\n> \n> Yeah, currently --cc with external makes no sense, but there are several\n> external tools which could present a 3-way diff in a useful way (or even\n> n-way with n>3), e.g. vimdiff, kdiff3, meld.\n\nI agree with Junio that we would need a new config option and external\ninterface for \"n-way combined diff\". However, isn't what things like\nvimdiff and meld do the reverse of our combined diff? That is, don't\nthey assume the 3 trees are: ours, theirs, and ancestor (i.e., merge\nbase)? Whereas in a combined diff, it is actually: merge parent 1\n(ours), merge parent 2 (theirs), and merge _result_.\n\nAlso, do those tools generally handle n-way comparisons as opposed to\n3-way?\n\n-Peff\n"},{"id":"168629","messageId":"4DDCB203.3020802@drmicha.warpmail.net","threadId":"27428","inReplyTo":"20110524191337.GB584@sigill.intra.peff.net","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-25T07:38:43Z","receivedAt":"2011-05-25T07:38:43Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 24.05.2011 21:13:\n> On Tue, May 24, 2011 at 09:19:43AM +0200, Michael J Gruber wrote:\n> \n>>> It is conceivable that we _could_ newly define a \"combined external diff\n>>> driver\" that would take 3 or more files, and compute and show the combined\n>>> result by itself, but that will certainly not go through the codepath you\n>>> touched with the textconv patch. Calling out to such a new type of\n>>> external diff driver would have to happen at the level where we have 1+N\n>>> blob object names for a N-parent commit, namely, at the beginning of\n>>> show_patch_diff(), bypassing the entire contents of that function and\n>>> instead letting the new n-way external diff driver do everything.\n>>>\n>>> I however highly doubt that such an interface would make sense. For\n>>> example, what would be the desirable format to compare three versions of\n>>> \"What's cooking\" postings, and how would the updated compare-cooking.perl\n>>> script would look like?\n>>\n>> Yeah, currently --cc with external makes no sense, but there are several\n>> external tools which could present a 3-way diff in a useful way (or even\n>> n-way with n>3), e.g. vimdiff, kdiff3, meld.\n> \n> I agree with Junio that we would need a new config option and external\n> interface for \"n-way combined diff\". However, isn't what things like\n> vimdiff and meld do the reverse of our combined diff? That is, don't\n> they assume the 3 trees are: ours, theirs, and ancestor (i.e., merge\n> base)? Whereas in a combined diff, it is actually: merge parent 1\n> (ours), merge parent 2 (theirs), and merge _result_.\n> \n> Also, do those tools generally handle n-way comparisons as opposed to\n> 3-way?\n\n\"Those\" tools do \"that\" which I mean :)\n\nIt depends on the tool, of course. vimdiff does not have any assumptions\nthat I know of, it marks those hunks which are not common to all\nbuffers, and marks them differently depending on what subset of buffers\nshares them. That aspect is the same for kdiff3 and meld (n<=3 for meld)\nin diff mode.\n\nThere are more differences in merge mode:\n\nIn vimdiff, you can \"put\" a hunk (i.e. apply the change) to another\nbuffer (or \"obtain\" one), i.e. you can easily move hunks between the\nbuffers (to do a merge). In kdiff3's auto merge mode there is the\nassumption you mention (because it actively does some merging).\n\nSo, in vimdiff, you could in principle change and write any buffer but\nour tools don't support it so far because it is not needed for a merge\nwhich has one \"target\" only. In the ui of my dreams, I would have 3\nbuffers HEAD INDEX WORKTREE (H I W) and moving hunks between them would\ndo all of add -p, reset -p and checkout -p (the HEAD buf would be\nread-only).\n\nWith \"differently abled\" tools it could still be useful to have, say a\ntool invoked for the merge HEAD+WORKTREE -> INDEX (with the current\nstate of the INDEX as the automatic resolution to start off from) so\nthat you can do add+reset -p with your mergetool, and maybe similarly\nfor HEAD+INDEX -> WORKTREE. I.e. addtool and checkouttool in addition to\nthe difftool and mergetool which we have. Just not for the current\nrelease cycle any more.\n\nMichael\n"},{"id":"168670","messageId":"20110525152935.GD8795@sigill.intra.peff.net","threadId":"27428","inReplyTo":"4DDCB203.3020802@drmicha.warpmail.net","subject":"Re: combined diff does not detect binary files and ignores -diff attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-25T15:29:35Z","receivedAt":"2011-05-25T15:29:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 25, 2011 at 09:38:43AM +0200, Michael J Gruber wrote:\n\n> > I agree with Junio that we would need a new config option and external\n> > interface for \"n-way combined diff\". However, isn't what things like\n> > vimdiff and meld do the reverse of our combined diff? That is, don't\n> > they assume the 3 trees are: ours, theirs, and ancestor (i.e., merge\n> > base)? Whereas in a combined diff, it is actually: merge parent 1\n> > (ours), merge parent 2 (theirs), and merge _result_.\n> > \n> > Also, do those tools generally handle n-way comparisons as opposed to\n> > 3-way?\n> \n> \"Those\" tools do \"that\" which I mean :)\n\nOK, they are more capable than I realized, then. :)\n\n> So, in vimdiff, you could in principle change and write any buffer but\n> our tools don't support it so far because it is not needed for a merge\n> which has one \"target\" only. In the ui of my dreams, I would have 3\n> buffers HEAD INDEX WORKTREE (H I W) and moving hunks between them would\n> do all of add -p, reset -p and checkout -p (the HEAD buf would be\n> read-only).\n\nThis should be relatively easy to implement on the git side, as you are\ndoing the hard parts of the \"-p\" operation in vim already. You just need\nto write vim's buffer into the worktree or into the index via\nhash-object / update-index.\n\nAlso, wouldn't you potentially want more than 3 buffers, if you were\ncherry-picking changes from another commit (as in \"git checkout -p\n$commit\" or even putting and taking things from a stash? I think that\nwould be a simple generalization of what you are proposing though.\n\n> With \"differently abled\" tools it could still be useful to have, say a\n> tool invoked for the merge HEAD+WORKTREE -> INDEX (with the current\n> state of the INDEX as the automatic resolution to start off from) so\n> that you can do add+reset -p with your mergetool, and maybe similarly\n> for HEAD+INDEX -> WORKTREE. I.e. addtool and checkouttool in addition to\n> the difftool and mergetool which we have. Just not for the current\n> release cycle any more.\n\nYeah, getting back to the original discussion. How badly do people\nactually want an external diff driver that can do fancy things with\nmultiple parents? It seems that for non-text diff viewers, the preferred\nsolution is to use difftool these days (but that is just my impression;\nI don't use either difftool or external diff drivers).\n\n-Peff\n"},{"id":"168975","messageId":"7vpqn0ofy5.fsf@alter.siamese.dyndns.org","threadId":"27428","inReplyTo":"20110523202734.GC6298@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] combine-diff: handle binary files as binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-30T06:33:38Z","receivedAt":"2011-05-30T06:33:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> @@ -852,6 +862,29 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n> \t\t}\n> \t}\n> \n> +\tif (userdiff->binary != -1)\n> +\t\tis_binary = userdiff->binary;\n> +\telse {\n> +\t\tis_binary = buffer_is_binary(result, result_size);\n> +\t\tfor (i = 0; !is_binary && i < num_parent; i++) {\n> +\t\t\tchar *buf;\n> +\t\t\tunsigned long size;\n> +\t\t\tbuf = grab_blob(elem->parent[i].sha1,\n> +\t\t\t\t\telem->parent[i].mode,\n> +\t\t\t\t\t&size);\n> +\t\t\tif (buffer_is_binary(buf, size))\n> +\t\t\t\tis_binary = 1;\n> +\t\t\tfree(buf);\n> +\t\t}\n> +\t}\n\nTwo comments.\n\n - This loop will grab the blob from all parents just to peek and discard\n for most of commits. It feels somewhat wasteful, especially because\n \"binary\" that is not marked as binary is a rare minor case (most of the\n paths are text). Stopping immediately at the first binary blob does not\n optimize the loop even though it is better than not having one.\n\n - It may make sense to compare [i].sha1 with earlier [j].sha1 (j < i) and\n avoid grab_blob() altogether? Cf. 3c39e9b (combine-diff: reuse diff\n from the same blob., 2006-02-01).\n"},{"id":"168997","messageId":"20110530143627.GC31490@sigill.intra.peff.net","threadId":"27428","inReplyTo":"7vpqn0ofy5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] combine-diff: handle binary files as binary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-30T14:36:27Z","receivedAt":"2011-05-30T14:36:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 29, 2011 at 11:33:38PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > @@ -852,6 +862,29 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n> > \t\t}\n> > \t}\n> > \n> > +\tif (userdiff->binary != -1)\n> > +\t\tis_binary = userdiff->binary;\n> > +\telse {\n> > +\t\tis_binary = buffer_is_binary(result, result_size);\n> > +\t\tfor (i = 0; !is_binary && i < num_parent; i++) {\n> > +\t\t\tchar *buf;\n> > +\t\t\tunsigned long size;\n> > +\t\t\tbuf = grab_blob(elem->parent[i].sha1,\n> > +\t\t\t\t\telem->parent[i].mode,\n> > +\t\t\t\t\t&size);\n> > +\t\t\tif (buffer_is_binary(buf, size))\n> > +\t\t\t\tis_binary = 1;\n> > +\t\t\tfree(buf);\n> > +\t\t}\n> > +\t}\n> \n> Two comments.\n> \n> - This loop will grab the blob from all parents just to peek and discard\n> for most of commits. It feels somewhat wasteful, especially because\n> \"binary\" that is not marked as binary is a rare minor case (most of the\n> paths are text). Stopping immediately at the first binary blob does not\n> optimize the loop even though it is better than not having one.\n\nYeah, I am not too happy with that bit. Our choices are:\n\n 1. Grab each blob, check binary-ness, and free. This double-loads in\n the common, non-binary case.\n\n 2. Grab each blob, check binary-ness, and keep it in memory. This\n means using N times as much memory, where N is the number of\n parents. In practice, N generally equals 2, and the file sizes\n aren't gigantic, so it's not that bad. But there are some corner\n cases.\n\n 3. Choose (1) or (2) based on file size, or based on the number of\n parents. The problem cases for (2) are going to be big files\n (bigger than bigFileThreshold?), and gigantic numbers of parents.\n\nI'd also eventually like to do a \"peek\" binary-ness check on top of your\nstreaming work, as I showed for the non-combined diff case (speaking of\nwhich, I need to polish that now that the streaming series seems to have\nsettled). And we would probably want to peek only in the big file case.\n\nI'll try to take a look at it this week and get some measurements on (1)\nversus (2) for both speed and peak memory usage. And then see if I can\ndo better with (3), and implement the \"peek\" solution both here and in\nregular diff.\n\n> - It may make sense to compare [i].sha1 with earlier [j].sha1 (j < i) and\n> avoid grab_blob() altogether? Cf. 3c39e9b (combine-diff: reuse diff\n> from the same blob., 2006-02-01).\n\nYeah, that is probably worth doing, as it collapses the \"N\" above into\n\"how many parents all modified the same file\". I'll add it to my list to\nmeasure.\n\n-Peff\n"},{"id":"169010","messageId":"20110530161927.GC24431@sigill.intra.peff.net","threadId":"27428","inReplyTo":"20110530143627.GC31490@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] combine-diff: handle binary files as binary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-30T16:19:27Z","receivedAt":"2011-05-30T16:19:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 30, 2011 at 10:36:27AM -0400, Jeff King wrote:\n\n> 1. Grab each blob, check binary-ness, and free. This double-loads in\n> the common, non-binary case.\n> [...]\n>\n> I'll try to take a look at it this week and get some measurements on (1)\n> versus (2) for both speed and peak memory usage. And then see if I can\n> do better with (3), and implement the \"peek\" solution both here and in\n> regular diff.\n\nI was curious about this, so I stole a few minutes to do some\npreliminary benchmarks this morning.\n\nThe first thing to look at is the performance of the original code, that\ndoes not check binary-ness at all. It's going to represent the best we\ncan do with any strategy. So I tried:\n\n git log -p --cc --merges origin/master\n\non git.git using both v1.7.5.3 and the jk/combine-diff-binary-etc\nbranch. And it turns out that the extra loads really don't make a\ndifference in practice. My best-of-5 for the two cases were:\n\n $ time git.v1.7.5.3 log -p --cc --merges origin/master >/dev/null\n real 0m59.518s\n user 0m58.672s\n sys 0m0.688s\n\n $ time git.jk.binary-combined-diff log -p --cc \\\n --merges origin/master >/dev/null\n real 0m58.949s\n user 0m58.220s\n sys 0m0.572s\n\nThe new code actually came out slightly faster. One reason may be that\nthere are 3 combined diffs of git-gui/lib/git-gui.ico that we avoid\ndoing (and just say \"Binary files differ\"). That's not a lot, but it\ngives us a very tiny edge (though that edge is very close to the amount\nof noise between runs). Still, I think it implies that the extra loads\nin the common non-binary case are not actually measurable.\n\nThe peak memory use between the two should be the same (since we free\neach blob immediately), but I didn't measure it.\n\nSo I think in practice it's not a big deal. I'll still take a look at\nthe \"peek\" optimization later this week, since that can make a\ndifference in some corner cases. And as part of that, it will probably\nmake sense to keep the buffers around for small-ish files, so we'll get\nthe optimization I mentioned more or less for free. I'll also do the\ncheck for duplicated sha1s that you mentioned.\n\n-Peff\n"},{"id":"169018","messageId":"7vipssm1bb.fsf@alter.siamese.dyndns.org","threadId":"27428","inReplyTo":"20110530161927.GC24431@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] combine-diff: handle binary files as binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-30T19:32:40Z","receivedAt":"2011-05-30T19:32:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> git log -p --cc --merges origin/master\n>\n> on git.git using both v1.7.5.3 and the jk/combine-diff-binary-etc\n> branch. And it turns out that the extra loads really don't make a\n> difference in practice. My best-of-5 for the two cases were:\n\nI am not surprised. git.git is small enough and developer machines these\ndays are beefy enough so that most of the data from the disk are sitting\nin the buffer cache, and if you are well packed and Linus's law \"a file\nalways grows\" holds true, it is likely that you are also benefiting from\nthe delta-base cache, as the history traversal of the command goes newer\nto older.\n\n> $ time git.v1.7.5.3 log -p --cc --merges origin/master >/dev/null\n> real 0m59.518s\n> user 0m58.672s\n> sys 0m0.688s\n\nComparing that with the run time of the same log without patch generation\nwould roughly give the cost of \"all text combined\" diff. I suspect more\nthan 90% of the time goes to running the textual diff algorithm.\n\nI guess you are hinting that we should optimize that part before worrying\nabout the wastage of reading these blobs always twice.\n"},{"id":"169083","messageId":"7vlixmija0.fsf@alter.siamese.dyndns.org","threadId":"27428","inReplyTo":"20110530143627.GC31490@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] combine-diff: handle binary files as binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-31T22:42:47Z","receivedAt":"2011-05-31T22:42:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... speaking of\n> which, I need to polish that now that the streaming series seems to have\n> settled...\n\nI merely put it on the back burner, not necessarily declaring it as\n\"settled\".\n\nI just created a pair of test repository with a single 800M binary junk\nfile. One repository has it as a single loose object file, and the other\nrepository has it in a packfile. The path is not explicitly marked with\nany attributes, but I have no CRLF funniness configured, so streaming\nwould be used but there won't be any filtering involved.\n\nRemoving the file from the working tree and then checking it out of the\nindex gave me a pleasant surprise. I originally did this only to help\nsmaller machines that cannot comfortably fit inflated blob data in core,\nbut it turns out that streaming write seems to perform better.\n\nThese are the typical /usr/bin/time output:\n\n $ /usr/bin/time git checkout a ;# w/o streaming\n 1.39user 2.23system 0:03.62elapsed 99%CPU (0avgtext+0avgdata 6297056maxresident)k\n 0inputs+1572872outputs (0major+399828minor)pagefaults 0swaps\n\n $ /usr/bin/time git checkout a ;# w/ streaming\n 1.35user 1.22system 0:02.52elapsed 101%CPU (0avgtext+0avgdata 3151536maxresident)k\n 0inputs+1572872outputs (0major+203226minor)pagefaults 0swaps\n\nSo now I can say I am reasonably happy with the series, but I haven't\nreally exercised the code heavily, so there may still be bugs lurking ;-).\n"}]}