Re: [PATCH v2 14/21] read_packed_refs(): ensure that references are ordered when read

From: Jeff King <peff@peff.net>
Date: Sep 20, 2017, 18:50 UTC
Message-ID: <20170920185047.6fzuspaseofb7r5x@sigill.intra.peff.net>
In-Reply-To: <5bb362d9cb74c7386cd072e3ecd49b22cea1d8d2.1505799700.git.mhagger@alum.mit.edu>

On Tue, Sep 19, 2017 at 08:22:22AM +0200, Michael Haggerty wrote:

Show 7 quoted lines

> If `packed-refs` was already sorted, then (if the system allows it) we
> can use the mmapped file contents directly. But if the system doesn't
> allow a file that is currently mmapped to be replaced using
> `rename()`, then it would be bad for us to keep the file mmapped for
> any longer than necessary. So, on such systems, always make a copy of
> the file contents, either as part of the sorting process, or
> afterwards.

So this sort-of answers my question from the previous commit (why we care about the distinction between NONE and TEMPORARY), since we now start treating them differently.

But I'm a bit confused why there's any advantage in the TEMPORARY case to doing the mmap-and-copy versus just treating it like NONE and reading it directly.

Hmm, I guess it comes down to the double-allocation thing again? Let me see if I have this right:

  1. For NO_MMAP, we'd read the buffer once. If it's sorted, good. If
     not, then we temporarily hold it in memory twice while we copy it
     into the new sorted buffer.

  2. For TEMPORARY, we mmap once. If it's sorted, then we make a single
     copy. If it's not sorted, then we do the copy+sort as a single
     step.

  3. For MMAP_OK, if it's sorted, we're done. Otherwise, we do the
     single copy.

So this is really there to help the TEMPORARY case reading an old unsorted file avoid needing to use double-the-ram during the copy?

That seems like a good reason (and it does not seem to add too much complexity).

-Peff