Re: [PATCH v2 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 8, 2025, 20:05 UTC
- Message-ID
- <xmqq4ixu6f6q.fsf@gitster.g>
- In-Reply-To
- <20250508195714.GA18229@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 25 quoted lines
>> - if (mmap_strategy != MMAP_OK && snapshot->mmapped) {
>> - /*
>> - * We don't want to leave the file mmapped, so we are
>> - * forced to make a copy now:
>> - */
>> - size_t size = snapshot->eof - snapshot->start;
>> - char *buf_copy = xmalloc(size);
>> -
>> - memcpy(buf_copy, snapshot->start, size);
>> - clear_snapshot_buffer(snapshot);
>> - snapshot->buf = snapshot->start = buf_copy;
>> - snapshot->eof = buf_copy + size;
>> - }
>> + if (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)
>> + munmap_temporary_snapshot(snapshot);
>
> The original triggers this conditional whenever the strategy is not
> MMAP_OK (so MMAP_TEMPORARY or MMAP_NONE). But in your post-image, we do
> so only for MMAP_TEMPORARY.
>
> I can guess that the two end up the same, because snapshot->mmapped
> would never be set when MMAP_NONE is set. But if we are going to make
> such a logical inference, it should be explained in the commit message
> (though my preference is to leave the code as-is, or to pull the
> refactor into its own commit).Good thinking.
An "extract" step that is meant as a preliminary refactoring should not make such a change. The change may or may not prepare the code for better maintainability, but I agree that such a change needs to be justified separately.
Thanks.