On 09/20/2017 08:40 PM, Jeff King wrote:
> [...]
> The overall strategy for this compile-time knob makes sense, but one
> thing confused me:
>
>> +ifdef MMAP_PREVENTS_DELETE
>> + BASIC_CFLAGS += -DMMAP_PREVENTS_DELETE
>> +else
>> + ifdef USE_WIN32_MMAP
>> + BASIC_CFLAGS += -DMMAP_PREVENTS_DELETE
>> + endif
>> +endif
>
> So setting the knob does what you'd expect. But if you don't set it,
> then we still auto-tweak it based on the USE_WIN32_MMAP knob. Do we need
> that? It seems like we set our new knob in config.mak.uname any time
> we'd set USE_WIN32_MMAP. So this only has an effect in two cases:
>
> 1. You aren't on Windows, but you set USE_WIN32_MMAP yourself.
>
> 2. You are on Windows, but you manually unset MMAP_PREVENTS_DELETE.
>
> I expect both cases are rare (and would probably involve somebody
> actively debugging these knobs). Probably it's a minor convenience in
> case 1, but in case 2 it would be actively confusing, I'd think.
Makes sense. I'll delete the `else` clause from the above hunk.
On 09/20/2017 08:51 PM, Jeff King wrote:
> On Wed, Sep 20, 2017 at 02:40:58PM -0400, Jeff King wrote:
>
>>> +enum mmap_strategy {
>>> + /*
>>> + * Don't use mmap() at all for reading `packed-refs`.
>>> + */
>>> + MMAP_NONE,
>>> +
>>> + /*
>>> + * Can use mmap() for reading `packed-refs`, but the file must
>>> + * not remain mmapped. This is the usual option on Windows,
>>> + * where you cannot rename a new version of a file onto a file
>>> + * that is currently mmapped.
>>> + */
>>> + MMAP_TEMPORARY,
>>
>> I suspect you originally distinguished these cases so that NO_MMAP does
>> not read into a fake-mmap buffer, followed by us copying it into another
>> buffer. But AFAICT we handle the "NONE" and "TEMPORARY" cases exactly
>> the same (by just doing a read_in_full() into our own buffer). Do we
>> actually need separate strategies?
>
> In case you are reading these sequentially, I think I talked myself into
> the utility of this during the next patch. ;)
Sorry about that. I'll add a forward breadcrumb to the log message of
this commit.
Michael
