align the behavior when opening "packed-refs"

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/[email protected]

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

Thanks,
Jialuo

shejialuo (4):
packed-backend: skip checking consistency of empty packed-refs file
packed-backend: extract snapshot allocation in load_contents
packed-backend: extract munmap operation for MMAP_TEMPORARY
packed-backend: use mmap when opening large "packed-refs" file

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

2.49.0

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.

---
 refs/packed-backend.c    |  3 +++
 t/t0602-reffiles-fsck.sh | 13 +++++++++++++
 2 files changed, 16 insertions(+)

index 3ad1ed0787..0dd6c6677b 100644
@@ -2103,6 +2103,9 @@ static int packed_fsck(struct ref_store *ref_store,
 		goto cleanup;
 	}
 
+	if (!st.st_size)
+		goto cleanup;
+
 	if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
 		ret = error_errno(_("unable to read '%s'"), refs->path);
 		goto cleanup;

index 9d1dc2144c..0a9e9ccc55 100755
@@ -647,6 +647,19 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '
 	)
 '
 
+test_expect_success 'empty packed-refs should not be reported' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit default &&
+
+		touch .git/packed-refs &&
+		git refs verify 2>err &&
+		test_must_be_empty err
+	)
+'
+
 test_expect_success 'packed-refs header should be checked' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&

2.49.0

Thanks for the suggestion, I didn't realise about this.

Neither the commit title nor the above paragraph hints that this is talking about "fsck" part of the packed-refs subsystem. That leaves the readers confused when they read "with the runtime behavior" below.

How about

During fsck, an empty "packed-refs" file gives an error; this is unwarranted. We should instead just return an empty "snapshot" and let the caller happily declare success, just like the code paths that implement the runtime use of the file do.

or something?

As to the title

packed-backend: fsck should allow an empty packed-refs file

is shorter and clearer, I would think.

The code change is trivially correct, I think. Nicely found.

That's right. My message is vague.

Thanks, I will improve this in the next version.

"load_contents" would choose which way to load the content of the "packed-refs". However, we cannot directly use this function when checking the consistency due to we don't want to open the file. And we also need to reuse the logic to avoid causing repetition.

Let's create a new helper function "allocate_snapshot_buffer" to extract the snapshot allocation logic in "load_contents" and update the "load_contents" to align with the behavior.

---
 refs/packed-backend.c | 54 +++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

index 0dd6c6677b..e582227772 100644
@@ -517,6 +517,32 @@ static int refname_contains_nul(struct strbuf *refname)
 
 #define SMALL_FILE_SIZE (32*1024)
 
+static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)
+{
+	ssize_t bytes_read;
+	size_t size;
+
+	size = xsize_t(st->st_size);
+	if (!size)
+		return 0;
+
+	if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
+		snapshot->buf = xmalloc(size);
+		bytes_read = read_in_full(fd, snapshot->buf, size);
+		if (bytes_read < 0 || bytes_read != size)
+			die_errno("couldn't read %s", snapshot->refs->path);
+		snapshot->mmapped = 0;
+	} else {
+		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+		snapshot->mmapped = 1;
+	}
+
+	snapshot->start = snapshot->buf;
+	snapshot->eof = snapshot->buf + size;
+
+	return 1;
+}
+
 /*
  * Depending on `mmap_strategy`, either mmap or read the contents of
  * the `packed-refs` file into the snapshot. Return 1 if the file

@@ -525,10 +551,9 @@ static int refname_contains_nul(struct strbuf *refname)
  */
 static int load_contents(struct snapshot *snapshot)
 {
-	int fd;
 	struct stat st;
-	size_t size;
-	ssize_t bytes_read;
+	int ret = 1;
+	int fd;
 
 	fd = open(snapshot->refs->path, O_RDONLY);
 	if (fd < 0) {

@@ -550,27 +575,12 @@ static int load_contents(struct snapshot *snapshot)
 
 	if (fstat(fd, &st) < 0)
 		die_errno("couldn't stat %s", snapshot->refs->path);
-	size = xsize_t(st.st_size);
-
-	if (!size) {
-		close(fd);
-		return 0;
-	} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
-		snapshot->buf = xmalloc(size);
-		bytes_read = read_in_full(fd, snapshot->buf, size);
-		if (bytes_read < 0 || bytes_read != size)
-			die_errno("couldn't read %s", snapshot->refs->path);
-		snapshot->mmapped = 0;
-	} else {
-		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-		snapshot->mmapped = 1;
-	}
-	close(fd);
 
-	snapshot->start = snapshot->buf;
-	snapshot->eof = snapshot->buf + size;
+	if (!allocate_snapshot_buffer(snapshot, fd, &st))
+		ret = 0;
 
-	return 1;
+	close(fd);
+	return ret;
 }
 
 static const char *find_reference_location_1(struct snapshot *snapshot,

2.49.0

Trivially correct, cleanly done, and nicely described.

Thanks.

"create_snapshot" would try to munmap the file when the "mmap_strategy" 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 change "create_snapshot" to align with the behavior.

---
 refs/packed-backend.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

index e582227772..dd903db301 100644
@@ -543,6 +543,23 @@ static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct st
 	return 1;
 }
 
