On 01/17/2018 11:09 PM, Jeff King wrote: > On Tue, Jan 16, 2018 at 08:38:15PM +0100, Kim Gybels wrote: > >> Take a hint from commit ea68b0ce9f8 (hash-object: don't use mmap() for >> small files, 2010-02-21) and use read() instead of mmap() for small >> packed-refs files. >> >> This also fixes the problem[1] where xmmap() returns NULL for zero >> length[2], for which munmap() later fails. >> >> Alternatively, we could simply check for NULL before munmap(), or >> introduce xmunmap() that could be used together with xmmap(). However, >> always setting snapshot->buf to a valid pointer, by relying on >> xmalloc(0)'s fallback to 1-byte allocation, makes using snapshots >> easier. >> >> [1] https://github.com/git-for-windows/git/issues/1410 >> [2] Logic introduced in commit 9130ac1e196 (Better error messages for >> corrupt databases, 2007-01-11) >> >> Signed-off-by: Kim Gybels <kgybels@xxxxxxxxxxxx> >> --- >> >> Change since v2: removed separate case for zero length as suggested by Peff, >> ensuring that snapshot->buf is always a valid pointer. > > Thanks, this looks fine to me (I'd be curious to hear from Michael if > this eliminates the need for the other patches). `snapshot->buf` can still be NULL if the `packed-refs` file didn't exist (see the earlier code path in `load_contents()`). So either that code path *also* has to get the `xmalloc()` treatment, or my third patch is still necessary. (My second patch wouldn't be necessary because the ENOENT case makes `load_contents()` return 0, triggering the early exit from `create_snapshot()`.) I don't have a strong preference either way. Michael
