{"thread":{"id":"11079","subject":"[PATCH] xdiff-interface.c (buffer_is_binary): Remove buffer size limitation","startedAt":"2007-12-01T16:01:13Z","lastAt":"2007-12-05T10:47:26Z","messageCount":7,"participants":["Dmitry V. Levin","Junio C Hamano","Linus Torvalds","Johannes Schindelin","David Kastrup"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"61587","messageId":"20071201160113.GA20849@nomad.office.altlinux.org","threadId":"11079","inReplyTo":null,"subject":"[PATCH] xdiff-interface.c (buffer_is_binary): Remove buffer size limitation","fromName":"Dmitry V. Levin","fromEmail":"ldv@altlinux.org","sentAt":"2007-12-01T16:01:13Z","receivedAt":"2007-12-01T16:01:13Z","isPatch":true,"sender":{"key":"ldv@altlinux.org","avatar":"https://avatars.githubusercontent.com/u/5281408?v=4"},"body":"When checking buffer for NUL byte, do not limit size of buffer we check.\nOtherwise we break git-rebase: git-format-patch may generate output which\ngit-mailinfo cannot handle properly.\n\nSigned-off-by: Dmitry V. Levin <ldv@altlinux.org>\n---\n t/t3407-rebase-binary.sh |   32 ++++++++++++++++++++++++++++++++\n xdiff-interface.c        |    3 ---\n 2 files changed, 32 insertions(+), 3 deletions(-)\n create mode 100755 t/t3407-rebase-binary.sh\n\ndiff --git a/t/t3407-rebase-binary.sh b/t/t3407-rebase-binary.sh\nnew file mode 100755\nindex 0000000..213dc9d\n--- /dev/null\n+++ b/t/t3407-rebase-binary.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description='rebase binary test'\n+\n+. ./test-lib.sh\n+\n+yes 1234567 |head -n 2003 >text\n+yes 1234567 |head -n 2000 >bin && printf 'binary\\0bin\\n' >>bin && yes 1234567 |head -n 3 >>bin\n+\n+test_expect_success setup '\n+\n+\tcat text >file &&\n+\tgit add file &&\n+\tgit commit -m\"text\" &&\n+\n+\tgit branch bin &&\n+\n+\techo side >side &&\n+\tgit add side &&\n+\tgit commit -m\"side\" &&\n+\n+\tgit checkout bin &&\n+\tcat bin >file &&\n+\tgit commit -a -m\"bin\"\n+'\n+\n+test_expect_success rebase '\n+\n+\tgit rebase master\n+'\n+\n+test_done\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex be866d1..fa9f58d 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -122,11 +122,8 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n \treturn 0;\n }\n \n-#define FIRST_FEW_BYTES 8000\n int buffer_is_binary(const char *ptr, unsigned long size)\n {\n-\tif (FIRST_FEW_BYTES < size)\n-\t\tsize = FIRST_FEW_BYTES;\n \treturn !!memchr(ptr, 0, size);\n }\n \n-- \nldv\n"},{"id":"61597","messageId":"7vlk8e42qb.fsf@gitster.siamese.dyndns.org","threadId":"11079","inReplyTo":"20071201160113.GA20849@nomad.office.altlinux.org","subject":"Re: [PATCH] xdiff-interface.c (buffer_is_binary): Remove buffer size limitation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-01T19:46:52Z","receivedAt":"2007-12-01T19:46:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Dmitry V. Levin\" <ldv@altlinux.org> writes:\n\n> When checking buffer for NUL byte, do not limit size of buffer we check.\n> Otherwise we break git-rebase: git-format-patch may generate output which\n> git-mailinfo cannot handle properly.\n\nI think this is tackling a valid problem but it is a wrong solution.\nThe change penalizes text changes which is the majority, just in case\nthere is an unusual change that has an embedded NUL far into the file\n(iow, exception).\n\nPerhaps mailinfo can be updated to handle embedded NUL.\n\nAnother alternative (I've been trying to find time to do so for quite a\nwhile now but dealing with list traffic always takes priority on my time\nallotment) is to update rebase not to rely on \"format-patch piped to\nam\", and I think that is more correct solution in the longer term.\n\nIn the meantime, a workaround would be to use \"rebase -i\".  It uses\ncherry-pick machinery instead of \"format-patch piped to am\", and\nhopefully would handle NULs better.  It probably is slower than non\ninteractive one exactly because it uses cherry-pick, and that is the\nreason I am first working on updating cherry-pick before actually making\nthe non-interactive rebase to use it.\n"},{"id":"61826","messageId":"20071203215007.GA14697@basalt.office.altlinux.org","threadId":"11079","inReplyTo":"7vlk8e42qb.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] xdiff-interface.c (buffer_is_binary): Remove buffer size limitation","fromName":"Dmitry V. Levin","fromEmail":"ldv@altlinux.org","sentAt":"2007-12-03T21:50:07Z","receivedAt":"2007-12-03T21:50:07Z","isPatch":true,"sender":{"key":"ldv@altlinux.org","avatar":"https://avatars.githubusercontent.com/u/5281408?v=4"},"body":"On Sat, Dec 01, 2007 at 11:46:52AM -0800, Junio C Hamano wrote:\n> On Sat, Dec 01, 2007 at 07:01:13PM +0300, Dmitry V. Levin wrote:\n> \n> > When checking buffer for NUL byte, do not limit size of buffer we check.\n> > Otherwise we break git-rebase: git-format-patch may generate output which\n> > git-mailinfo cannot handle properly.\n> \n> I think this is tackling a valid problem but it is a wrong solution.\n> The change penalizes text changes which is the majority, just in case\n> there is an unusual change that has an embedded NUL far into the file\n> (iow, exception).\n\nPenalizes?\nAverage file size in the linux-2.6.23.9 kernel tree is 10944 bytes,\nFIRST_FEW_BYTES limit is 8000 bytes.\nWell, I prefer slightly penalized but working properly git-rebase.\nAttached test case demonstrates how current git-rebase can just run\nsuccessfully but produce a wrong result.\n\nP.S. The real life example where you can hit this git-rebase problem is\nGNU .info files.\n\n\n-- \nldv\n"},{"id":"61839","messageId":"7veje3e4zn.fsf@gitster.siamese.dyndns.org","threadId":"11079","inReplyTo":"20071203215007.GA14697@basalt.office.altlinux.org","subject":"Re: [PATCH] xdiff-interface.c (buffer_is_binary): Remove buffer size limitation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-03T23:24:44Z","receivedAt":"2007-12-03T23:24:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Dmitry V. Levin\" <ldv@altlinux.org> writes:\n\n> On Sat, Dec 01, 2007 at 11:46:52AM -0800, Junio C Hamano wrote:\n>> On Sat, Dec 01, 2007 at 07:01:13PM +0300, Dmitry V. Levin wrote:\n>> \n>> > When checking buffer for NUL byte, do not limit size of buffer we check.\n>> > Otherwise we break git-rebase: git-format-patch may generate output which\n>> > git-mailinfo cannot handle properly.\n>> \n>> I think this is tackling a valid problem but it is a wrong solution.\n>> The change penalizes text changes which is the majority, just in case\n>> there is an unusual change that has an embedded NUL far into the file\n>> (iow, exception).\n>\n> Penalizes?\n> Average file size in the linux-2.6.23.9 kernel tree is 10944 bytes,\n> FIRST_FEW_BYTES limit is 8000 bytes.\n\nI really wish we were living in a simpler time, back when I could just\nsay \"we optimize for the kernel\" and did not have to be worried about\ngetting laughed at.\n"},{"id":"61842","messageId":"alpine.LFD.0.9999.0712031559480.8458@woody.linux-foundation.org","threadId":"11079","inReplyTo":"20071203215007.GA14697@basalt.office.altlinux.org","subject":"Re: [PATCH] xdiff-interface.c (buffer_is_binary): Remove buffer size limitation","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-12-04T00:00:10Z","receivedAt":"2007-12-04T00:00:10Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 4 Dec 2007, Dmitry V. Levin wrote:\n>\n> Average file size in the linux-2.6.23.9 kernel tree is 10944 bytes,\n\nDon't do \"average\" sizes. That's an almost totally meaningless number.\n\n\"Average\" makes sense if you have some kind of gaussian distribution or \nsimilar. File sizes tend to be exponential distributions, and what makes \nmuch more sense is to look at the median. That doesn't show the effect of \na few larger files, and also gives you a much better \"half the files are \nsmaller than x\" idea.\n\nAnd the median filesize for the kernel is just a few bytes over 4k.\n\nOf the 23,000+ files in the current kernel, about 15,500 are less than \n8kB. And 17,179 are smaller than the 10944 bytes you mention.\n\nI'd argue that 8kB (or even 4kB) is probably a good number for things like \nthat: it catches the bulk of all files in their entirety, but it *avoids* \nspending tons of time on the (few) really large files.\n\n\t\t\tLinus\n"},{"id":"61845","messageId":"Pine.LNX.4.64.0712040054280.27959@racer.site","threadId":"11079","inReplyTo":"alpine.LFD.0.9999.0712031559480.8458@woody.linux-foundation.org","subject":"Re: [PATCH] xdiff-interface.c (buffer_is_binary): Remove buffer size limitation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-04T01:00:39Z","receivedAt":"2007-12-04T01:00:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 3 Dec 2007, Linus Torvalds wrote:\n\n> On Tue, 4 Dec 2007, Dmitry V. Levin wrote:\n> >\n> > Average file size in the linux-2.6.23.9 kernel tree is 10944 bytes,\n> \n> Don't do \"average\" sizes. That's an almost totally meaningless number.\n> \n> \"Average\" makes sense if you have some kind of gaussian distribution or \n> similar.\n\nTo enhance on that: Gaussian is symmetric, which cannot be the proper \ndistribution for anything that is non-negative.\n\nI see so many mis-applications of statistics/probability theory in my day \njob that I cannot resist pointing people to the Poisson distribution here \n(in whose context \"average\" actually makes kind of sense).\n\nBut back to the problem: if you have a truly binary file, then _every_ \nbyte (absent further information, of course) has a probability of 1/256 of \nbeing 0.\n\nWhich means that if a file is binary, but is unusual enough to have that \nproperty only for half of the first 8192 bytes, you get a probability of \n1 - 1 / 256^4096 = 1 - 1 / 2 ^ 32768 that the current test succeeds.\n\nI fail to see how this test can possibly fail for the average case.\n\nSo if it fails only for special cases, we are probably (in the common, not \nthe mathematical, sense) better off asking those people encountering them \nto add git-attributes for the files.\n\nIMHO that is not asking for too much.\n\nCiao,\nDscho\n"},{"id":"61996","messageId":"85bq95wh8h.fsf@lola.goethe.zz","threadId":"11079","inReplyTo":"Pine.LNX.4.64.0712040054280.27959@racer.site","subject":"Re: [PATCH] xdiff-interface.c (buffer_is_binary): Remove buffer size limitation","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-12-05T10:47:26Z","receivedAt":"2007-12-05T10:47:26Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Mon, 3 Dec 2007, Linus Torvalds wrote:\n>\n>> On Tue, 4 Dec 2007, Dmitry V. Levin wrote:\n>> >\n>> > Average file size in the linux-2.6.23.9 kernel tree is 10944 bytes,\n>> \n>> Don't do \"average\" sizes. That's an almost totally meaningless number.\n>> \n>> \"Average\" makes sense if you have some kind of gaussian distribution or \n>> similar.\n>\n> To enhance on that: Gaussian is symmetric, which cannot be the proper \n> distribution for anything that is non-negative.\n\nThis reasoning is nonsense, since Gaussians are not necessarily\nsymmetric about zero, and for example the equally distributed\nprobability between 0 and 1 is both symmetric and non-negative.  And the\ntrivial distribution of \"always zero\" is even _both_ symmetric around\nzero and non-negative.\n\nWhat is true for Gaussians is that their probability is non-zero\neverywhere.  But with a meaning of \"non-zero\" that should let the kind\nof people using hashes for unique file identification sleep well.\n\n> I see so many mis-applications of statistics/probability theory in my\n> day job that I cannot resist pointing people to the Poisson\n> distribution here (in whose context \"average\" actually makes kind of\n> sense).\n\nThe main point of Gaussians is that they approximate a distribution\ncoming from a sum of independent random sources pretty well, even if the\nindividual sources are not Gaussians themselves.\n\nPoisson distributions come about as the sum of independent exponential\ndistributions, and yes, when the number of summands grows, the result is\nquite well modeled by a Gaussian: the impossible outliers predicted by\nthe Gaussian approximation take a negligible probability of the total.\n\nIf the distribution is more like coming from a product of independent\nsources, the results will be better modeled by log-Gaussian\ndistributions (which happen to be non-negative).\n\nThe exponential distribution happens to be the log-distribution of the\n(0,1) equal probability distribution, so for lower order Poisson\ndistributions, approximation with log-Gaussians may seem more\nstraightforward.\n\n> But back to the problem: if you have a truly binary file, then _every_\n> byte (absent further information, of course) has a probability of\n> 1/256 of being 0.\n\nAbsent any information, there is no reason to assume equal probability\nas more likely than other probabilities.\n\n> Which means that if a file is binary,\n\nis _random_ binary.  Few people version random binary files.  It is\nrather pointless.\n\n> but is unusual enough to have that property only for half of the first\n> 8192 bytes, you get a probability of 1 - 1 / 256^4096 = 1 - 1 / 2 ^\n> 32768 that the current test succeeds.\n\nRather\n\n(1-1/256)^4096 ~= exp(-4096/256) = about 1 in 10 million\n\nWhich means that the test would yield a false negative (with regard to\nthe data being binary) about once in 10000000 times when done on 4096\nbytes, even given your rather absurd assumption of random binary data\nbeing versioned.\n\nTo make this somewhat more obvious: let's take a look at the probability\nof 128 random bytes being all non-zero.  According to your model, that\nshould be (1-1/256^128)=1-1/32768.  According to mine, (1-1/256)^128 ~=\nexp(-0.5) ~= 60%.  Why is this even more than 50%, which would be the\nnaive assumption?  Because it is likely that we will have duplicate\nbytes among our 128 bytes, so the probability for an occurence of 0\nbecomes less.\n\n> I fail to see how this test can possibly fail for the average case.\n\nYou start with a nonsensical mathematical model, and don't even do the\nmath right that would follow from it.\n\nThe \"average case\" is not random equidistributed uncorrelated data,\nanyway.\n\n> So if it fails only for special cases, we are probably (in the common,\n> not the mathematical, sense) better off asking those people\n> encountering them to add git-attributes for the files.\n>\n> IMHO that is not asking for too much.\n\nThat the problem has actually been encountered in real life does not\nexactly do much to support your assumptions and your conclusions, does\nit?\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"}]}