+static void munmap_snapshot_if_temporary(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);
+
+		memcpy(buf_copy, snapshot->start, size);
+		clear_snapshot_buffer(snapshot);
+		snapshot->buf = snapshot->start = buf_copy;
+		snapshot->eof = buf_copy + size;
+	}
+}
+
 /*
  * Depending on `mmap_strategy`, either mmap or read the contents of
  * the `packed-refs` file into the snapshot. Return 1 if the file

@@ -761,19 +778,7 @@ 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;
-	}
+	munmap_snapshot_if_temporary(snapshot);
 
 	return snapshot;
 }

2.49.0

Oops. I obviously meant "the callee" here.

That's right. Actually when I wake up in the morning, I suddenly find out I made a mistake here. We should try to make the function as pure as possible. So, this function would just do one thing, call mumap when the mmap strategy is "MMAP_TEMPORARY".

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

As we have introduced two helper functions "allocate_snapshot_buffer" and "munmap_snapshot_if_temporary", we could simply call these functions to use mmap mechanism.

---
 refs/packed-backend.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

index dd903db301..1370b982df 100644
@@ -2074,7 +2074,7 @@ static int packed_fsck(struct ref_store *ref_store,
 {
 	struct packed_ref_store *refs = packed_downcast(ref_store,
 							REF_STORE_READ, "fsck");
-	struct strbuf packed_ref_content = STRBUF_INIT;
+	struct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));
 	unsigned int sorted = 0;
 	struct stat st;
 	int ret = 0;

@@ -2121,21 +2121,21 @@ static int packed_fsck(struct ref_store *ref_store,
 	if (!st.st_size)
 		goto cleanup;
 
-	if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
-		ret = error_errno(_("unable to read '%s'"), refs->path);
+	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);
+	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);
+	free(snapshot);
 	return ret;
 }
 

2.49.0

Neither this nor the commit title says that the issue is limited to the code path that runs fsck on packed-refs file, but I thought the code paths to use packed-refs to resolve refs correctly uses mmap() and does not share this issue? If it is limited to one single code path, please mention it explicitly.

Also, I think it was already pointed out that "multiple processes" is not all that interesting issue. Even if there is a single process using a single large packed-refs file, alloc+read gives the system more memory pressure than the read-only mmap like we do.

As to the title

packed-backend: use mmap when opening large "packed-refs" file
packed-backend: mmap large "packed-refs" file during fsck

would be shorter and clearer.

The patch looks OK. Nice to see this one-off strbuf use going away.

Sorry to be cryptic. I meant to suggest the second one. The first one is what you gave us, and was left to contrast its length with the suggested altenative.

Nice advice, I will improve this in the next version.

That's right. Even there is only one process, we could avoid copying the file content from kernel space to the user space. I will improve the commit message.

Yes, the second one is better. Will improve this.

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/[email protected]

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 <[email protected]>

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

@@ 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 <[email protected]>
    @@ 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 <[email protected]>

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

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

---
 refs/packed-backend.c    |  3 +++
 t/t0602-reffiles-fsck.sh | 13 +++++++++++++
 2 files changed, 16 insertions(+)

index 3ad1ed0787..0dd6c6677b 100644
@@ -2103,6 +2103,9 @@ static int packed_fsck(struct ref_store *ref_store,
 		goto cleanup;
 	}
 
+	if (!st.st_size)
+		goto cleanup;
+
 	if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
 		ret = error_errno(_("unable to read '%s'"), refs->path);
 		goto cleanup;

index 9d1dc2144c..e04967581c 100755
@@ -647,6 +647,19 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '
 	)
 '
 
+test_expect_success 'empty packed-refs should not be reported' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit default &&
+
+		>.git/packed-refs &&
+		git refs verify 2>err &&
+		test_must_be_empty err
+	)
+'
+
 test_expect_success 'packed-refs header should be checked' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&

2.49.0

"load_contents" would choose which way to load the content of the "packed-refs". However, we cannot directly use this function when checking the consistency due to we don't want to open the file. And we also need to reuse the logic to avoid causing repetition.

Let's create a new helper function "allocate_snapshot_buffer" to extract the snapshot allocation logic in "load_contents" and update the "load_contents" to align with the behavior.

---
 refs/packed-backend.c | 54 +++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

index 0dd6c6677b..e582227772 100644
@@ -517,6 +517,32 @@ static int refname_contains_nul(struct strbuf *refname)
 
 #define SMALL_FILE_SIZE (32*1024)
 
+static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)
+{
+	ssize_t bytes_read;
+	size_t size;
+
+	size = xsize_t(st->st_size);
+	if (!size)
+		return 0;
+
+	if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
+		snapshot->buf = xmalloc(size);
+		bytes_read = read_in_full(fd, snapshot->buf, size);
+		if (bytes_read < 0 || bytes_read != size)
+			die_errno("couldn't read %s", snapshot->refs->path);
+		snapshot->mmapped = 0;
+	} else {
+		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+		snapshot->mmapped = 1;
+	}
+
+	snapshot->start = snapshot->buf;
+	snapshot->eof = snapshot->buf + size;
+
+	return 1;
+}
+
 /*
  * Depending on `mmap_strategy`, either mmap or read the contents of
  * the `packed-refs` file into the snapshot. Return 1 if the file

@@ -525,10 +551,9 @@ static int refname_contains_nul(struct strbuf *refname)
  */
 static int load_contents(struct snapshot *snapshot)
 {
-	int fd;
 	struct stat st;
-	size_t size;
-	ssize_t bytes_read;
+	int ret = 1;
+	int fd;
 
 	fd = open(snapshot->refs->path, O_RDONLY);
 	if (fd < 0) {

@@ -550,27 +575,12 @@ static int load_contents(struct snapshot *snapshot)
 
 	if (fstat(fd, &st) < 0)
 		die_errno("couldn't stat %s", snapshot->refs->path);
-	size = xsize_t(st.st_size);
-
-	if (!size) {
-		close(fd);
-		return 0;
-	} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
-		snapshot->buf = xmalloc(size);
-		bytes_read = read_in_full(fd, snapshot->buf, size);
-		if (bytes_read < 0 || bytes_read != size)
-			die_errno("couldn't read %s", snapshot->refs->path);
-		snapshot->mmapped = 0;
-	} else {
-		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-		snapshot->mmapped = 1;
-	}
-	close(fd);
 
-	snapshot->start = snapshot->buf;
-	snapshot->eof = snapshot->buf + size;
+	if (!allocate_snapshot_buffer(snapshot, fd, &st))
+		ret = 0;
 
-	return 1;
+	close(fd);
+	return ret;
 }
 
 static const char *find_reference_location_1(struct snapshot *snapshot,

2.49.0

"create_snapshot" would try to munmap the file when the "mmap_strategy" is "MMAP_TEMPORARY". We also need to do this operation when checking the consistency of the "packed-refs" file.

Create a new function "munmap_temporary_snapshot" to do above and change "create_snapshot" to align with the behavior.

---
 refs/packed-backend.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

index e582227772..ae6b6845a6 100644
@@ -543,6 +543,27 @@ static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct st
 	return 1;
 }
 
