Re: [PATCH v3] packed_ref_cache: don't use mmap() for small files

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jan 21, 2018, 04:41 UTC
Message-ID: <29c51594-6e29-be34-3d5f-2b9f399490f2@alum.mit.edu>
In-Reply-To: <20180117220902.GA14952@sigill.intra.peff.net>

On 01/17/2018 11:09 PM, Jeff King wrote:

Show 27 quoted lines

> 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@infogroep.be>
>> ---
>>
>> 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