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

[shejialuo <shejialuo@gmail.com>]

On Thu, May 08, 2025 at 01:05:49PM -0700, Junio C Hamano wrote:
> 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).

That's right, I made a mistake here. I somehow think that we should just
check whether "mmap_strategy" is "MMAP_TEMPORARY". And this would be
enough. Because when "mmap_strategy" is "MMAP_NONE", we would never call
`mmap`.

Actually, when I refactor the code, this one makes me quite confusing.
And I think we should not change. I will revert in the next version.

> 
> 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.
> 

Yeah, sure. I didn't consider well.

> Thanks.

Thanks,
Jialuo