{"thread":{"id":"63407","subject":"[PATCH 0/4] align the behavior when opening \"packed-refs\"","startedAt":"2025-05-06T16:38:59Z","lastAt":"2025-05-23T15:59:00Z","messageCount":67,"participants":["shejialuo","Junio C Hamano","Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"517358","messageId":"aBo7OiCKHTyT4DzH@ArchLinux","threadId":"63407","inReplyTo":null,"subject":"[PATCH 0/4] align the behavior when opening \"packed-refs\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-06T16:39:22Z","receivedAt":"2025-05-06T16:38:59Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Hi All:\n\nAs discussed in [1], we need to use mmap mechanism to open large\n\"packed_refs\" file to save the memory usage. This patch mainly does the\nfollowing things:\n\n1: Fix an issue that we would report an error when the \"packed-refs\"\nfile is empty, which does not align with the runtime behavior.\n2-4: Extract some logic from the existing code and then use these\ncreated helper functions to let fsck code to use mmap necessarily\n\n[1] https://lore.kernel.org/git/20250503133158.GA4450@coredump.intra.peff.net\n\nReally thank Peff and Patrick to suggest me to do above change.\n\nThanks,\nJialuo\n\nshejialuo (4):\n packed-backend: skip checking consistency of empty packed-refs file\n packed-backend: extract snapshot allocation in `load_contents`\n packed-backend: extract munmap operation for `MMAP_TEMPORARY`\n packed-backend: use mmap when opening large \"packed-refs\" file\n\n refs/packed-backend.c | 106 +++++++++++++++++++++++----------------\n t/t0602-reffiles-fsck.sh | 13 +++++\n 2 files changed, 75 insertions(+), 44 deletions(-)\n\n-- \n2.49.0\n\n"},{"id":"517359","messageId":"aBo7nBOl18WWYIsA@ArchLinux","threadId":"63407","inReplyTo":"aBo7OiCKHTyT4DzH@ArchLinux","subject":"[PATCH 1/4] packed-backend: skip checking consistency of empty packed-refs file","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-06T16:41:00Z","receivedAt":"2025-05-06T16:40:37Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"In \"load_contents\", when the \"packed-refs\" is empty, we will just return\nthe snapshot. However, we would report an error to the user when\nchecking the consistency of the empty \"packed-refs\".\n\nWe should align with the runtime behavior. As what \"load_contents\" does,\nlet's check whether the file size is zero and if so, we will skip\nchecking the consistency and simply return.\n\nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 3 +++\n t/t0602-reffiles-fsck.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 3ad1ed0787..0dd6c6677b 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2103,6 +2103,9 @@ static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n+\tif (!st.st_size)\n+\t\tgoto cleanup;\n+\n \tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n \t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n \t\tgoto cleanup;\ndiff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\nindex 9d1dc2144c..0a9e9ccc55 100755\n--- a/t/t0602-reffiles-fsck.sh\n+++ b/t/t0602-reffiles-fsck.sh\n@@ -647,6 +647,19 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '\n \t)\n '\n \n+test_expect_success 'empty packed-refs should not be reported' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit default &&\n+\n+\t\ttouch .git/packed-refs &&\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n test_expect_success 'packed-refs header should be checked' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.49.0\n\n"},{"id":"517360","messageId":"aBo7pcimOG19oInQ@ArchLinux","threadId":"63407","inReplyTo":"aBo7OiCKHTyT4DzH@ArchLinux","subject":"[PATCH 2/4] packed-backend: extract snapshot allocation in `load_contents`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-06T16:41:09Z","receivedAt":"2025-05-06T16:40:47Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"\"load_contents\" would choose which way to load the content of the\n\"packed-refs\". However, we cannot directly use this function when\nchecking the consistency due to we don't want to open the file. And we\nalso need to reuse the logic to avoid causing repetition.\n\nLet's create a new helper function \"allocate_snapshot_buffer\" to extract\nthe snapshot allocation logic in \"load_contents\" and update the\n\"load_contents\" to align with the behavior.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 54 +++++++++++++++++++++++++------------------\n 1 file changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 0dd6c6677b..e582227772 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -517,6 +517,32 @@ static int refname_contains_nul(struct strbuf *refname)\n \n #define SMALL_FILE_SIZE (32*1024)\n \n+static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)\n+{\n+\tssize_t bytes_read;\n+\tsize_t size;\n+\n+\tsize = xsize_t(st->st_size);\n+\tif (!size)\n+\t\treturn 0;\n+\n+\tif (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n+\t\tsnapshot->buf = xmalloc(size);\n+\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n+\t\tif (bytes_read < 0 || bytes_read != size)\n+\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n+\t\tsnapshot->mmapped = 0;\n+\t} else {\n+\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\tsnapshot->mmapped = 1;\n+\t}\n+\n+\tsnapshot->start = snapshot->buf;\n+\tsnapshot->eof = snapshot->buf + size;\n+\n+\treturn 1;\n+}\n+\n /*\n * Depending on `mmap_strategy`, either mmap or read the contents of\n * the `packed-refs` file into the snapshot. Return 1 if the file\n@@ -525,10 +551,9 @@ static int refname_contains_nul(struct strbuf *refname)\n */\n static int load_contents(struct snapshot *snapshot)\n {\n-\tint fd;\n \tstruct stat st;\n-\tsize_t size;\n-\tssize_t bytes_read;\n+\tint ret = 1;\n+\tint fd;\n \n \tfd = open(snapshot->refs->path, O_RDONLY);\n \tif (fd < 0) {\n@@ -550,27 +575,12 @@ static int load_contents(struct snapshot *snapshot)\n \n \tif (fstat(fd, &st) < 0)\n \t\tdie_errno(\"couldn't stat %s\", snapshot->refs->path);\n-\tsize = xsize_t(st.st_size);\n-\n-\tif (!size) {\n-\t\tclose(fd);\n-\t\treturn 0;\n-\t} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n-\t\tsnapshot->buf = xmalloc(size);\n-\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n-\t\tif (bytes_read < 0 || bytes_read != size)\n-\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n-\t\tsnapshot->mmapped = 0;\n-\t} else {\n-\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\tsnapshot->mmapped = 1;\n-\t}\n-\tclose(fd);\n \n-\tsnapshot->start = snapshot->buf;\n-\tsnapshot->eof = snapshot->buf + size;\n+\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n+\t\tret = 0;\n \n-\treturn 1;\n+\tclose(fd);\n+\treturn ret;\n }\n \n static const char *find_reference_location_1(struct snapshot *snapshot,\n-- \n2.49.0\n\n"},{"id":"517361","messageId":"aBo7rXx46_jQhTGA@ArchLinux","threadId":"63407","inReplyTo":"aBo7OiCKHTyT4DzH@ArchLinux","subject":"[PATCH 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-06T16:41:17Z","receivedAt":"2025-05-06T16:40:54Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"\"create_snapshot\" would try to munmap the file when the \"mmap_strategy\"\nis \"MMAP_TEMPORARY\". We also need to do this operation when checking the\nconsistency of the \"packed-refs\" file.\n\nCreate a new function \"munmap_snapshot_if_temporary\" to do above and\nchange \"create_snapshot\" to align with the behavior.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 31 ++++++++++++++++++-------------\n 1 file changed, 18 insertions(+), 13 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex e582227772..dd903db301 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -543,6 +543,23 @@ static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct st\n \treturn 1;\n }\n \n+static void munmap_snapshot_if_temporary(struct snapshot *snapshot)\n+{\n+\tif (mmap_strategy != MMAP_OK && snapshot->mmapped) {\n+\t\t/*\n+\t\t * We don't want to leave the file mmapped, so we are\n+\t\t * forced to make a copy now:\n+\t\t */\n+\t\tsize_t size = snapshot->eof - snapshot->start;\n+\t\tchar *buf_copy = xmalloc(size);\n+\n+\t\tmemcpy(buf_copy, snapshot->start, size);\n+\t\tclear_snapshot_buffer(snapshot);\n+\t\tsnapshot->buf = snapshot->start = buf_copy;\n+\t\tsnapshot->eof = buf_copy + size;\n+\t}\n+}\n+\n /*\n * Depending on `mmap_strategy`, either mmap or read the contents of\n * the `packed-refs` file into the snapshot. Return 1 if the file\n@@ -761,19 +778,7 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n \t\tverify_buffer_safe(snapshot);\n \t}\n \n-\tif (mmap_strategy != MMAP_OK && snapshot->mmapped) {\n-\t\t/*\n-\t\t * We don't want to leave the file mmapped, so we are\n-\t\t * forced to make a copy now:\n-\t\t */\n-\t\tsize_t size = snapshot->eof - snapshot->start;\n-\t\tchar *buf_copy = xmalloc(size);\n-\n-\t\tmemcpy(buf_copy, snapshot->start, size);\n-\t\tclear_snapshot_buffer(snapshot);\n-\t\tsnapshot->buf = snapshot->start = buf_copy;\n-\t\tsnapshot->eof = buf_copy + size;\n-\t}\n+\tmunmap_snapshot_if_temporary(snapshot);\n \n \treturn snapshot;\n }\n-- \n2.49.0\n\n"},{"id":"517362","messageId":"aBo7tOkheM6zOJpe@ArchLinux","threadId":"63407","inReplyTo":"aBo7OiCKHTyT4DzH@ArchLinux","subject":"[PATCH 4/4] packed-backend: use mmap when opening large \"packed-refs\" file","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-06T16:41:24Z","receivedAt":"2025-05-06T16:41:01Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We use \"strbuf_read\" to read the content of \"packed-refs\". However, this\nis a bad practice which would consume a lot of memory usage if there are\nmultiple processes reading large \"packed-refs\".\n\nAs we have introduced two helper functions \"allocate_snapshot_buffer\"\nand \"munmap_snapshot_if_temporary\", we could simply call these functions\nto use mmap mechanism.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex dd903db301..1370b982df 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2074,7 +2074,7 @@ static int packed_fsck(struct ref_store *ref_store,\n {\n \tstruct packed_ref_store *refs = packed_downcast(ref_store,\n \t\t\t\t\t\t\tREF_STORE_READ, \"fsck\");\n-\tstruct strbuf packed_ref_content = STRBUF_INIT;\n+\tstruct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));\n \tunsigned int sorted = 0;\n \tstruct stat st;\n \tint ret = 0;\n@@ -2121,21 +2121,21 @@ static int packed_fsck(struct ref_store *ref_store,\n \tif (!st.st_size)\n \t\tgoto cleanup;\n \n-\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n-\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n+\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n \t\tgoto cleanup;\n-\t}\n+\tmunmap_snapshot_if_temporary(snapshot);\n \n-\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n-\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n+\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot->start,\n+\t\t\t\t snapshot->eof);\n \tif (!ret && sorted)\n-\t\tret = packed_fsck_ref_sorted(o, ref_store, packed_ref_content.buf,\n-\t\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n+\t\tret = packed_fsck_ref_sorted(o, ref_store, snapshot->start,\n+\t\t\t\t\t snapshot->eof);\n \n cleanup:\n \tif (fd >= 0)\n \t\tclose(fd);\n-\tstrbuf_release(&packed_ref_content);\n+\tclear_snapshot_buffer(snapshot);\n+\tfree(snapshot);\n \treturn ret;\n }\n \n-- \n2.49.0\n\n"},{"id":"517376","messageId":"xmqqr011k2ci.fsf@gitster.g","threadId":"63407","inReplyTo":"aBo7nBOl18WWYIsA@ArchLinux","subject":"Re: [PATCH 1/4] packed-backend: skip checking consistency of empty packed-refs file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-06T18:42:37Z","receivedAt":"2025-05-06T18:42:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> +\t\ttouch .git/packed-refs &&\n\nUnless you want to signal your readers that you care about file\ntimestamp somehow, don't use \"touch\" as a means to create a file.\nReaders would have to wonder if .git/packed-refs existed before,\nor what git command that follows this part cares about its last\nmodified time, or what behaviour the timestamp would affect.\n\nWhat you are interested in doing with this is to ensure that *AN*\n*EMPTY* file exists there, hence you should say\n\n\t\t>.git/packed-refs &&\n\ninstread.\n"},{"id":"517377","messageId":"xmqqldr9k1wt.fsf@gitster.g","threadId":"63407","inReplyTo":"aBo7rXx46_jQhTGA@ArchLinux","subject":"Re: [PATCH 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-06T18:52:02Z","receivedAt":"2025-05-06T18:52:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> +static void munmap_snapshot_if_temporary(struct snapshot *snapshot)\n> +{\n> +\tif (mmap_strategy != MMAP_OK && snapshot->mmapped) {\n\nIn general, a refactoring tomove conditionals like this to the\ncallee and make the callers unconditionally call the helper is an\nantipattern from maintainability's point of view.\n\nImagine what would happen when we acquire a different mmap_strategy\nin the future, and by that time, there are callers in the codebase\nother than what we currently have (which is just one below after\nthis patch). Do you have to verify if all existing callers that\ntrusted \"if_temporary\" really meant MMAP_TEMPORARY, as the name of\nthe helper function suggested, or do some of the callers meant \"any\nstrategy other than MMAP_OK\"? What if some callers want the former\nand others want the latter semantics?\n\nWithout even talking about longer term maintainability, at the\ncallsite, _if_temporary in the name is a much weaker sign than an\nexplicit if() condition that says what is going on.\n\nI'd prefer the caller to be more like\n\n\tif (mmap_strategy == MMAP_TEMPORARY)\n\t\tmunmap_temporary_stapshot(snapshot)\n\nand make the caller to return immediately when !snapshot, i.e.\n\n\tstatic void munmap_temporary_stapshot(struct snapshot *snapshot\n\t{\n\t\tif (!snapshot)\n\t\t\treturn;\n\t\t... the rest of the helper function ...\n\t}\n\nThanks.\n"},{"id":"517378","messageId":"xmqqecx1k1ig.fsf@gitster.g","threadId":"63407","inReplyTo":"aBo7tOkheM6zOJpe@ArchLinux","subject":"Re: [PATCH 4/4] packed-backend: use mmap when opening large \"packed-refs\" file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-06T19:00:39Z","receivedAt":"2025-05-06T19:00:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> We use \"strbuf_read\" to read the content of \"packed-refs\". However, this\n> is a bad practice which would consume a lot of memory usage if there are\n> multiple processes reading large \"packed-refs\".\n\nNeither this nor the commit title says that the issue is limited to\nthe code path that runs fsck on packed-refs file, but I thought the\ncode paths to use packed-refs to resolve refs correctly uses mmap()\nand does not share this issue? If it is limited to one single code\npath, please mention it explicitly.\n\nAlso, I think it was already pointed out that \"multiple processes\"\nis not all that interesting issue. Even if there is a single\nprocess using a single large packed-refs file, alloc+read gives the\nsystem more memory pressure than the read-only mmap like we do.\n\nAs to the title\n\n\tpacked-backend: use mmap when opening large \"packed-refs\" file\n\tpacked-backend: mmap large \"packed-refs\" file during fsck\n\nwould be shorter and clearer.\n\nThe patch looks OK. Nice to see this one-off strbuf use going away.\n\n> -\tstruct strbuf packed_ref_content = STRBUF_INIT;\n> +\tstruct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));\n> \tunsigned int sorted = 0;\n> \tstruct stat st;\n> \tint ret = 0;\n> @@ -2121,21 +2121,21 @@ static int packed_fsck(struct ref_store *ref_store,\n> \tif (!st.st_size)\n> \t\tgoto cleanup;\n> \n> -\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n> -\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n> +\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n> \t\tgoto cleanup;\n> -\t}\n> +\tmunmap_snapshot_if_temporary(snapshot);\n> \n> -\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n> -\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n> +\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot->start,\n> +\t\t\t\t snapshot->eof);\n> \tif (!ret && sorted)\n> -\t\tret = packed_fsck_ref_sorted(o, ref_store, packed_ref_content.buf,\n> -\t\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n> +\t\tret = packed_fsck_ref_sorted(o, ref_store, snapshot->start,\n> +\t\t\t\t\t snapshot->eof);\n> \n> cleanup:\n> \tif (fd >= 0)\n> \t\tclose(fd);\n> -\tstrbuf_release(&packed_ref_content);\n> +\tclear_snapshot_buffer(snapshot);\n> +\tfree(snapshot);\n> \treturn ret;\n> }\n"},{"id":"517379","messageId":"xmqqzffpima4.fsf@gitster.g","threadId":"63407","inReplyTo":"aBo7nBOl18WWYIsA@ArchLinux","subject":"Re: [PATCH 1/4] packed-backend: skip checking consistency of empty packed-refs file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-06T19:14:59Z","receivedAt":"2025-05-06T19:15:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> In \"load_contents\", when the \"packed-refs\" is empty, we will just return\n> the snapshot. However, we would report an error to the user when\n> checking the consistency of the empty \"packed-refs\".\n\nNeither the commit title nor the above paragraph hints that this is\ntalking about \"fsck\" part of the packed-refs subsystem. That leaves\nthe readers confused when they read \"with the runtime behavior\"\nbelow.\n\n> We should align with the runtime behavior. As what \"load_contents\" does,\n> let's check whether the file size is zero and if so, we will skip\n> checking the consistency and simply return.\n\nHow about\n\n\tDuring fsck, an empty \"packed-refs\" file gives an error;\n\tthis is unwarranted. We should instead just return an empty\n\t\"snapshot\" and let the caller happily declare success, just\n\tlike the code paths that implement the runtime use of the\n\tfile do.\n\nor something?\n\nAs to the title\n\n\tpacked-backend: fsck should allow an empty packed-refs file\n\nis shorter and clearer, I would think.\n\nThe code change is trivially correct, I think. Nicely found.\n"},{"id":"517380","messageId":"xmqqv7qdim88.fsf@gitster.g","threadId":"63407","inReplyTo":"aBo7pcimOG19oInQ@ArchLinux","subject":"Re: [PATCH 2/4] packed-backend: extract snapshot allocation in `load_contents`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-06T19:16:07Z","receivedAt":"2025-05-06T19:16:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> \"load_contents\" would choose which way to load the content of the\n> \"packed-refs\". However, we cannot directly use this function when\n> checking the consistency due to we don't want to open the file. And we\n> also need to reuse the logic to avoid causing repetition.\n>\n> Let's create a new helper function \"allocate_snapshot_buffer\" to extract\n> the snapshot allocation logic in \"load_contents\" and update the\n> \"load_contents\" to align with the behavior.\n>\n> Suggested-by: Jeff King \n> Suggested-by: Patrick Steinhardt \n> Signed-off-by: shejialuo \n> ---\n> refs/packed-backend.c | 54 +++++++++++++++++++++++++------------------\n> 1 file changed, 32 insertions(+), 22 deletions(-)\n\nTrivially correct, cleanly done, and nicely described.\n\nThanks.\n"},{"id":"517398","messageId":"xmqqo6w5fko8.fsf@gitster.g","threadId":"63407","inReplyTo":"xmqqldr9k1wt.fsf@gitster.g","subject":"Re: [PATCH 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-06T22:17:59Z","receivedAt":"2025-05-06T22:18:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> shejialuo writes:\n>\n>> +static void munmap_snapshot_if_temporary(struct snapshot *snapshot)\n>> +{\n>> +\tif (mmap_strategy != MMAP_OK && snapshot->mmapped) {\n>\n> In general, a refactoring tomove conditionals like this to the\n> callee and make the callers unconditionally call the helper is an\n> antipattern from maintainability's point of view.\n>\n> Imagine what would happen when we acquire a different mmap_strategy\n> in the future, and by that time, there are callers in the codebase\n> other than what we currently have (which is just one below after\n> this patch). Do you have to verify if all existing callers that\n> trusted \"if_temporary\" really meant MMAP_TEMPORARY, as the name of\n> the helper function suggested, or do some of the callers meant \"any\n> strategy other than MMAP_OK\"? What if some callers want the former\n> and others want the latter semantics?\n>\n> Without even talking about longer term maintainability, at the\n> callsite, _if_temporary in the name is a much weaker sign than an\n> explicit if() condition that says what is going on.\n>\n> I'd prefer the caller to be more like\n>\n> \tif (mmap_strategy == MMAP_TEMPORARY)\n> \t\tmunmap_temporary_stapshot(snapshot)\n>\n> and make the caller to return immediately when !snapshot, i.e.\n\nOops. I obviously meant \"the callee\" here.\n\n> \tstatic void munmap_temporary_stapshot(struct snapshot *snapshot\n> \t{\n> \t\tif (!snapshot)\n> \t\t\treturn;\n> \t\t... the rest of the helper function ...\n> \t}\n>\n> Thanks.\n"},{"id":"517399","messageId":"xmqqjz6tfkmv.fsf@gitster.g","threadId":"63407","inReplyTo":"xmqqecx1k1ig.fsf@gitster.g","subject":"Re: [PATCH 4/4] packed-backend: use mmap when opening large \"packed-refs\" file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-06T22:18:48Z","receivedAt":"2025-05-06T22:18:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> As to the title\n>\n> \tpacked-backend: use mmap when opening large \"packed-refs\" file\n> \tpacked-backend: mmap large \"packed-refs\" file during fsck\n>\n> would be shorter and clearer.\n\nSorry to be cryptic. I meant to suggest the second one. The first\none is what you gave us, and was left to contrast its length with\nthe suggested altenative.\n\n"},{"id":"517474","messageId":"aBtNhXVpnvPuV6b7@ArchLinux","threadId":"63407","inReplyTo":"xmqqr011k2ci.fsf@gitster.g","subject":"Re: [PATCH 1/4] packed-backend: skip checking consistency of empty packed-refs file","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-07T12:09:41Z","receivedAt":"2025-05-07T12:09:18Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, May 06, 2025 at 11:42:37AM -0700, Junio C Hamano wrote:\n> shejialuo writes:\n> \n> > +\t\ttouch .git/packed-refs &&\n> \n> Unless you want to signal your readers that you care about file\n> timestamp somehow, don't use \"touch\" as a means to create a file.\n> Readers would have to wonder if .git/packed-refs existed before,\n> or what git command that follows this part cares about its last\n> modified time, or what behaviour the timestamp would affect.\n> \n> What you are interested in doing with this is to ensure that *AN*\n> *EMPTY* file exists there, hence you should say\n> \n> \t\t>.git/packed-refs &&\n> \n> instread.\n\nThanks for the suggestion, I didn't realise about this.\n"},{"id":"517475","messageId":"aBtNtHni7uT70_Nu@ArchLinux","threadId":"63407","inReplyTo":"xmqqzffpima4.fsf@gitster.g","subject":"Re: [PATCH 1/4] packed-backend: skip checking consistency of empty packed-refs file","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-07T12:10:28Z","receivedAt":"2025-05-07T12:10:05Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, May 06, 2025 at 12:14:59PM -0700, Junio C Hamano wrote:\n> shejialuo writes:\n> \n> > In \"load_contents\", when the \"packed-refs\" is empty, we will just return\n> > the snapshot. However, we would report an error to the user when\n> > checking the consistency of the empty \"packed-refs\".\n> \n> Neither the commit title nor the above paragraph hints that this is\n> talking about \"fsck\" part of the packed-refs subsystem. That leaves\n> the readers confused when they read \"with the runtime behavior\"\n> below.\n> \n\nThat's right. My message is vague.\n\n> > We should align with the runtime behavior. As what \"load_contents\" does,\n> > let's check whether the file size is zero and if so, we will skip\n> > checking the consistency and simply return.\n> \n> How about\n> \n> \tDuring fsck, an empty \"packed-refs\" file gives an error;\n> \tthis is unwarranted. We should instead just return an empty\n> \t\"snapshot\" and let the caller happily declare success, just\n> \tlike the code paths that implement the runtime use of the\n> \tfile do.\n> \n> or something?\n> \n> As to the title\n> \n> \tpacked-backend: fsck should allow an empty packed-refs file\n> \n> is shorter and clearer, I would think.\n> \n\nThanks, I will improve this in the next version.\n\n> The code change is trivially correct, I think. Nicely found.\n"},{"id":"517477","messageId":"aBtQWySMoj-lNOhv@ArchLinux","threadId":"63407","inReplyTo":"xmqqldr9k1wt.fsf@gitster.g","subject":"Re: [PATCH 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-07T12:21:47Z","receivedAt":"2025-05-07T12:21:24Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, May 06, 2025 at 11:52:02AM -0700, Junio C Hamano wrote:\n> shejialuo writes:\n> \n> > +static void munmap_snapshot_if_temporary(struct snapshot *snapshot)\n> > +{\n> > +\tif (mmap_strategy != MMAP_OK && snapshot->mmapped) {\n> \n> In general, a refactoring tomove conditionals like this to the\n> callee and make the callers unconditionally call the helper is an\n> antipattern from maintainability's point of view.\n> \n> Imagine what would happen when we acquire a different mmap_strategy\n> in the future, and by that time, there are callers in the codebase\n> other than what we currently have (which is just one below after\n> this patch). Do you have to verify if all existing callers that\n> trusted \"if_temporary\" really meant MMAP_TEMPORARY, as the name of\n> the helper function suggested, or do some of the callers meant \"any\n> strategy other than MMAP_OK\"? What if some callers want the former\n> and others want the latter semantics?\n> \n\nThat's right. Actually when I wake up in the morning, I suddenly find\nout I made a mistake here. We should try to make the function as pure as\npossible. So, this function would just do one thing, call `mumap` when\nthe mmap strategy is \"MMAP_TEMPORARY\".\n\n> Without even talking about longer term maintainability, at the\n> callsite, _if_temporary in the name is a much weaker sign than an\n> explicit if() condition that says what is going on.\n> \n> I'd prefer the caller to be more like\n> \n> \tif (mmap_strategy == MMAP_TEMPORARY)\n> \t\tmunmap_temporary_stapshot(snapshot)\n> \n> and make the caller to return immediately when !snapshot, i.e.\n> \n> \tstatic void munmap_temporary_stapshot(struct snapshot *snapshot\n> \t{\n> \t\tif (!snapshot)\n> \t\t\treturn;\n> \t\t... the rest of the helper function ...\n> \t}\n> \n> Thanks.\n"},{"id":"517479","messageId":"aBtTXIpCPlQJtVqr@ArchLinux","threadId":"63407","inReplyTo":"xmqqecx1k1ig.fsf@gitster.g","subject":"Re: [PATCH 4/4] packed-backend: use mmap when opening large \"packed-refs\" file","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-07T12:34:36Z","receivedAt":"2025-05-07T12:34:13Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, May 06, 2025 at 12:00:39PM -0700, Junio C Hamano wrote:\n> shejialuo writes:\n> \n> > We use \"strbuf_read\" to read the content of \"packed-refs\". However, this\n> > is a bad practice which would consume a lot of memory usage if there are\n> > multiple processes reading large \"packed-refs\".\n> \n> Neither this nor the commit title says that the issue is limited to\n> the code path that runs fsck on packed-refs file, but I thought the\n> code paths to use packed-refs to resolve refs correctly uses mmap()\n> and does not share this issue? If it is limited to one single code\n> path, please mention it explicitly.\n> \n\nNice advice, I will improve this in the next version.\n\n> Also, I think it was already pointed out that \"multiple processes\"\n> is not all that interesting issue. Even if there is a single\n> process using a single large packed-refs file, alloc+read gives the\n> system more memory pressure than the read-only mmap like we do.\n> \n\nThat's right. Even there is only one process, we could avoid copying the\nfile content from kernel space to the user space. I will improve the\ncommit message.\n\n> As to the title\n> \n> \tpacked-backend: use mmap when opening large \"packed-refs\" file\n> \tpacked-backend: mmap large \"packed-refs\" file during fsck\n> \n> would be shorter and clearer.\n> \n\nYes, the second one is better. Will improve this.\n\n"},{"id":"517494","messageId":"aBtzn4nwLsI9p5Cp@ArchLinux","threadId":"63407","inReplyTo":"aBo7OiCKHTyT4DzH@ArchLinux","subject":"[PATCH v2 0/4] align the behavior when opening \"packed-refs\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-07T14:52:15Z","receivedAt":"2025-05-07T14:52:19Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Hi All:\n\nAs discussed in [1], we need to use mmap mechanism to open large\n\"packed_refs\" file to save the memory usage. This patch mainly does the\nfollowing things:\n\n1: Fix an issue that we would report an error when the \"packed-refs\"\nfile is empty, which does not align with the runtime behavior.\n2-4: Extract some logic from the existing code and then use these\ncreated helper functions to let fsck code to use mmap necessarily\n\n[1] https://lore.kernel.org/git/20250503133158.GA4450@coredump.intra.peff.net\n\nReally thank Peff and Patrick to suggest me to do above change.\n\n---\n\nChange since v1:\n\n1. Update the commit message of [PATCH 1/4]. And use redirection to\ncreate an empty file instead of using `touch`.\n2. Don't use if for the refactored function in [PATCH 3/4] and then\nupdate the commit message to align with the new function name.\n3. Enhance the commit message of [PATCH 4/4].\n\nThanks,\nJialuo\n\nshejialuo (4):\n packed-backend: fsck should allow an empty \"packed-refs\" file\n packed-backend: extract snapshot allocation in `load_contents`\n packed-backend: extract munmap operation for `MMAP_TEMPORARY`\n packed-backend: mmap large \"packed-refs\" file during fsck\n\n refs/packed-backend.c | 113 ++++++++++++++++++++++++---------------\n t/t0602-reffiles-fsck.sh | 13 +++++\n 2 files changed, 82 insertions(+), 44 deletions(-)\n\nRange-diff against v1:\n1: aa9037ebfa ! 1: 26c3fd55a8 packed-backend: skip checking consistency of empty packed-refs file\n @@ Metadata\n Author: shejialuo \n \n ## Commit message ##\n - packed-backend: skip checking consistency of empty packed-refs file\n + packed-backend: fsck should allow an empty \"packed-refs\" file\n \n - In \"load_contents\", when the \"packed-refs\" is empty, we will just return\n - the snapshot. However, we would report an error to the user when\n - checking the consistency of the empty \"packed-refs\".\n -\n - We should align with the runtime behavior. As what \"load_contents\" does,\n - let's check whether the file size is zero and if so, we will skip\n - checking the consistency and simply return.\n + During fsck, an empty \"packed-refs\" gives an error; this is unwarranted.\n + We should just skip checking the content of \"packed-refs\" just like the\n + runtime code paths such as \"create_snapshot\" which simply returns the\n + \"snapshot\" without checking the content of \"packed-refs\".\n \n Signed-off-by: shejialuo \n \n @@ t/t0602-reffiles-fsck.sh: test_expect_success SYMLINKS 'the filetype of packed-r\n +\t\tcd repo &&\n +\t\ttest_commit default &&\n +\n -+\t\ttouch .git/packed-refs &&\n ++\t\t>.git/packed-refs &&\n +\t\tgit refs verify 2>err &&\n +\t\ttest_must_be_empty err\n +\t)\n2: 852e8a606b = 2: 4604be8b51 packed-backend: extract snapshot allocation in `load_contents`\n3: de146155f6 ! 3: c0609afac9 packed-backend: extract munmap operation for `MMAP_TEMPORARY`\n @@ Commit message\n is \"MMAP_TEMPORARY\". We also need to do this operation when checking the\n consistency of the \"packed-refs\" file.\n \n - Create a new function \"munmap_snapshot_if_temporary\" to do above and\n + Create a new function \"munmap_temporary_snapshot\" to do above and\n change \"create_snapshot\" to align with the behavior.\n \n Suggested-by: Jeff King \n @@ refs/packed-backend.c: static int allocate_snapshot_buffer(struct snapshot *snap\n \treturn 1;\n }\n \n -+static void munmap_snapshot_if_temporary(struct snapshot *snapshot)\n ++static void munmap_temporary_snapshot(struct snapshot *snapshot)\n +{\n -+\tif (mmap_strategy != MMAP_OK && snapshot->mmapped) {\n -+\t\t/*\n -+\t\t * We don't want to leave the file mmapped, so we are\n -+\t\t * forced to make a copy now:\n -+\t\t */\n -+\t\tsize_t size = snapshot->eof - snapshot->start;\n -+\t\tchar *buf_copy = xmalloc(size);\n ++\tchar *buf_copy;\n ++\tsize_t size;\n +\n -+\t\tmemcpy(buf_copy, snapshot->start, size);\n -+\t\tclear_snapshot_buffer(snapshot);\n -+\t\tsnapshot->buf = snapshot->start = buf_copy;\n -+\t\tsnapshot->eof = buf_copy + size;\n -+\t}\n ++\tif (!snapshot)\n ++\t\treturn;\n ++\n ++\t/*\n ++\t * We don't want to leave the file mmapped, so we are\n ++\t * forced to make a copy now:\n ++\t */\n ++\tsize = snapshot->eof - snapshot->start;\n ++\tbuf_copy = xmalloc(size);\n ++\n ++\tmemcpy(buf_copy, snapshot->start, size);\n ++\tclear_snapshot_buffer(snapshot);\n ++\tsnapshot->buf = snapshot->start = buf_copy;\n ++\tsnapshot->eof = buf_copy + size;\n +}\n +\n /*\n @@ refs/packed-backend.c: static struct snapshot *create_snapshot(struct packed_ref\n -\t\tsnapshot->buf = snapshot->start = buf_copy;\n -\t\tsnapshot->eof = buf_copy + size;\n -\t}\n -+\tmunmap_snapshot_if_temporary(snapshot);\n ++\tif (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)\n ++\t\tmunmap_temporary_snapshot(snapshot);\n \n \treturn snapshot;\n }\n4: be1e9e2540 ! 4: c868e3dd16 packed-backend: use mmap when opening large \"packed-refs\" file\n @@ Metadata\n Author: shejialuo \n \n ## Commit message ##\n - packed-backend: use mmap when opening large \"packed-refs\" file\n + packed-backend: mmap large \"packed-refs\" file during fsck\n \n - We use \"strbuf_read\" to read the content of \"packed-refs\". However, this\n - is a bad practice which would consume a lot of memory usage if there are\n - multiple processes reading large \"packed-refs\".\n + During fsck, we use \"strbuf_read\" to read the content of \"packed-refs\"\n + without using mmap mechanism. This is a bad practice which would consume\n + more memory than using mmap mechanism. Besides, as all code paths in\n + \"packed-backend.c\" use this way, we should make \"fsck\" align with the\n + current codebase.\n \n As we have introduced two helper functions \"allocate_snapshot_buffer\"\n and \"munmap_snapshot_if_temporary\", we could simply call these functions\n @@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,\n +\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n \t\tgoto cleanup;\n -\t}\n -+\tmunmap_snapshot_if_temporary(snapshot);\n \n -\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n -\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n ++\tif (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)\n ++\t\tmunmap_temporary_snapshot(snapshot);\n ++\n +\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot->start,\n +\t\t\t\t snapshot->eof);\n \tif (!ret && sorted)\n-- \n2.49.0\n\n"},{"id":"517495","messageId":"aBtz8xAYze-Kobk7@ArchLinux","threadId":"63407","inReplyTo":"aBtzn4nwLsI9p5Cp@ArchLinux","subject":"[PATCH v2 1/4] packed-backend: fsck should allow an empty \"packed-refs\" file","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-07T14:53:39Z","receivedAt":"2025-05-07T14:53:44Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"During fsck, an empty \"packed-refs\" gives an error; this is unwarranted.\nWe should just skip checking the content of \"packed-refs\" just like the\nruntime code paths such as \"create_snapshot\" which simply returns the\n\"snapshot\" without checking the content of \"packed-refs\".\n\nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 3 +++\n t/t0602-reffiles-fsck.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 3ad1ed0787..0dd6c6677b 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2103,6 +2103,9 @@ static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n+\tif (!st.st_size)\n+\t\tgoto cleanup;\n+\n \tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n \t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n \t\tgoto cleanup;\ndiff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\nindex 9d1dc2144c..e04967581c 100755\n--- a/t/t0602-reffiles-fsck.sh\n+++ b/t/t0602-reffiles-fsck.sh\n@@ -647,6 +647,19 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '\n \t)\n '\n \n+test_expect_success 'empty packed-refs should not be reported' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit default &&\n+\n+\t\t>.git/packed-refs &&\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n test_expect_success 'packed-refs header should be checked' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.49.0\n\n"},{"id":"517496","messageId":"aBtz-2A2VkhdNAyH@ArchLinux","threadId":"63407","inReplyTo":"aBtzn4nwLsI9p5Cp@ArchLinux","subject":"[PATCH v2 2/4] packed-backend: extract snapshot allocation in `load_contents`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-07T14:53:47Z","receivedAt":"2025-05-07T14:53:52Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"\"load_contents\" would choose which way to load the content of the\n\"packed-refs\". However, we cannot directly use this function when\nchecking the consistency due to we don't want to open the file. And we\nalso need to reuse the logic to avoid causing repetition.\n\nLet's create a new helper function \"allocate_snapshot_buffer\" to extract\nthe snapshot allocation logic in \"load_contents\" and update the\n\"load_contents\" to align with the behavior.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 54 +++++++++++++++++++++++++------------------\n 1 file changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 0dd6c6677b..e582227772 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -517,6 +517,32 @@ static int refname_contains_nul(struct strbuf *refname)\n \n #define SMALL_FILE_SIZE (32*1024)\n \n+static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)\n+{\n+\tssize_t bytes_read;\n+\tsize_t size;\n+\n+\tsize = xsize_t(st->st_size);\n+\tif (!size)\n+\t\treturn 0;\n+\n+\tif (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n+\t\tsnapshot->buf = xmalloc(size);\n+\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n+\t\tif (bytes_read < 0 || bytes_read != size)\n+\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n+\t\tsnapshot->mmapped = 0;\n+\t} else {\n+\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\tsnapshot->mmapped = 1;\n+\t}\n+\n+\tsnapshot->start = snapshot->buf;\n+\tsnapshot->eof = snapshot->buf + size;\n+\n+\treturn 1;\n+}\n+\n /*\n * Depending on `mmap_strategy`, either mmap or read the contents of\n * the `packed-refs` file into the snapshot. Return 1 if the file\n@@ -525,10 +551,9 @@ static int refname_contains_nul(struct strbuf *refname)\n */\n static int load_contents(struct snapshot *snapshot)\n {\n-\tint fd;\n \tstruct stat st;\n-\tsize_t size;\n-\tssize_t bytes_read;\n+\tint ret = 1;\n+\tint fd;\n \n \tfd = open(snapshot->refs->path, O_RDONLY);\n \tif (fd < 0) {\n@@ -550,27 +575,12 @@ static int load_contents(struct snapshot *snapshot)\n \n \tif (fstat(fd, &st) < 0)\n \t\tdie_errno(\"couldn't stat %s\", snapshot->refs->path);\n-\tsize = xsize_t(st.st_size);\n-\n-\tif (!size) {\n-\t\tclose(fd);\n-\t\treturn 0;\n-\t} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n-\t\tsnapshot->buf = xmalloc(size);\n-\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n-\t\tif (bytes_read < 0 || bytes_read != size)\n-\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n-\t\tsnapshot->mmapped = 0;\n-\t} else {\n-\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\tsnapshot->mmapped = 1;\n-\t}\n-\tclose(fd);\n \n-\tsnapshot->start = snapshot->buf;\n-\tsnapshot->eof = snapshot->buf + size;\n+\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n+\t\tret = 0;\n \n-\treturn 1;\n+\tclose(fd);\n+\treturn ret;\n }\n \n static const char *find_reference_location_1(struct snapshot *snapshot,\n-- \n2.49.0\n\n"},{"id":"517497","messageId":"aBt0BDTuOfUuCHE4@ArchLinux","threadId":"63407","inReplyTo":"aBtzn4nwLsI9p5Cp@ArchLinux","subject":"[PATCH v2 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-07T14:53:56Z","receivedAt":"2025-05-07T14:54:00Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"\"create_snapshot\" would try to munmap the file when the \"mmap_strategy\"\nis \"MMAP_TEMPORARY\". We also need to do this operation when checking the\nconsistency of the \"packed-refs\" file.\n\nCreate a new function \"munmap_temporary_snapshot\" to do above and\nchange \"create_snapshot\" to align with the behavior.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 36 +++++++++++++++++++++++-------------\n 1 file changed, 23 insertions(+), 13 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex e582227772..ae6b6845a6 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -543,6 +543,27 @@ static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct st\n \treturn 1;\n }\n \n+static void munmap_temporary_snapshot(struct snapshot *snapshot)\n+{\n+\tchar *buf_copy;\n+\tsize_t size;\n+\n+\tif (!snapshot)\n+\t\treturn;\n+\n+\t/*\n+\t * We don't want to leave the file mmapped, so we are\n+\t * forced to make a copy now:\n+\t */\n+\tsize = snapshot->eof - snapshot->start;\n+\tbuf_copy = xmalloc(size);\n+\n+\tmemcpy(buf_copy, snapshot->start, size);\n+\tclear_snapshot_buffer(snapshot);\n+\tsnapshot->buf = snapshot->start = buf_copy;\n+\tsnapshot->eof = buf_copy + size;\n+}\n+\n /*\n * Depending on `mmap_strategy`, either mmap or read the contents of\n * the `packed-refs` file into the snapshot. Return 1 if the file\n@@ -761,19 +782,8 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n \t\tverify_buffer_safe(snapshot);\n \t}\n \n-\tif (mmap_strategy != MMAP_OK && snapshot->mmapped) {\n-\t\t/*\n-\t\t * We don't want to leave the file mmapped, so we are\n-\t\t * forced to make a copy now:\n-\t\t */\n-\t\tsize_t size = snapshot->eof - snapshot->start;\n-\t\tchar *buf_copy = xmalloc(size);\n-\n-\t\tmemcpy(buf_copy, snapshot->start, size);\n-\t\tclear_snapshot_buffer(snapshot);\n-\t\tsnapshot->buf = snapshot->start = buf_copy;\n-\t\tsnapshot->eof = buf_copy + size;\n-\t}\n+\tif (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)\n+\t\tmunmap_temporary_snapshot(snapshot);\n \n \treturn snapshot;\n }\n-- \n2.49.0\n\n"},{"id":"517498","messageId":"aBt0C8gdBecq5f8U@ArchLinux","threadId":"63407","inReplyTo":"aBtzn4nwLsI9p5Cp@ArchLinux","subject":"[PATCH v2 4/4] packed-backend: mmap large \"packed-refs\" file during fsck","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-07T14:54:03Z","receivedAt":"2025-05-07T14:54:08Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"During fsck, we use \"strbuf_read\" to read the content of \"packed-refs\"\nwithout using mmap mechanism. This is a bad practice which would consume\nmore memory than using mmap mechanism. Besides, as all code paths in\n\"packed-backend.c\" use this way, we should make \"fsck\" align with the\ncurrent codebase.\n\nAs we have introduced two helper functions \"allocate_snapshot_buffer\"\nand \"munmap_snapshot_if_temporary\", we could simply call these functions\nto use mmap mechanism.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex ae6b6845a6..ff744f1d4c 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2079,7 +2079,7 @@ static int packed_fsck(struct ref_store *ref_store,\n {\n \tstruct packed_ref_store *refs = packed_downcast(ref_store,\n \t\t\t\t\t\t\tREF_STORE_READ, \"fsck\");\n-\tstruct strbuf packed_ref_content = STRBUF_INIT;\n+\tstruct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));\n \tunsigned int sorted = 0;\n \tstruct stat st;\n \tint ret = 0;\n@@ -2126,21 +2126,23 @@ static int packed_fsck(struct ref_store *ref_store,\n \tif (!st.st_size)\n \t\tgoto cleanup;\n \n-\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n-\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n+\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n \t\tgoto cleanup;\n-\t}\n \n-\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n-\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n+\tif (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)\n+\t\tmunmap_temporary_snapshot(snapshot);\n+\n+\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot->start,\n+\t\t\t\t snapshot->eof);\n \tif (!ret && sorted)\n-\t\tret = packed_fsck_ref_sorted(o, ref_store, packed_ref_content.buf,\n-\t\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n+\t\tret = packed_fsck_ref_sorted(o, ref_store, snapshot->start,\n+\t\t\t\t\t snapshot->eof);\n \n cleanup:\n \tif (fd >= 0)\n \t\tclose(fd);\n-\tstrbuf_release(&packed_ref_content);\n+\tclear_snapshot_buffer(snapshot);\n+\tfree(snapshot);\n \treturn ret;\n }\n \n-- \n2.49.0\n\n"},{"id":"517525","messageId":"xmqqv7qc9grt.fsf@gitster.g","threadId":"63407","inReplyTo":"aBtzn4nwLsI9p5Cp@ArchLinux","subject":"Re: [PATCH v2 0/4] align the behavior when opening \"packed-refs\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-07T22:51:02Z","receivedAt":"2025-05-07T22:51:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> Hi All:\n>\n> As discussed in [1], we need to use mmap mechanism to open large\n> \"packed_refs\" file to save the memory usage. This patch mainly does the\n> following things:\n>\n> 1: Fix an issue that we would report an error when the \"packed-refs\"\n> file is empty, which does not align with the runtime behavior.\n> 2-4: Extract some logic from the existing code and then use these\n> created helper functions to let fsck code to use mmap necessarily\n>\n> [1] https://lore.kernel.org/git/20250503133158.GA4450@coredump.intra.peff.net\n>\n> Really thank Peff and Patrick to suggest me to do above change.\n\nThis round looks good to me. Others?\n\nThanks.\n"},{"id":"517592","messageId":"20250508195714.GA18229@coredump.intra.peff.net","threadId":"63407","inReplyTo":"aBt0BDTuOfUuCHE4@ArchLinux","subject":"Re: [PATCH v2 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-08T19:57:14Z","receivedAt":"2025-05-08T19:57:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 07, 2025 at 10:53:56PM +0800, shejialuo wrote:\n\n> @@ -761,19 +782,8 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n> \t\tverify_buffer_safe(snapshot);\n> \t}\n> \n> -\tif (mmap_strategy != MMAP_OK && snapshot->mmapped) {\n> -\t\t/*\n> -\t\t * We don't want to leave the file mmapped, so we are\n> -\t\t * forced to make a copy now:\n> -\t\t */\n> -\t\tsize_t size = snapshot->eof - snapshot->start;\n> -\t\tchar *buf_copy = xmalloc(size);\n> -\n> -\t\tmemcpy(buf_copy, snapshot->start, size);\n> -\t\tclear_snapshot_buffer(snapshot);\n> -\t\tsnapshot->buf = snapshot->start = buf_copy;\n> -\t\tsnapshot->eof = buf_copy + size;\n> -\t}\n> +\tif (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)\n> +\t\tmunmap_temporary_snapshot(snapshot);\n\nThe original triggers this conditional whenever the strategy is not\nMMAP_OK (so MMAP_TEMPORARY or MMAP_NONE). But in your post-image, we do\nso only for MMAP_TEMPORARY.\n\nI can guess that the two end up the same, because snapshot->mmapped\nwould never be set when MMAP_NONE is set. But if we are going to make\nsuch a logical inference, it should be explained in the commit message\n(though my preference is to leave the code as-is, or to pull the\nrefactor into its own commit).\n\n-Peff\n"},{"id":"517593","messageId":"xmqq4ixu6f6q.fsf@gitster.g","threadId":"63407","inReplyTo":"20250508195714.GA18229@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-08T20:05:49Z","receivedAt":"2025-05-08T20:05:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> -\tif (mmap_strategy != MMAP_OK && snapshot->mmapped) {\n>> -\t\t/*\n>> -\t\t * We don't want to leave the file mmapped, so we are\n>> -\t\t * forced to make a copy now:\n>> -\t\t */\n>> -\t\tsize_t size = snapshot->eof - snapshot->start;\n>> -\t\tchar *buf_copy = xmalloc(size);\n>> -\n>> -\t\tmemcpy(buf_copy, snapshot->start, size);\n>> -\t\tclear_snapshot_buffer(snapshot);\n>> -\t\tsnapshot->buf = snapshot->start = buf_copy;\n>> -\t\tsnapshot->eof = buf_copy + size;\n>> -\t}\n>> +\tif (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)\n>> +\t\tmunmap_temporary_snapshot(snapshot);\n>\n> The original triggers this conditional whenever the strategy is not\n> MMAP_OK (so MMAP_TEMPORARY or MMAP_NONE). But in your post-image, we do\n> so only for MMAP_TEMPORARY.\n>\n> I can guess that the two end up the same, because snapshot->mmapped\n> would never be set when MMAP_NONE is set. But if we are going to make\n> such a logical inference, it should be explained in the commit message\n> (though my preference is to leave the code as-is, or to pull the\n> refactor into its own commit).\n\nGood thinking.\n\nAn \"extract\" step that is meant as a preliminary refactoring should\nnot make such a change. The change may or may not prepare the code\nfor better maintainability, but I agree that such a change needs to\nbe justified separately.\n\nThanks.\n"},{"id":"517595","messageId":"20250508200741.GB18229@coredump.intra.peff.net","threadId":"63407","inReplyTo":"aBt0C8gdBecq5f8U@ArchLinux","subject":"Re: [PATCH v2 4/4] packed-backend: mmap large \"packed-refs\" file during fsck","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-08T20:07:41Z","receivedAt":"2025-05-08T20:07:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 07, 2025 at 10:54:03PM +0800, shejialuo wrote:\n\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index ae6b6845a6..ff744f1d4c 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -2079,7 +2079,7 @@ static int packed_fsck(struct ref_store *ref_store,\n> {\n> \tstruct packed_ref_store *refs = packed_downcast(ref_store,\n> \t\t\t\t\t\t\tREF_STORE_READ, \"fsck\");\n> -\tstruct strbuf packed_ref_content = STRBUF_INIT;\n> +\tstruct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));\n\nMinor, but is there any reason to allocate this here and not just:\n\n struct snapshot snapshot = { 0 };\n\n?\n\n> @@ -2126,21 +2126,23 @@ static int packed_fsck(struct ref_store *ref_store,\n> \tif (!st.st_size)\n> \t\tgoto cleanup;\n> \n> -\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n> -\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n> +\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n> \t\tgoto cleanup;\n> -\t}\n\nLooking at allocate_snapshot_buffer(), it will return 0 only when the\nfile is empty (and thus there is nothing to allocate) and will\notherwise die(). So we do not need to report any error when it fails.\nGood.\n\nBut that makes the \"!st.st_size\" check in the context redundant, doesn't\nit? It can just go away.\n\n> -\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n> -\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n> +\tif (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)\n> +\t\tmunmap_temporary_snapshot(snapshot);\n> +\n> +\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot->start,\n> +\t\t\t\t snapshot->eof);\n\nWhy are we unmapping here before we use the content? That will create an\nallocated in-memory copy of the mmap'd content. I thought the whole\npoint here was to avoid doing so.\n\nIt does shorten the amount of time we hold the temporary mmap in place,\nbut I don't think we care about that here. The whole point of\nMMAP_TEMPORARY is that we usually hold the packed-refs file open across\nmany requests, and on some platforms (like Windows) we don't want to do\nthat. But in this code path we plan to mmap, do our verification, and\nthen drop the snapshot. So we're always \"temporary\" anyway.\n\nI.e., I'd have expected this code to allocate_snapshot_buffer(), do its\nchecks, and then call clear_snapshot_buffer().\n\n-Peff\n"},{"id":"517596","messageId":"20250508200802.GC18229@coredump.intra.peff.net","threadId":"63407","inReplyTo":"xmqqv7qc9grt.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] align the behavior when opening \"packed-refs\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-08T20:08:02Z","receivedAt":"2025-05-08T20:08:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 07, 2025 at 03:51:02PM -0700, Junio C Hamano wrote:\n\n> shejialuo writes:\n> \n> > Hi All:\n> >\n> > As discussed in [1], we need to use mmap mechanism to open large\n> > \"packed_refs\" file to save the memory usage. This patch mainly does the\n> > following things:\n> >\n> > 1: Fix an issue that we would report an error when the \"packed-refs\"\n> > file is empty, which does not align with the runtime behavior.\n> > 2-4: Extract some logic from the existing code and then use these\n> > created helper functions to let fsck code to use mmap necessarily\n> >\n> > [1] https://lore.kernel.org/git/20250503133158.GA4450@coredump.intra.peff.net\n> >\n> > Really thank Peff and Patrick to suggest me to do above change.\n> \n> This round looks good to me. Others?\n\nI left a few comments that I think bear addressing (or at least some\ndiscussion).\n\n-Peff\n"},{"id":"517598","messageId":"xmqqzffm4zx5.fsf@gitster.g","threadId":"63407","inReplyTo":"20250508200802.GC18229@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/4] align the behavior when opening \"packed-refs\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-08T20:20:54Z","receivedAt":"2025-05-08T20:20:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, May 07, 2025 at 03:51:02PM -0700, Junio C Hamano wrote:\n>\n>> shejialuo writes:\n>> \n>> > Hi All:\n>> >\n>> > As discussed in [1], we need to use mmap mechanism to open large\n>> > \"packed_refs\" file to save the memory usage. This patch mainly does the\n>> > following things:\n>> >\n>> > 1: Fix an issue that we would report an error when the \"packed-refs\"\n>> > file is empty, which does not align with the runtime behavior.\n>> > 2-4: Extract some logic from the existing code and then use these\n>> > created helper functions to let fsck code to use mmap necessarily\n>> >\n>> > [1] https://lore.kernel.org/git/20250503133158.GA4450@coredump.intra.peff.net\n>> >\n>> > Really thank Peff and Patrick to suggest me to do above change.\n>> \n>> This round looks good to me. Others?\n>\n> I left a few comments that I think bear addressing (or at least some\n> discussion).\n\nI found both of your points very good ones, especially the \"why are\nwe making an in-core copy anyway later?\"\n\nWhich may probably mean that munmap_temporary_snapshot() is not the\nhelper function we want in the code path, so one of the preliminary\nrefactoring patches can be removed.\n\nEven on mmap-incapable platforms, we have enough emulation in\ngit_mmap() and git_munmap(), and this code path that wants to read a\npacked-refs file just mmap(), do its thing, and then munmap(),\nwithout worrying anything about \"ah, temporary, so we need to make\nan in-core copy for ourselves\".\n\nTHanks.\n"},{"id":"517600","messageId":"20250508203350.GG18229@coredump.intra.peff.net","threadId":"63407","inReplyTo":"xmqqzffm4zx5.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] align the behavior when opening \"packed-refs\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-08T20:33:50Z","receivedAt":"2025-05-08T20:33:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 08, 2025 at 01:20:54PM -0700, Junio C Hamano wrote:\n\n> > I left a few comments that I think bear addressing (or at least some\n> > discussion).\n> \n> I found both of your points very good ones, especially the \"why are\n> we making an in-core copy anyway later?\"\n> \n> Which may probably mean that munmap_temporary_snapshot() is not the\n> helper function we want in the code path, so one of the preliminary\n> refactoring patches can be removed.\n\nYep.\n\n> Even on mmap-incapable platforms, we have enough emulation in\n> git_mmap() and git_munmap(), and this code path that wants to read a\n> packed-refs file just mmap(), do its thing, and then munmap(),\n> without worrying anything about \"ah, temporary, so we need to make\n> an in-core copy for ourselves\".\n\nHmm, yeah. I had thought that somebody could explicitly set\nmmap_strategy themselves via config, in which case we'd want to avoid\ncalling git_mmap() on systems where it actually does mmap. But it\ndoesn't look like we have any mechanism for setting the config. So\nMMAP_NONE happens only when NO_MMAP is set.\n\n I briefly wondered if the existing code could be simplified to get rid\n of MMAP_NONE, since it could also rely on git_mmap() to make an\n in-memory copy. But it gets weird with MMAP_PREVENTS_DELETE and\n NO_MMAP combined, since then we make a pointless extra copy (one to\n fake-mmap, and one into the new buffer). OTOH, I'd expect those to be\n mutually exclusive.\n\n I kind of wonder with MMAP_TEMPORARY why we bother mapping at all and\n not just reading into a buffer. Maybe it's more efficient?\n\n Probably not worth revisiting all of this old code, though. And\n anyway, because of the SMALL_FILE_SIZE check, we couldn't even\n simplify away the read_in_full() code.\n\n-Peff\n"},{"id":"517685","messageId":"aB4ZN5IY585Qlz9r@ArchLinux","threadId":"63407","inReplyTo":"xmqq4ixu6f6q.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-09T15:03:19Z","receivedAt":"2025-05-09T15:03:23Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, May 08, 2025 at 01:05:49PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> >> -\tif (mmap_strategy != MMAP_OK && snapshot->mmapped) {\n> >> -\t\t/*\n> >> -\t\t * We don't want to leave the file mmapped, so we are\n> >> -\t\t * forced to make a copy now:\n> >> -\t\t */\n> >> -\t\tsize_t size = snapshot->eof - snapshot->start;\n> >> -\t\tchar *buf_copy = xmalloc(size);\n> >> -\n> >> -\t\tmemcpy(buf_copy, snapshot->start, size);\n> >> -\t\tclear_snapshot_buffer(snapshot);\n> >> -\t\tsnapshot->buf = snapshot->start = buf_copy;\n> >> -\t\tsnapshot->eof = buf_copy + size;\n> >> -\t}\n> >> +\tif (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)\n> >> +\t\tmunmap_temporary_snapshot(snapshot);\n> >\n> > The original triggers this conditional whenever the strategy is not\n> > MMAP_OK (so MMAP_TEMPORARY or MMAP_NONE). But in your post-image, we do\n> > so only for MMAP_TEMPORARY.\n> >\n> > I can guess that the two end up the same, because snapshot->mmapped\n> > would never be set when MMAP_NONE is set. But if we are going to make\n> > such a logical inference, it should be explained in the commit message\n> > (though my preference is to leave the code as-is, or to pull the\n> > refactor into its own commit).\n\nThat's right, I made a mistake here. I somehow think that we should just\ncheck whether \"mmap_strategy\" is \"MMAP_TEMPORARY\". And this would be\nenough. Because when \"mmap_strategy\" is \"MMAP_NONE\", we would never call\n`mmap`.\n\nActually, when I refactor the code, this one makes me quite confusing.\nAnd I think we should not change. I will revert in the next version.\n\n> \n> Good thinking.\n> \n> An \"extract\" step that is meant as a preliminary refactoring should\n> not make such a change. The change may or may not prepare the code\n> for better maintainability, but I agree that such a change needs to\n> be justified separately.\n> \n\nYeah, sure. I didn't consider well.\n\n> Thanks.\n\nThanks,\nJialuo\n"},{"id":"517686","messageId":"aB4dflpFNW4mJlq6@ArchLinux","threadId":"63407","inReplyTo":"20250508200741.GB18229@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/4] packed-backend: mmap large \"packed-refs\" file during fsck","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-09T15:21:34Z","receivedAt":"2025-05-09T15:21:39Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, May 08, 2025 at 04:07:41PM -0400, Jeff King wrote:\n> On Wed, May 07, 2025 at 10:54:03PM +0800, shejialuo wrote:\n> \n> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> > index ae6b6845a6..ff744f1d4c 100644\n> > --- a/refs/packed-backend.c\n> > +++ b/refs/packed-backend.c\n> > @@ -2079,7 +2079,7 @@ static int packed_fsck(struct ref_store *ref_store,\n> > {\n> > \tstruct packed_ref_store *refs = packed_downcast(ref_store,\n> > \t\t\t\t\t\t\tREF_STORE_READ, \"fsck\");\n> > -\tstruct strbuf packed_ref_content = STRBUF_INIT;\n> > +\tstruct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));\n> \n> Minor, but is there any reason to allocate this here and not just:\n> \n> struct snapshot snapshot = { 0 };\n> \n> ?\n\nI simply copy the code from the existing code... I will change.\n\n> \n> > @@ -2126,21 +2126,23 @@ static int packed_fsck(struct ref_store *ref_store,\n> > \tif (!st.st_size)\n> > \t\tgoto cleanup;\n> > \n> > -\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n> > -\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n> > +\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n> > \t\tgoto cleanup;\n> > -\t}\n> \n> Looking at allocate_snapshot_buffer(), it will return 0 only when the\n> file is empty (and thus there is nothing to allocate) and will\n> otherwise die(). So we do not need to report any error when it fails.\n> Good.\n> \n> But that makes the \"!st.st_size\" check in the context redundant, doesn't\n> it? It can just go away.\n> \n\nGood catch. I remember in the V1, this does not exist. I may make\nsomething wrong when rebasing the code. Thanks!\n\n> > -\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n> > -\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n> > +\tif (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)\n> > +\t\tmunmap_temporary_snapshot(snapshot);\n> > +\n> > +\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot->start,\n> > +\t\t\t\t snapshot->eof);\n> \n> Why are we unmapping here before we use the content? That will create an\n> allocated in-memory copy of the mmap'd content. I thought the whole\n> point here was to avoid doing so.\n> \n\nI simply follow how \"create_snapshot\" does. Actually, I am also quite\nconfused about this. If we would eventually copy the content into the\nuser space's memory. What is the reason that we mmap at Windows in the\nfirst place?\n\nMy understanding is that after mmaping, we need to do some sanity checks\nand then if there is a need, we may sort the \"packed-refs\" file. So, we\nwould improve some efficiency at Windows for this part?\n\n> It does shorten the amount of time we hold the temporary mmap in place,\n> but I don't think we care about that here. The whole point of\n> MMAP_TEMPORARY is that we usually hold the packed-refs file open across\n> many requests, and on some platforms (like Windows) we don't want to do\n> that. But in this code path we plan to mmap, do our verification, and\n> then drop the snapshot. So we're always \"temporary\" anyway.\n> \n> I.e., I'd have expected this code to allocate_snapshot_buffer(), do its\n> checks, and then call clear_snapshot_buffer().\n> \n\nI will improve this in the next version.\n\n> -Peff\n"},{"id":"517688","messageId":"aB4ewBsxnq0Yv3Fd@ArchLinux","threadId":"63407","inReplyTo":"20250508203350.GG18229@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/4] align the behavior when opening \"packed-refs\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-09T15:26:56Z","receivedAt":"2025-05-09T15:27:00Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, May 08, 2025 at 04:33:50PM -0400, Jeff King wrote:\n> On Thu, May 08, 2025 at 01:20:54PM -0700, Junio C Hamano wrote:\n> \n> > > I left a few comments that I think bear addressing (or at least some\n> > > discussion).\n> > \n> > I found both of your points very good ones, especially the \"why are\n> > we making an in-core copy anyway later?\"\n> > \n> > Which may probably mean that munmap_temporary_snapshot() is not the\n> > helper function we want in the code path, so one of the preliminary\n> > refactoring patches can be removed.\n> \n> Yep.\n> \n\nRight, I will remove this patch in the next version.\n\n> > Even on mmap-incapable platforms, we have enough emulation in\n> > git_mmap() and git_munmap(), and this code path that wants to read a\n> > packed-refs file just mmap(), do its thing, and then munmap(),\n> > without worrying anything about \"ah, temporary, so we need to make\n> > an in-core copy for ourselves\".\n> \n> Hmm, yeah. I had thought that somebody could explicitly set\n> mmap_strategy themselves via config, in which case we'd want to avoid\n> calling git_mmap() on systems where it actually does mmap. But it\n> doesn't look like we have any mechanism for setting the config. So\n> MMAP_NONE happens only when NO_MMAP is set.\n> \n> I briefly wondered if the existing code could be simplified to get rid\n> of MMAP_NONE, since it could also rely on git_mmap() to make an\n> in-memory copy. But it gets weird with MMAP_PREVENTS_DELETE and\n> NO_MMAP combined, since then we make a pointless extra copy (one to\n> fake-mmap, and one into the new buffer). OTOH, I'd expect those to be\n> mutually exclusive.\n> \n> I kind of wonder with MMAP_TEMPORARY why we bother mapping at all and\n> not just reading into a buffer. Maybe it's more efficient?\n> \n\nI am also confused about this. If we eventually would allocate a buffer,\nit is wired that we map in the first place.\n\n> Probably not worth revisiting all of this old code, though. And\n> anyway, because of the SMALL_FILE_SIZE check, we couldn't even\n> simplify away the read_in_full() code.\n> \n\nRight, I will try to clean the code later. At now, I think we need to\nconcentrate on using `mmap`. I will add this into my TODOs. Thanks for\nthe suggestion.\n\n> -Peff\n\nThanks,\nJialuo\n"},{"id":"517691","messageId":"20250509155934.GA25686@coredump.intra.peff.net","threadId":"63407","inReplyTo":"aB4dflpFNW4mJlq6@ArchLinux","subject":"Re: [PATCH v2 4/4] packed-backend: mmap large \"packed-refs\" file during fsck","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-09T15:59:34Z","receivedAt":"2025-05-09T15:59:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 09, 2025 at 11:21:34PM +0800, shejialuo wrote:\n\n> > > -\tstruct strbuf packed_ref_content = STRBUF_INIT;\n> > > +\tstruct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));\n> > \n> > Minor, but is there any reason to allocate this here and not just:\n> > \n> > struct snapshot snapshot = { 0 };\n> > \n> > ?\n> \n> I simply copy the code from the existing code... I will change.\n\nAh, I see. The existing code must allocate on the heap because it is\nreturning the snapshot to its caller. But here the variable is\ncompletely local to the function.\n\nHowever, if we stop using mmap_strategy altogether and just use xmmap()\ndirectly, I don't think you'd even need a snapshot variable.\n\n> > Why are we unmapping here before we use the content? That will create an\n> > allocated in-memory copy of the mmap'd content. I thought the whole\n> > point here was to avoid doing so.\n> > \n> \n> I simply follow how \"create_snapshot\" does. Actually, I am also quite\n> confused about this. If we would eventually copy the content into the\n> user space's memory. What is the reason that we mmap at Windows in the\n> first place?\n> \n> My understanding is that after mmaping, we need to do some sanity checks\n> and then if there is a need, we may sort the \"packed-refs\" file. So, we\n> would improve some efficiency at Windows for this part?\n\nAh, yes, that makes sense for the existing code. If we do have to sort,\nthen mapping on Windows means we could sort into our internal buffer,\nsaving an extra copy. That is probably a rare case these days (once upon\na time the file was not guaranteed to be sorted, but these days the\nwriter makes sure it is sorted and puts a marker in the header claiming\nit is so). So maybe that approach is not as useful as it once was, but I\ndon't know if it's worth spending effort to rip it out now.\n\nI don't think any of that applies for packed_fsck(), though, since it is\njust processing the file linearly in that case (rather than making a\nsorted copy).\n\n-Peff\n"},{"id":"517696","messageId":"aB4v6kBdNHmFlPlR@ArchLinux","threadId":"63407","inReplyTo":"20250509155934.GA25686@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/4] packed-backend: mmap large \"packed-refs\" file during fsck","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-09T16:40:10Z","receivedAt":"2025-05-09T16:39:43Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Fri, May 09, 2025 at 11:59:34AM -0400, Jeff King wrote:\n> On Fri, May 09, 2025 at 11:21:34PM +0800, shejialuo wrote:\n> \n> > > > -\tstruct strbuf packed_ref_content = STRBUF_INIT;\n> > > > +\tstruct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));\n> > > \n> > > Minor, but is there any reason to allocate this here and not just:\n> > > \n> > > struct snapshot snapshot = { 0 };\n> > > \n> > > ?\n> > \n> > I simply copy the code from the existing code... I will change.\n> \n> Ah, I see. The existing code must allocate on the heap because it is\n> returning the snapshot to its caller. But here the variable is\n> completely local to the function.\n> \n> However, if we stop using mmap_strategy altogether and just use xmmap()\n> directly, I don't think you'd even need a snapshot variable.\n> \n\nYes, that's right. Maybe the simplest way is to use `xmmap()`. But I\ndon't want to introduce repetition. In the current codebase, we already\nhave the logic to load the \"packed-refs\". Let's just reuse it.\n\nOut of topic, I have more to express.\n\nActually, my eventual goal is to unify the fsck and other parts. For\nexample, \"create_snapshot\" would also do some basic sanity checks. And\nof course, there is some overlap between fsck and this function. And\nalso for \"next_record\", it would also check something.\n\nDuring my implementation, I find it hard to unify and it would require a\nlot of effort. So, I simply introduce some redundant logic. But this is\nnot perfect. Because I still parse the \"packed-refs\" file just like\n\"next_record\" and \"create_snapshot\" do. And the most disappointed thing\nis that I cannot reuse them at all. But the things I want to do is very\nsimilar to these functions.\n\nSo, I think I would eventually to find out a way to do above in the\nfuture.\n\nThanks,\nJialuo\n"},{"id":"517780","messageId":"aCCtQDnWII-knmEc@ArchLinux","threadId":"63407","inReplyTo":"aBtzn4nwLsI9p5Cp@ArchLinux","subject":"[PATCH v3 0/3] align the behavior when opening \"packed-refs\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-11T13:59:28Z","receivedAt":"2025-05-11T13:59:34Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Hi All:\n\nAs discussed in [1], we need to use mmap mechanism to open large\n\"packed_refs\" file to save the memory usage. This patch mainly does the\nfollowing things:\n\n1: Fix an issue that we would report an error when the \"packed-refs\"\nfile is empty, which does not align with the runtime behavior.\n2-4: Extract some logic from the existing code and then use these\ncreated helper functions to let fsck code to use mmap necessarily\n\n[1] https://lore.kernel.org/git/20250503133158.GA4450@coredump.intra.peff.net\n\nReally thank Peff and Patrick to suggest me to do above change.\n\n---\n\nChange in v2:\n\n1. Update the commit message of [PATCH 1/4]. And use redirection to\ncreate an empty file instead of using `touch`.\n2. Don't use if for the refactored function in [PATCH 3/4] and then\nupdate the commit message to align with the new function name.\n3. Enhance the commit message of [PATCH 4/4].\n\n---\n\nChange in v3:\n\n1. Drop the patch which creates a new function\n\"munmap_temporary_snapshot\". As discussed, there is no need to munmap\nthe file during fsck.\n2. Allocate snapshot variable in the stack instead of heap.\n3. Fix rebase issue, remove unneeded code to check the file size\nexplicitly.\n\nThe range-diff seems unreadable. Briefly, for this new version, the only\nchange is the last patch compared to the v2.\n\n packed-backend: mmap large \"packed-refs\" file during fsck\n\nThanks,\nJialuo\n\nshejialuo (3):\n packed-backend: fsck should allow an empty \"packed-refs\" file\n packed-backend: extract snapshot allocation in `load_contents`\n packed-backend: mmap large \"packed-refs\" file during fsck\n\n refs/packed-backend.c | 70 ++++++++++++++++++++++------------------\n t/t0602-reffiles-fsck.sh | 13 ++++++++\n 2 files changed, 52 insertions(+), 31 deletions(-)\n\nRange-diff against v2:\n-: ---------- > 1: 26c3fd55a8 packed-backend: fsck should allow an empty \"packed-refs\" file\n1: c0609afac9 ! 2: 4604be8b51 packed-backend: extract munmap operation for `MMAP_TEMPORARY`\n @@ Metadata\n Author: shejialuo \n \n ## Commit message ##\n - packed-backend: extract munmap operation for `MMAP_TEMPORARY`\n + packed-backend: extract snapshot allocation in `load_contents`\n \n - \"create_snapshot\" would try to munmap the file when the \"mmap_strategy\"\n - is \"MMAP_TEMPORARY\". We also need to do this operation when checking the\n - consistency of the \"packed-refs\" file.\n + \"load_contents\" would choose which way to load the content of the\n + \"packed-refs\". However, we cannot directly use this function when\n + checking the consistency due to we don't want to open the file. And we\n + also need to reuse the logic to avoid causing repetition.\n \n - Create a new function \"munmap_temporary_snapshot\" to do above and\n - change \"create_snapshot\" to align with the behavior.\n + Let's create a new helper function \"allocate_snapshot_buffer\" to extract\n + the snapshot allocation logic in \"load_contents\" and update the\n + \"load_contents\" to align with the behavior.\n \n Suggested-by: Jeff King \n Suggested-by: Patrick Steinhardt \n Signed-off-by: shejialuo \n \n ## refs/packed-backend.c ##\n -@@ refs/packed-backend.c: static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct st\n - \treturn 1;\n - }\n +@@ refs/packed-backend.c: static int refname_contains_nul(struct strbuf *refname)\n + \n + #define SMALL_FILE_SIZE (32*1024)\n \n -+static void munmap_temporary_snapshot(struct snapshot *snapshot)\n ++static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)\n +{\n -+\tchar *buf_copy;\n ++\tssize_t bytes_read;\n +\tsize_t size;\n +\n -+\tif (!snapshot)\n -+\t\treturn;\n ++\tsize = xsize_t(st->st_size);\n ++\tif (!size)\n ++\t\treturn 0;\n ++\n ++\tif (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n ++\t\tsnapshot->buf = xmalloc(size);\n ++\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n ++\t\tif (bytes_read < 0 || bytes_read != size)\n ++\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n ++\t\tsnapshot->mmapped = 0;\n ++\t} else {\n ++\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n ++\t\tsnapshot->mmapped = 1;\n ++\t}\n +\n -+\t/*\n -+\t * We don't want to leave the file mmapped, so we are\n -+\t * forced to make a copy now:\n -+\t */\n -+\tsize = snapshot->eof - snapshot->start;\n -+\tbuf_copy = xmalloc(size);\n ++\tsnapshot->start = snapshot->buf;\n ++\tsnapshot->eof = snapshot->buf + size;\n +\n -+\tmemcpy(buf_copy, snapshot->start, size);\n -+\tclear_snapshot_buffer(snapshot);\n -+\tsnapshot->buf = snapshot->start = buf_copy;\n -+\tsnapshot->eof = buf_copy + size;\n ++\treturn 1;\n +}\n +\n /*\n * Depending on `mmap_strategy`, either mmap or read the contents of\n * the `packed-refs` file into the snapshot. Return 1 if the file\n -@@ refs/packed-backend.c: static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n - \t\tverify_buffer_safe(snapshot);\n - \t}\n +@@ refs/packed-backend.c: static int refname_contains_nul(struct strbuf *refname)\n + */\n + static int load_contents(struct snapshot *snapshot)\n + {\n +-\tint fd;\n + \tstruct stat st;\n +-\tsize_t size;\n +-\tssize_t bytes_read;\n ++\tint ret = 1;\n ++\tint fd;\n \n --\tif (mmap_strategy != MMAP_OK && snapshot->mmapped) {\n --\t\t/*\n --\t\t * We don't want to leave the file mmapped, so we are\n --\t\t * forced to make a copy now:\n --\t\t */\n --\t\tsize_t size = snapshot->eof - snapshot->start;\n --\t\tchar *buf_copy = xmalloc(size);\n + \tfd = open(snapshot->refs->path, O_RDONLY);\n + \tif (fd < 0) {\n +@@ refs/packed-backend.c: static int load_contents(struct snapshot *snapshot)\n + \n + \tif (fstat(fd, &st) < 0)\n + \t\tdie_errno(\"couldn't stat %s\", snapshot->refs->path);\n +-\tsize = xsize_t(st.st_size);\n -\n --\t\tmemcpy(buf_copy, snapshot->start, size);\n --\t\tclear_snapshot_buffer(snapshot);\n --\t\tsnapshot->buf = snapshot->start = buf_copy;\n --\t\tsnapshot->eof = buf_copy + size;\n +-\tif (!size) {\n +-\t\tclose(fd);\n +-\t\treturn 0;\n +-\t} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n +-\t\tsnapshot->buf = xmalloc(size);\n +-\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n +-\t\tif (bytes_read < 0 || bytes_read != size)\n +-\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n +-\t\tsnapshot->mmapped = 0;\n +-\t} else {\n +-\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n +-\t\tsnapshot->mmapped = 1;\n -\t}\n -+\tif (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)\n -+\t\tmunmap_temporary_snapshot(snapshot);\n +-\tclose(fd);\n + \n +-\tsnapshot->start = snapshot->buf;\n +-\tsnapshot->eof = snapshot->buf + size;\n ++\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n ++\t\tret = 0;\n \n - \treturn snapshot;\n +-\treturn 1;\n ++\tclose(fd);\n ++\treturn ret;\n }\n + \n + static const char *find_reference_location_1(struct snapshot *snapshot,\n2: c868e3dd16 ! 3: 82e19a65c6 packed-backend: mmap large \"packed-refs\" file during fsck\n @@ Commit message\n \"packed-backend.c\" use this way, we should make \"fsck\" align with the\n current codebase.\n \n - As we have introduced two helper functions \"allocate_snapshot_buffer\"\n - and \"munmap_snapshot_if_temporary\", we could simply call these functions\n - to use mmap mechanism.\n + As we have introduced the helper function \"allocate_snapshot_buffer\", we\n + could simple use this function to use mmap mechanism.\n \n Suggested-by: Jeff King \n Suggested-by: Patrick Steinhardt \n @@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,\n \tstruct packed_ref_store *refs = packed_downcast(ref_store,\n \t\t\t\t\t\t\tREF_STORE_READ, \"fsck\");\n -\tstruct strbuf packed_ref_content = STRBUF_INIT;\n -+\tstruct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));\n ++\tstruct snapshot snapshot = { 0 };\n \tunsigned int sorted = 0;\n \tstruct stat st;\n \tint ret = 0;\n @@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,\n - \tif (!st.st_size)\n + \t\tgoto cleanup;\n + \t}\n + \n +-\tif (!st.st_size)\n ++\tif (!allocate_snapshot_buffer(&snapshot, fd, &st))\n \t\tgoto cleanup;\n \n -\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n -\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n -+\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n - \t\tgoto cleanup;\n +-\t\tgoto cleanup;\n -\t}\n - \n +-\n -\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n -\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n -+\tif (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)\n -+\t\tmunmap_temporary_snapshot(snapshot);\n -+\n -+\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot->start,\n -+\t\t\t\t snapshot->eof);\n ++\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot.start,\n ++\t\t\t\t snapshot.eof);\n \tif (!ret && sorted)\n -\t\tret = packed_fsck_ref_sorted(o, ref_store, packed_ref_content.buf,\n -\t\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n -+\t\tret = packed_fsck_ref_sorted(o, ref_store, snapshot->start,\n -+\t\t\t\t\t snapshot->eof);\n ++\t\tret = packed_fsck_ref_sorted(o, ref_store, snapshot.start,\n ++\t\t\t\t\t snapshot.eof);\n \n cleanup:\n \tif (fd >= 0)\n \t\tclose(fd);\n -\tstrbuf_release(&packed_ref_content);\n -+\tclear_snapshot_buffer(snapshot);\n -+\tfree(snapshot);\n ++\tclear_snapshot_buffer(&snapshot);\n \treturn ret;\n }\n \n-- \n2.49.0\n\n"},{"id":"517781","messageId":"aCCtx2mqihlc0M7H@ArchLinux","threadId":"63407","inReplyTo":"aCCtQDnWII-knmEc@ArchLinux","subject":"[PATCH v3 1/3] packed-backend: fsck should allow an empty \"packed-refs\" file","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-11T14:01:43Z","receivedAt":"2025-05-11T14:01:48Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"During fsck, an empty \"packed-refs\" gives an error; this is unwarranted.\nWe should just skip checking the content of \"packed-refs\" just like the\nruntime code paths such as \"create_snapshot\" which simply returns the\n\"snapshot\" without checking the content of \"packed-refs\".\n\nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 3 +++\n t/t0602-reffiles-fsck.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 3ad1ed0787..0dd6c6677b 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2103,6 +2103,9 @@ static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n+\tif (!st.st_size)\n+\t\tgoto cleanup;\n+\n \tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n \t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n \t\tgoto cleanup;\ndiff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\nindex 9d1dc2144c..e04967581c 100755\n--- a/t/t0602-reffiles-fsck.sh\n+++ b/t/t0602-reffiles-fsck.sh\n@@ -647,6 +647,19 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '\n \t)\n '\n \n+test_expect_success 'empty packed-refs should not be reported' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit default &&\n+\n+\t\t>.git/packed-refs &&\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n test_expect_success 'packed-refs header should be checked' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.49.0\n\n"},{"id":"517782","messageId":"aCCtzm2bDRSTgEO-@ArchLinux","threadId":"63407","inReplyTo":"aCCtQDnWII-knmEc@ArchLinux","subject":"[PATCH v3 2/3] packed-backend: extract snapshot allocation in `load_contents`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-11T14:01:50Z","receivedAt":"2025-05-11T14:01:56Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"\"load_contents\" would choose which way to load the content of the\n\"packed-refs\". However, we cannot directly use this function when\nchecking the consistency due to we don't want to open the file. And we\nalso need to reuse the logic to avoid causing repetition.\n\nLet's create a new helper function \"allocate_snapshot_buffer\" to extract\nthe snapshot allocation logic in \"load_contents\" and update the\n\"load_contents\" to align with the behavior.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 54 +++++++++++++++++++++++++------------------\n 1 file changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 0dd6c6677b..e582227772 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -517,6 +517,32 @@ static int refname_contains_nul(struct strbuf *refname)\n \n #define SMALL_FILE_SIZE (32*1024)\n \n+static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)\n+{\n+\tssize_t bytes_read;\n+\tsize_t size;\n+\n+\tsize = xsize_t(st->st_size);\n+\tif (!size)\n+\t\treturn 0;\n+\n+\tif (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n+\t\tsnapshot->buf = xmalloc(size);\n+\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n+\t\tif (bytes_read < 0 || bytes_read != size)\n+\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n+\t\tsnapshot->mmapped = 0;\n+\t} else {\n+\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\tsnapshot->mmapped = 1;\n+\t}\n+\n+\tsnapshot->start = snapshot->buf;\n+\tsnapshot->eof = snapshot->buf + size;\n+\n+\treturn 1;\n+}\n+\n /*\n * Depending on `mmap_strategy`, either mmap or read the contents of\n * the `packed-refs` file into the snapshot. Return 1 if the file\n@@ -525,10 +551,9 @@ static int refname_contains_nul(struct strbuf *refname)\n */\n static int load_contents(struct snapshot *snapshot)\n {\n-\tint fd;\n \tstruct stat st;\n-\tsize_t size;\n-\tssize_t bytes_read;\n+\tint ret = 1;\n+\tint fd;\n \n \tfd = open(snapshot->refs->path, O_RDONLY);\n \tif (fd < 0) {\n@@ -550,27 +575,12 @@ static int load_contents(struct snapshot *snapshot)\n \n \tif (fstat(fd, &st) < 0)\n \t\tdie_errno(\"couldn't stat %s\", snapshot->refs->path);\n-\tsize = xsize_t(st.st_size);\n-\n-\tif (!size) {\n-\t\tclose(fd);\n-\t\treturn 0;\n-\t} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n-\t\tsnapshot->buf = xmalloc(size);\n-\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n-\t\tif (bytes_read < 0 || bytes_read != size)\n-\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n-\t\tsnapshot->mmapped = 0;\n-\t} else {\n-\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\tsnapshot->mmapped = 1;\n-\t}\n-\tclose(fd);\n \n-\tsnapshot->start = snapshot->buf;\n-\tsnapshot->eof = snapshot->buf + size;\n+\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n+\t\tret = 0;\n \n-\treturn 1;\n+\tclose(fd);\n+\treturn ret;\n }\n \n static const char *find_reference_location_1(struct snapshot *snapshot,\n-- \n2.49.0\n\n"},{"id":"517783","messageId":"aCCt1zJ2yviOz--l@ArchLinux","threadId":"63407","inReplyTo":"aCCtQDnWII-knmEc@ArchLinux","subject":"[PATCH v3 3/3] packed-backend: mmap large \"packed-refs\" file during fsck","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-11T14:01:59Z","receivedAt":"2025-05-11T14:02:05Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"During fsck, we use \"strbuf_read\" to read the content of \"packed-refs\"\nwithout using mmap mechanism. This is a bad practice which would consume\nmore memory than using mmap mechanism. Besides, as all code paths in\n\"packed-backend.c\" use this way, we should make \"fsck\" align with the\ncurrent codebase.\n\nAs we have introduced the helper function \"allocate_snapshot_buffer\", we\ncould simple use this function to use mmap mechanism.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 19 +++++++------------\n 1 file changed, 7 insertions(+), 12 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex e582227772..85f5a45160 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2069,7 +2069,7 @@ static int packed_fsck(struct ref_store *ref_store,\n {\n \tstruct packed_ref_store *refs = packed_downcast(ref_store,\n \t\t\t\t\t\t\tREF_STORE_READ, \"fsck\");\n-\tstruct strbuf packed_ref_content = STRBUF_INIT;\n+\tstruct snapshot snapshot = { 0 };\n \tunsigned int sorted = 0;\n \tstruct stat st;\n \tint ret = 0;\n@@ -2113,24 +2113,19 @@ static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n-\tif (!st.st_size)\n+\tif (!allocate_snapshot_buffer(&snapshot, fd, &st))\n \t\tgoto cleanup;\n \n-\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n-\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n-\t\tgoto cleanup;\n-\t}\n-\n-\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n-\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n+\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot.start,\n+\t\t\t\t snapshot.eof);\n \tif (!ret && sorted)\n-\t\tret = packed_fsck_ref_sorted(o, ref_store, packed_ref_content.buf,\n-\t\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n+\t\tret = packed_fsck_ref_sorted(o, ref_store, snapshot.start,\n+\t\t\t\t\t snapshot.eof);\n \n cleanup:\n \tif (fd >= 0)\n \t\tclose(fd);\n-\tstrbuf_release(&packed_ref_content);\n+\tclear_snapshot_buffer(&snapshot);\n \treturn ret;\n }\n \n-- \n2.49.0\n\n"},{"id":"517805","messageId":"aCGzIlLH_ESNg6-v@pks.im","threadId":"63407","inReplyTo":"aCCtx2mqihlc0M7H@ArchLinux","subject":"Re: [PATCH v3 1/3] packed-backend: fsck should allow an empty \"packed-refs\" file","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-12T08:36:50Z","receivedAt":"2025-05-12T08:36:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 11, 2025 at 10:01:43PM +0800, shejialuo wrote:\n> During fsck, an empty \"packed-refs\" gives an error; this is unwarranted.\n> We should just skip checking the content of \"packed-refs\" just like the\n> runtime code paths such as \"create_snapshot\" which simply returns the\n> \"snapshot\" without checking the content of \"packed-refs\".\n\nI think this doesn't quite answer the question whether this is a _good_\nidea though. The question that we need to answer is whether there are\nany writing code paths that may end up writing a \"packed-refs\" file that\nis completely empty. Modern Git would at least write the packed-refs\nheader, wouldn't it?\n\nThe reason why I'm a little sceptical is that there is a common problem\nwith ext4 caused by its delayed allocation [1]. If you:\n\n 1. Write data to a temporary file.\n 2. Rename the file into place.\n 3. The host system crashes.\n\nThen it may happen that the renamed file is now completely empty.\n\nThe root cause is a bug in the application: before renaming the file\ninto place it _must_ fsync the file to disk. Git does that by default,\nbut it is extremely easy to get wrong and we had bugs around this until\n~2 years ago, if I remember correctly. We hit the problem several times\nin our production systems.\n\nSo I wonder whether ignoring empty files would cause us to miss such a\ncommon error. But I guess if there are valid cases where we may end up\nwith an empty \"packed-refs\" file we cannot do anything about it.\n\nPatrick\n\n[1]: https://thunk.org/tytso/blog/2009/03/12/delayed-allocation-and-the-zero-length-file-problem/\n"},{"id":"517806","messageId":"aCGzLxcXlcQLtorC@pks.im","threadId":"63407","inReplyTo":"aCCtzm2bDRSTgEO-@ArchLinux","subject":"Re: [PATCH v3 2/3] packed-backend: extract snapshot allocation in `load_contents`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-12T08:37:03Z","receivedAt":"2025-05-12T08:37:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 11, 2025 at 10:01:50PM +0800, shejialuo wrote:\n> \"load_contents\" would choose which way to load the content of the\n> \"packed-refs\". However, we cannot directly use this function when\n> checking the consistency due to we don't want to open the file. And we\n> also need to reuse the logic to avoid causing repetition.\n> \n> Let's create a new helper function \"allocate_snapshot_buffer\" to extract\n> the snapshot allocation logic in \"load_contents\" and update the\n> \"load_contents\" to align with the behavior.\n> \n> Suggested-by: Jeff King \n> Suggested-by: Patrick Steinhardt \n\nHuh. Are you sure I suggested this? :) I cannot remember at least.\n\nThat being said, the change looks sensible.\n\nPatrick\n"},{"id":"517827","messageId":"aCHO2dqWM2m6xt9m@ArchLinux","threadId":"63407","inReplyTo":"aCGzLxcXlcQLtorC@pks.im","subject":"Re: [PATCH v3 2/3] packed-backend: extract snapshot allocation in `load_contents`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-12T10:35:05Z","receivedAt":"2025-05-12T10:34:36Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 12, 2025 at 10:37:03AM +0200, Patrick Steinhardt wrote:\n> On Sun, May 11, 2025 at 10:01:50PM +0800, shejialuo wrote:\n> > \"load_contents\" would choose which way to load the content of the\n> > \"packed-refs\". However, we cannot directly use this function when\n> > checking the consistency due to we don't want to open the file. And we\n> > also need to reuse the logic to avoid causing repetition.\n> > \n> > Let's create a new helper function \"allocate_snapshot_buffer\" to extract\n> > the snapshot allocation logic in \"load_contents\" and update the\n> > \"load_contents\" to align with the behavior.\n> > \n> > Suggested-by: Jeff King \n> > Suggested-by: Patrick Steinhardt \n> \n> Huh. Are you sure I suggested this? :) I cannot remember at least.\n> \n\nBecause you explain me a lot how Gitlab handles and Peff tells me how\nGithub handles, I add both of you.\n\n> That being said, the change looks sensible.\n> \n> Patrick\n"},{"id":"517830","messageId":"aCHoovrKiSUemBCL@ArchLinux","threadId":"63407","inReplyTo":"aCGzIlLH_ESNg6-v@pks.im","subject":"Re: [PATCH v3 1/3] packed-backend: fsck should allow an empty \"packed-refs\" file","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-12T12:25:06Z","receivedAt":"2025-05-12T12:24:38Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 12, 2025 at 10:36:50AM +0200, Patrick Steinhardt wrote:\n> On Sun, May 11, 2025 at 10:01:43PM +0800, shejialuo wrote:\n> > During fsck, an empty \"packed-refs\" gives an error; this is unwarranted.\n> > We should just skip checking the content of \"packed-refs\" just like the\n> > runtime code paths such as \"create_snapshot\" which simply returns the\n> > \"snapshot\" without checking the content of \"packed-refs\".\n> \n> I think this doesn't quite answer the question whether this is a _good_\n> idea though. The question that we need to answer is whether there are\n> any writing code paths that may end up writing a \"packed-refs\" file that\n> is completely empty. Modern Git would at least write the packed-refs\n> header, wouldn't it?\n> \n\nThat's right. In the current codebase, we would always write the header\nwhich could be easily reproduced by using the following command:\n\n git init repo\n cd repo && git pack-refs\n cat .git/packed-refs\n\nAnd in \"packed-backend.c::write_with_updates\", we would always write the\nheader.\n\n> The reason why I'm a little sceptical is that there is a common problem\n> with ext4 caused by its delayed allocation [1]. If you:\n> \n> 1. Write data to a temporary file.\n> 2. Rename the file into place.\n> 3. The host system crashes.\n> \n> Then it may happen that the renamed file is now completely empty.\n> \n> The root cause is a bug in the application: before renaming the file\n> into place it _must_ fsync the file to disk. Git does that by default,\n> but it is extremely easy to get wrong and we had bugs around this until\n> ~2 years ago, if I remember correctly. We hit the problem several times\n> in our production systems.\n> \n\nI see. I agree that in the most situation, an empty \"packed-refs\" file\nmeans that there is an issue.\n\n> So I wonder whether ignoring empty files would cause us to miss such a\n> common error. But I guess if there are valid cases where we may end up\n> with an empty \"packed-refs\" file we cannot do anything about it.\n> \n\nI somehow think we would always write header in the Modern Git. But\n\"create_snapshot\" accept an empty existing \"packed-refs\" file at\nruntime.\n\nAnd header is introduced in 694b7a1999 (repack_without_ref(): write peeled\nrefs in the rewritten file, 2013-04-22). At this commit, we would always\nwrite the header into the \"packed-refs\" file.\n\nBut in runtime, we accept empty file or no header of the file content as we\nwant to keep compatible. In my humble word, I think we should allow\nempty file at now. Then, In Git 3.0, we tighten all the rules (there\nmust always be a header etc) and also update the runtime behavior.\n\n> Patrick\n> \n> [1]: https://thunk.org/tytso/blog/2009/03/12/delayed-allocation-and-the-zero-length-file-problem/\n\nThanks,\nJialuo\n"},{"id":"517839","messageId":"20250512130619.GB1191360@coredump.intra.peff.net","threadId":"63407","inReplyTo":"aCCtzm2bDRSTgEO-@ArchLinux","subject":"Re: [PATCH v3 2/3] packed-backend: extract snapshot allocation in `load_contents`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-12T13:06:19Z","receivedAt":"2025-05-12T13:06:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 11, 2025 at 10:01:50PM +0800, shejialuo wrote:\n\n> \"load_contents\" would choose which way to load the content of the\n> \"packed-refs\". However, we cannot directly use this function when\n> checking the consistency due to we don't want to open the file. And we\n> also need to reuse the logic to avoid causing repetition.\n> \n> Let's create a new helper function \"allocate_snapshot_buffer\" to extract\n> the snapshot allocation logic in \"load_contents\" and update the\n> \"load_contents\" to align with the behavior.\n\nThis looks good to me. One thing that did give me a slight pause while\nreviewing:\n\n> static int load_contents(struct snapshot *snapshot)\n> {\n> -\tint fd;\n> \tstruct stat st;\n> -\tsize_t size;\n> -\tssize_t bytes_read;\n> +\tint ret = 1;\n> [...]\n> +\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n> +\t\tret = 0;\n> \n> -\treturn 1;\n> +\tclose(fd);\n> +\treturn ret;\n> }\n\nI wanted to see what the semantics of \"ret\" were, but there aren't any\nother assignments. So I think this is equivalent to:\n\n int ret;\n ...\n ret = allocate_snapshot_buffer(snapshot, fd, &st);\n\nwhich makes it a little more clear we are just relaying the value from\nthat function.\n\nProbably not worth re-rolling for that, though.\n\n-Peff\n"},{"id":"517840","messageId":"20250512130815.GC1191360@coredump.intra.peff.net","threadId":"63407","inReplyTo":"aCCt1zJ2yviOz--l@ArchLinux","subject":"Re: [PATCH v3 3/3] packed-backend: mmap large \"packed-refs\" file during fsck","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-12T13:08:15Z","receivedAt":"2025-05-12T13:08:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 11, 2025 at 10:01:59PM +0800, shejialuo wrote:\n\n> @@ -2113,24 +2113,19 @@ static int packed_fsck(struct ref_store *ref_store,\n> \t\tgoto cleanup;\n> \t}\n> \n> -\tif (!st.st_size)\n> +\tif (!allocate_snapshot_buffer(&snapshot, fd, &st))\n> \t\tgoto cleanup;\n> \n> -\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n> -\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n> -\t\tgoto cleanup;\n> -\t}\n> -\n> -\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n> -\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n> +\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot.start,\n> +\t\t\t\t snapshot.eof);\n> \tif (!ret && sorted)\n> -\t\tret = packed_fsck_ref_sorted(o, ref_store, packed_ref_content.buf,\n> -\t\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n> +\t\tret = packed_fsck_ref_sorted(o, ref_store, snapshot.start,\n> +\t\t\t\t\t snapshot.eof);\n\nOK, so we still use allocate_snapshot_buffer(), but then we hold on to\nthe snapshot. Good.\n\nI do think just using xmmap() would have been sufficient (and not needed\npatch 2 then), but I'm OK with this direction under the logic that we\nmay end up sharing more code between the normal and fsck code paths\neventually.\n\n-Peff\n"},{"id":"517854","messageId":"aCIIL6IWiiWiGbFd@pks.im","threadId":"63407","inReplyTo":"aCHoovrKiSUemBCL@ArchLinux","subject":"Re: [PATCH v3 1/3] packed-backend: fsck should allow an empty \"packed-refs\" file","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-12T14:39:43Z","receivedAt":"2025-05-12T14:39:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 12, 2025 at 08:25:06PM +0800, shejialuo wrote:\n> On Mon, May 12, 2025 at 10:36:50AM +0200, Patrick Steinhardt wrote:\n> > So I wonder whether ignoring empty files would cause us to miss such a\n> > common error. But I guess if there are valid cases where we may end up\n> > with an empty \"packed-refs\" file we cannot do anything about it.\n> > \n> \n> I somehow think we would always write header in the Modern Git. But\n> \"create_snapshot\" accept an empty existing \"packed-refs\" file at\n> runtime.\n> \n> And header is introduced in 694b7a1999 (repack_without_ref(): write peeled\n> refs in the rewritten file, 2013-04-22). At this commit, we would always\n> write the header into the \"packed-refs\" file.\n\nSo what happened before this commit? Would we end up writing a\ncompletely empty file?\n\n> But in runtime, we accept empty file or no header of the file content as we\n> want to keep compatible. In my humble word, I think we should allow\n> empty file at now. Then, In Git 3.0, we tighten all the rules (there\n> must always be a header etc) and also update the runtime behavior.\n\nI think it depends. If we can prove that Git (and other, third-party\nimplementations thereof) wouldn't have ever written such \"packed-refs\"\nfiles then we should start warning now already, even if we accept such\nfiles at runtime. Otherwise, if there were versions of Git that could\nhave ended up writing such files I agree that we have to accept this for\nnow.\n\nBut if we were to deprecate the ability to read an empty file I wouldn't\nadapt both paths at the Git 3.0 boundary. If we decide that an empty\nfile is broken we should introduce the check for `git refs verify` way\nbefore we tighten the runtime behaviour. Otherwise there isn't really an\nopportunity for people to learn that we are about to remove this.\n\nPatrick\n"},{"id":"517855","messageId":"aCIIiMGSF51-qUua@pks.im","threadId":"63407","inReplyTo":"aCHO2dqWM2m6xt9m@ArchLinux","subject":"Re: [PATCH v3 2/3] packed-backend: extract snapshot allocation in `load_contents`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-12T14:41:12Z","receivedAt":"2025-05-12T14:41:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 12, 2025 at 06:35:05PM +0800, shejialuo wrote:\n> On Mon, May 12, 2025 at 10:37:03AM +0200, Patrick Steinhardt wrote:\n> > On Sun, May 11, 2025 at 10:01:50PM +0800, shejialuo wrote:\n> > > \"load_contents\" would choose which way to load the content of the\n> > > \"packed-refs\". However, we cannot directly use this function when\n> > > checking the consistency due to we don't want to open the file. And we\n> > > also need to reuse the logic to avoid causing repetition.\n> > > \n> > > Let's create a new helper function \"allocate_snapshot_buffer\" to extract\n> > > the snapshot allocation logic in \"load_contents\" and update the\n> > > \"load_contents\" to align with the behavior.\n> > > \n> > > Suggested-by: Jeff King \n> > > Suggested-by: Patrick Steinhardt \n> > \n> > Huh. Are you sure I suggested this? :) I cannot remember at least.\n> > \n> \n> Because you explain me a lot how Gitlab handles and Peff tells me how\n> Github handles, I add both of you.\n\nI think that would've made sense for the last step where you introduce\nthe adapted logic. But the intermediate steps have all been designed by\nyourself without my help :)\n\nAnyway, I don't mind this, I just found it to be funny.\n\nPatrick\n"},{"id":"517861","messageId":"20250512155654.GA1219668@coredump.intra.peff.net","threadId":"63407","inReplyTo":"aCIIL6IWiiWiGbFd@pks.im","subject":"Re: [PATCH v3 1/3] packed-backend: fsck should allow an empty \"packed-refs\" file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-12T15:56:54Z","receivedAt":"2025-05-12T15:56:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 12, 2025 at 04:39:43PM +0200, Patrick Steinhardt wrote:\n\n> > And header is introduced in 694b7a1999 (repack_without_ref(): write peeled\n> > refs in the rewritten file, 2013-04-22). At this commit, we would always\n> > write the header into the \"packed-refs\" file.\n> \n> So what happened before this commit? Would we end up writing a\n> completely empty file?\n\nYeah, prior to that if you delete the final entry, you'd get an empty\nfile. Reproducible with:\n\n # you can use modern git for this part\n git init\n git commit --allow-empty -m foo\n git checkout --detach\n git pack-refs --all\n\n # old git writes empty file during deletion\n git.v1.8.4 branch -D main\n ls -l .git/packed-refs\n\nGoing back even further, we'd also write an empty file via pack-refs.\nWith git v1.4.4, doing \"git init && git pack-refs\" will get you an empty\nfile. I didn't bisect completely, but presumably that changed in\nf4204ab9f6 (Store peeled refs in packed-refs (take 2)., 2006-11-21).\n\n> I think it depends. If we can prove that Git (and other, third-party\n> implementations thereof) wouldn't have ever written such \"packed-refs\"\n> files then we should start warning now already, even if we accept such\n> files at runtime. Otherwise, if there were versions of Git that could\n> have ended up writing such files I agree that we have to accept this for\n> now.\n\nSo we definitely did write those files. Those are pretty old versions of\nGit, but something we should continue to support.\n\nIt may be useful for fsck to detect this, though, even if the default\nmessage severity is set to \"info\" or even \"ignore. That would allow\npeople who know they are using modern Git to increase it themselves (I\ndon't expect normal users to do this, but it would probably be useful\nfor forges which run automated \"fsck\" across a lot of repos).\n\nAnd then the backwards-incompatible Git 3.0 thing would just be tweaking\nthe severity of the config (and in the meantime, it would help flush out\nany unexpected instances people run into).\n\n-Peff\n"},{"id":"517878","messageId":"xmqqh61pu4r9.fsf@gitster.g","threadId":"63407","inReplyTo":"20250512155654.GA1219668@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/3] packed-backend: fsck should allow an empty \"packed-refs\" file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-12T17:18:34Z","receivedAt":"2025-05-12T17:18:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It may be useful for fsck to detect this, though, even if the default\n> message severity is set to \"info\" or even \"ignore. That would allow\n> people who know they are using modern Git to increase it themselves (I\n> don't expect normal users to do this, but it would probably be useful\n> for forges which run automated \"fsck\" across a lot of repos).\n>\n> And then the backwards-incompatible Git 3.0 thing would just be tweaking\n> the severity of the config (and in the meantime, it would help flush out\n> any unexpected instances people run into).\n\nI came to make a same comment but the above has everything I wanted\nto say (and more).\n"},{"id":"517927","messageId":"aCLTsqZSWklaEOq6@pks.im","threadId":"63407","inReplyTo":"xmqqh61pu4r9.fsf@gitster.g","subject":"Re: [PATCH v3 1/3] packed-backend: fsck should allow an empty \"packed-refs\" file","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-13T05:08:02Z","receivedAt":"2025-05-13T05:08:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 12, 2025 at 10:18:34AM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > It may be useful for fsck to detect this, though, even if the default\n> > message severity is set to \"info\" or even \"ignore. That would allow\n> > people who know they are using modern Git to increase it themselves (I\n> > don't expect normal users to do this, but it would probably be useful\n> > for forges which run automated \"fsck\" across a lot of repos).\n> >\n> > And then the backwards-incompatible Git 3.0 thing would just be tweaking\n> > the severity of the config (and in the meantime, it would help flush out\n> > any unexpected instances people run into).\n> \n> I came to make a same comment but the above has everything I wanted\n> to say (and more).\n\nYup, agreed, that sounds like a reasonable approach indeed.\n\nPatrick\n"},{"id":"517928","messageId":"aCLs_A1DV7ZQSm-O@ArchLinux","threadId":"63407","inReplyTo":"20250512130619.GB1191360@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/3] packed-backend: extract snapshot allocation in `load_contents`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-13T06:55:56Z","receivedAt":"2025-05-13T06:55:27Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, May 12, 2025 at 09:06:19AM -0400, Jeff King wrote:\n> On Sun, May 11, 2025 at 10:01:50PM +0800, shejialuo wrote:\n> \n> > \"load_contents\" would choose which way to load the content of the\n> > \"packed-refs\". However, we cannot directly use this function when\n> > checking the consistency due to we don't want to open the file. And we\n> > also need to reuse the logic to avoid causing repetition.\n> > \n> > Let's create a new helper function \"allocate_snapshot_buffer\" to extract\n> > the snapshot allocation logic in \"load_contents\" and update the\n> > \"load_contents\" to align with the behavior.\n> \n> This looks good to me. One thing that did give me a slight pause while\n> reviewing:\n> \n> > static int load_contents(struct snapshot *snapshot)\n> > {\n> > -\tint fd;\n> > \tstruct stat st;\n> > -\tsize_t size;\n> > -\tssize_t bytes_read;\n> > +\tint ret = 1;\n> > [...]\n> > +\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n> > +\t\tret = 0;\n> > \n> > -\treturn 1;\n> > +\tclose(fd);\n> > +\treturn ret;\n> > }\n> \n> I wanted to see what the semantics of \"ret\" were, but there aren't any\n> other assignments. So I think this is equivalent to:\n> \n> int ret;\n> ...\n> ret = allocate_snapshot_buffer(snapshot, fd, &st);\n> \n> that function.\n> \n\nThat's right, it would be more clear. I will update in the next version.\n\n> Probably not worth re-rolling for that, though.\n> \n> -Peff\n"},{"id":"517929","messageId":"aCLvjNrZYYROlIm3@ArchLinux","threadId":"63407","inReplyTo":"aCLTsqZSWklaEOq6@pks.im","subject":"Re: [PATCH v3 1/3] packed-backend: fsck should allow an empty \"packed-refs\" file","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-13T07:06:52Z","receivedAt":"2025-05-13T07:06:22Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, May 13, 2025 at 07:08:02AM +0200, Patrick Steinhardt wrote:\n> On Mon, May 12, 2025 at 10:18:34AM -0700, Junio C Hamano wrote:\n> > Jeff King writes:\n> > \n> > > It may be useful for fsck to detect this, though, even if the default\n> > > message severity is set to \"info\" or even \"ignore. That would allow\n> > > people who know they are using modern Git to increase it themselves (I\n> > > don't expect normal users to do this, but it would probably be useful\n> > > for forges which run automated \"fsck\" across a lot of repos).\n> > >\n> > > And then the backwards-incompatible Git 3.0 thing would just be tweaking\n> > > the severity of the config (and in the meantime, it would help flush out\n> > > any unexpected instances people run into).\n> > \n> > I came to make a same comment but the above has everything I wanted\n> > to say (and more).\n> \n> Yup, agreed, that sounds like a reasonable approach indeed.\n> \n\nAgree, I will use a \"info\" to report an empty \"packed-refs\" file. Thank\neveryone.\n\n> Patrick\n\nJialuo\n"},{"id":"517936","messageId":"aCMnrwkoJ2WyqGZT@ArchLinux","threadId":"63407","inReplyTo":"aCCtQDnWII-knmEc@ArchLinux","subject":"[PATCH v4 0/3] align the behavior when opening \"packed-refs\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-13T11:06:23Z","receivedAt":"2025-05-13T11:05:54Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Hi All:\n\nAs discussed in [1], we need to use mmap mechanism to open large\n\"packed_refs\" file to save the memory usage. This patch mainly does the\nfollowing things:\n\n1: Fix an issue that we would report an error when the \"packed-refs\"\nfile is empty, which does not align with the runtime behavior.\n2-4: Extract some logic from the existing code and then use these\ncreated helper functions to let fsck code to use mmap necessarily\n\n[1] https://lore.kernel.org/git/20250503133158.GA4450@coredump.intra.peff.net\n\nReally thank Peff and Patrick to suggest me to do above change.\n\n---\n\nChange in v2:\n\n1. Update the commit message of [PATCH 1/4]. And use redirection to\ncreate an empty file instead of using `touch`.\n2. Don't use if for the refactored function in [PATCH 3/4] and then\nupdate the commit message to align with the new function name.\n3. Enhance the commit message of [PATCH 4/4].\n\n---\n\nChange in v3:\n\n1. Drop the patch which creates a new function\n\"munmap_temporary_snapshot\". As discussed, there is no need to munmap\nthe file during fsck.\n2. Allocate snapshot variable in the stack instead of heap.\n3. Fix rebase issue, remove unneeded code to check the file size\nexplicitly.\n\n---\n\nChange in v4:\n\n1. Report the \"emptyPackedRefsFile(INFO)\" to the user when the\n\"packed-refs\" is empty instead of ONLY skipping checking the content and\nupdate the shell script and commit message.\n2. Apply Peff's advice to make [PATCH v3 2/3] more clear.\n\n\nThanks,\nJialuo\n\nshejialuo (3):\n packed-backend: fsck should warn when \"packed-refs\" file is empty\n packed-backend: extract snapshot allocation in `load_contents`\n packed-backend: mmap large \"packed-refs\" file during fsck\n\n Documentation/fsck-msgids.adoc | 6 +++\n fsck.h | 1 +\n refs/packed-backend.c | 73 ++++++++++++++++++++--------------\n t/t0602-reffiles-fsck.sh | 17 ++++++++\n 4 files changed, 67 insertions(+), 30 deletions(-)\n\nRange-diff against v3:\n1: 26c3fd55a8 ! 1: 75636c9c85 packed-backend: fsck should allow an empty \"packed-refs\" file\n @@ Metadata\n Author: shejialuo \n \n ## Commit message ##\n - packed-backend: fsck should allow an empty \"packed-refs\" file\n + packed-backend: fsck should warn when \"packed-refs\" file is empty\n \n During fsck, an empty \"packed-refs\" gives an error; this is unwarranted.\n - We should just skip checking the content of \"packed-refs\" just like the\n - runtime code paths such as \"create_snapshot\" which simply returns the\n - \"snapshot\" without checking the content of \"packed-refs\".\n + The runtime code paths would accept an empty \"packed-refs\" file, such as\n + \"create_snapshot\" would simply return the \"snapshot\" without checking\n + the content of \"packed-refs\".\n +\n + And we should also skip checking the content of \"packed-refs\" when it is\n + empty during fsck. However, we should think about whether we need to\n + report something to the users in this case.\n +\n + After 694b7a1999 (repack_without_ref(): write peeled refs in the\n + rewritten file, 2013-04-22), we would always write a header into the\n + \"packed-refs\" file where we would never create empty file since then.\n + Because we only create empty \"packed-refs\" in the very early versions,\n + we may tighten this rule in the future. In order to notify the users\n + about this, we should at least report an warning to the users.\n +\n + But we need to consider the fsck message type carefully, it is not\n + appropriate that we use \"FSCK_ERROR\". This is because we would\n + definitely break the compatibility. Let's create a \"FSCK_INFO\" message\n + id EMPTY_PACKED_REFS_FILE\" to indicate that \"packed-refs\" is empty.\n \n Signed-off-by: shejialuo \n \n + ## Documentation/fsck-msgids.adoc ##\n +@@\n + `emptyName`::\n + \t(WARN) A path contains an empty name.\n + \n ++`emptyPackedRefsFile`::\n ++\t(INFO) \"packed-refs\" file is empty. Report to the\n ++\tgit@vger.kernel.org mailing list if you see this error. As only\n ++\tvery early versions of Git would create such an empty\n ++\t\"packed_refs\" file, we might tighten this rule in the future.\n ++\n + `extraHeaderEntry`::\n + \t(IGNORE) Extra headers found after `tagger`.\n + \n +\n + ## fsck.h ##\n +@@ fsck.h: enum fsck_msg_type {\n + \tFUNC(LARGE_PATHNAME, WARN) \\\n + \t/* infos (reported as warnings, but ignored by default) */ \\\n + \tFUNC(BAD_FILEMODE, INFO) \\\n ++\tFUNC(EMPTY_PACKED_REFS_FILE, INFO) \\\n + \tFUNC(GITMODULES_PARSE, INFO) \\\n + \tFUNC(GITIGNORE_SYMLINK, INFO) \\\n + \tFUNC(GITATTRIBUTES_SYMLINK, INFO) \\\n +\n ## refs/packed-backend.c ##\n @@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n -+\tif (!st.st_size)\n ++\tif (!st.st_size) {\n ++\t\tstruct fsck_ref_report report = { 0 };\n ++\t\treport.path = \"packed-refs\";\n ++\t\tret = fsck_report_ref(o, &report,\n ++\t\t\t\t FSCK_MSG_EMPTY_PACKED_REFS_FILE,\n ++\t\t\t\t \"file is empty\");\n +\t\tgoto cleanup;\n ++\t}\n +\n \tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n \t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n @@ t/t0602-reffiles-fsck.sh: test_expect_success SYMLINKS 'the filetype of packed-r\n \t)\n '\n \n -+test_expect_success 'empty packed-refs should not be reported' '\n ++test_expect_success 'empty packed-refs should be reported' '\n +\ttest_when_finished \"rm -rf repo\" &&\n +\tgit init repo &&\n +\t(\n @@ t/t0602-reffiles-fsck.sh: test_expect_success SYMLINKS 'the filetype of packed-r\n +\n +\t\t>.git/packed-refs &&\n +\t\tgit refs verify 2>err &&\n -+\t\ttest_must_be_empty err\n ++\t\tcat >expect <<-EOF &&\n ++\t\twarning: packed-refs: emptyPackedRefsFile: file is empty\n ++\t\tEOF\n ++\t\trm .git/packed-refs &&\n ++\t\ttest_cmp expect err\n +\t)\n +'\n +\n2: 4604be8b51 ! 2: 1a5893379d packed-backend: extract snapshot allocation in `load_contents`\n @@ refs/packed-backend.c: static int refname_contains_nul(struct strbuf *refname)\n \tstruct stat st;\n -\tsize_t size;\n -\tssize_t bytes_read;\n -+\tint ret = 1;\n ++\tint ret;\n +\tint fd;\n \n \tfd = open(snapshot->refs->path, O_RDONLY);\n @@ refs/packed-backend.c: static int load_contents(struct snapshot *snapshot)\n \n -\tsnapshot->start = snapshot->buf;\n -\tsnapshot->eof = snapshot->buf + size;\n -+\tif (!allocate_snapshot_buffer(snapshot, fd, &st))\n -+\t\tret = 0;\n ++\tret = allocate_snapshot_buffer(snapshot, fd, &st);\n \n -\treturn 1;\n +\tclose(fd);\n3: 82e19a65c6 ! 3: 31e272db7e packed-backend: mmap large \"packed-refs\" file during fsck\n @@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n --\tif (!st.st_size)\n -+\tif (!allocate_snapshot_buffer(&snapshot, fd, &st))\n +-\tif (!st.st_size) {\n ++\tif (!allocate_snapshot_buffer(&snapshot, fd, &st)) {\n + \t\tstruct fsck_ref_report report = { 0 };\n + \t\treport.path = \"packed-refs\";\n + \t\tret = fsck_report_ref(o, &report,\n +@@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n + \t}\n \n -\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n -\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n-- \n2.49.0\n\n"},{"id":"517937","messageId":"aCMn_Ktrg4GY8jHe@ArchLinux","threadId":"63407","inReplyTo":"aCMnrwkoJ2WyqGZT@ArchLinux","subject":"[PATCH v4 1/3] packed-backend: fsck should warn when \"packed-refs\" file is empty","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-13T11:07:40Z","receivedAt":"2025-05-13T11:07:21Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"During fsck, an empty \"packed-refs\" gives an error; this is unwarranted.\nThe runtime code paths would accept an empty \"packed-refs\" file, such as\n\"create_snapshot\" would simply return the \"snapshot\" without checking\nthe content of \"packed-refs\".\n\nAnd we should also skip checking the content of \"packed-refs\" when it is\nempty during fsck. However, we should think about whether we need to\nreport something to the users in this case.\n\nAfter 694b7a1999 (repack_without_ref(): write peeled refs in the\nrewritten file, 2013-04-22), we would always write a header into the\n\"packed-refs\" file where we would never create empty file since then.\nBecause we only create empty \"packed-refs\" in the very early versions,\nwe may tighten this rule in the future. In order to notify the users\nabout this, we should at least report an warning to the users.\n\nBut we need to consider the fsck message type carefully, it is not\nappropriate that we use \"FSCK_ERROR\". This is because we would\ndefinitely break the compatibility. Let's create a \"FSCK_INFO\" message\nid EMPTY_PACKED_REFS_FILE\" to indicate that \"packed-refs\" is empty.\n\nSigned-off-by: shejialuo \n---\n Documentation/fsck-msgids.adoc | 6 ++++++\n fsck.h | 1 +\n refs/packed-backend.c | 9 +++++++++\n t/t0602-reffiles-fsck.sh | 17 +++++++++++++++++\n 4 files changed, 33 insertions(+)\n\ndiff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc\nindex 9601fff228..0ba4f9a27e 100644\n--- a/Documentation/fsck-msgids.adoc\n+++ b/Documentation/fsck-msgids.adoc\n@@ -59,6 +59,12 @@\n `emptyName`::\n \t(WARN) A path contains an empty name.\n \n+`emptyPackedRefsFile`::\n+\t(INFO) \"packed-refs\" file is empty. Report to the\n+\tgit@vger.kernel.org mailing list if you see this error. As only\n+\tvery early versions of Git would create such an empty\n+\t\"packed_refs\" file, we might tighten this rule in the future.\n+\n `extraHeaderEntry`::\n \t(IGNORE) Extra headers found after `tagger`.\n \ndiff --git a/fsck.h b/fsck.h\nindex b1deae61ee..0c5869ac34 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -84,6 +84,7 @@ enum fsck_msg_type {\n \tFUNC(LARGE_PATHNAME, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(BAD_FILEMODE, INFO) \\\n+\tFUNC(EMPTY_PACKED_REFS_FILE, INFO) \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\n \tFUNC(GITIGNORE_SYMLINK, INFO) \\\n \tFUNC(GITATTRIBUTES_SYMLINK, INFO) \\\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 3ad1ed0787..fb91833e76 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2103,6 +2103,15 @@ static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n+\tif (!st.st_size) {\n+\t\tstruct fsck_ref_report report = { 0 };\n+\t\treport.path = \"packed-refs\";\n+\t\tret = fsck_report_ref(o, &report,\n+\t\t\t\t FSCK_MSG_EMPTY_PACKED_REFS_FILE,\n+\t\t\t\t \"file is empty\");\n+\t\tgoto cleanup;\n+\t}\n+\n \tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n \t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n \t\tgoto cleanup;\ndiff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\nindex 9d1dc2144c..f671ac4d3a 100755\n--- a/t/t0602-reffiles-fsck.sh\n+++ b/t/t0602-reffiles-fsck.sh\n@@ -647,6 +647,23 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '\n \t)\n '\n \n+test_expect_success 'empty packed-refs should be reported' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit default &&\n+\n+\t\t>.git/packed-refs &&\n+\t\tgit refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\twarning: packed-refs: emptyPackedRefsFile: file is empty\n+\t\tEOF\n+\t\trm .git/packed-refs &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_expect_success 'packed-refs header should be checked' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.49.0\n\n"},{"id":"517938","messageId":"aCMoD-c_oHlu0c5c@ArchLinux","threadId":"63407","inReplyTo":"aCMnrwkoJ2WyqGZT@ArchLinux","subject":"[PATCH v4 3/3] packed-backend: mmap large \"packed-refs\" file during fsck","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-13T11:07:59Z","receivedAt":"2025-05-13T11:07:30Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"During fsck, we use \"strbuf_read\" to read the content of \"packed-refs\"\nwithout using mmap mechanism. This is a bad practice which would consume\nmore memory than using mmap mechanism. Besides, as all code paths in\n\"packed-backend.c\" use this way, we should make \"fsck\" align with the\ncurrent codebase.\n\nAs we have introduced the helper function \"allocate_snapshot_buffer\", we\ncould simple use this function to use mmap mechanism.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 19 +++++++------------\n 1 file changed, 7 insertions(+), 12 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 1da44a3d6d..7fd73a0e6d 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2068,7 +2068,7 @@ static int packed_fsck(struct ref_store *ref_store,\n {\n \tstruct packed_ref_store *refs = packed_downcast(ref_store,\n \t\t\t\t\t\t\tREF_STORE_READ, \"fsck\");\n-\tstruct strbuf packed_ref_content = STRBUF_INIT;\n+\tstruct snapshot snapshot = { 0 };\n \tunsigned int sorted = 0;\n \tstruct stat st;\n \tint ret = 0;\n@@ -2112,7 +2112,7 @@ static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n-\tif (!st.st_size) {\n+\tif (!allocate_snapshot_buffer(&snapshot, fd, &st)) {\n \t\tstruct fsck_ref_report report = { 0 };\n \t\treport.path = \"packed-refs\";\n \t\tret = fsck_report_ref(o, &report,\n@@ -2121,21 +2121,16 @@ static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n-\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n-\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n-\t\tgoto cleanup;\n-\t}\n-\n-\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n-\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n+\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot.start,\n+\t\t\t\t snapshot.eof);\n \tif (!ret && sorted)\n-\t\tret = packed_fsck_ref_sorted(o, ref_store, packed_ref_content.buf,\n-\t\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n+\t\tret = packed_fsck_ref_sorted(o, ref_store, snapshot.start,\n+\t\t\t\t\t snapshot.eof);\n \n cleanup:\n \tif (fd >= 0)\n \t\tclose(fd);\n-\tstrbuf_release(&packed_ref_content);\n+\tclear_snapshot_buffer(&snapshot);\n \treturn ret;\n }\n \n-- \n2.49.0\n\n"},{"id":"517939","messageId":"aCMoBMajlgYOVB9w@ArchLinux","threadId":"63407","inReplyTo":"aCMnrwkoJ2WyqGZT@ArchLinux","subject":"[PATCH v4 2/3] packed-backend: extract snapshot allocation in `load_contents`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-13T11:07:48Z","receivedAt":"2025-05-13T11:07:30Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"\"load_contents\" would choose which way to load the content of the\n\"packed-refs\". However, we cannot directly use this function when\nchecking the consistency due to we don't want to open the file. And we\nalso need to reuse the logic to avoid causing repetition.\n\nLet's create a new helper function \"allocate_snapshot_buffer\" to extract\nthe snapshot allocation logic in \"load_contents\" and update the\n\"load_contents\" to align with the behavior.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 53 +++++++++++++++++++++++++------------------\n 1 file changed, 31 insertions(+), 22 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex fb91833e76..1da44a3d6d 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -517,6 +517,32 @@ static int refname_contains_nul(struct strbuf *refname)\n \n #define SMALL_FILE_SIZE (32*1024)\n \n+static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)\n+{\n+\tssize_t bytes_read;\n+\tsize_t size;\n+\n+\tsize = xsize_t(st->st_size);\n+\tif (!size)\n+\t\treturn 0;\n+\n+\tif (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n+\t\tsnapshot->buf = xmalloc(size);\n+\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n+\t\tif (bytes_read < 0 || bytes_read != size)\n+\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n+\t\tsnapshot->mmapped = 0;\n+\t} else {\n+\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\tsnapshot->mmapped = 1;\n+\t}\n+\n+\tsnapshot->start = snapshot->buf;\n+\tsnapshot->eof = snapshot->buf + size;\n+\n+\treturn 1;\n+}\n+\n /*\n * Depending on `mmap_strategy`, either mmap or read the contents of\n * the `packed-refs` file into the snapshot. Return 1 if the file\n@@ -525,10 +551,9 @@ static int refname_contains_nul(struct strbuf *refname)\n */\n static int load_contents(struct snapshot *snapshot)\n {\n-\tint fd;\n \tstruct stat st;\n-\tsize_t size;\n-\tssize_t bytes_read;\n+\tint ret;\n+\tint fd;\n \n \tfd = open(snapshot->refs->path, O_RDONLY);\n \tif (fd < 0) {\n@@ -550,27 +575,11 @@ static int load_contents(struct snapshot *snapshot)\n \n \tif (fstat(fd, &st) < 0)\n \t\tdie_errno(\"couldn't stat %s\", snapshot->refs->path);\n-\tsize = xsize_t(st.st_size);\n-\n-\tif (!size) {\n-\t\tclose(fd);\n-\t\treturn 0;\n-\t} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n-\t\tsnapshot->buf = xmalloc(size);\n-\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n-\t\tif (bytes_read < 0 || bytes_read != size)\n-\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n-\t\tsnapshot->mmapped = 0;\n-\t} else {\n-\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\tsnapshot->mmapped = 1;\n-\t}\n-\tclose(fd);\n \n-\tsnapshot->start = snapshot->buf;\n-\tsnapshot->eof = snapshot->buf + size;\n+\tret = allocate_snapshot_buffer(snapshot, fd, &st);\n \n-\treturn 1;\n+\tclose(fd);\n+\treturn ret;\n }\n \n static const char *find_reference_location_1(struct snapshot *snapshot,\n-- \n2.49.0\n\n"},{"id":"517954","messageId":"xmqqplgch3r2.fsf@gitster.g","threadId":"63407","inReplyTo":"aCMn_Ktrg4GY8jHe@ArchLinux","subject":"Re: [PATCH v4 1/3] packed-backend: fsck should warn when \"packed-refs\" file is empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-13T16:30:57Z","receivedAt":"2025-05-13T16:31:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> During fsck, an empty \"packed-refs\" gives an error; this is unwarranted.\n> The runtime code paths would accept an empty \"packed-refs\" file, such as\n> \"create_snapshot\" would simply return the \"snapshot\" without checking\n> the content of \"packed-refs\".\n\nPerhaps \"unwarranted\" is now too strong a word; we still want to\nconsider it an anomaly (that is why emptyPackedRefsFile warning is\nintroduced after all). I think the problem description you want to\nhere in the above pragraph is that fsck giving an error and runtime\ncompletely silent is inconsistent. \n\n Side note: and you'd probably want to say what \"an error\"\n reported here is. The problem, if I understand correctly, is\n that the code assumes the file won't be empty and instead has at\n least one line in it (even when there are no refs packed, there\n is the file header line) and insists that all lines must be well\n terminated---if we tolerate an empty file, of course such a\n check will fail, as there is no terminating LF in a file with 0\n lines in it.\n\nAnd because versions of Git that are not too ancient never wrote an\nempty packed-refs file, and often having an empty file there is/was\na sign of a filesystem-level issue, the way we want resolve this\ninconsistency is not make everybody totally silent but notice and\nreport the anomaly.\n\n> But we need to consider the fsck message type carefully, it is not\n> appropriate that we use \"FSCK_ERROR\". This is because we would\n> definitely break the compatibility. Let's create a \"FSCK_INFO\" message\n> id EMPTY_PACKED_REFS_FILE\" to indicate that \"packed-refs\" is empty.\n\nOK.\n\n> Signed-off-by: shejialuo \n> ---\n> Documentation/fsck-msgids.adoc | 6 ++++++\n> fsck.h | 1 +\n> refs/packed-backend.c | 9 +++++++++\n> t/t0602-reffiles-fsck.sh | 17 +++++++++++++++++\n> 4 files changed, 33 insertions(+)\n>\n> diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc\n> index 9601fff228..0ba4f9a27e 100644\n> --- a/Documentation/fsck-msgids.adoc\n> +++ b/Documentation/fsck-msgids.adoc\n> @@ -59,6 +59,12 @@\n> `emptyName`::\n> \t(WARN) A path contains an empty name.\n> \n> +`emptyPackedRefsFile`::\n> +\t(INFO) \"packed-refs\" file is empty. Report to the\n> +\tgit@vger.kernel.org mailing list if you see this error. As only\n> +\tvery early versions of Git would create such an empty\n> +\t\"packed_refs\" file, we might tighten this rule in the future.\n\nI am not too happy to see \"Report to ...\" and everything after that\nhere, primarily because it takes one extra step for the user to find\nit out when they see such an informational message. There are other\nexisting error classes, like refMissingNewline, etc., that have the\nsame problem. One thing to make it easier for the users to report\nis to put it in the error/info messages themselves, but I think it\nis OK to make such a clean-up (including the existing offenders)\nafter the dust settles from this topic.\n\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index 3ad1ed0787..fb91833e76 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -2103,6 +2103,15 @@ static int packed_fsck(struct ref_store *ref_store,\n> \t\tgoto cleanup;\n> \t}\n> \n> +\tif (!st.st_size) {\n> +\t\tstruct fsck_ref_report report = { 0 };\n> +\t\treport.path = \"packed-refs\";\n> +\t\tret = fsck_report_ref(o, &report,\n> +\t\t\t\t FSCK_MSG_EMPTY_PACKED_REFS_FILE,\n> +\t\t\t\t \"file is empty\");\n> +\t\tgoto cleanup;\n> +\t}\n\nOK.\n\n> diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\n> index 9d1dc2144c..f671ac4d3a 100755\n> --- a/t/t0602-reffiles-fsck.sh\n> +++ b/t/t0602-reffiles-fsck.sh\n> @@ -647,6 +647,23 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '\n> \t)\n> '\n> \n> +test_expect_success 'empty packed-refs should be reported' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit default &&\n> +\n> +\t\t>.git/packed-refs &&\n> +\t\tgit refs verify 2>err &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\twarning: packed-refs: emptyPackedRefsFile: file is empty\n> +\t\tEOF\n> +\t\trm .git/packed-refs &&\n> +\t\ttest_cmp expect err\n> +\t)\n> +'\n> +\n> test_expect_success 'packed-refs header should be checked' '\n> \ttest_when_finished \"rm -rf repo\" &&\n> \tgit init repo &&\n"},{"id":"517955","messageId":"xmqqcycch2t3.fsf@gitster.g","threadId":"63407","inReplyTo":"aCMoD-c_oHlu0c5c@ArchLinux","subject":"Re: [PATCH v4 3/3] packed-backend: mmap large \"packed-refs\" file during fsck","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-13T16:51:20Z","receivedAt":"2025-05-13T16:51:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> During fsck, we use \"strbuf_read\" to read the content of \"packed-refs\"\n> without using mmap mechanism. This is a bad practice which would consume\n> more memory than using mmap mechanism. Besides, as all code paths in\n> \"packed-backend.c\" use this way, we should make \"fsck\" align with the\n> current codebase.\n>\n> As we have introduced the helper function \"allocate_snapshot_buffer\", we\n> could simple use this function to use mmap mechanism.\n\n\"could simple\" -> \"can simply\".\n\n> Suggested-by: Jeff King \n> Suggested-by: Patrick Steinhardt \n> Signed-off-by: shejialuo \n> ---\n> refs/packed-backend.c | 19 +++++++------------\n> 1 file changed, 7 insertions(+), 12 deletions(-)\n\nNice loss of line count ;-)\n\n> -\tif (!st.st_size) {\n> +\tif (!allocate_snapshot_buffer(&snapshot, fd, &st)) {\n\nIt is a bit funny to see that a helper function that works at a much\nhigher conceptual level treat an empty file so specially (namely,\nshould it be different from a header-only packed-refs file?). If I\nwere doing this refactoring in 2 & 3, I would probalby have made the\nhelper return \"void\", and have callers who do care about st.st_size\ncheck that themselves.\n\nBut I'll let it pass.\n\n> @@ -2121,21 +2121,16 @@ static int packed_fsck(struct ref_store *ref_store,\n> \t\tgoto cleanup;\n> \t}\n> \n> -\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n> -\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n> -\t\tgoto cleanup;\n> -\t}\n> -\n> -\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n> -\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n> +\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot.start,\n> +\t\t\t\t snapshot.eof);\n> \tif (!ret && sorted)\n> -\t\tret = packed_fsck_ref_sorted(o, ref_store, packed_ref_content.buf,\n> -\t\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n> +\t\tret = packed_fsck_ref_sorted(o, ref_store, snapshot.start,\n> +\t\t\t\t\t snapshot.eof);\n> \n> cleanup:\n> \tif (fd >= 0)\n> \t\tclose(fd);\n> -\tstrbuf_release(&packed_ref_content);\n> +\tclear_snapshot_buffer(&snapshot);\n> \treturn ret;\n> }\n"},{"id":"518029","messageId":"aCSR7wju99Eszv8d@ArchLinux","threadId":"63407","inReplyTo":"xmqqplgch3r2.fsf@gitster.g","subject":"Re: [PATCH v4 1/3] packed-backend: fsck should warn when \"packed-refs\" file is empty","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-14T12:51:59Z","receivedAt":"2025-05-14T12:51:39Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, May 13, 2025 at 09:30:57AM -0700, Junio C Hamano wrote:\n> shejialuo writes:\n> \n> > During fsck, an empty \"packed-refs\" gives an error; this is unwarranted.\n> > The runtime code paths would accept an empty \"packed-refs\" file, such as\n> > \"create_snapshot\" would simply return the \"snapshot\" without checking\n> > the content of \"packed-refs\".\n> \n> Perhaps \"unwarranted\" is now too strong a word; we still want to\n> consider it an anomaly (that is why emptyPackedRefsFile warning is\n> introduced after all). I think the problem description you want to\n> here in the above pragraph is that fsck giving an error and runtime\n> completely silent is inconsistent. \n> \n> Side note: and you'd probably want to say what \"an error\"\n> reported here is. The problem, if I understand correctly, is\n> that the code assumes the file won't be empty and instead has at\n> least one line in it (even when there are no refs packed, there\n> is the file header line) and insists that all lines must be well\n> terminated---if we tolerate an empty file, of course such a\n> check will fail, as there is no terminating LF in a file with 0\n> lines in it.\n> \n\nGood idea, I will improve the commit message.\n\n> And because versions of Git that are not too ancient never wrote an\n> empty packed-refs file, and often having an empty file there is/was\n> a sign of a filesystem-level issue, the way we want resolve this\n> inconsistency is not make everybody totally silent but notice and\n> report the anomaly.\n> \n\nThat's right. I should talk about this problem.\n\n> > But we need to consider the fsck message type carefully, it is not\n> > appropriate that we use \"FSCK_ERROR\". This is because we would\n> > definitely break the compatibility. Let's create a \"FSCK_INFO\" message\n> > id EMPTY_PACKED_REFS_FILE\" to indicate that \"packed-refs\" is empty.\n> \n> OK.\n> \n> > Signed-off-by: shejialuo \n> > ---\n> > Documentation/fsck-msgids.adoc | 6 ++++++\n> > fsck.h | 1 +\n> > refs/packed-backend.c | 9 +++++++++\n> > t/t0602-reffiles-fsck.sh | 17 +++++++++++++++++\n> > 4 files changed, 33 insertions(+)\n> >\n> > diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc\n> > index 9601fff228..0ba4f9a27e 100644\n> > --- a/Documentation/fsck-msgids.adoc\n> > +++ b/Documentation/fsck-msgids.adoc\n> > @@ -59,6 +59,12 @@\n> > `emptyName`::\n> > \t(WARN) A path contains an empty name.\n> > \n> > +`emptyPackedRefsFile`::\n> > +\t(INFO) \"packed-refs\" file is empty. Report to the\n> > +\tgit@vger.kernel.org mailing list if you see this error. As only\n> > +\tvery early versions of Git would create such an empty\n> > +\t\"packed_refs\" file, we might tighten this rule in the future.\n> \n> I am not too happy to see \"Report to ...\" and everything after that\n> here, primarily because it takes one extra step for the user to find\n> it out when they see such an informational message. There are other\n> existing error classes, like refMissingNewline, etc., that have the\n> same problem. One thing to make it easier for the users to report\n> is to put it in the error/info messages themselves, but I think it\n> is OK to make such a clean-up (including the existing offenders)\n> after the dust settles from this topic.\n> \n\nThat's right. Actually we introduce redirection here. I think I will\nimprove this in the next release cycle. I'll add this into my TODO list.\n"},{"id":"518032","messageId":"aCSU_unPGsUAZ0CM@ArchLinux","threadId":"63407","inReplyTo":"xmqqcycch2t3.fsf@gitster.g","subject":"Re: [PATCH v4 3/3] packed-backend: mmap large \"packed-refs\" file during fsck","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-14T13:05:02Z","receivedAt":"2025-05-14T13:04:42Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, May 13, 2025 at 09:51:20AM -0700, Junio C Hamano wrote:\n> shejialuo writes:\n> \n> > During fsck, we use \"strbuf_read\" to read the content of \"packed-refs\"\n> > without using mmap mechanism. This is a bad practice which would consume\n> > more memory than using mmap mechanism. Besides, as all code paths in\n> > \"packed-backend.c\" use this way, we should make \"fsck\" align with the\n> > current codebase.\n> >\n> > As we have introduced the helper function \"allocate_snapshot_buffer\", we\n> > could simple use this function to use mmap mechanism.\n> \n> \"could simple\" -> \"can simply\".\n> \n\nNice catch.\n\n> > Suggested-by: Jeff King \n> > Suggested-by: Patrick Steinhardt \n> > Signed-off-by: shejialuo \n> > ---\n> > refs/packed-backend.c | 19 +++++++------------\n> > 1 file changed, 7 insertions(+), 12 deletions(-)\n> \n> Nice loss of line count ;-)\n> \n> > -\tif (!st.st_size) {\n> > +\tif (!allocate_snapshot_buffer(&snapshot, fd, &st)) {\n> \n> It is a bit funny to see that a helper function that works at a much\n> higher conceptual level treat an empty file so specially (namely,\n> should it be different from a header-only packed-refs file?). If I\n> were doing this refactoring in 2 & 3, I would probalby have made the\n> helper return \"void\", and have callers who do care about st.st_size\n> check that themselves.\n> \n\nThis is a good question. Actually, I intentionally design this. In the\nvery first implementation, I use above way. But later I think this is\ninappropriate to let callers check `st.st_size`.\n\nThe reason is that we would do the same thing in \"load_content\" function\nand fsck function. But after thinking, I think we should make the\nruntime behavior and fsck consistent. That's our goal. And by using\nthis, let's say if one day we want to tighten the rule in the future, we\ncould just change `allocate_snapshot_buffer`.\n\nI agree that introducing side effect into a function is not a good\ndesign. But in this situation, we could make things consistent.\n\nThanks,\nJialuo\n"},{"id":"518051","messageId":"aCS7O8tNekg_u9Wp@ArchLinux","threadId":"63407","inReplyTo":"aCMnrwkoJ2WyqGZT@ArchLinux","subject":"[PATCH v5 0/3] align the behavior when opening \"packed-refs\"","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-14T15:48:11Z","receivedAt":"2025-05-14T15:47:41Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Hi All:\n\nAs discussed in [1], we need to use mmap mechanism to open large\n\"packed_refs\" file to save the memory usage. This patch mainly does the\nfollowing things:\n\n1: Fix an issue that we would report an error when the \"packed-refs\"\nfile is empty, which does not align with the runtime behavior.\n2-4: Extract some logic from the existing code and then use these\ncreated helper functions to let fsck code to use mmap necessarily\n\n[1] https://lore.kernel.org/git/20250503133158.GA4450@coredump.intra.peff.net\n\nReally thank Peff and Patrick to suggest me to do above change.\n\n---\n\nChange in v2:\n\n1. Update the commit message of [PATCH 1/4]. And use redirection to\ncreate an empty file instead of using `touch`.\n2. Don't use if for the refactored function in [PATCH 3/4] and then\nupdate the commit message to align with the new function name.\n3. Enhance the commit message of [PATCH 4/4].\n\n---\n\nChange in v3:\n\n1. Drop the patch which creates a new function\n\"munmap_temporary_snapshot\". As discussed, there is no need to munmap\nthe file during fsck.\n2. Allocate snapshot variable in the stack instead of heap.\n3. Fix rebase issue, remove unneeded code to check the file size\nexplicitly.\n\n---\n\nChange in v4:\n\n1. Report the \"emptyPackedRefsFile(INFO)\" to the user when the\n\"packed-refs\" is empty instead of ONLY skipping checking the content and\nupdate the shell script and commit message.\n2. Apply Peff's advice to make [PATCH v3 2/3] more clear.\n\n---\n\nChange in v5:\n\n1. Improve the commit message in the first patch to be more clear:\n 1. Talk about the current behavior, what error we would report if\n \"packed-refs\" is empty.\n 2. To align with the runtime behavior, we should skip checking the\n content of \"packed-refs\".\n 3. Why do we need to report to the user when the \"packed-refs\" is\n empty\n2. Fix grammar issue in the last patch.\n\nThanks,\nJialuo\n\nshejialuo (3):\n packed-backend: fsck should warn when \"packed-refs\" file is empty\n packed-backend: extract snapshot allocation in `load_contents`\n packed-backend: mmap large \"packed-refs\" file during fsck\n\n Documentation/fsck-msgids.adoc | 6 +++\n fsck.h | 1 +\n refs/packed-backend.c | 73 ++++++++++++++++++++--------------\n t/t0602-reffiles-fsck.sh | 17 ++++++++\n 4 files changed, 67 insertions(+), 30 deletions(-)\n\nRange-diff against v4:\n1: 75636c9c85 ! 1: 3487692a03 packed-backend: fsck should warn when \"packed-refs\" file is empty\n @@ Metadata\n ## Commit message ##\n packed-backend: fsck should warn when \"packed-refs\" file is empty\n \n - During fsck, an empty \"packed-refs\" gives an error; this is unwarranted.\n - The runtime code paths would accept an empty \"packed-refs\" file, such as\n - \"create_snapshot\" would simply return the \"snapshot\" without checking\n - the content of \"packed-refs\".\n + We assume the \"packed-refs\" won't be empty and instead has at least one\n + line in it (even when there are no refs packed, there is the file header\n + line). Because there is no terminating LF in the empty file, we will\n + report \"packedRefEntryNotTerminated(ERROR)\" to the user.\n \n - And we should also skip checking the content of \"packed-refs\" when it is\n - empty during fsck. However, we should think about whether we need to\n - report something to the users in this case.\n + However, the runtime code paths would accept an empty \"packed-refs\"\n + file, for example, \"create_snapshot\" would simply return the \"snapshot\"\n + without checking the content of \"packed-refs\". So, we should skip\n + checking the content of \"packed-refs\" when it is empty during fsck.\n \n After 694b7a1999 (repack_without_ref(): write peeled refs in the\n rewritten file, 2013-04-22), we would always write a header into the\n - \"packed-refs\" file where we would never create empty file since then.\n - Because we only create empty \"packed-refs\" in the very early versions,\n - we may tighten this rule in the future. In order to notify the users\n - about this, we should at least report an warning to the users.\n + \"packed-refs\" file. So, versions of Git that are not too ancient never\n + write such an empty \"packed-refs\" file.\n \n - But we need to consider the fsck message type carefully, it is not\n - appropriate that we use \"FSCK_ERROR\". This is because we would\n - definitely break the compatibility. Let's create a \"FSCK_INFO\" message\n - id EMPTY_PACKED_REFS_FILE\" to indicate that \"packed-refs\" is empty.\n + As an empty file often indicates a sign of a filesystem-level issue, the\n + way we want to resolve this inconsistency is not make everybody totally\n + silent but notice and report the anomaly.\n +\n + Let's create a \"FSCK_INFO\" message id \"EMPTY_PACKED_REFS_FILE\" to report\n + to the users that \"packed-refs\" is empty.\n \n Signed-off-by: shejialuo \n \n2: 1a5893379d = 2: 0d050849bc packed-backend: extract snapshot allocation in `load_contents`\n3: 31e272db7e ! 3: fe5ffec8fb packed-backend: mmap large \"packed-refs\" file during fsck\n @@ Commit message\n current codebase.\n \n As we have introduced the helper function \"allocate_snapshot_buffer\", we\n - could simple use this function to use mmap mechanism.\n + can simply use this function to use mmap mechanism.\n \n Suggested-by: Jeff King \n Suggested-by: Patrick Steinhardt \n-- \n2.49.0\n\n"},{"id":"518052","messageId":"aCS7y24E22TRzFV7@ArchLinux","threadId":"63407","inReplyTo":"aCS7O8tNekg_u9Wp@ArchLinux","subject":"[PATCH v5 2/3] packed-backend: extract snapshot allocation in `load_contents`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-14T15:50:35Z","receivedAt":"2025-05-14T15:50:04Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"\"load_contents\" would choose which way to load the content of the\n\"packed-refs\". However, we cannot directly use this function when\nchecking the consistency due to we don't want to open the file. And we\nalso need to reuse the logic to avoid causing repetition.\n\nLet's create a new helper function \"allocate_snapshot_buffer\" to extract\nthe snapshot allocation logic in \"load_contents\" and update the\n\"load_contents\" to align with the behavior.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 53 +++++++++++++++++++++++++------------------\n 1 file changed, 31 insertions(+), 22 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex fb91833e76..1da44a3d6d 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -517,6 +517,32 @@ static int refname_contains_nul(struct strbuf *refname)\n \n #define SMALL_FILE_SIZE (32*1024)\n \n+static int allocate_snapshot_buffer(struct snapshot *snapshot, int fd, struct stat *st)\n+{\n+\tssize_t bytes_read;\n+\tsize_t size;\n+\n+\tsize = xsize_t(st->st_size);\n+\tif (!size)\n+\t\treturn 0;\n+\n+\tif (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n+\t\tsnapshot->buf = xmalloc(size);\n+\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n+\t\tif (bytes_read < 0 || bytes_read != size)\n+\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n+\t\tsnapshot->mmapped = 0;\n+\t} else {\n+\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\tsnapshot->mmapped = 1;\n+\t}\n+\n+\tsnapshot->start = snapshot->buf;\n+\tsnapshot->eof = snapshot->buf + size;\n+\n+\treturn 1;\n+}\n+\n /*\n * Depending on `mmap_strategy`, either mmap or read the contents of\n * the `packed-refs` file into the snapshot. Return 1 if the file\n@@ -525,10 +551,9 @@ static int refname_contains_nul(struct strbuf *refname)\n */\n static int load_contents(struct snapshot *snapshot)\n {\n-\tint fd;\n \tstruct stat st;\n-\tsize_t size;\n-\tssize_t bytes_read;\n+\tint ret;\n+\tint fd;\n \n \tfd = open(snapshot->refs->path, O_RDONLY);\n \tif (fd < 0) {\n@@ -550,27 +575,11 @@ static int load_contents(struct snapshot *snapshot)\n \n \tif (fstat(fd, &st) < 0)\n \t\tdie_errno(\"couldn't stat %s\", snapshot->refs->path);\n-\tsize = xsize_t(st.st_size);\n-\n-\tif (!size) {\n-\t\tclose(fd);\n-\t\treturn 0;\n-\t} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n-\t\tsnapshot->buf = xmalloc(size);\n-\t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n-\t\tif (bytes_read < 0 || bytes_read != size)\n-\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n-\t\tsnapshot->mmapped = 0;\n-\t} else {\n-\t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\tsnapshot->mmapped = 1;\n-\t}\n-\tclose(fd);\n \n-\tsnapshot->start = snapshot->buf;\n-\tsnapshot->eof = snapshot->buf + size;\n+\tret = allocate_snapshot_buffer(snapshot, fd, &st);\n \n-\treturn 1;\n+\tclose(fd);\n+\treturn ret;\n }\n \n static const char *find_reference_location_1(struct snapshot *snapshot,\n-- \n2.49.0\n\n"},{"id":"518053","messageId":"aCS7wk-ZCBRiHMqF@ArchLinux","threadId":"63407","inReplyTo":"aCS7O8tNekg_u9Wp@ArchLinux","subject":"[PATCH v5 1/3] packed-backend: fsck should warn when \"packed-refs\" file is empty","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-14T15:50:26Z","receivedAt":"2025-05-14T15:50:05Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"We assume the \"packed-refs\" won't be empty and instead has at least one\nline in it (even when there are no refs packed, there is the file header\nline). Because there is no terminating LF in the empty file, we will\nreport \"packedRefEntryNotTerminated(ERROR)\" to the user.\n\nHowever, the runtime code paths would accept an empty \"packed-refs\"\nfile, for example, \"create_snapshot\" would simply return the \"snapshot\"\nwithout checking the content of \"packed-refs\". So, we should skip\nchecking the content of \"packed-refs\" when it is empty during fsck.\n\nAfter 694b7a1999 (repack_without_ref(): write peeled refs in the\nrewritten file, 2013-04-22), we would always write a header into the\n\"packed-refs\" file. So, versions of Git that are not too ancient never\nwrite such an empty \"packed-refs\" file.\n\nAs an empty file often indicates a sign of a filesystem-level issue, the\nway we want to resolve this inconsistency is not make everybody totally\nsilent but notice and report the anomaly.\n\nLet's create a \"FSCK_INFO\" message id \"EMPTY_PACKED_REFS_FILE\" to report\nto the users that \"packed-refs\" is empty.\n\nSigned-off-by: shejialuo \n---\n Documentation/fsck-msgids.adoc | 6 ++++++\n fsck.h | 1 +\n refs/packed-backend.c | 9 +++++++++\n t/t0602-reffiles-fsck.sh | 17 +++++++++++++++++\n 4 files changed, 33 insertions(+)\n\ndiff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc\nindex 9601fff228..0ba4f9a27e 100644\n--- a/Documentation/fsck-msgids.adoc\n+++ b/Documentation/fsck-msgids.adoc\n@@ -59,6 +59,12 @@\n `emptyName`::\n \t(WARN) A path contains an empty name.\n \n+`emptyPackedRefsFile`::\n+\t(INFO) \"packed-refs\" file is empty. Report to the\n+\tgit@vger.kernel.org mailing list if you see this error. As only\n+\tvery early versions of Git would create such an empty\n+\t\"packed_refs\" file, we might tighten this rule in the future.\n+\n `extraHeaderEntry`::\n \t(IGNORE) Extra headers found after `tagger`.\n \ndiff --git a/fsck.h b/fsck.h\nindex b1deae61ee..0c5869ac34 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -84,6 +84,7 @@ enum fsck_msg_type {\n \tFUNC(LARGE_PATHNAME, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(BAD_FILEMODE, INFO) \\\n+\tFUNC(EMPTY_PACKED_REFS_FILE, INFO) \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\n \tFUNC(GITIGNORE_SYMLINK, INFO) \\\n \tFUNC(GITATTRIBUTES_SYMLINK, INFO) \\\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 3ad1ed0787..fb91833e76 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2103,6 +2103,15 @@ static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n+\tif (!st.st_size) {\n+\t\tstruct fsck_ref_report report = { 0 };\n+\t\treport.path = \"packed-refs\";\n+\t\tret = fsck_report_ref(o, &report,\n+\t\t\t\t FSCK_MSG_EMPTY_PACKED_REFS_FILE,\n+\t\t\t\t \"file is empty\");\n+\t\tgoto cleanup;\n+\t}\n+\n \tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n \t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n \t\tgoto cleanup;\ndiff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\nindex 9d1dc2144c..f671ac4d3a 100755\n--- a/t/t0602-reffiles-fsck.sh\n+++ b/t/t0602-reffiles-fsck.sh\n@@ -647,6 +647,23 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '\n \t)\n '\n \n+test_expect_success 'empty packed-refs should be reported' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit default &&\n+\n+\t\t>.git/packed-refs &&\n+\t\tgit refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\twarning: packed-refs: emptyPackedRefsFile: file is empty\n+\t\tEOF\n+\t\trm .git/packed-refs &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_expect_success 'packed-refs header should be checked' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.49.0\n\n"},{"id":"518054","messageId":"aCS70gT90mBNqL4V@ArchLinux","threadId":"63407","inReplyTo":"aCS7O8tNekg_u9Wp@ArchLinux","subject":"[PATCH v5 3/3] packed-backend: mmap large \"packed-refs\" file during fsck","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-05-14T15:50:42Z","receivedAt":"2025-05-14T15:50:11Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"During fsck, we use \"strbuf_read\" to read the content of \"packed-refs\"\nwithout using mmap mechanism. This is a bad practice which would consume\nmore memory than using mmap mechanism. Besides, as all code paths in\n\"packed-backend.c\" use this way, we should make \"fsck\" align with the\ncurrent codebase.\n\nAs we have introduced the helper function \"allocate_snapshot_buffer\", we\ncan simply use this function to use mmap mechanism.\n\nSuggested-by: Jeff King \nSuggested-by: Patrick Steinhardt \nSigned-off-by: shejialuo \n---\n refs/packed-backend.c | 19 +++++++------------\n 1 file changed, 7 insertions(+), 12 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 1da44a3d6d..7fd73a0e6d 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2068,7 +2068,7 @@ static int packed_fsck(struct ref_store *ref_store,\n {\n \tstruct packed_ref_store *refs = packed_downcast(ref_store,\n \t\t\t\t\t\t\tREF_STORE_READ, \"fsck\");\n-\tstruct strbuf packed_ref_content = STRBUF_INIT;\n+\tstruct snapshot snapshot = { 0 };\n \tunsigned int sorted = 0;\n \tstruct stat st;\n \tint ret = 0;\n@@ -2112,7 +2112,7 @@ static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n-\tif (!st.st_size) {\n+\tif (!allocate_snapshot_buffer(&snapshot, fd, &st)) {\n \t\tstruct fsck_ref_report report = { 0 };\n \t\treport.path = \"packed-refs\";\n \t\tret = fsck_report_ref(o, &report,\n@@ -2121,21 +2121,16 @@ static int packed_fsck(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n-\tif (strbuf_read(&packed_ref_content, fd, 0) < 0) {\n-\t\tret = error_errno(_(\"unable to read '%s'\"), refs->path);\n-\t\tgoto cleanup;\n-\t}\n-\n-\tret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,\n-\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n+\tret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot.start,\n+\t\t\t\t snapshot.eof);\n \tif (!ret && sorted)\n-\t\tret = packed_fsck_ref_sorted(o, ref_store, packed_ref_content.buf,\n-\t\t\t\t\t packed_ref_content.buf + packed_ref_content.len);\n+\t\tret = packed_fsck_ref_sorted(o, ref_store, snapshot.start,\n+\t\t\t\t\t snapshot.eof);\n \n cleanup:\n \tif (fd >= 0)\n \t\tclose(fd);\n-\tstrbuf_release(&packed_ref_content);\n+\tclear_snapshot_buffer(&snapshot);\n \treturn ret;\n }\n \n-- \n2.49.0\n\n"},{"id":"518119","messageId":"xmqqmsbe3uaw.fsf@gitster.g","threadId":"63407","inReplyTo":"aCS7O8tNekg_u9Wp@ArchLinux","subject":"Re: [PATCH v5 0/3] align the behavior when opening \"packed-refs\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-15T12:57:59Z","receivedAt":"2025-05-15T12:58:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> As discussed in [1], we need to use mmap mechanism to open large\n> \"packed_refs\" file to save the memory usage. This patch mainly does the\n> following things:\n>\n> 1: Fix an issue that we would report an error when the \"packed-refs\"\n> file is empty, which does not align with the runtime behavior.\n> 2-4: Extract some logic from the existing code and then use these\n> created helper functions to let fsck code to use mmap necessarily\n>\n> [1] https://lore.kernel.org/git/20250503133158.GA4450@coredump.intra.peff.net\n>\n> Really thank Peff and Patrick to suggest me to do above change.\n\nThis round looks good to me. Queued. Thanks.\n"},{"id":"518601","messageId":"xmqq7c2aapte.fsf@gitster.g","threadId":"63407","inReplyTo":"aCS7O8tNekg_u9Wp@ArchLinux","subject":"Re: [PATCH v5 0/3] align the behavior when opening \"packed-refs\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-21T16:31:09Z","receivedAt":"2025-05-21T16:31:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> As discussed in [1], we need to use mmap mechanism to open large\n> \"packed_refs\" file to save the memory usage. This patch mainly does the\n> following things:\n>\n> 1: Fix an issue that we would report an error when the \"packed-refs\"\n> file is empty, which does not align with the runtime behavior.\n> 2-4: Extract some logic from the existing code and then use these\n> created helper functions to let fsck code to use mmap necessarily\n>\n> [1] https://lore.kernel.org/git/20250503133158.GA4450@coredump.intra.peff.net\n>\n> Really thank Peff and Patrick to suggest me to do above change.\n> ...\n> Change in v5:\n>\n> 1. Improve the commit message in the first patch to be more clear:\n> 1. Talk about the current behavior, what error we would report if\n> \"packed-refs\" is empty.\n> 2. To align with the runtime behavior, we should skip checking the\n> content of \"packed-refs\".\n> 3. Why do we need to report to the user when the \"packed-refs\" is\n> empty\n> 2. Fix grammar issue in the last patch.\n\nThe thread has gone quiet on this topic. Is everybody happy with\nthis version?\n\nThanks.\n\n"},{"id":"518643","messageId":"20250522055006.GA1135327@coredump.intra.peff.net","threadId":"63407","inReplyTo":"xmqq7c2aapte.fsf@gitster.g","subject":"Re: [PATCH v5 0/3] align the behavior when opening \"packed-refs\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-22T05:50:06Z","receivedAt":"2025-05-22T05:50:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 21, 2025 at 09:31:09AM -0700, Junio C Hamano wrote:\n\n> > Change in v5:\n> >\n> > 1. Improve the commit message in the first patch to be more clear:\n> > 1. Talk about the current behavior, what error we would report if\n> > \"packed-refs\" is empty.\n> > 2. To align with the runtime behavior, we should skip checking the\n> > content of \"packed-refs\".\n> > 3. Why do we need to report to the user when the \"packed-refs\" is\n> > empty\n> > 2. Fix grammar issue in the last patch.\n> \n> The thread has gone quiet on this topic. Is everybody happy with\n> this version?\n\nYep, it looks good to me. Thanks.\n\n-Peff\n"},{"id":"518757","messageId":"aDBCdoNPkTq0xzOP@pks.im","threadId":"63407","inReplyTo":"20250522055006.GA1135327@coredump.intra.peff.net","subject":"Re: [PATCH v5 0/3] align the behavior when opening \"packed-refs\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-23T09:40:06Z","receivedAt":"2025-05-23T09:40:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 22, 2025 at 01:50:06AM -0400, Jeff King wrote:\n> On Wed, May 21, 2025 at 09:31:09AM -0700, Junio C Hamano wrote:\n> \n> > > Change in v5:\n> > >\n> > > 1. Improve the commit message in the first patch to be more clear:\n> > > 1. Talk about the current behavior, what error we would report if\n> > > \"packed-refs\" is empty.\n> > > 2. To align with the runtime behavior, we should skip checking the\n> > > content of \"packed-refs\".\n> > > 3. Why do we need to report to the user when the \"packed-refs\" is\n> > > empty\n> > > 2. Fix grammar issue in the last patch.\n> > \n> > The thread has gone quiet on this topic. Is everybody happy with\n> > this version?\n> \n> Yep, it looks good to me. Thanks.\n\nDidn't have anything else to add, either. Thanks!\n\nPatrick\n"},{"id":"518776","messageId":"xmqqjz67z5bx.fsf@gitster.g","threadId":"63407","inReplyTo":"aDBCdoNPkTq0xzOP@pks.im","subject":"Re: [PATCH v5 0/3] align the behavior when opening \"packed-refs\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-23T15:58:58Z","receivedAt":"2025-05-23T15:59:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, May 22, 2025 at 01:50:06AM -0400, Jeff King wrote:\n>> On Wed, May 21, 2025 at 09:31:09AM -0700, Junio C Hamano wrote:\n>> \n>> > > Change in v5:\n>> > >\n>> > > 1. Improve the commit message in the first patch to be more clear:\n>> > > 1. Talk about the current behavior, what error we would report if\n>> > > \"packed-refs\" is empty.\n>> > > 2. To align with the runtime behavior, we should skip checking the\n>> > > content of \"packed-refs\".\n>> > > 3. Why do we need to report to the user when the \"packed-refs\" is\n>> > > empty\n>> > > 2. Fix grammar issue in the last patch.\n>> > \n>> > The thread has gone quiet on this topic. Is everybody happy with\n>> > this version?\n>> \n>> Yep, it looks good to me. Thanks.\n>\n> Didn't have anything else to add, either. Thanks!\n\nThanks, all. Let's merge it down.\n"}]}