Re: [PATCH v4 3/3] packed-backend: mmap large "packed-refs" file during fsck

From: Junio C Hamano <gitster@pobox.com>
Date: May 13, 2025, 16:51 UTC
Message-ID: <xmqqcycch2t3.fsf@gitster.g>
In-Reply-To: <aCMoD-c_oHlu0c5c@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

Show 8 quoted lines

> During fsck, we use "strbuf_read" to read the content of "packed-refs"
> without using mmap mechanism. This is a bad practice which would consume
> more memory than using mmap mechanism. Besides, as all code paths in
> "packed-backend.c" use this way, we should make "fsck" align with the
> current codebase.
>
> As we have introduced the helper function "allocate_snapshot_buffer", we
> could simple use this function to use mmap mechanism.

"could simple" -> "can simply".

Show 6 quoted lines

> Suggested-by: Jeff King <peff@peff.net>
> Suggested-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
>  refs/packed-backend.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)

Nice loss of line count ;-)

> -	if (!st.st_size) {
> +	if (!allocate_snapshot_buffer(&snapshot, fd, &st)) {

It is a bit funny to see that a helper function that works at a much higher conceptual level treat an empty file so specially (namely, should it be different from a header-only packed-refs file?). If I were doing this refactoring in 2 & 3, I would probalby have made the helper return "void", and have callers who do care about st.st_size check that themselves.

But I'll let it pass.

Show 26 quoted lines

> @@ -2121,21 +2121,16 @@ static int packed_fsck(struct ref_store *ref_store,
>  		goto cleanup;
>  	}
>  
> -	if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
> -		ret = error_errno(_("unable to read '%s'"), refs->path);
> -		goto cleanup;
> -	}
> -
> -	ret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,
> -				      packed_ref_content.buf + packed_ref_content.len);
> +	ret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot.start,
> +				      snapshot.eof);
>  	if (!ret && sorted)
> -		ret = packed_fsck_ref_sorted(o, ref_store, packed_ref_content.buf,
> -					     packed_ref_content.buf + packed_ref_content.len);
> +		ret = packed_fsck_ref_sorted(o, ref_store, snapshot.start,
> +					     snapshot.eof);
>  
>  cleanup:
>  	if (fd >= 0)
>  		close(fd);
> -	strbuf_release(&packed_ref_content);
> +	clear_snapshot_buffer(&snapshot);
>  	return ret;
>  }