+static void munmap_temporary_snapshot(struct snapshot *snapshot)
+{
+	char *buf_copy;
+	size_t 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;
+}
+
 /*
  * Depending on `mmap_strategy`, either mmap or read the contents of
  * the `packed-refs` file into the snapshot. Return 1 if the file

@@ -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);
 
 	return snapshot;
 }

2.49.0

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.

Thanks.

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.

Yeah, sure. I didn't consider well.

Thanks,
Jialuo

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 to use mmap mechanism.

---
 refs/packed-backend.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

index ae6b6845a6..ff744f1d4c 100644
@@ -2079,7 +2079,7 @@ static int packed_fsck(struct ref_store *ref_store,
 {
 	struct packed_ref_store *refs = packed_downcast(ref_store,
 							REF_STORE_READ, "fsck");
-	struct strbuf packed_ref_content = STRBUF_INIT;
+	struct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));
 	unsigned int sorted = 0;
 	struct stat st;
 	int ret = 0;

@@ -2126,21 +2126,23 @@ static int packed_fsck(struct ref_store *ref_store,
 	if (!st.st_size)
 		goto cleanup;
 
-	if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
-		ret = error_errno(_("unable to read '%s'"), refs->path);
+	if (!allocate_snapshot_buffer(snapshot, fd, &st))
 		goto cleanup;
-	}
 
-	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)
-		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);
+	free(snapshot);
 	return ret;
 }
 

2.49.0

I simply copy the code from the existing code... I will change.

Good catch. I remember in the V1, this does not exist. I may make something wrong when rebasing the code. Thanks!

I simply follow how "create_snapshot" does. Actually, I am also quite confused about this. If we would eventually copy the content into the user space's memory. What is the reason that we mmap at Windows in the first place?

My understanding is that after mmaping, we need to do some sanity checks and then if there is a need, we may sort the "packed-refs" file. So, we would improve some efficiency at Windows for this part?

I will improve this in the next version.

Ah, I see. The existing code must allocate on the heap because it is returning the snapshot to its caller. But here the variable is completely local to the function.

However, if we stop using mmap_strategy altogether and just use xmmap() directly, I don't think you'd even need a snapshot variable.

Ah, yes, that makes sense for the existing code. If we do have to sort, then mapping on Windows means we could sort into our internal buffer, saving an extra copy. That is probably a rare case these days (once upon a time the file was not guaranteed to be sorted, but these days the writer makes sure it is sorted and puts a marker in the header claiming it is so). So maybe that approach is not as useful as it once was, but I don't know if it's worth spending effort to rip it out now.

I don't think any of that applies for packed_fsck(), though, since it is just processing the file linearly in that case (rather than making a sorted copy).

-Peff

Yes, that's right. Maybe the simplest way is to use xmmap(). But I don't want to introduce repetition. In the current codebase, we already have the logic to load the "packed-refs". Let's just reuse it.

Out of topic, I have more to express.

Actually, my eventual goal is to unify the fsck and other parts. For example, "create_snapshot" would also do some basic sanity checks. And of course, there is some overlap between fsck and this function. And also for "next_record", it would also check something.

During my implementation, I find it hard to unify and it would require a lot of effort. So, I simply introduce some redundant logic. But this is not perfect. Because I still parse the "packed-refs" file just like "next_record" and "create_snapshot" do. And the most disappointed thing is that I cannot reuse them at all. But the things I want to do is very similar to these functions.

So, I think I would eventually to find out a way to do above in the future.

Thanks,
Jialuo

This round looks good to me. Others?

Thanks.

I left a few comments that I think bear addressing (or at least some discussion).

-Peff

I found both of your points very good ones, especially the "why are we making an in-core copy anyway later?"

Which may probably mean that munmap_temporary_snapshot() is not the helper function we want in the code path, so one of the preliminary refactoring patches can be removed.

Even on mmap-incapable platforms, we have enough emulation in git_mmap() and git_munmap(), and this code path that wants to read a packed-refs file just mmap(), do its thing, and then munmap(), without worrying anything about "ah, temporary, so we need to make an in-core copy for ourselves".

THanks.

Yep.

Hmm, yeah. I had thought that somebody could explicitly set mmap_strategy themselves via config, in which case we'd want to avoid calling git_mmap() on systems where it actually does mmap. But it doesn't look like we have any mechanism for setting the config. So MMAP_NONE happens only when NO_MMAP is set.

I briefly wondered if the existing code could be simplified to get rid of MMAP_NONE, since it could also rely on git_mmap() to make an in-memory copy. But it gets weird with MMAP_PREVENTS_DELETE and NO_MMAP combined, since then we make a pointless extra copy (one to fake-mmap, and one into the new buffer). OTOH, I'd expect those to be mutually exclusive.

I kind of wonder with MMAP_TEMPORARY why we bother mapping at all and not just reading into a buffer. Maybe it's more efficient?

Probably not worth revisiting all of this old code, though. And anyway, because of the SMALL_FILE_SIZE check, we couldn't even simplify away the read_in_full() code.

-Peff

Right, I will remove this patch in the next version.

I am also confused about this. If we eventually would allocate a buffer, it is wired that we map in the first place.

Right, I will try to clean the code later. At now, I think we need to concentrate on using mmap. I will add this into my TODOs. Thanks for the suggestion.

Thanks,
Jialuo

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/[email protected]

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

---

Change in v2:

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

---

Change in v3:

1. Drop the patch which creates a new function "munmap_temporary_snapshot". As discussed, there is no need to munmap the file during fsck.
2. Allocate snapshot variable in the stack instead of heap.
3. Fix rebase issue, remove unneeded code to check the file size explicitly.

The range-diff seems unreadable. Briefly, for this new version, the only change is the last patch compared to the v2.

packed-backend: mmap large "packed-refs" file during fsck

Thanks,
Jialuo

shejialuo (3):
packed-backend: fsck should allow an empty "packed-refs" file
packed-backend: extract snapshot allocation in load_contents
packed-backend: mmap large "packed-refs" file during fsck

 refs/packed-backend.c    | 70 ++++++++++++++++++++++------------------
 t/t0602-reffiles-fsck.sh | 13 ++++++++
 2 files changed, 52 insertions(+), 31 deletions(-)

Range-diff against v2:

-:  ---------- > 1:  26c3fd55a8 packed-backend: fsck should allow an empty "packed-refs" file
1:  c0609afac9 ! 2:  4604be8b51 packed-backend: extract munmap operation for `MMAP_TEMPORARY`
    @@ Metadata
     Author: shejialuo <[email protected]>
     
      ## Commit message ##
    -    packed-backend: extract munmap operation for `MMAP_TEMPORARY`
    +    packed-backend: extract snapshot allocation in `load_contents`
     
    -    "create_snapshot" would try to munmap the file when the "mmap_strategy"
    -    is "MMAP_TEMPORARY". We also need to do this operation when checking the
    -    consistency of the "packed-refs" file.
    +    "load_contents" would choose which way to load the content of the
    +    "packed-refs". However, we cannot directly use this function when
    +    checking the consistency due to we don't want to open the file. And we
    +    also need to reuse the logic to avoid causing repetition.
     
    -    Create a new function "munmap_temporary_snapshot" to do above and
    -    change "create_snapshot" to align with the behavior.
    +    Let's create a new helper function "allocate_snapshot_buffer" to extract
    +    the snapshot allocation logic in "load_contents" and update the
    +    "load_contents" to align with the behavior.
     
         Suggested-by: Jeff King <[email protected]>
         Suggested-by: Patrick Steinhardt <[email protected]>
         Signed-off-by: shejialuo <[email protected]>
     
      ## refs/packed-backend.c ##
    -@@ refs/packed-backend.c: static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct st
    - 	return 1;
    - }
    +@@ refs/packed-backend.c: static int refname_contains_nul(struct strbuf *refname)
    + 
    + #define SMALL_FILE_SIZE (32*1024)
      
    -+static void munmap_temporary_snapshot(struct snapshot *snapshot)
    ++static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)
     +{
    -+	char *buf_copy;
    ++	ssize_t bytes_read;
     +	size_t size;
     +
    -+	if (!snapshot)
    -+		return;
    ++	size = xsize_t(st->st_size);
    ++	if (!size)
    ++		return 0;
    ++
    ++	if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
    ++		snapshot->buf = xmalloc(size);
    ++		bytes_read = read_in_full(fd, snapshot->buf, size);
    ++		if (bytes_read < 0 || bytes_read != size)
    ++			die_errno("couldn't read %s", snapshot->refs->path);
    ++		snapshot->mmapped = 0;
    ++	} else {
    ++		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
    ++		snapshot->mmapped = 1;
    ++	}
     +
    -+	/*
    -+	 * 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);
    ++	snapshot->start = snapshot->buf;
    ++	snapshot->eof = snapshot->buf + size;
     +
    -+	memcpy(buf_copy, snapshot->start, size);
    -+	clear_snapshot_buffer(snapshot);
    -+	snapshot->buf = snapshot->start = buf_copy;
    -+	snapshot->eof = buf_copy + size;
    ++	return 1;
     +}
     +
      /*
       * Depending on `mmap_strategy`, either mmap or read the contents of
       * the `packed-refs` file into the snapshot. Return 1 if the file
    -@@ refs/packed-backend.c: static struct snapshot *create_snapshot(struct packed_ref_store *refs)
    - 		verify_buffer_safe(snapshot);
    - 	}
    +@@ refs/packed-backend.c: static int refname_contains_nul(struct strbuf *refname)
    +  */
    + static int load_contents(struct snapshot *snapshot)
    + {
    +-	int fd;
    + 	struct stat st;
    +-	size_t size;
    +-	ssize_t bytes_read;
    ++	int ret = 1;
    ++	int fd;
      
    --	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);
    + 	fd = open(snapshot->refs->path, O_RDONLY);
    + 	if (fd < 0) {
    +@@ refs/packed-backend.c: static int load_contents(struct snapshot *snapshot)
    + 
    + 	if (fstat(fd, &st) < 0)
    + 		die_errno("couldn't stat %s", snapshot->refs->path);
    +-	size = xsize_t(st.st_size);
     -
    --		memcpy(buf_copy, snapshot->start, size);
    --		clear_snapshot_buffer(snapshot);
    --		snapshot->buf = snapshot->start = buf_copy;
    --		snapshot->eof = buf_copy + size;
    +-	if (!size) {
    +-		close(fd);
    +-		return 0;
    +-	} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
    +-		snapshot->buf = xmalloc(size);
    +-		bytes_read = read_in_full(fd, snapshot->buf, size);
    +-		if (bytes_read < 0 || bytes_read != size)
    +-			die_errno("couldn't read %s", snapshot->refs->path);
    +-		snapshot->mmapped = 0;
    +-	} else {
    +-		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
    +-		snapshot->mmapped = 1;
     -	}
    -+	if (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)
    -+		munmap_temporary_snapshot(snapshot);
    +-	close(fd);
    + 
    +-	snapshot->start = snapshot->buf;
    +-	snapshot->eof = snapshot->buf + size;
    ++	if (!allocate_snapshot_buffer(snapshot, fd, &st))
    ++		ret = 0;
      
    - 	return snapshot;
    +-	return 1;
    ++	close(fd);
    ++	return ret;
      }
    + 
    + static const char *find_reference_location_1(struct snapshot *snapshot,
2:  c868e3dd16 ! 3:  82e19a65c6 packed-backend: mmap large "packed-refs" file during fsck
    @@ Commit message
         "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
    -    to use mmap mechanism.
    +    As we have introduced the helper function "allocate_snapshot_buffer", we
    +    could simple use this function to use mmap mechanism.
     
         Suggested-by: Jeff King <[email protected]>
         Suggested-by: Patrick Steinhardt <[email protected]>
    @@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,
      	struct packed_ref_store *refs = packed_downcast(ref_store,
      							REF_STORE_READ, "fsck");
     -	struct strbuf packed_ref_content = STRBUF_INIT;
    -+	struct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));
    ++	struct snapshot snapshot = { 0 };
      	unsigned int sorted = 0;
      	struct stat st;
      	int ret = 0;
     @@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,
    - 	if (!st.st_size)
    + 		goto cleanup;
    + 	}
    + 
    +-	if (!st.st_size)
    ++	if (!allocate_snapshot_buffer(&snapshot, fd, &st))
      		goto cleanup;
      
     -	if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
     -		ret = error_errno(_("unable to read '%s'"), refs->path);
    -+	if (!allocate_snapshot_buffer(snapshot, fd, &st))
    - 		goto cleanup;
    +-		goto cleanup;
     -	}
    - 
    +-
     -	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);
    ++	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);
    ++		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);
    -+	free(snapshot);
    ++	clear_snapshot_buffer(&snapshot);
      	return ret;
      }

