# Commit/merge messages for binary files 12 messages from 2011-02-17 to 2011-02-22. Participants: Piotr Krukowiecki, Matthieu Moy, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/26538 ## Piotr Krukowiecki, 2011-02-17 20:40 Subject: Commit/merge messages for binary files Message-ID: URL: https://gitlist.dev/e/AANLkTikXMi92iUd-1bEfs5WfawyHp4G7%3DYnd%2Beaq_wsR%40mail.gmail.com ``` Hi, there's a different output when committing change and when merging change for a binary file. Do the insertions/deletions have any meaning for binary files? For example: $ git commit -m Updated [topic 5da30ce] Updated 1 files changed, 8 insertions(+), 103 deletions(-) rewrite blob.o (100%) $ git merge topic Updating 75ab259..5da30ce Fast-forward blob.o | Bin 25920 -> 4364 bytes 1 files changed, 0 insertions(+), 0 deletions(-) -- Piotrek ``` ## Matthieu Moy, 2011-02-18 13:53 Subject: Re: Commit/merge messages for binary files Message-ID: URL: https://gitlist.dev/e/vpq39nlsb3r.fsf%40bauges.imag.fr In-Reply-To: ``` Piotr Krukowiecki writes: > Hi, > > there's a different output when committing change and when merging > change for a binary file. > Do the insertions/deletions have any meaning for binary files? No. They're inserted/deleted *lines*, and that wouldn't make sense for binary files. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Piotr Krukowiecki, 2011-02-18 20:30 Subject: Re: Commit/merge messages for binary files Message-ID: <4D5ED6F2.8030008@gmail.com> URL: https://gitlist.dev/e/4D5ED6F2.8030008%40gmail.com In-Reply-To: ``` W dniu 18.02.2011 14:53, Matthieu Moy pisze: > Piotr Krukowiecki writes: > >> Hi, >> >> there's a different output when committing change and when merging >> change for a binary file. >> Do the insertions/deletions have any meaning for binary files? > > No. They're inserted/deleted *lines*, and that wouldn't make sense for > binary files. > So it's a bug? -- Piotr Krukowiecki ``` ## Matthieu Moy, 2011-02-18 21:19 Subject: Re: Commit/merge messages for binary files Message-ID: URL: https://gitlist.dev/e/vpqwrkx9h2z.fsf%40bauges.imag.fr In-Reply-To: <4D5ED6F2.8030008@gmail.com> ``` Piotr Krukowiecki writes: > W dniu 18.02.2011 14:53, Matthieu Moy pisze: >> Piotr Krukowiecki writes: >> >>> Hi, >>> >>> there's a different output when committing change and when merging >>> change for a binary file. >>> Do the insertions/deletions have any meaning for binary files? >> >> No. They're inserted/deleted *lines*, and that wouldn't make sense for >> binary files. > > So it's a bug? I don't see any bug. There were no insertion/deletion in text files, hence you see 0 for both. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Piotr Krukowiecki, 2011-02-18 21:42 Subject: Re: Commit/merge messages for binary files Message-ID: <4D5EE7D2.5060703@gmail.com> URL: https://gitlist.dev/e/4D5EE7D2.5060703%40gmail.com In-Reply-To: ``` W dniu 18.02.2011 22:19, Matthieu Moy pisze: > Piotr Krukowiecki writes: > >> W dniu 18.02.2011 14:53, Matthieu Moy pisze: >>> Piotr Krukowiecki writes: >>> >>>> Hi, >>>> >>>> there's a different output when committing change and when merging >>>> change for a binary file. >>>> Do the insertions/deletions have any meaning for binary files? >>> >>> No. They're inserted/deleted *lines*, and that wouldn't make sense for >>> binary files. >> >> So it's a bug? > > I don't see any bug. There were no insertion/deletion in text files, > hence you see 0 for both. Maybe I wasn't clear. Both the commit and the merge was for the same binary file. In case of commit there were non-zero insertions/deletions. See example below - notice both update binary file blob.o: $ git commit -m Updated [topic 5da30ce] Updated 1 files changed, 8 insertions(+), 103 deletions(-) rewrite blob.o (100%) $ git merge topic Updating 75ab259..5da30ce Fast-forward blob.o | Bin 25920 -> 4364 bytes 1 files changed, 0 insertions(+), 0 deletions(-) Hope that helps, -- Piotr Krukowiecki ``` ## Jeff King, 2011-02-19 07:06 Subject: Re: Commit/merge messages for binary files Message-ID: <20110219070601.GA14059@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110219070601.GA14059%40sigill.intra.peff.net In-Reply-To: <4D5EE7D2.5060703@gmail.com> ``` On Fri, Feb 18, 2011 at 10:42:42PM +0100, Piotr Krukowiecki wrote: > Maybe I wasn't clear. Both the commit and the merge was for the same binary > file. > > In case of commit there were non-zero insertions/deletions. > See example below - notice both update binary file blob.o: > > $ git commit -m Updated > [topic 5da30ce] Updated > 1 files changed, 8 insertions(+), 103 deletions(-) > rewrite blob.o (100%) Yeah, that seems weird. I think it might have to do with break detection in the diff, which is on during the commit summary diff: $ git init $ dd if=/dev/urandom of=foo.rand bs=1k count=4 $ git add foo.rand $ git commit -m one [master (root-commit) 8c5783a] one 1 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 foo.rand $ git show --oneline --stat --summary 8c5783a one foo.rand | Bin 0 -> 4096 bytes 1 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 foo.rand So far so good. Now let's rewrite it. $ dd if=/dev/urandom of=foo.rand bs=1k count=4 $ git commit -a -m two [master 43961bc] two 1 files changed, 15 insertions(+), 19 deletions(-) rewrite foo.rand (100%) Now that seems wrong. What about doing another diff: $ git show --oneline --stat --summary 43961bc two foo.rand | Bin 4096 -> 4096 bytes 1 files changed, 0 insertions(+), 0 deletions(-) That's right. Now let's turn on break detection: $ git show --oneline --stat --summary -B 43961bc two foo.rand | 34 +++++++++++++++------------------- 1 files changed, 15 insertions(+), 19 deletions(-) rewrite foo.rand (100%) Broken again. So I guess we have some problem with making sure we treat broken filepairs as binary. -Peff ``` ## Jeff King, 2011-02-19 08:03 Subject: Re: Commit/merge messages for binary files Message-ID: <20110219080307.GA18039@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110219080307.GA18039%40sigill.intra.peff.net In-Reply-To: <20110219070601.GA14059@sigill.intra.peff.net> ``` On Sat, Feb 19, 2011 at 02:06:01AM -0500, Jeff King wrote: > Now that seems wrong. What about doing another diff: > > $ git show --oneline --stat --summary > 43961bc two > foo.rand | Bin 4096 -> 4096 bytes > 1 files changed, 0 insertions(+), 0 deletions(-) > > That's right. Now let's turn on break detection: > > $ git show --oneline --stat --summary -B > 43961bc two > foo.rand | 34 +++++++++++++++------------------- > 1 files changed, 15 insertions(+), 19 deletions(-) > rewrite foo.rand (100%) > > Broken again. So I guess we have some problem with making sure we treat > broken filepairs as binary. Yep, builtin_diffstat just didn't handle that case. Two patch series to follow: [1/2]: diff: handle diffstat of rewritten binary files [2/2]: diff: don't retrieve binary blobs for diffstat The first one fixes the bug, and the second is a convenient optimization I noticed while there. -Peff ``` ## Jeff King, 2011-02-19 08:04 Subject: [PATCH 1/2] diff: handle diffstat of rewritten binary files Message-ID: <20110219080456.GA18056@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110219080456.GA18056%40sigill.intra.peff.net In-Reply-To: <20110219080307.GA18039@sigill.intra.peff.net> ``` The logic in builtin_diffstat assumes that a complete_rewrite pair should have its lines counted. This is nonsensical for binary files and leads to confusing things like: $ git diff --stat --summary HEAD^ HEAD foo.rand | Bin 4096 -> 4096 bytes 1 files changed, 0 insertions(+), 0 deletions(-) $ git diff --stat --summary -B HEAD^ HEAD foo.rand | 34 +++++++++++++++------------------- 1 files changed, 15 insertions(+), 19 deletions(-) rewrite foo.rand (100%) So let's reorder the function to handle binary files first (which from diffstat's perspective look like complete rewrites anyway), then rewrites, then actual diffstats. There are two bonus prizes to this reorder: 1. It gets rid of a now-superfluous goto. 2. The binary case is at the top, which means we can further optimize it in the next patch. Signed-off-by: Jeff King --- diff.c | 24 ++++++++++++++---------- 1 files changed, 14 insertions(+), 10 deletions(-) diff --git a/diff.c b/diff.c index 5422c43..2ac0fe9 100644 --- a/diff.c +++ b/diff.c @@ -2079,25 +2079,30 @@ static void builtin_diffstat(const char *name_a, const char *name_b, data->is_unmerged = 1; return; } - if (complete_rewrite) { + + if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) { + if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0) + die("unable to read files to diff"); + data->is_binary = 1; + data->added = mf2.size; + data->deleted = mf1.size; + } + + else if (complete_rewrite) { diff_populate_filespec(one, 0); diff_populate_filespec(two, 0); data->deleted = count_lines(one->data, one->size); data->added = count_lines(two->data, two->size); - goto free_and_return; } - if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0) - die("unable to read files to diff"); - if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) { - data->is_binary = 1; - data->added = mf2.size; - data->deleted = mf1.size; - } else { + else { /* Crazy xdl interfaces.. */ xpparam_t xpp; xdemitconf_t xecfg; + if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0) + die("unable to read files to diff"); + memset(&xpp, 0, sizeof(xpp)); memset(&xecfg, 0, sizeof(xecfg)); xpp.flags = o->xdl_opts; @@ -2105,7 +2110,6 @@ static void builtin_diffstat(const char *name_a, const char *name_b, &xpp, &xecfg); } - free_and_return: diff_free_filespec_data(one); diff_free_filespec_data(two); } -- 1.7.4.1.26.g3372c ``` ## Jeff King, 2011-02-19 08:16 Subject: [PATCH 2/2] diff: don't retrieve binary blobs for diffstat Message-ID: <20110219081631.GB18056@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110219081631.GB18056%40sigill.intra.peff.net In-Reply-To: <20110219080307.GA18039@sigill.intra.peff.net> ``` We only need the size, which is much cheaper to get, especially if it is a big binary file. Signed-off-by: Jeff King --- This of course is only really helpful if you have marked the files as binary via gitattributes, since otherwise we have to pull in the blob to find out that it's binary. :) But in my real-world photo/video repo, which has media files marked via gitattributes as binary (but to textconv exif tags, of course). The commit in question has 26 files totalling 88 megabytes. $ time git show --stat >old real 0m0.428s user 0m0.392s sys 0m0.032s $ time git.jk.diffstat-binary show --stat >new real 0m0.005s user 0m0.004s sys 0m0.000s $ cmp old new && echo ok ok 8500% speedup isn't too bad. :) diff.c | 15 +++++++++++---- 1 files changed, 11 insertions(+), 4 deletions(-) diff --git a/diff.c b/diff.c index 2ac0fe9..6640857 100644 --- a/diff.c +++ b/diff.c @@ -245,6 +245,15 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one) return 0; } +/* like fill_mmfile, but only for size, so we can avoid retrieving blob */ +static unsigned long diff_filespec_size(struct diff_filespec *one) +{ + if (!DIFF_FILE_VALID(one)) + return 0; + diff_populate_filespec(one, 1); + return one->size; +} + static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule) { char *ptr = mf->ptr; @@ -2081,11 +2090,9 @@ static void builtin_diffstat(const char *name_a, const char *name_b, } if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) { - if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0) - die("unable to read files to diff"); data->is_binary = 1; - data->added = mf2.size; - data->deleted = mf1.size; + data->added = diff_filespec_size(two); + data->deleted = diff_filespec_size(one); } else if (complete_rewrite) { -- 1.7.4.1.26.g3372c ``` ## Junio C Hamano, 2011-02-21 23:33 Subject: Re: [PATCH 2/2] diff: don't retrieve binary blobs for diffstat Message-ID: <7v62sdhsjy.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v62sdhsjy.fsf%40alter.siamese.dyndns.org In-Reply-To: <20110219081631.GB18056@sigill.intra.peff.net> ``` Jeff King writes: > We only need the size, which is much cheaper to get, > especially if it is a big binary file. > > Signed-off-by: Jeff King > --- Nice ;-) Do we want a test or two for 1/2 by the way? ``` ## Jeff King, 2011-02-22 15:37 Subject: Re: [PATCH 2/2] diff: don't retrieve binary blobs for diffstat Message-ID: <20110222153729.GA27178@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110222153729.GA27178%40sigill.intra.peff.net In-Reply-To: <7v62sdhsjy.fsf@alter.siamese.dyndns.org> ``` On Mon, Feb 21, 2011 at 03:33:05PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > We only need the size, which is much cheaper to get, > > especially if it is a big binary file. > > > > Signed-off-by: Jeff King > > --- > > Nice ;-) Do we want a test or two for 1/2 by the way? Yeah, it's probably a good idea. Can you squash this in, or should I resend? --- diff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh index 7e7b307..7d7470f 100755 --- a/t/t4031-diff-rewrite-binary.sh +++ b/t/t4031-diff-rewrite-binary.sh @@ -44,6 +44,13 @@ test_expect_success 'rewrite diff can show binary patch' ' grep "GIT binary patch" diff ' +test_expect_success 'rewrite diff --stat shows binary changes' ' + git diff -B --stat --summary >diff && + grep "Bin" diff && + grep "0 insertions.*0 deletions" diff && + grep " rewrite file" diff +' + { echo "#!$SHELL_PATH" cat <<'EOF' ``` ## Junio C Hamano, 2011-02-22 19:00 Subject: Re: [PATCH 2/2] diff: don't retrieve binary blobs for diffstat Message-ID: <7voc63dhdn.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7voc63dhdn.fsf%40alter.siamese.dyndns.org In-Reply-To: <20110222153729.GA27178@sigill.intra.peff.net> ``` Jeff King writes: >> Nice ;-) Do we want a test or two for 1/2 by the way? > > Yeah, it's probably a good idea. Can you squash this in,... Done; thanks. ```