Re: [PATCH v2 00/21] Read `packed-refs` using mmap()
- From
Jeff King <peff@peff.net>
- Date
- Sep 20, 2017, 18:57 UTC
- Message-ID
- <20170920185724.ix7isdxfrsecxkjj@sigill.intra.peff.net>
- In-Reply-To
- <cover.1505799700.git.mhagger@alum.mit.edu>
On Tue, Sep 19, 2017 at 08:22:08AM +0200, Michael Haggerty wrote:
Show 15 quoted lines
> This branch is also available from my fork on GitHub as branch > `mmap-packed-refs`. > > My main uncertainties are: > > 1. Does this code actually work on Windows? > > 2. Did I implement the new compile-time option correctly? (I just > cargo-culted some existing options.) Is there some existing option > that I could piggy-back off of instead of adding a new one? > > 3. Is a compile-time option sufficient, or would the `mmap()` option > need to be configurable at runtime, or even tested at repository > create time as is done for some other filesystem properties in > `create_default_files()`?
I can't really answer (1) due to lack of knowledge. For (2), I think it's mostly good, though I raised a small nit. For (3), I think this is really an OS restriction and not a filesystem one, so probably build-time is OK (though of course Windows people may be able to correct me).
I left a few comments, but overall it looks quite good to me.
-Peff