2.49.0

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

---
 refs/packed-backend.c    |  3 +++
 t/t0602-reffiles-fsck.sh | 13 +++++++++++++
 2 files changed, 16 insertions(+)

index 3ad1ed0787..0dd6c6677b 100644
@@ -2103,6 +2103,9 @@ static int packed_fsck(struct ref_store *ref_store,
 		goto cleanup;
 	}
 
+	if (!st.st_size)
+		goto cleanup;
+
 	if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
 		ret = error_errno(_("unable to read '%s'"), refs->path);
 		goto cleanup;

index 9d1dc2144c..e04967581c 100755
@@ -647,6 +647,19 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '
 	)
 '
 
+test_expect_success 'empty packed-refs should not be reported' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit default &&
+
+		>.git/packed-refs &&
+		git refs verify 2>err &&
+		test_must_be_empty err
+	)
+'
+
 test_expect_success 'packed-refs header should be checked' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&

2.49.0

I think this doesn't quite answer the question whether this is a _good_ idea though. The question that we need to answer is whether there are any writing code paths that may end up writing a "packed-refs" file that is completely empty. Modern Git would at least write the packed-refs header, wouldn't it?

The reason why I'm a little sceptical is that there is a common problem with ext4 caused by its delayed allocation [1]. If you:

1. Write data to a temporary file.
2. Rename the file into place.
3. The host system crashes.

Then it may happen that the renamed file is now completely empty.

The root cause is a bug in the application: before renaming the file into place it _must_ fsync the file to disk. Git does that by default, but it is extremely easy to get wrong and we had bugs around this until ~2 years ago, if I remember correctly. We hit the problem several times in our production systems.

So I wonder whether ignoring empty files would cause us to miss such a common error. But I guess if there are valid cases where we may end up with an empty "packed-refs" file we cannot do anything about it.

Patrick

[1]: https://thunk.org/tytso/blog/2009/03/12/delayed-allocation-and-the-zero-length-file-problem/

That's right. In the current codebase, we would always write the header which could be easily reproduced by using the following command:

git init repo
cd repo && git pack-refs
cat .git/packed-refs

And in "packed-backend.c::write_with_updates", we would always write the header.

I see. I agree that in the most situation, an empty "packed-refs" file means that there is an issue.

I somehow think we would always write header in the Modern Git. But "create_snapshot" accept an empty existing "packed-refs" file at runtime.

And header is introduced in 694b7a1999 (repack_without_ref(): write peeled refs in the rewritten file, 2013-04-22). At this commit, we would always write the header into the "packed-refs" file.

But in runtime, we accept empty file or no header of the file content as we want to keep compatible. In my humble word, I think we should allow empty file at now. Then, In Git 3.0, we tighten all the rules (there must always be a header etc) and also update the runtime behavior.

Thanks,
Jialuo

So what happened before this commit? Would we end up writing a completely empty file?

I think it depends. If we can prove that Git (and other, third-party implementations thereof) wouldn't have ever written such "packed-refs" files then we should start warning now already, even if we accept such files at runtime. Otherwise, if there were versions of Git that could have ended up writing such files I agree that we have to accept this for now.

But if we were to deprecate the ability to read an empty file I wouldn't adapt both paths at the Git 3.0 boundary. If we decide that an empty file is broken we should introduce the check for git refs verify way before we tighten the runtime behaviour. Otherwise there isn't really an opportunity for people to learn that we are about to remove this.

Patrick

Yeah, prior to that if you delete the final entry, you'd get an empty file. Reproducible with:

  # you can use modern git for this part
  git init
  git commit --allow-empty -m foo
  git checkout --detach
  git pack-refs --all

  # old git writes empty file during deletion
  git.v1.8.4 branch -D main
  ls -l .git/packed-refs

Going back even further, we'd also write an empty file via pack-refs. With git v1.4.4, doing "git init && git pack-refs" will get you an empty file. I didn't bisect completely, but presumably that changed in f4204ab9f6 (Store peeled refs in packed-refs (take 2)., 2006-11-21).

So we definitely did write those files. Those are pretty old versions of Git, but something we should continue to support.

It may be useful for fsck to detect this, though, even if the default message severity is set to "info" or even "ignore. That would allow people who know they are using modern Git to increase it themselves (I don't expect normal users to do this, but it would probably be useful for forges which run automated "fsck" across a lot of repos).

And then the backwards-incompatible Git 3.0 thing would just be tweaking the severity of the config (and in the meantime, it would help flush out any unexpected instances people run into).

-Peff

I came to make a same comment but the above has everything I wanted to say (and more).

Yup, agreed, that sounds like a reasonable approach indeed.

Patrick

Agree, I will use a "info" to report an empty "packed-refs" file. Thank everyone.

Jialuo

"load_contents" would choose which way to load the content of the "packed-refs". However, we cannot directly use this function when checking the consistency due to we don't want to open the file. And we also need to reuse the logic to avoid causing repetition.

Let's create a new helper function "allocate_snapshot_buffer" to extract the snapshot allocation logic in "load_contents" and update the "load_contents" to align with the behavior.

---
 refs/packed-backend.c | 54 +++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

