[PATCH v2 0/4] align the behavior when opening "packed-refs"

[shejialuo <shejialuo@gmail.com>]

Hi All:

As discussed in [1], we need to use mmap mechanism to open large
"packed_refs" file to save the memory usage. This patch mainly does the
following things:

1: Fix an issue that we would report an error when the "packed-refs"
file is empty, which does not align with the runtime behavior.
2-4: Extract some logic from the existing code and then use these
created helper functions to let fsck code to use mmap necessarily

[1] https://lore.kernel.org/git/20250503133158.GA4450@coredump.intra.peff.net

Really thank Peff and Patrick to suggest me to do above change.

---

Change since v1:

1. Update the commit message of [PATCH 1/4]. And use redirection to
create an empty file instead of using `touch`.
2. Don't use if for the refactored function in [PATCH 3/4] and then
update the commit message to align with the new function name.
3. Enhance the commit message of [PATCH 4/4].

Thanks,
Jialuo

shejialuo (4):
  packed-backend: fsck should allow an empty "packed-refs" file
  packed-backend: extract snapshot allocation in `load_contents`
  packed-backend: extract munmap operation for `MMAP_TEMPORARY`
  packed-backend: mmap large "packed-refs" file during fsck

 refs/packed-backend.c    | 113 ++++++++++++++++++++++++---------------
 t/t0602-reffiles-fsck.sh |  13 +++++
 2 files changed, 82 insertions(+), 44 deletions(-)

Range-diff against v1:
1:  aa9037ebfa ! 1:  26c3fd55a8 packed-backend: skip checking consistency of empty packed-refs file
    @@ Metadata
     Author: shejialuo <shejialuo@gmail.com>
     
      ## Commit message ##
    -    packed-backend: skip checking consistency of empty packed-refs file
    +    packed-backend: fsck should allow an empty "packed-refs" file
     
    -    In "load_contents", when the "packed-refs" is empty, we will just return
    -    the snapshot. However, we would report an error to the user when
    -    checking the consistency of the empty "packed-refs".
    -
    -    We should align with the runtime behavior. As what "load_contents" does,
    -    let's check whether the file size is zero and if so, we will skip
    -    checking the consistency and simply return.
    +    During fsck, an empty "packed-refs" gives an error; this is unwarranted.
    +    We should just skip checking the content of "packed-refs" just like the
    +    runtime code paths such as "create_snapshot" which simply returns the
    +    "snapshot" without checking the content of "packed-refs".
     
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
    @@ t/t0602-reffiles-fsck.sh: test_expect_success SYMLINKS 'the filetype of packed-r
     +		cd repo &&
     +		test_commit default &&
     +
    -+		touch .git/packed-refs &&
    ++		>.git/packed-refs &&
     +		git refs verify 2>err &&
     +		test_must_be_empty err
     +	)
2:  852e8a606b = 2:  4604be8b51 packed-backend: extract snapshot allocation in `load_contents`
3:  de146155f6 ! 3:  c0609afac9 packed-backend: extract munmap operation for `MMAP_TEMPORARY`
    @@ Commit message
         is "MMAP_TEMPORARY". We also need to do this operation when checking the
         consistency of the "packed-refs" file.
     
    -    Create a new function "munmap_snapshot_if_temporary" to do above and
    +    Create a new function "munmap_temporary_snapshot" to do above and
         change "create_snapshot" to align with the behavior.
     
         Suggested-by: Jeff King <peff@peff.net>
    @@ refs/packed-backend.c: static int allocate_snapshot_buffer(struct snapshot *snap
      	return 1;
      }
      
    -+static void munmap_snapshot_if_temporary(struct snapshot *snapshot)
    ++static void munmap_temporary_snapshot(struct snapshot *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);
    ++	char *buf_copy;
    ++	size_t size;
     +
    -+		memcpy(buf_copy, snapshot->start, size);
    -+		clear_snapshot_buffer(snapshot);
    -+		snapshot->buf = snapshot->start = buf_copy;
    -+		snapshot->eof = buf_copy + size;
    -+	}
    ++	if (!snapshot)
    ++		return;
    ++
    ++	/*
    ++	 * We don't want to leave the file mmapped, so we are
    ++	 * forced to make a copy now:
    ++	 */
    ++	size = snapshot->eof - snapshot->start;
    ++	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;
     +}
     +
      /*
    @@ refs/packed-backend.c: static struct snapshot *create_snapshot(struct packed_ref
     -		snapshot->buf = snapshot->start = buf_copy;
     -		snapshot->eof = buf_copy + size;
     -	}
    -+	munmap_snapshot_if_temporary(snapshot);
    ++	if (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)
    ++		munmap_temporary_snapshot(snapshot);
      
      	return snapshot;
      }
4:  be1e9e2540 ! 4:  c868e3dd16 packed-backend: use mmap when opening large "packed-refs" file
    @@ Metadata
     Author: shejialuo <shejialuo@gmail.com>
     
      ## Commit message ##
    -    packed-backend: use mmap when opening large "packed-refs" file
    +    packed-backend: mmap large "packed-refs" file during fsck
     
    -    We use "strbuf_read" to read the content of "packed-refs". However, this
    -    is a bad practice which would consume a lot of memory usage if there are
    -    multiple processes reading large "packed-refs".
    +    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 two helper functions "allocate_snapshot_buffer"
         and "munmap_snapshot_if_temporary", we could simply call these functions
    @@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,
     +	if (!allocate_snapshot_buffer(snapshot, fd, &st))
      		goto cleanup;
     -	}
    -+	munmap_snapshot_if_temporary(snapshot);
      
     -	ret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,
     -				      packed_ref_content.buf + packed_ref_content.len);
    ++	if (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)
    ++		munmap_temporary_snapshot(snapshot);
    ++
     +	ret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot->start,
     +				      snapshot->eof);
      	if (!ret && sorted)
-- 
2.49.0