Jeff King <peff@xxxxxxxx> writes:
>> - 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.