index 0dd6c6677b..e582227772 100644
@@ -517,6 +517,32 @@ static int refname_contains_nul(struct strbuf *refname)
 
 #define SMALL_FILE_SIZE (32*1024)
 
+static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)
+{
+	ssize_t bytes_read;
+	size_t size;
+
+	size = xsize_t(st->st_size);
+	if (!size)
+		return 0;
+
+	if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
+		snapshot->buf = xmalloc(size);
+		bytes_read = read_in_full(fd, snapshot->buf, size);
+		if (bytes_read < 0 || bytes_read != size)
+			die_errno("couldn't read %s", snapshot->refs->path);
+		snapshot->mmapped = 0;
+	} else {
+		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+		snapshot->mmapped = 1;
+	}
+
+	snapshot->start = snapshot->buf;
+	snapshot->eof = snapshot->buf + size;
+
+	return 1;
+}
+
 /*
  * Depending on `mmap_strategy`, either mmap or read the contents of
  * the `packed-refs` file into the snapshot. Return 1 if the file

@@ -525,10 +551,9 @@ static int refname_contains_nul(struct strbuf *refname)
  */
 static int load_contents(struct snapshot *snapshot)
 {
-	int fd;
 	struct stat st;
-	size_t size;
-	ssize_t bytes_read;
+	int ret = 1;
+	int fd;
 
 	fd = open(snapshot->refs->path, O_RDONLY);
 	if (fd < 0) {

@@ -550,27 +575,12 @@ static int load_contents(struct snapshot *snapshot)
 
 	if (fstat(fd, &st) < 0)
 		die_errno("couldn't stat %s", snapshot->refs->path);
-	size = xsize_t(st.st_size);
-
-	if (!size) {
-		close(fd);
-		return 0;
-	} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
-		snapshot->buf = xmalloc(size);
-		bytes_read = read_in_full(fd, snapshot->buf, size);
-		if (bytes_read < 0 || bytes_read != size)
-			die_errno("couldn't read %s", snapshot->refs->path);
-		snapshot->mmapped = 0;
-	} else {
-		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-		snapshot->mmapped = 1;
-	}
-	close(fd);
 
-	snapshot->start = snapshot->buf;
-	snapshot->eof = snapshot->buf + size;
+	if (!allocate_snapshot_buffer(snapshot, fd, &st))
+		ret = 0;
 
-	return 1;
+	close(fd);
+	return ret;
 }
 
 static const char *find_reference_location_1(struct snapshot *snapshot,

2.49.0

Huh. Are you sure I suggested this? :) I cannot remember at least.

That being said, the change looks sensible.

Patrick

Because you explain me a lot how Gitlab handles and Peff tells me how Github handles, I add both of you.

I think that would've made sense for the last step where you introduce the adapted logic. But the intermediate steps have all been designed by yourself without my help :)

Anyway, I don't mind this, I just found it to be funny.

Patrick

This looks good to me. One thing that did give me a slight pause while reviewing:

  int ret;
  ...
  ret = allocate_snapshot_buffer(snapshot, fd, &st);

That's right, it would be more clear. I will update in the next version.

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.

---
 refs/packed-backend.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

index e582227772..85f5a45160 100644
@@ -2069,7 +2069,7 @@ static int packed_fsck(struct ref_store *ref_store,
 {
 	struct packed_ref_store *refs = packed_downcast(ref_store,
 							REF_STORE_READ, "fsck");
-	struct strbuf packed_ref_content = STRBUF_INIT;
+	struct snapshot snapshot = { 0 };
 	unsigned int sorted = 0;
 	struct stat st;
 	int ret = 0;

@@ -2113,24 +2113,19 @@ static int packed_fsck(struct ref_store *ref_store,
 		goto cleanup;
 	}
 
-	if (!st.st_size)
+	if (!allocate_snapshot_buffer(&snapshot, fd, &st))
 		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;
 }
 

2.49.0

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/[email protected]

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

---

Change in v2:

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

---

Change in v3:

1. Drop the patch which creates a new function "munmap_temporary_snapshot". As discussed, there is no need to munmap the file during fsck.
2. Allocate snapshot variable in the stack instead of heap.
3. Fix rebase issue, remove unneeded code to check the file size explicitly.

---

Change in v4:

1. Report the "emptyPackedRefsFile(INFO)" to the user when the "packed-refs" is empty instead of ONLY skipping checking the content and update the shell script and commit message.
2. Apply Peff's advice to make [PATCH v3 2/3] more clear.

Thanks,
Jialuo

shejialuo (3):
packed-backend: fsck should warn when "packed-refs" file is empty
packed-backend: extract snapshot allocation in load_contents
packed-backend: mmap large "packed-refs" file during fsck

 Documentation/fsck-msgids.adoc |  6 +++
 fsck.h                         |  1 +
 refs/packed-backend.c          | 73 ++++++++++++++++++++--------------
 t/t0602-reffiles-fsck.sh       | 17 ++++++++
 4 files changed, 67 insertions(+), 30 deletions(-)

Range-diff against v3:
1: 26c3fd55a8 ! 1: 75636c9c85 packed-backend: fsck should allow an empty "packed-refs" file
@@ Metadata
Author: shejialuo <[email protected]>

## Commit message ##
- packed-backend: fsck should allow an empty "packed-refs" file
+ packed-backend: fsck should warn when "packed-refs" file is empty

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".
+ The runtime code paths would accept an empty "packed-refs" file, such as
+ "create_snapshot" would simply return the "snapshot" without checking
+ the content of "packed-refs".
+
+ And we should also skip checking the content of "packed-refs" when it is
+ empty during fsck. However, we should think about whether we need to
+ report something to the users in this case.
+
+ After 694b7a1999 (repack_without_ref(): write peeled refs in the
+ rewritten file, 2013-04-22), we would always write a header into the
+ "packed-refs" file where we would never create empty file since then.
+ Because we only create empty "packed-refs" in the very early versions,
+ we may tighten this rule in the future. In order to notify the users
+ about this, we should at least report an warning to the users.
+
+ But we need to consider the fsck message type carefully, it is not
+ appropriate that we use "FSCK_ERROR". This is because we would
+ definitely break the compatibility. Let's create a "FSCK_INFO" message
+ id EMPTY_PACKED_REFS_FILE" to indicate that "packed-refs" is empty.

+ ## Documentation/fsck-msgids.adoc ##
+@@
+ emptyName::
+ (WARN) A path contains an empty name.
+
++emptyPackedRefsFile::
++ (INFO) "packed-refs" file is empty. Report to the
++ [email protected] mailing list if you see this error. As only
++ very early versions of Git would create such an empty
++ "packed_refs" file, we might tighten this rule in the future.
++
+ extraHeaderEntry::
+ (IGNORE) Extra headers found after tagger.
+
+
+ ## fsck.h ##
+@@ fsck.h: enum fsck_msg_type {
+ FUNC(LARGE_PATHNAME, WARN) \
+ /* infos (reported as warnings, but ignored by default) */ \
+ FUNC(BAD_FILEMODE, INFO) \
++ FUNC(EMPTY_PACKED_REFS_FILE, INFO) \
+ FUNC(GITMODULES_PARSE, INFO) \
+ FUNC(GITIGNORE_SYMLINK, INFO) \
+ FUNC(GITATTRIBUTES_SYMLINK, INFO) \
+
## refs/packed-backend.c ##
@@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,
goto cleanup;
}

-+ if (!st.st_size)
++ if (!st.st_size) {
++ struct fsck_ref_report report = { 0 };
++ report.path = "packed-refs";
++ ret = fsck_report_ref(o, &report,
++ FSCK_MSG_EMPTY_PACKED_REFS_FILE,
++ "file is empty");
+ goto cleanup;
++ }
+
if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
ret = error_errno(_("unable to read '%s'"), refs->path);
@@ t/t0602-reffiles-fsck.sh: test_expect_success SYMLINKS 'the filetype of packed-r
)
'

