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

From: Dmitry Potapov <dpotapov@gmail.com>
Date: Feb 14, 2010, 02:42 UTC
Message-ID: <20100214024259.GB9704@dpotapov.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.1002140249410.20986@pacific.mpi-cbg.de>

On Sun, Feb 14, 2010 at 02:53:58AM +0100, Johannes Schindelin wrote:

Show 6 quoted lines

> 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).

Show 5 quoted lines

> 
> 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