{"thread":{"id":"26538","subject":"Commit/merge messages for binary files","startedAt":"2011-02-17T20:40:08Z","lastAt":"2011-02-22T19:00:20Z","messageCount":12,"participants":["Piotr Krukowiecki","Matthieu Moy","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"161450","messageId":"AANLkTikXMi92iUd-1bEfs5WfawyHp4G7=Ynd+eaq_wsR@mail.gmail.com","threadId":"26538","inReplyTo":null,"subject":"Commit/merge messages for binary files","fromName":"Piotr Krukowiecki","fromEmail":"piotr.krukowiecki@gmail.com","sentAt":"2011-02-17T20:40:08Z","receivedAt":"2011-02-17T20:40:08Z","isPatch":false,"sender":{"key":"piotr.krukowiecki@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3259959?v=4"},"body":"Hi,\n\nthere's a different output when committing change and when merging\nchange for a binary file.\nDo the insertions/deletions have any meaning for binary files?\n\nFor example:\n\n$ git commit -m Updated\n[topic 5da30ce] Updated\n 1 files changed, 8 insertions(+), 103 deletions(-)\n rewrite blob.o (100%)\n\n$ git merge topic\nUpdating 75ab259..5da30ce\nFast-forward\n blob.o | Bin 25920 -> 4364 bytes\n 1 files changed, 0 insertions(+), 0 deletions(-)\n\n-- \nPiotrek\n"},{"id":"161542","messageId":"vpq39nlsb3r.fsf@bauges.imag.fr","threadId":"26538","inReplyTo":"AANLkTikXMi92iUd-1bEfs5WfawyHp4G7=Ynd+eaq_wsR@mail.gmail.com","subject":"Re: Commit/merge messages for binary files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-02-18T13:53:12Z","receivedAt":"2011-02-18T13:53:12Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Piotr Krukowiecki writes:\n\n> Hi,\n>\n> there's a different output when committing change and when merging\n> change for a binary file.\n> Do the insertions/deletions have any meaning for binary files?\n\nNo. They're inserted/deleted *lines*, and that wouldn't make sense for\nbinary files.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"161562","messageId":"4D5ED6F2.8030008@gmail.com","threadId":"26538","inReplyTo":"vpq39nlsb3r.fsf@bauges.imag.fr","subject":"Re: Commit/merge messages for binary files","fromName":"Piotr Krukowiecki","fromEmail":"piotr.krukowiecki@gmail.com","sentAt":"2011-02-18T20:30:42Z","receivedAt":"2011-02-18T20:30:42Z","isPatch":false,"sender":{"key":"piotr.krukowiecki@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3259959?v=4"},"body":"W dniu 18.02.2011 14:53, Matthieu Moy pisze:\n> Piotr Krukowiecki writes:\n> \n>> Hi,\n>>\n>> there's a different output when committing change and when merging\n>> change for a binary file.\n>> Do the insertions/deletions have any meaning for binary files?\n> \n> No. They're inserted/deleted *lines*, and that wouldn't make sense for\n> binary files.\n> \n\nSo it's a bug?\n\n-- \nPiotr Krukowiecki\n"},{"id":"161563","messageId":"vpqwrkx9h2z.fsf@bauges.imag.fr","threadId":"26538","inReplyTo":"4D5ED6F2.8030008@gmail.com","subject":"Re: Commit/merge messages for binary files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-02-18T21:19:00Z","receivedAt":"2011-02-18T21:19:00Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Piotr Krukowiecki writes:\n\n> W dniu 18.02.2011 14:53, Matthieu Moy pisze:\n>> Piotr Krukowiecki writes:\n>> \n>>> Hi,\n>>>\n>>> there's a different output when committing change and when merging\n>>> change for a binary file.\n>>> Do the insertions/deletions have any meaning for binary files?\n>> \n>> No. They're inserted/deleted *lines*, and that wouldn't make sense for\n>> binary files.\n>\n> So it's a bug?\n\nI don't see any bug. There were no insertion/deletion in text files,\nhence you see 0 for both.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"161565","messageId":"4D5EE7D2.5060703@gmail.com","threadId":"26538","inReplyTo":"vpqwrkx9h2z.fsf@bauges.imag.fr","subject":"Re: Commit/merge messages for binary files","fromName":"Piotr Krukowiecki","fromEmail":"piotr.krukowiecki@gmail.com","sentAt":"2011-02-18T21:42:42Z","receivedAt":"2011-02-18T21:42:42Z","isPatch":false,"sender":{"key":"piotr.krukowiecki@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3259959?v=4"},"body":"W dniu 18.02.2011 22:19, Matthieu Moy pisze:\n> Piotr Krukowiecki writes:\n> \n>> W dniu 18.02.2011 14:53, Matthieu Moy pisze:\n>>> Piotr Krukowiecki writes:\n>>>\n>>>> Hi,\n>>>>\n>>>> there's a different output when committing change and when merging\n>>>> change for a binary file.\n>>>> Do the insertions/deletions have any meaning for binary files?\n>>>\n>>> No. They're inserted/deleted *lines*, and that wouldn't make sense for\n>>> binary files.\n>>\n>> So it's a bug?\n> \n> I don't see any bug. There were no insertion/deletion in text files,\n> hence you see 0 for both.\n\nMaybe I wasn't clear. Both the commit and the merge was for the same binary\nfile.\n\nIn case of commit there were non-zero insertions/deletions. \nSee example below - notice both update binary file blob.o:\n\n$ git commit -m Updated\n[topic 5da30ce] Updated\n 1 files changed, 8 insertions(+), 103 deletions(-)\n rewrite blob.o (100%)\n\n$ git merge topic\nUpdating 75ab259..5da30ce\nFast-forward\n blob.o | Bin 25920 -> 4364 bytes\n 1 files changed, 0 insertions(+), 0 deletions(-)\n\n\nHope that helps,\n\n-- \nPiotr Krukowiecki\n"},{"id":"161586","messageId":"20110219070601.GA14059@sigill.intra.peff.net","threadId":"26538","inReplyTo":"4D5EE7D2.5060703@gmail.com","subject":"Re: Commit/merge messages for binary files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-19T07:06:01Z","receivedAt":"2011-02-19T07:06:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 18, 2011 at 10:42:42PM +0100, Piotr Krukowiecki wrote:\n\n> Maybe I wasn't clear. Both the commit and the merge was for the same binary\n> file.\n> \n> In case of commit there were non-zero insertions/deletions. \n> See example below - notice both update binary file blob.o:\n> \n> $ git commit -m Updated\n> [topic 5da30ce] Updated\n> 1 files changed, 8 insertions(+), 103 deletions(-)\n> rewrite blob.o (100%)\n\nYeah, that seems weird. I think it might have to do with break\ndetection in the diff, which is on during the commit summary diff:\n\n $ git init\n $ dd if=/dev/urandom of=foo.rand bs=1k count=4\n $ git add foo.rand\n $ git commit -m one\n [master (root-commit) 8c5783a] one\n 1 files changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 foo.rand\n $ git show --oneline --stat --summary\n 8c5783a one\n foo.rand | Bin 0 -> 4096 bytes\n 1 files changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 foo.rand\n\nSo far so good. Now let's rewrite it.\n\n $ dd if=/dev/urandom of=foo.rand bs=1k count=4\n $ git commit -a -m two\n [master 43961bc] two\n 1 files changed, 15 insertions(+), 19 deletions(-)\n rewrite foo.rand (100%)\n\nNow that seems wrong. What about doing another diff:\n\n $ git show --oneline --stat --summary\n 43961bc two\n foo.rand | Bin 4096 -> 4096 bytes\n 1 files changed, 0 insertions(+), 0 deletions(-)\n\nThat's right. Now let's turn on break detection:\n\n $ git show --oneline --stat --summary -B\n 43961bc two\n foo.rand | 34 +++++++++++++++-------------------\n 1 files changed, 15 insertions(+), 19 deletions(-)\n rewrite foo.rand (100%)\n\nBroken again. So I guess we have some problem with making sure we treat\nbroken filepairs as binary.\n\n-Peff\n"},{"id":"161588","messageId":"20110219080307.GA18039@sigill.intra.peff.net","threadId":"26538","inReplyTo":"20110219070601.GA14059@sigill.intra.peff.net","subject":"Re: Commit/merge messages for binary files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-19T08:03:07Z","receivedAt":"2011-02-19T08:03:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 19, 2011 at 02:06:01AM -0500, Jeff King wrote:\n\n> Now that seems wrong. What about doing another diff:\n> \n> $ git show --oneline --stat --summary\n> 43961bc two\n> foo.rand | Bin 4096 -> 4096 bytes\n> 1 files changed, 0 insertions(+), 0 deletions(-)\n> \n> That's right. Now let's turn on break detection:\n> \n> $ git show --oneline --stat --summary -B\n> 43961bc two\n> foo.rand | 34 +++++++++++++++-------------------\n> 1 files changed, 15 insertions(+), 19 deletions(-)\n> rewrite foo.rand (100%)\n> \n> Broken again. So I guess we have some problem with making sure we treat\n> broken filepairs as binary.\n\nYep, builtin_diffstat just didn't handle that case. Two patch series to\nfollow:\n\n [1/2]: diff: handle diffstat of rewritten binary files\n [2/2]: diff: don't retrieve binary blobs for diffstat\n\nThe first one fixes the bug, and the second is a convenient optimization\nI noticed while there.\n\n-Peff\n"},{"id":"161589","messageId":"20110219080456.GA18056@sigill.intra.peff.net","threadId":"26538","inReplyTo":"20110219080307.GA18039@sigill.intra.peff.net","subject":"[PATCH 1/2] diff: handle diffstat of rewritten binary files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-19T08:04:56Z","receivedAt":"2011-02-19T08:04:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The logic in builtin_diffstat assumes that a\ncomplete_rewrite pair should have its lines counted. This is\nnonsensical for binary files and leads to confusing things\nlike:\n\n $ git diff --stat --summary HEAD^ HEAD\n foo.rand | Bin 4096 -> 4096 bytes\n 1 files changed, 0 insertions(+), 0 deletions(-)\n\n $ git diff --stat --summary -B HEAD^ HEAD\n foo.rand | 34 +++++++++++++++-------------------\n 1 files changed, 15 insertions(+), 19 deletions(-)\n rewrite foo.rand (100%)\n\nSo let's reorder the function to handle binary files first\n(which from diffstat's perspective look like complete\nrewrites anyway), then rewrites, then actual diffstats.\n\nThere are two bonus prizes to this reorder:\n\n 1. It gets rid of a now-superfluous goto.\n\n 2. The binary case is at the top, which means we can\n further optimize it in the next patch.\n\nSigned-off-by: Jeff King \n---\n diff.c | 24 ++++++++++++++----------\n 1 files changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 5422c43..2ac0fe9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2079,25 +2079,30 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tdata->is_unmerged = 1;\n \t\treturn;\n \t}\n-\tif (complete_rewrite) {\n+\n+\tif (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {\n+\t\tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n+\t\t\tdie(\"unable to read files to diff\");\n+\t\tdata->is_binary = 1;\n+\t\tdata->added = mf2.size;\n+\t\tdata->deleted = mf1.size;\n+\t}\n+\n+\telse if (complete_rewrite) {\n \t\tdiff_populate_filespec(one, 0);\n \t\tdiff_populate_filespec(two, 0);\n \t\tdata->deleted = count_lines(one->data, one->size);\n \t\tdata->added = count_lines(two->data, two->size);\n-\t\tgoto free_and_return;\n \t}\n-\tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n-\t\tdie(\"unable to read files to diff\");\n \n-\tif (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {\n-\t\tdata->is_binary = 1;\n-\t\tdata->added = mf2.size;\n-\t\tdata->deleted = mf1.size;\n-\t} else {\n+\telse {\n \t\t/* Crazy xdl interfaces.. */\n \t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n \n+\t\tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n+\t\t\tdie(\"unable to read files to diff\");\n+\n \t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = o->xdl_opts;\n@@ -2105,7 +2110,6 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\t\t &xpp, &xecfg);\n \t}\n \n- free_and_return:\n \tdiff_free_filespec_data(one);\n \tdiff_free_filespec_data(two);\n }\n-- \n1.7.4.1.26.g3372c\n"},{"id":"161590","messageId":"20110219081631.GB18056@sigill.intra.peff.net","threadId":"26538","inReplyTo":"20110219080307.GA18039@sigill.intra.peff.net","subject":"[PATCH 2/2] diff: don't retrieve binary blobs for diffstat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-19T08:16:32Z","receivedAt":"2011-02-19T08:16:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We only need the size, which is much cheaper to get,\nespecially if it is a big binary file.\n\nSigned-off-by: Jeff King \n---\nThis of course is only really helpful if you have marked the files as\nbinary via gitattributes, since otherwise we have to pull in the blob to\nfind out that it's binary. :)\n\nBut in my real-world photo/video repo, which has media files marked via\ngitattributes as binary (but to textconv exif tags, of course). The\ncommit in question has 26 files totalling 88 megabytes.\n\n $ time git show --stat >old\n real 0m0.428s\n user 0m0.392s\n sys 0m0.032s\n\n $ time git.jk.diffstat-binary show --stat >new\n real 0m0.005s\n user 0m0.004s\n sys 0m0.000s\n\n $ cmp old new && echo ok\n ok\n\n8500% speedup isn't too bad. :)\n\n diff.c | 15 +++++++++++----\n 1 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2ac0fe9..6640857 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -245,6 +245,15 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)\n \treturn 0;\n }\n \n+/* like fill_mmfile, but only for size, so we can avoid retrieving blob */\n+static unsigned long diff_filespec_size(struct diff_filespec *one)\n+{\n+\tif (!DIFF_FILE_VALID(one))\n+\t\treturn 0;\n+\tdiff_populate_filespec(one, 1);\n+\treturn one->size;\n+}\n+\n static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)\n {\n \tchar *ptr = mf->ptr;\n@@ -2081,11 +2090,9 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t}\n \n \tif (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {\n-\t\tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n-\t\t\tdie(\"unable to read files to diff\");\n \t\tdata->is_binary = 1;\n-\t\tdata->added = mf2.size;\n-\t\tdata->deleted = mf1.size;\n+\t\tdata->added = diff_filespec_size(two);\n+\t\tdata->deleted = diff_filespec_size(one);\n \t}\n \n \telse if (complete_rewrite) {\n-- \n1.7.4.1.26.g3372c\n"},{"id":"161843","messageId":"7v62sdhsjy.fsf@alter.siamese.dyndns.org","threadId":"26538","inReplyTo":"20110219081631.GB18056@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] diff: don't retrieve binary blobs for diffstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-21T23:33:05Z","receivedAt":"2011-02-21T23:33:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We only need the size, which is much cheaper to get,\n> especially if it is a big binary file.\n>\n> Signed-off-by: Jeff King \n> ---\n\nNice ;-) Do we want a test or two for 1/2 by the way?\n"},{"id":"161886","messageId":"20110222153729.GA27178@sigill.intra.peff.net","threadId":"26538","inReplyTo":"7v62sdhsjy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] diff: don't retrieve binary blobs for diffstat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-22T15:37:29Z","receivedAt":"2011-02-22T15:37:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 21, 2011 at 03:33:05PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > We only need the size, which is much cheaper to get,\n> > especially if it is a big binary file.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> \n> Nice ;-) Do we want a test or two for 1/2 by the way?\n\nYeah, it's probably a good idea. Can you squash this in, or should I\nresend?\n\n---\ndiff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh\nindex 7e7b307..7d7470f 100755\n--- a/t/t4031-diff-rewrite-binary.sh\n+++ b/t/t4031-diff-rewrite-binary.sh\n@@ -44,6 +44,13 @@ test_expect_success 'rewrite diff can show binary patch' '\n \tgrep \"GIT binary patch\" diff\n '\n \n+test_expect_success 'rewrite diff --stat shows binary changes' '\n+\tgit diff -B --stat --summary >diff &&\n+\tgrep \"Bin\" diff &&\n+\tgrep \"0 insertions.*0 deletions\" diff &&\n+\tgrep \" rewrite file\" diff\n+'\n+\n {\n \techo \"#!$SHELL_PATH\"\n \tcat <<'EOF'\n"},{"id":"161904","messageId":"7voc63dhdn.fsf@alter.siamese.dyndns.org","threadId":"26538","inReplyTo":"20110222153729.GA27178@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] diff: don't retrieve binary blobs for diffstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-22T19:00:20Z","receivedAt":"2011-02-22T19:00:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Nice ;-) Do we want a test or two for 1/2 by the way?\n>\n> Yeah, it's probably a good idea. Can you squash this in,...\n\nDone; thanks.\n"}]}