-+test_expect_success 'empty packed-refs should not be reported' ' ++test_expect_success 'empty packed-refs should be reported' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
@@ t/t0602-reffiles-fsck.sh: test_expect_success SYMLINKS 'the filetype of packed-r
+
+ >.git/packed-refs &&
+ git refs verify 2>err &&
-+ test_must_be_empty err
++ cat >expect <<-EOF &&
++ warning: packed-refs: emptyPackedRefsFile: file is empty
++ EOF
++ rm .git/packed-refs &&
++ test_cmp expect err
+ )
+'
+
2: 4604be8b51 ! 2: 1a5893379d packed-backend: extract snapshot allocation in load_contents

    @@ refs/packed-backend.c: static int refname_contains_nul(struct strbuf *refname)
      	struct stat st;
     -	size_t size;
     -	ssize_t bytes_read;
    -+	int ret = 1;
    ++	int ret;
     +	int fd;
      
      	fd = open(snapshot->refs->path, O_RDONLY);
    @@ refs/packed-backend.c: static int load_contents(struct snapshot *snapshot)
      
     -	snapshot->start = snapshot->buf;
     -	snapshot->eof = snapshot->buf + size;
    -+	if (!allocate_snapshot_buffer(snapshot, fd, &st))
    -+		ret = 0;
    ++	ret = allocate_snapshot_buffer(snapshot, fd, &st);
      
     -	return 1;
     +	close(fd);
