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

From: Jeff King <peff@peff.net>
Date: May 8, 2025, 19:57 UTC
Message-ID: <20250508195714.GA18229@coredump.intra.peff.net>
In-Reply-To: <aBt0BDTuOfUuCHE4@ArchLinux>

On Wed, May 07, 2025 at 10:53:56PM +0800, shejialuo wrote:

Show 19 quoted lines

> @@ -761,19 +782,8 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)
>  		verify_buffer_safe(snapshot);
>  	}
>  
> -	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).

-Peff