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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html