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