Re: [PATCH] don't use mmap() to hash files

[Dmitry Potapov <dpotapov@gmail.com>]

On Sun, Feb 14, 2010 at 02:53:58AM +0100, Johannes Schindelin wrote:
> On Sun, 14 Feb 2010, Dmitry Potapov wrote:
> 
> > +	if (strbuf_read(&sbuf, fd, 4096) >= 0)
> 
> How certain are you at this point that all of fd's contents fit into your 
> memory?

You can't be sure... In fact, we know mmap() also may fail for huge
files, so can strbuf_read(). Perhaps, mmap() behaves better when you
want to hash a huge file that does not fit in free physical memory, but
I do not think it is an important use case for any VCS, which mostly
stores small text files and a few not so big binary files. Git is not
design to store your video collection. (probably, Git can be improved to
handle big files better but I leave that exercise to those who want to
store their media files in Git).

> 
> And even if you could be certain, a hint is missing that strbuf_read(), 
> its name notwithstanding, does not read NUL-terminated strings. Oh, and 
> the size is just a hint for the initial size, and it reads until EOF. That 
> has to be said in the commit message.

I did not add _any_ new code, including the above line. It was there
before my patch. I only removed a few lines for the case when we used
mmap() and left the code that used strbuf_read() (mmap() was used for
regular files and strbuf_read() for other type of descriptors).

The fact that I re-aligned those lines could not introduce any bug, so
if you think this code incorrect then it was before my patch, but I do
not see why. And, I see no reason to comment on the code that does not
change at all (including that the third parameter of strbuf_read() is
just a hint). I may agree with you that strbuf_read with 'hint' is a bit
confusing, but it has nothing to do with my patch...


Dmitry