3:  82e19a65c6 ! 3:  31e272db7e packed-backend: mmap large "packed-refs" file during fsck
    @@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,
      		goto cleanup;
      	}
      
    --	if (!st.st_size)
    -+	if (!allocate_snapshot_buffer(&snapshot, fd, &st))
    +-	if (!st.st_size) {
    ++	if (!allocate_snapshot_buffer(&snapshot, fd, &st)) {
    + 		struct fsck_ref_report report = { 0 };
    + 		report.path = "packed-refs";
    + 		ret = fsck_report_ref(o, &report,
    +@@ refs/packed-backend.c: 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);

2.49.0

During fsck, an empty "packed-refs" gives an error; this is unwarranted. The runtime code paths would accept an empty "packed-refs" file, such as "create_snapshot" would simply return the "snapshot" without checking the content of "packed-refs".

And we should also skip checking the content of "packed-refs" when it is empty during fsck. However, we should think about whether we need to report something to the users in this case.

After 694b7a1999 (repack_without_ref(): write peeled refs in the rewritten file, 2013-04-22), we would always write a header into the "packed-refs" file where we would never create empty file since then. Because we only create empty "packed-refs" in the very early versions, we may tighten this rule in the future. In order to notify the users about this, we should at least report an warning to the users.

But we need to consider the fsck message type carefully, it is not appropriate that we use "FSCK_ERROR". This is because we would definitely break the compatibility. Let's create a "FSCK_INFO" message id EMPTY_PACKED_REFS_FILE" to indicate that "packed-refs" is empty.

---
 Documentation/fsck-msgids.adoc |  6 ++++++
 fsck.h                         |  1 +
 refs/packed-backend.c          |  9 +++++++++
 t/t0602-reffiles-fsck.sh       | 17 +++++++++++++++++
 4 files changed, 33 insertions(+)

index 9601fff228..0ba4f9a27e 100644
@@ -59,6 +59,12 @@
 `emptyName`::
 	(WARN) A path contains an empty name.
 
+`emptyPackedRefsFile`::
+	(INFO) "packed-refs" file is empty. Report to the
+	[email protected] mailing list if you see this error. As only
+	very early versions of Git would create such an empty
+	"packed_refs" file, we might tighten this rule in the future.
+
 `extraHeaderEntry`::
 	(IGNORE) Extra headers found after `tagger`.
 

index b1deae61ee..0c5869ac34 100644
@@ -84,6 +84,7 @@ enum fsck_msg_type {
 	FUNC(LARGE_PATHNAME, WARN) \
 	/* infos (reported as warnings, but ignored by default) */ \
 	FUNC(BAD_FILEMODE, INFO) \
+	FUNC(EMPTY_PACKED_REFS_FILE, INFO) \
 	FUNC(GITMODULES_PARSE, INFO) \
 	FUNC(GITIGNORE_SYMLINK, INFO) \
 	FUNC(GITATTRIBUTES_SYMLINK, INFO) \

index 3ad1ed0787..fb91833e76 100644
@@ -2103,6 +2103,15 @@ static int packed_fsck(struct ref_store *ref_store,
 		goto cleanup;
 	}
 
+	if (!st.st_size) {
+		struct fsck_ref_report report = { 0 };
+		report.path = "packed-refs";
+		ret = fsck_report_ref(o, &report,
+				      FSCK_MSG_EMPTY_PACKED_REFS_FILE,
+				      "file is empty");
+		goto cleanup;
+	}
+
 	if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
 		ret = error_errno(_("unable to read '%s'"), refs->path);
 		goto cleanup;

index 9d1dc2144c..f671ac4d3a 100755
@@ -647,6 +647,23 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '
 	)
 '
 
+test_expect_success 'empty packed-refs should be reported' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit default &&
+
+		>.git/packed-refs &&
+		git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		warning: packed-refs: emptyPackedRefsFile: file is empty
+		EOF
+		rm .git/packed-refs &&
+		test_cmp expect err
+	)
+'
+
 test_expect_success 'packed-refs header should be checked' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&

2.49.0

Perhaps "unwarranted" is now too strong a word; we still want to consider it an anomaly (that is why emptyPackedRefsFile warning is introduced after all). I think the problem description you want to here in the above pragraph is that fsck giving an error and runtime completely silent is inconsistent.

Side note: and you'd probably want to say what "an error" reported here is. The problem, if I understand correctly, is that the code assumes the file won't be empty and instead has at least one line in it (even when there are no refs packed, there is the file header line) and insists that all lines must be well terminated---if we tolerate an empty file, of course such a check will fail, as there is no terminating LF in a file with 0 lines in it.

And because versions of Git that are not too ancient never wrote an empty packed-refs file, and often having an empty file there is/was a sign of a filesystem-level issue, the way we want resolve this inconsistency is not make everybody totally silent but notice and report the anomaly.

OK.

Good idea, I will improve the commit message.

That's right. I should talk about this problem.

That's right. Actually we introduce redirection here. I think I will improve this in the next release cycle. I'll add this into my TODO list.

"load_contents" would choose which way to load the content of the "packed-refs". However, we cannot directly use this function when checking the consistency due to we don't want to open the file. And we also need to reuse the logic to avoid causing repetition.

Let's create a new helper function "allocate_snapshot_buffer" to extract the snapshot allocation logic in "load_contents" and update the "load_contents" to align with the behavior.

---
 refs/packed-backend.c | 53 +++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

index fb91833e76..1da44a3d6d 100644
@@ -517,6 +517,32 @@ static int refname_contains_nul(struct strbuf *refname)
 
 #define SMALL_FILE_SIZE (32*1024)
 
+static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)
+{
+	ssize_t bytes_read;
+	size_t size;
+
+	size = xsize_t(st->st_size);
+	if (!size)
+		return 0;
+
+	if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
+		snapshot->buf = xmalloc(size);
+		bytes_read = read_in_full(fd, snapshot->buf, size);
+		if (bytes_read < 0 || bytes_read != size)
+			die_errno("couldn't read %s", snapshot->refs->path);
+		snapshot->mmapped = 0;
+	} else {
+		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+		snapshot->mmapped = 1;
+	}
+
+	snapshot->start = snapshot->buf;
+	snapshot->eof = snapshot->buf + size;
+
+	return 1;
+}
+
 /*
  * Depending on `mmap_strategy`, either mmap or read the contents of
  * the `packed-refs` file into the snapshot. Return 1 if the file

@@ -525,10 +551,9 @@ static int refname_contains_nul(struct strbuf *refname)
  */
 static int load_contents(struct snapshot *snapshot)
 {
-	int fd;
 	struct stat st;
-	size_t size;
-	ssize_t bytes_read;
+	int ret;
+	int fd;
 
 	fd = open(snapshot->refs->path, O_RDONLY);
 	if (fd < 0) {

@@ -550,27 +575,11 @@ static int load_contents(struct snapshot *snapshot)
 
 	if (fstat(fd, &st) < 0)
 		die_errno("couldn't stat %s", snapshot->refs->path);
-	size = xsize_t(st.st_size);
-
-	if (!size) {
-		close(fd);
-		return 0;
-	} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
-		snapshot->buf = xmalloc(size);
-		bytes_read = read_in_full(fd, snapshot->buf, size);
-		if (bytes_read < 0 || bytes_read != size)
-			die_errno("couldn't read %s", snapshot->refs->path);
-		snapshot->mmapped = 0;
-	} else {
-		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-		snapshot->mmapped = 1;
-	}
-	close(fd);
 
-	snapshot->start = snapshot->buf;
-	snapshot->eof = snapshot->buf + size;
+	ret = allocate_snapshot_buffer(snapshot, fd, &st);
 
-	return 1;
+	close(fd);
+	return ret;
 }
 
 static const char *find_reference_location_1(struct snapshot *snapshot,

2.49.0

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.

---
 refs/packed-backend.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

index 1da44a3d6d..7fd73a0e6d 100644
@@ -2068,7 +2068,7 @@ static int packed_fsck(struct ref_store *ref_store,
 {
 	struct packed_ref_store *refs = packed_downcast(ref_store,
 							REF_STORE_READ, "fsck");
-	struct strbuf packed_ref_content = STRBUF_INIT;
+	struct snapshot snapshot = { 0 };
 	unsigned int sorted = 0;
 	struct stat st;
 	int ret = 0;

@@ -2112,7 +2112,7 @@ static int packed_fsck(struct ref_store *ref_store,
 		goto cleanup;
 	}
 
-	if (!st.st_size) {
+	if (!allocate_snapshot_buffer(&snapshot, fd, &st)) {
 		struct fsck_ref_report report = { 0 };
 		report.path = "packed-refs";
 		ret = fsck_report_ref(o, &report,

@@ -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;
 }
 

2.49.0

"could simple" -> "can simply".

Nice loss of line count ;-)

Nice catch.

This is a good question. Actually, I intentionally design this. In the very first implementation, I use above way. But later I think this is inappropriate to let callers check st.st_size.

The reason is that we would do the same thing in "load_content" function and fsck function. But after thinking, I think we should make the runtime behavior and fsck consistent. That's our goal. And by using this, let's say if one day we want to tighten the rule in the future, we could just change allocate_snapshot_buffer.

I agree that introducing side effect into a function is not a good design. But in this situation, we could make things consistent.

Thanks,
Jialuo

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/[email protected]

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

---

Change in v2:

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

---

Change in v3:

1. Drop the patch which creates a new function "munmap_temporary_snapshot". As discussed, there is no need to munmap the file during fsck.
2. Allocate snapshot variable in the stack instead of heap.
3. Fix rebase issue, remove unneeded code to check the file size explicitly.

---

Change in v4:

1. Report the "emptyPackedRefsFile(INFO)" to the user when the "packed-refs" is empty instead of ONLY skipping checking the content and update the shell script and commit message.
2. Apply Peff's advice to make [PATCH v3 2/3] more clear.

---

Change in v5:

1. Improve the commit message in the first patch to be more clear:
1. Talk about the current behavior, what error we would report if "packed-refs" is empty.
2. To align with the runtime behavior, we should skip checking the content of "packed-refs".
3. Why do we need to report to the user when the "packed-refs" is empty
2. Fix grammar issue in the last patch.

Thanks,
Jialuo

shejialuo (3):
packed-backend: fsck should warn when "packed-refs" file is empty
packed-backend: extract snapshot allocation in load_contents
packed-backend: mmap large "packed-refs" file during fsck

 Documentation/fsck-msgids.adoc |  6 +++
 fsck.h                         |  1 +
 refs/packed-backend.c          | 73 ++++++++++++++++++++--------------
 t/t0602-reffiles-fsck.sh       | 17 ++++++++
 4 files changed, 67 insertions(+), 30 deletions(-)

Range-diff against v4:
1: 75636c9c85 ! 1: 3487692a03 packed-backend: fsck should warn when "packed-refs" file is empty
@@ Metadata
## Commit message ##
packed-backend: fsck should warn when "packed-refs" file is empty

- During fsck, an empty "packed-refs" gives an error; this is unwarranted.
- The runtime code paths would accept an empty "packed-refs" file, such as
- "create_snapshot" would simply return the "snapshot" without checking
- the content of "packed-refs".
+ We assume the "packed-refs" won't be empty and instead has at least one
+ line in it (even when there are no refs packed, there is the file header
+ line). Because there is no terminating LF in the empty file, we will
+ report "packedRefEntryNotTerminated(ERROR)" to the user.

- And we should also skip checking the content of "packed-refs" when it is
- empty during fsck. However, we should think about whether we need to
- report something to the users in this case.
+ However, the runtime code paths would accept an empty "packed-refs"
+ file, for example, "create_snapshot" would simply return the "snapshot"
+ without checking the content of "packed-refs". So, we should skip
+ checking the content of "packed-refs" when it is empty during fsck.

After 694b7a1999 (repack_without_ref(): write peeled refs in the rewritten file, 2013-04-22), we would always write a header into the
- "packed-refs" file where we would never create empty file since then.
- Because we only create empty "packed-refs" in the very early versions,
- we may tighten this rule in the future. In order to notify the users
- about this, we should at least report an warning to the users.
+ "packed-refs" file. So, versions of Git that are not too ancient never
+ write such an empty "packed-refs" file.

- But we need to consider the fsck message type carefully, it is not
- appropriate that we use "FSCK_ERROR". This is because we would
- definitely break the compatibility. Let's create a "FSCK_INFO" message
- id EMPTY_PACKED_REFS_FILE" to indicate that "packed-refs" is empty.
+ As an empty file often indicates a sign of a filesystem-level issue, the
+ way we want to resolve this inconsistency is not make everybody totally
+ silent but notice and report the anomaly.
+
+ Let's create a "FSCK_INFO" message id "EMPTY_PACKED_REFS_FILE" to report
+ to the users that "packed-refs" is empty.

2:  1a5893379d = 2:  0d050849bc packed-backend: extract snapshot allocation in `load_contents`
3:  31e272db7e ! 3:  fe5ffec8fb packed-backend: mmap large "packed-refs" file during fsck

@@ Commit message
current codebase.

As we have introduced the helper function "allocate_snapshot_buffer", we
- could simple use this function to use mmap mechanism.
+ can simply use this function to use mmap mechanism.

2.49.0

We assume the "packed-refs" won't be empty and instead has at least one line in it (even when there are no refs packed, there is the file header line). Because there is no terminating LF in the empty file, we will report "packedRefEntryNotTerminated(ERROR)" to the user.

However, the runtime code paths would accept an empty "packed-refs" file, for example, "create_snapshot" would simply return the "snapshot" without checking the content of "packed-refs". So, we should skip checking the content of "packed-refs" when it is empty during fsck.

After 694b7a1999 (repack_without_ref(): write peeled refs in the rewritten file, 2013-04-22), we would always write a header into the "packed-refs" file. So, versions of Git that are not too ancient never write such an empty "packed-refs" file.

As an empty file often indicates a sign of a filesystem-level issue, the way we want to resolve this inconsistency is not make everybody totally silent but notice and report the anomaly.

Let's create a "FSCK_INFO" message id "EMPTY_PACKED_REFS_FILE" to report to the users that "packed-refs" is empty.

---
 Documentation/fsck-msgids.adoc |  6 ++++++
 fsck.h                         |  1 +
 refs/packed-backend.c          |  9 +++++++++
 t/t0602-reffiles-fsck.sh       | 17 +++++++++++++++++
 4 files changed, 33 insertions(+)

index 9601fff228..0ba4f9a27e 100644
@@ -59,6 +59,12 @@
 `emptyName`::
 	(WARN) A path contains an empty name.
 
+`emptyPackedRefsFile`::
+	(INFO) "packed-refs" file is empty. Report to the
+	[email protected] mailing list if you see this error. As only
+	very early versions of Git would create such an empty
+	"packed_refs" file, we might tighten this rule in the future.
+
 `extraHeaderEntry`::
 	(IGNORE) Extra headers found after `tagger`.
 

index b1deae61ee..0c5869ac34 100644
@@ -84,6 +84,7 @@ enum fsck_msg_type {
 	FUNC(LARGE_PATHNAME, WARN) \
 	/* infos (reported as warnings, but ignored by default) */ \
 	FUNC(BAD_FILEMODE, INFO) \
+	FUNC(EMPTY_PACKED_REFS_FILE, INFO) \
 	FUNC(GITMODULES_PARSE, INFO) \
 	FUNC(GITIGNORE_SYMLINK, INFO) \
 	FUNC(GITATTRIBUTES_SYMLINK, INFO) \

index 3ad1ed0787..fb91833e76 100644
@@ -2103,6 +2103,15 @@ static int packed_fsck(struct ref_store *ref_store,
 		goto cleanup;
 	}
 
+	if (!st.st_size) {
+		struct fsck_ref_report report = { 0 };
+		report.path = "packed-refs";
+		ret = fsck_report_ref(o, &report,
+				      FSCK_MSG_EMPTY_PACKED_REFS_FILE,
+				      "file is empty");
+		goto cleanup;
+	}
+
 	if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
 		ret = error_errno(_("unable to read '%s'"), refs->path);
 		goto cleanup;

index 9d1dc2144c..f671ac4d3a 100755
@@ -647,6 +647,23 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '
 	)
 '
 
+test_expect_success 'empty packed-refs should be reported' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit default &&
+
+		>.git/packed-refs &&
+		git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		warning: packed-refs: emptyPackedRefsFile: file is empty
+		EOF
+		rm .git/packed-refs &&
+		test_cmp expect err
+	)
+'
+
 test_expect_success 'packed-refs header should be checked' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&

2.49.0

"load_contents" would choose which way to load the content of the "packed-refs". However, we cannot directly use this function when checking the consistency due to we don't want to open the file. And we also need to reuse the logic to avoid causing repetition.

Let's create a new helper function "allocate_snapshot_buffer" to extract the snapshot allocation logic in "load_contents" and update the "load_contents" to align with the behavior.

---
 refs/packed-backend.c | 53 +++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

index fb91833e76..1da44a3d6d 100644
@@ -517,6 +517,32 @@ static int refname_contains_nul(struct strbuf *refname)
 
 #define SMALL_FILE_SIZE (32*1024)
 
+static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)
+{
+	ssize_t bytes_read;
+	size_t size;
+
+	size = xsize_t(st->st_size);
+	if (!size)
+		return 0;
+
+	if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
+		snapshot->buf = xmalloc(size);
+		bytes_read = read_in_full(fd, snapshot->buf, size);
+		if (bytes_read < 0 || bytes_read != size)
+			die_errno("couldn't read %s", snapshot->refs->path);
+		snapshot->mmapped = 0;
+	} else {
+		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+		snapshot->mmapped = 1;
+	}
+
+	snapshot->start = snapshot->buf;
+	snapshot->eof = snapshot->buf + size;
+
+	return 1;
+}
+
 /*
  * Depending on `mmap_strategy`, either mmap or read the contents of
  * the `packed-refs` file into the snapshot. Return 1 if the file

@@ -525,10 +551,9 @@ static int refname_contains_nul(struct strbuf *refname)
  */
 static int load_contents(struct snapshot *snapshot)
 {
-	int fd;
 	struct stat st;
-	size_t size;
-	ssize_t bytes_read;
+	int ret;
+	int fd;
 
 	fd = open(snapshot->refs->path, O_RDONLY);
 	if (fd < 0) {

@@ -550,27 +575,11 @@ static int load_contents(struct snapshot *snapshot)
 
 	if (fstat(fd, &st) < 0)
 		die_errno("couldn't stat %s", snapshot->refs->path);
-	size = xsize_t(st.st_size);
-
-	if (!size) {
-		close(fd);
-		return 0;
-	} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
-		snapshot->buf = xmalloc(size);
-		bytes_read = read_in_full(fd, snapshot->buf, size);
-		if (bytes_read < 0 || bytes_read != size)
-			die_errno("couldn't read %s", snapshot->refs->path);
-		snapshot->mmapped = 0;
-	} else {
-		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-		snapshot->mmapped = 1;
-	}
-	close(fd);
 
-	snapshot->start = snapshot->buf;
-	snapshot->eof = snapshot->buf + size;
+	ret = allocate_snapshot_buffer(snapshot, fd, &st);
 
-	return 1;
+	close(fd);
+	return ret;
 }
 
 static const char *find_reference_location_1(struct snapshot *snapshot,

2.49.0

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 can simply use this function to use mmap mechanism.

---
 refs/packed-backend.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

index 1da44a3d6d..7fd73a0e6d 100644
@@ -2068,7 +2068,7 @@ static int packed_fsck(struct ref_store *ref_store,
 {
 	struct packed_ref_store *refs = packed_downcast(ref_store,
 							REF_STORE_READ, "fsck");
-	struct strbuf packed_ref_content = STRBUF_INIT;
+	struct snapshot snapshot = { 0 };
 	unsigned int sorted = 0;
 	struct stat st;
 	int ret = 0;

@@ -2112,7 +2112,7 @@ static int packed_fsck(struct ref_store *ref_store,
 		goto cleanup;
 	}
 
-	if (!st.st_size) {
+	if (!allocate_snapshot_buffer(&snapshot, fd, &st)) {
 		struct fsck_ref_report report = { 0 };
 		report.path = "packed-refs";
 		ret = fsck_report_ref(o, &report,

@@ -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;
 }
 

2.49.0

This round looks good to me. Queued. Thanks.

The thread has gone quiet on this topic. Is everybody happy with this version?

Thanks.

Yep, it looks good to me. Thanks.

-Peff

Didn't have anything else to add, either. Thanks!

Patrick

Thanks, all. Let's merge it down.