Re: [PATCH v2 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> 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.