Re: [PATCH v3] packed_ref_cache: don't use mmap() for small files
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 22, 2018, 19:31 UTC
- Message-ID
- <xmqqh8rdn113.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <29c51594-6e29-be34-3d5f-2b9f399490f2@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 8 quoted lines
> `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.
Which would be a two-liner, like the attached, which does not look too bad by itself.
The direction, if we take this approach, means that we are declaring that .buf being NULL is an invalid state for a snapshot to be in, instead of saying "an empty snapshot looks exactly like one that was freshly initialized", which seems to be the intention of the original design.
After Kim's fix and with 3/3 in your follow-up series, various helpers are still unsafe against .buf being NULL, like sort_snapshot(), verify_buffer_safe(), clear_snapshot_buffer() (only when mmapped bit is set), find_reference_location().
packed_ref_iterator_begin() checks if snapshot->buf is NULL and returns early. At the first glance, this appears a useful short cut to optimize the empty case away, but the check also is acting as a guard to prevent a snapshot with NULL .buf from being fed to an unsafe find_reference_location(). An implicit guard like this feels a bit more brittle than my liking. If we ensure .buf is never NULL, that check can become a pure short-cut optimization and stop being a correctness thing.
So...
refs/packed-backend.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/refs/packed-backend.c b/refs/packed-backend.c index b6e2bc3c1d..1eeb5c7f80 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -473,12 +473,11 @@ static int load_contents(struct snapshot *snapshot) if (fd < 0) { if (errno == ENOENT) { /* - * This is OK; it just means that no - * "packed-refs" file has been written yet, - * which is equivalent to it being empty, - * which is its state when initialized with - * zeros. + * Treat missing "packed-refs" as equivalent to + * it being empty. */ + snapshot->eof = snapshot->buf = xmalloc(0); + snapshot->mmapped = 0; return 0; } else { die_errno("couldn't read %s", snapshot->refs->path);