Re: [PATCH] xdiff-interface.c (buffer_is_binary): Remove buffer size limitation

"Dmitry V. Levin" [off-list ref] writes:

When checking buffer for NUL byte, do not limit size of buffer we check.
Otherwise we break git-rebase: git-format-patch may generate output which
git-mailinfo cannot handle properly.

I think this is tackling a valid problem but it is a wrong solution.
The change penalizes text changes which is the majority, just in case
there is an unusual change that has an embedded NUL far into the file
(iow, exception).

Perhaps mailinfo can be updated to handle embedded NUL.

Another alternative (I've been trying to find time to do so for quite a
while now but dealing with list traffic always takes priority on my time
allotment) is to update rebase not to rely on "format-patch piped to
am", and I think that is more correct solution in the longer term.

In the meantime, a workaround would be to use "rebase -i".  It uses
cherry-pick machinery instead of "format-patch piped to am", and
hopefully would handle NULs better.  It probably is slower than non
interactive one exactly because it uses cherry-pick, and that is the
reason I am first working on updating cherry-pick before actually making
the non-interactive rebase to use it.

On Sat, Dec 01, 2007 at 11:46:52AM -0800, Junio C Hamano wrote:

On Sat, Dec 01, 2007 at 07:01:13PM +0300, Dmitry V. Levin wrote:

quoted

When checking buffer for NUL byte, do not limit size of buffer we check.
Otherwise we break git-rebase: git-format-patch may generate output which
git-mailinfo cannot handle properly.

I think this is tackling a valid problem but it is a wrong solution.
The change penalizes text changes which is the majority, just in case
there is an unusual change that has an embedded NUL far into the file
(iow, exception).

Penalizes?
Average file size in the linux-2.6.23.9 kernel tree is 10944 bytes,
FIRST_FEW_BYTES limit is 8000 bytes.
Well, I prefer slightly penalized but working properly git-rebase.
Attached test case demonstrates how current git-rebase can just run
successfully but produce a wrong result.

P.S. The real life example where you can hit this git-rebase problem is
GNU .info files.


-- 
ldv

On Tue, 4 Dec 2007, Dmitry V. Levin wrote:

Average file size in the linux-2.6.23.9 kernel tree is 10944 bytes,

Don't do "average" sizes. That's an almost totally meaningless number.

"Average" makes sense if you have some kind of gaussian distribution or 
similar. File sizes tend to be exponential distributions, and what makes 
much more sense is to look at the median. That doesn't show the effect of 
a few larger files, and also gives you a much better "half the files are 
smaller than x" idea.

And the median filesize for the kernel is just a few bytes over 4k.

Of the 23,000+ files in the current kernel, about 15,500 are less than 
8kB. And 17,179 are smaller than the 10944 bytes you mention.

I'd argue that 8kB (or even 4kB) is probably a good number for things like 
that: it catches the bulk of all files in their entirety, but it *avoids* 
spending tons of time on the (few) really large files.

			Linus

Hi,

On Mon, 3 Dec 2007, Linus Torvalds wrote:

On Tue, 4 Dec 2007, Dmitry V. Levin wrote:

quoted

Average file size in the linux-2.6.23.9 kernel tree is 10944 bytes,

Don't do "average" sizes. That's an almost totally meaningless number.

"Average" makes sense if you have some kind of gaussian distribution or 
similar.

To enhance on that: Gaussian is symmetric, which cannot be the proper 
distribution for anything that is non-negative.

I see so many mis-applications of statistics/probability theory in my day 
job that I cannot resist pointing people to the Poisson distribution here 
(in whose context "average" actually makes kind of sense).

But back to the problem: if you have a truly binary file, then _every_ 
byte (absent further information, of course) has a probability of 1/256 of 
being 0.

Which means that if a file is binary, but is unusual enough to have that 
property only for half of the first 8192 bytes, you get a probability of 
1 - 1 / 256^4096 = 1 - 1 / 2 ^ 32768 that the current test succeeds.

I fail to see how this test can possibly fail for the average case.

So if it fails only for special cases, we are probably (in the common, not 
the mathematical, sense) better off asking those people encountering them 
to add git-attributes for the files.

IMHO that is not asking for too much.

Ciao,
Dscho