{"thread":{"id":"64056","subject":"[PATCH 0/5] midx-write: fix segfault and do several cleanups","startedAt":"2025-08-28T17:39:58Z","lastAt":"2025-09-11T23:44:57Z","messageCount":47,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Taylor Blau","Derrick Stolee","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"525127","messageId":"pull.1965.git.1756402795.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":null,"subject":"[PATCH 0/5] midx-write: fix segfault and do several cleanups","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-28T17:39:50Z","receivedAt":"2025-08-28T17:39:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I was motivated to start looking closely at midx-write.c due to multiple\nusers reporting Git crashes in their background maintenance, specifically\nduring git multi-pack-index repack calls. I was eventually able to reproduce\nit in git multi-pack-index expire as well.\n\nPatch 1 is the only change we need to fix this bug. It includes a test case\nthat will fail under --stress with SANITIZE=address. It requires creating\nmany packfiles (50 was not enough, but 100 is enough). As far as I can tell,\nthis bug has existed since Git 2.47.0 in October 2024, but I started hearing\nreports of this from users in July 2025 (and took a while to get a\ndump/repro).\n\nThe remaining patches are cleanups based on my careful rereading of\nmidx-write.c. There are some issues about error handling that needed some\ncleanup as well as a removal of the DISABLE_SIGN_COMPARE_WARNINGS macro.\n\nThanks, -Stolee\n\nDerrick Stolee (5):\n midx-write: only load initialized packs\n midx-write: put failing response value back\n midx-write: use cleanup when incremental midx fails\n midx-write: use uint32_t for preferred_pack_idx\n midx-write: reenable signed comparison errors\n\n midx-write.c | 118 ++++++++++++++++++------------------\n t/t5319-multi-pack-index.sh | 17 ++++++\n 2 files changed, 75 insertions(+), 60 deletions(-)\n\n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1965%2Fderrickstolee%2Fmidx-write-cleanup-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1965/derrickstolee/midx-write-cleanup-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1965\n-- \ngitgitgadget\n"},{"id":"525128","messageId":"4a4b35c69413ff18f87930dd15335f018ec71910.1756402795.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.git.1756402795.gitgitgadget@gmail.com","subject":"[PATCH 1/5] midx-write: only load initialized packs","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-28T17:39:51Z","receivedAt":"2025-08-28T17:39:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe fill_packs_from_midx() method was refactored in fcb2205b77 (midx:\nimplement support for writing incremental MIDX chains, 2024-08-06) to\nallow for preferred packfiles and incremental multi-pack-indexes.\nHowever, this led to some conditions that can cause improperly\ninitialized memory in the context's list of packfiles.\n\nThe conditions caring about the preferred pack name or the incremental\nflag are currently necessary to load a packfile. But the context is\nstill being populated with pack_info structs based on the packfile array\nfor the existing multi-pack-index even if prepare_midx_pack() isn't\ncalled.\n\nAdd a new test that breaks under --stress when compiled with\nSANITIZE=address. The chosen number of 100 packfiles was selected to get\nthe --stress output to fail about 50% of the time, while 50 packfiles\ncould not get a failure in most --stress runs. This test has a very\nminor check at the end confirming only one packfile remaining. The\nfailing nature of this test actually relies on auto-GC cleaning up some\npackfiles during the creation of the commits, as tests setting gc.auto\nto zero make the packfile count match the number of added commits but\nalso avoids hitting the memory issue.\n\nThe test case is marked as EXPENSIVE not only because of the number of\npackfiles it creates, but because some CI environments were reporting\nerrors during the test that I could not reproduce, specifically around\nbeing unable to open the packfiles or their pack-indexes.\n\nWhen it fails under SANITIZE=address, it provides the following error:\n\nAddressSanitizer:DEADLYSIGNAL\n=================================================================\n==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027\n==3263517==The signal is caused by a READ memory access.\n==3263517==Hint: address points to the zero page.\n #0 0x562d5d82d1fb in close_pack_windows packfile.c:299\n #1 0x562d5d82d3ab in close_pack packfile.c:354\n #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490\n #3 0x562d5d7c7aec in midx_repack midx-write.c:1795\n #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305\n ...\n\nThis failure stack trace is disconnected from the real fix because it\nthe bad pointers are accessed later when closing the packfiles from the\ncontext.\n\nThere are a few different aspects to this fix that are worth noting:\n\n 1. We return to the previous behavior of fill_packs_from_midx to not\n rely on the incremental flag or existence of a preferred pack.\n\n 2. The behavior to scan all layers of an incremental midx is kept, so\n this is not a full revert of the change.\n\n 3. We skip allocating more room in the pack_info array if the pack\n fails prepare_midx_pack().\n\n 4. The method has always returned 0 for success and 1 for failure, but\n the condition checking for error added a check for a negative result\n for failure, so that is now updated.\n\n 5. The call to open_pack_index() is removed, but this is needed later\n in the case of a preferred pack. That call is moved to immediately\n before its result is needed (checking for the object count).\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 38 ++++++++++++-------------------------\n t/t5319-multi-pack-index.sh | 17 +++++++++++++++++\n 2 files changed, 29 insertions(+), 26 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex a0aceab5e0..d8f9679868 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -920,8 +920,7 @@ static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,\n \treturn get_multi_pack_index(source);\n }\n \n-static int fill_packs_from_midx(struct write_midx_context *ctx,\n-\t\t\t\tconst char *preferred_pack_name, uint32_t flags)\n+static int fill_packs_from_midx(struct write_midx_context *ctx)\n {\n \tstruct multi_pack_index *m;\n \n@@ -929,30 +928,13 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n \t\tuint32_t i;\n \n \t\tfor (i = 0; i < m->num_packs; i++) {\n-\t\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n-\n-\t\t\t/*\n-\t\t\t * If generating a reverse index, need to have\n-\t\t\t * packed_git's loaded to compare their\n-\t\t\t * mtimes and object count.\n-\t\t\t *\n-\t\t\t * If a preferred pack is specified, need to\n-\t\t\t * have packed_git's loaded to ensure the chosen\n-\t\t\t * preferred pack has a non-zero object count.\n-\t\t\t */\n-\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n-\t\t\t preferred_pack_name) {\n-\t\t\t\tif (prepare_midx_pack(ctx->repo, m,\n-\t\t\t\t\t\t m->num_packs_in_base + i)) {\n-\t\t\t\t\terror(_(\"could not load pack\"));\n-\t\t\t\t\treturn 1;\n-\t\t\t\t}\n-\n-\t\t\t\tif (open_pack_index(m->packs[i]))\n-\t\t\t\t\tdie(_(\"could not open index for %s\"),\n-\t\t\t\t\t m->packs[i]->pack_name);\n+\t\t\tif (prepare_midx_pack(ctx->repo, m,\n+\t\t\t\t\t m->num_packs_in_base + i)) {\n+\t\t\t\terror(_(\"could not load pack\"));\n+\t\t\t\treturn 1;\n \t\t\t}\n \n+\t\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n \t\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n \t\t\t\t m->pack_names[i],\n \t\t\t\t m->num_packs_in_base + i);\n@@ -1123,8 +1105,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\tctx.num_multi_pack_indexes_before++;\n \t\t\tm = m->base_midx;\n \t\t}\n-\t} else if (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n-\t\t\t\t\t\t flags) < 0) {\n+\t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n \t\tgoto cleanup;\n \t}\n \n@@ -1223,6 +1204,11 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \tif (ctx.preferred_pack_idx > -1) {\n \t\tstruct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;\n+\n+\t\tif (open_pack_index(preferred))\n+\t\t\tdie(_(\"failed to open preferred pack %s\"),\n+\t\t\t ctx.info[ctx.preferred_pack_idx].pack_name);\n+\n \t\tif (!preferred->num_objects) {\n \t\t\terror(_(\"cannot select preferred pack %s with no objects\"),\n \t\t\t preferred->pack_name);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex bd75dea950..49705c62a2 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -989,6 +989,23 @@ test_expect_success 'repack --batch-size=0 repacks everything' '\n \t)\n '\n \n+test_expect_success EXPENSIVE 'repack/expire with many packs' '\n+\tcp -r dup many &&\n+\t(\n+\t\tcd many &&\n+\n+\t\tfor i in $(test_seq 1 100)\n+\t\tdo\n+\t\t\ttest_commit extra$i &&\n+\t\t\tgit maintenance run --task=loose-objects || return 1\n+\t\tdone &&\n+\n+\t\tgit multi-pack-index write &&\n+\t\tgit multi-pack-index repack &&\n+\t\tgit multi-pack-index expire\n+\t)\n+'\n+\n test_expect_success 'repack --batch-size= repacks everything' '\n \t(\n \t\tcd dup2 &&\n-- \ngitgitgadget\n\n"},{"id":"525129","messageId":"709555c5316014b2b4d6648db407d15160769146.1756402795.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.git.1756402795.gitgitgadget@gmail.com","subject":"[PATCH 2/5] midx-write: put failing response value back","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-28T17:39:52Z","receivedAt":"2025-08-28T17:40:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThis instance of setting the result to 1 before going to cleanup was\naccidentally removed in fcb2205b77 (midx: implement support for writing\nincremental MIDX chains, 2024-08-06).\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex d8f9679868..85b2d471ef 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1106,6 +1106,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\tm = m->base_midx;\n \t\t}\n \t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n+\t\tresult = 1;\n \t\tgoto cleanup;\n \t}\n \n-- \ngitgitgadget\n\n"},{"id":"525131","messageId":"a5bee036019a044273f93434f9e382a4cfa6533c.1756402795.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.git.1756402795.gitgitgadget@gmail.com","subject":"[PATCH 3/5] midx-write: use cleanup when incremental midx fails","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-28T17:39:53Z","receivedAt":"2025-08-28T17:40:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe incremental mode of writing a multi-pack-index has a few extra\nconditions that could lead to failure, but these are currently\nshort-ciruiting with 'return -1' instead of setting the method's\n'result' variable and going to the cleanup tag.\n\nReplace these returns with gotos to avoid memory issues when exiting\nearly due to error conditions.\n\nUnfortunately, these error conditions are difficult to reproduce with\ntest cases, which is perhaps one reason why the memory loss was not\ncaught by existing test cases in memory tracking modes.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 85b2d471ef..f2d9a990e6 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1321,13 +1321,15 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tincr = mks_tempfile_m(midx_name.buf, 0444);\n \t\tif (!incr) {\n \t\t\terror(_(\"unable to create temporary MIDX layer\"));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n \t\tif (adjust_shared_perm(r, get_tempfile_path(incr))) {\n \t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n \t\t\t get_tempfile_path(incr));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n \t\tf = hashfd(r->hash_algo, get_tempfile_fd(incr),\n@@ -1427,18 +1429,22 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \t\tif (!chainf) {\n \t\t\terror_errno(_(\"unable to open multi-pack-index chain file\"));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n-\t\tif (link_midx_to_chain(ctx.base_midx) < 0)\n-\t\t\treturn -1;\n+\t\tif (link_midx_to_chain(ctx.base_midx) < 0) {\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \n \t\tget_split_midx_filename_ext(r->hash_algo, &final_midx_name,\n \t\t\t\t\t object_dir, midx_hash, MIDX_EXT_MIDX);\n \n \t\tif (rename_tempfile(&incr, final_midx_name.buf) < 0) {\n \t\t\terror_errno(_(\"unable to rename new multi-pack-index layer\"));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n \t\tstrbuf_release(&final_midx_name);\n-- \ngitgitgadget\n\n"},{"id":"525130","messageId":"bd97db26f7f789315134dc796403b1ab9976135b.1756402795.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.git.1756402795.gitgitgadget@gmail.com","subject":"[PATCH 4/5] midx-write: use uint32_t for preferred_pack_idx","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-28T17:39:54Z","receivedAt":"2025-08-28T17:40:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nmidx-write.c has the DISABLE_SIGN_COMPARE_WARNINGS macro defined for a\nfew reasons, but the biggest one is the use of a signed\npreferred_pack_idx member inside the write_midx_context struct. The code\ncurrently uses -1 to indicate an unset preferred pack but pack int ids\nare normally handled as uint32_t. There are also a few loops that search\nfor the preferred pack by name and those iterators will need updates to\nuint32_t in the next change.\n\nFor now, replace the use of -1 with a 'NO_PREFERRED_PACK' macro and an\nequality check. The macro stores the max value of a uint32_t, so we\ncannot store a preferred pack that appears last in a list of 2^32 total\npacks, but that's expected to be unreasonable already. This improves the\nrange from 2^31 already.\n\nThere are some careful things to worry about with initializing the\npreferred pack in the struct and using that value when searching for a\npreferred pack that was already incorrect but accidentally working when\nthe index was initialized to zero.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 26 +++++++++++++++-----------\n 1 file changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex f2d9a990e6..ea1b3a199c 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -24,6 +24,7 @@\n #define BITMAP_POS_UNKNOWN (~((uint32_t)0))\n #define MIDX_CHUNK_FANOUT_SIZE (sizeof(uint32_t) * 256)\n #define MIDX_CHUNK_LARGE_OFFSET_WIDTH (sizeof(uint64_t))\n+#define NO_PREFERRED_PACK (~((uint32_t)0))\n \n extern int midx_checksum_valid(struct multi_pack_index *m);\n extern void clear_midx_files_ext(const char *object_dir, const char *ext,\n@@ -104,7 +105,7 @@ struct write_midx_context {\n \tunsigned large_offsets_needed:1;\n \tuint32_t num_large_offsets;\n \n-\tint preferred_pack_idx;\n+\tuint32_t preferred_pack_idx;\n \n \tint incremental;\n \tuint32_t num_multi_pack_indexes_before;\n@@ -260,7 +261,7 @@ static void midx_fanout_sort(struct midx_fanout *fanout)\n static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \t\t\t\t\tstruct multi_pack_index *m,\n \t\t\t\t\tuint32_t cur_fanout,\n-\t\t\t\t\tint preferred_pack)\n+\t\t\t\t\tuint32_t preferred_pack)\n {\n \tuint32_t start = m->num_objects_in_base, end;\n \tuint32_t cur_object;\n@@ -274,7 +275,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \tend = m->num_objects_in_base + ntohl(m->chunk_oid_fanout[cur_fanout]);\n \n \tfor (cur_object = start; cur_object < end; cur_object++) {\n-\t\tif ((preferred_pack > -1) &&\n+\t\tif ((preferred_pack != NO_PREFERRED_PACK) &&\n \t\t (preferred_pack == nth_midxed_pack_int_id(m, cur_object))) {\n \t\t\t/*\n \t\t\t * Objects from preferred packs are added\n@@ -364,7 +365,8 @@ static void compute_sorted_entries(struct write_midx_context *ctx,\n \t\t\t\t\t\t preferred, cur_fanout);\n \t\t}\n \n-\t\tif (-1 < ctx->preferred_pack_idx && ctx->preferred_pack_idx < start_pack)\n+\t\tif (ctx->preferred_pack_idx != NO_PREFERRED_PACK &&\n+\t\t ctx->preferred_pack_idx < start_pack)\n \t\t\tmidx_fanout_add_pack_fanout(&fanout, ctx->info,\n \t\t\t\t\t\t ctx->preferred_pack_idx, 1,\n \t\t\t\t\t\t cur_fanout);\n@@ -1042,7 +1044,9 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct tempfile *incr;\n-\tstruct write_midx_context ctx = { 0 };\n+\tstruct write_midx_context ctx = {\n+\t\t.preferred_pack_idx = NO_PREFERRED_PACK,\n+\t };\n \tint bitmapped_packs_concat_len = 0;\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n@@ -1150,7 +1154,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tgoto cleanup; /* nothing to do */\n \n \tif (preferred_pack_name) {\n-\t\tctx.preferred_pack_idx = -1;\n+\t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \n \t\tfor (i = 0; i < ctx.nr; i++) {\n \t\t\tif (!cmp_idx_or_pack_name(preferred_pack_name,\n@@ -1160,12 +1164,12 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t}\n \t\t}\n \n-\t\tif (ctx.preferred_pack_idx == -1)\n+\t\tif (ctx.preferred_pack_idx == NO_PREFERRED_PACK)\n \t\t\twarning(_(\"unknown preferred pack: '%s'\"),\n \t\t\t\tpreferred_pack_name);\n \t} else if (ctx.nr &&\n \t\t (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))) {\n-\t\tstruct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p;\n+\t\tstruct packed_git *oldest = ctx.info[0].p;\n \t\tctx.preferred_pack_idx = 0;\n \n \t\tif (packs_to_drop && packs_to_drop->nr)\n@@ -1193,17 +1197,17 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t * objects to resolve, so the preferred value doesn't\n \t\t\t * matter.\n \t\t\t */\n-\t\t\tctx.preferred_pack_idx = -1;\n+\t\t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \t\t}\n \t} else {\n \t\t/*\n \t\t * otherwise don't mark any pack as preferred to avoid\n \t\t * interfering with expiration logic below\n \t\t */\n-\t\tctx.preferred_pack_idx = -1;\n+\t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \t}\n \n-\tif (ctx.preferred_pack_idx > -1) {\n+\tif (ctx.preferred_pack_idx != NO_PREFERRED_PACK) {\n \t\tstruct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;\n \n \t\tif (open_pack_index(preferred))\n-- \ngitgitgadget\n\n"},{"id":"525132","messageId":"eb1abdca32c64cdabbc92008bf318c7ed67b2c1f.1756402795.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.git.1756402795.gitgitgadget@gmail.com","subject":"[PATCH 5/5] midx-write: reenable signed comparison errors","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-28T17:39:55Z","receivedAt":"2025-08-28T17:40:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nRemove the remaining signed comparison warnings in midx-write.c so that\nthey can be enforced as errors in the future. After the previous change,\nthe remaining errors are due to iterator variables named 'i'.\n\nThe strategy here involves defining the variable within the for loop\nsyntax to make sure we use the appropriate bitness for the loop\nsentinel. This matters in at least one method where the variable was\ncompared to uint32_t in some loops and size_t in others.\n\nWhile adjusting these loops, there were some where the loop boundary was\nchecking against a uint32_t value _plus one_. These were replaced with\nnon-strict comparisons, but also the value is checked to not be\nUINT32_MAX. Since the value is the number of incremental multi-pack-\nindexes, this is not a meaningful restriction. The new die() is about\ndefensive programming more than it being realistically possible.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 35 ++++++++++++++++++-----------------\n 1 file changed, 18 insertions(+), 17 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex ea1b3a199c..93f33e5baa 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1,5 +1,3 @@\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n #include \"abspath.h\"\n #include \"config.h\"\n@@ -845,7 +843,7 @@ static int write_midx_bitmap(struct write_midx_context *ctx,\n \t\t\t uint32_t commits_nr,\n \t\t\t unsigned flags)\n {\n-\tint ret, i;\n+\tint ret;\n \tuint16_t options = 0;\n \tstruct bitmap_writer writer;\n \tstruct pack_idx_entry **index;\n@@ -873,7 +871,7 @@ static int write_midx_bitmap(struct write_midx_context *ctx,\n \t * this order).\n \t */\n \tALLOC_ARRAY(index, pdata->nr_objects);\n-\tfor (i = 0; i < pdata->nr_objects; i++)\n+\tfor (uint32_t i = 0; i < pdata->nr_objects; i++)\n \t\tindex[i] = &pdata->objects[i].idx;\n \n \tbitmap_writer_init(&writer, ctx->repo, pdata,\n@@ -894,7 +892,7 @@ static int write_midx_bitmap(struct write_midx_context *ctx,\n \t * happens between bitmap_writer_build_type_index() and\n \t * bitmap_writer_finish().\n \t */\n-\tfor (i = 0; i < pdata->nr_objects; i++)\n+\tfor (uint32_t i = 0; i < pdata->nr_objects; i++)\n \t\tindex[ctx->pack_order[i]] = &pdata->objects[i].idx;\n \n \tbitmap_writer_select_commits(&writer, commits, commits_nr);\n@@ -1040,7 +1038,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n {\n \tstruct strbuf midx_name = STRBUF_INIT;\n \tunsigned char midx_hash[GIT_MAX_RAWSZ];\n-\tuint32_t i, start_pack;\n+\tuint32_t start_pack;\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct tempfile *incr;\n@@ -1156,7 +1154,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tif (preferred_pack_name) {\n \t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \n-\t\tfor (i = 0; i < ctx.nr; i++) {\n+\t\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\t\tif (!cmp_idx_or_pack_name(preferred_pack_name,\n \t\t\t\t\t\t ctx.info[i].pack_name)) {\n \t\t\t\tctx.preferred_pack_idx = i;\n@@ -1181,7 +1179,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t * pack-order has all of its objects selected from that pack\n \t\t * (and not another pack containing a duplicate)\n \t\t */\n-\t\tfor (i = 1; i < ctx.nr; i++) {\n+\t\tfor (size_t i = 1; i < ctx.nr; i++) {\n \t\t\tstruct packed_git *p = ctx.info[i].p;\n \n \t\t\tif (!oldest->num_objects || p->mtime < oldest->mtime) {\n@@ -1225,7 +1223,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tcompute_sorted_entries(&ctx, start_pack);\n \n \tctx.large_offsets_needed = 0;\n-\tfor (i = 0; i < ctx.entries_nr; i++) {\n+\tfor (size_t i = 0; i < ctx.entries_nr; i++) {\n \t\tif (ctx.entries[i].offset > 0x7fffffff)\n \t\t\tctx.num_large_offsets++;\n \t\tif (ctx.entries[i].offset > 0xffffffff)\n@@ -1235,10 +1233,10 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tQSORT(ctx.info, ctx.nr, pack_info_compare);\n \n \tif (packs_to_drop && packs_to_drop->nr) {\n-\t\tint drop_index = 0;\n+\t\tsize_t drop_index = 0;\n \t\tint missing_drops = 0;\n \n-\t\tfor (i = 0; i < ctx.nr && drop_index < packs_to_drop->nr; i++) {\n+\t\tfor (size_t i = 0; i < ctx.nr && drop_index < packs_to_drop->nr; i++) {\n \t\t\tint cmp = strcmp(ctx.info[i].pack_name,\n \t\t\t\t\t packs_to_drop->items[drop_index].string);\n \n@@ -1269,7 +1267,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t * pack_perm[old_id] = new_id\n \t */\n \tALLOC_ARRAY(ctx.pack_perm, ctx.nr);\n-\tfor (i = 0; i < ctx.nr; i++) {\n+\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\tif (ctx.info[i].expired) {\n \t\t\tdropped_packs++;\n \t\t\tctx.pack_perm[ctx.info[i].orig_pack_int_id] = PACK_EXPIRED;\n@@ -1278,7 +1276,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t}\n \t}\n \n-\tfor (i = 0; i < ctx.nr; i++) {\n+\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\tif (ctx.info[i].expired)\n \t\t\tcontinue;\n \t\tpack_name_concat_len += strlen(ctx.info[i].pack_name) + 1;\n@@ -1424,6 +1422,9 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t * have been freed in the previous if block.\n \t */\n \n+\tif (ctx.num_multi_pack_indexes_before == UINT32_MAX)\n+\t\tdie(\"too many multi-pack-indexes\");\n+\n \tCALLOC_ARRAY(keep_hashes, ctx.num_multi_pack_indexes_before + 1);\n \n \tif (ctx.incremental) {\n@@ -1456,7 +1457,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tkeep_hashes[ctx.num_multi_pack_indexes_before] =\n \t\t\txstrdup(hash_to_hex_algop(midx_hash, r->hash_algo));\n \n-\t\tfor (i = 0; i < ctx.num_multi_pack_indexes_before; i++) {\n+\t\tfor (uint32_t i = 0; i < ctx.num_multi_pack_indexes_before; i++) {\n \t\t\tuint32_t j = ctx.num_multi_pack_indexes_before - i - 1;\n \n \t\t\tkeep_hashes[j] = xstrdup(hash_to_hex_algop(get_midx_checksum(m),\n@@ -1464,7 +1465,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\tm = m->base_midx;\n \t\t}\n \n-\t\tfor (i = 0; i < ctx.num_multi_pack_indexes_before + 1; i++)\n+\t\tfor (uint32_t i = 0; i <= ctx.num_multi_pack_indexes_before; i++)\n \t\t\tfprintf(get_lock_file_fp(&lk), \"%s\\n\", keep_hashes[i]);\n \t} else {\n \t\tkeep_hashes[ctx.num_multi_pack_indexes_before] =\n@@ -1482,7 +1483,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t ctx.incremental);\n \n cleanup:\n-\tfor (i = 0; i < ctx.nr; i++) {\n+\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\tif (ctx.info[i].p) {\n \t\t\tclose_pack(ctx.info[i].p);\n \t\t\tfree(ctx.info[i].p);\n@@ -1495,7 +1496,7 @@ cleanup:\n \tfree(ctx.pack_perm);\n \tfree(ctx.pack_order);\n \tif (keep_hashes) {\n-\t\tfor (i = 0; i < ctx.num_multi_pack_indexes_before + 1; i++)\n+\t\tfor (uint32_t i = 0; i <= ctx.num_multi_pack_indexes_before; i++)\n \t\t\tfree((char *)keep_hashes[i]);\n \t\tfree(keep_hashes);\n \t}\n-- \ngitgitgadget\n"},{"id":"525145","messageId":"xmqqbjnzurm1.fsf@gitster.g","threadId":"64056","inReplyTo":"4a4b35c69413ff18f87930dd15335f018ec71910.1756402795.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] midx-write: only load initialized packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-28T20:19:02Z","receivedAt":"2025-08-28T20:19:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> The fill_packs_from_midx() method was refactored in fcb2205b77 (midx:\n> implement support for writing incremental MIDX chains, 2024-08-06) to\n> allow for preferred packfiles and incremental multi-pack-indexes.\n> However, this led to some conditions that can cause improperly\n> initialized memory in the context's list of packfiles.\n>\n> The conditions caring about the preferred pack name or the incremental\n> flag are currently necessary to load a packfile. But the context is\n> still being populated with pack_info structs based on the packfile array\n> for the existing multi-pack-index even if prepare_midx_pack() isn't\n> called.\n>\n> Add a new test that breaks under --stress when compiled with\n> SANITIZE=address. The chosen number of 100 packfiles was selected to get\n> the --stress output to fail about 50% of the time, while 50 packfiles\n> could not get a failure in most --stress runs. This test has a very\n> minor check at the end confirming only one packfile remaining. The\n> failing nature of this test actually relies on auto-GC cleaning up some\n> packfiles during the creation of the commits, as tests setting gc.auto\n> to zero make the packfile count match the number of added commits but\n> also avoids hitting the memory issue.\n>\n> The test case is marked as EXPENSIVE not only because of the number of\n> packfiles it creates, but because some CI environments were reporting\n> errors during the test that I could not reproduce, specifically around\n> being unable to open the packfiles or their pack-indexes.\n>\n> When it fails under SANITIZE=address, it provides the following error:\n>\n> AddressSanitizer:DEADLYSIGNAL\n> =================================================================\n> ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027\n> ==3263517==The signal is caused by a READ memory access.\n> ==3263517==Hint: address points to the zero page.\n> #0 0x562d5d82d1fb in close_pack_windows packfile.c:299\n> #1 0x562d5d82d3ab in close_pack packfile.c:354\n> #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490\n> #3 0x562d5d7c7aec in midx_repack midx-write.c:1795\n> #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305\n> ...\n>\n> This failure stack trace is disconnected from the real fix because it\n> the bad pointers are accessed later when closing the packfiles from the\n> context.\n\n\"because it the bad pointers\" -> ??? Perhaps just drop \"it\"?\n\n> There are a few different aspects to this fix that are worth noting:\n>\n> 1. We return to the previous behavior of fill_packs_from_midx to not\n> rely on the incremental flag or existence of a preferred pack.\n>\n> 2. The behavior to scan all layers of an incremental midx is kept, so\n> this is not a full revert of the change.\n>\n> 3. We skip allocating more room in the pack_info array if the pack\n> fails prepare_midx_pack().\n>\n> 4. The method has always returned 0 for success and 1 for failure, but\n> the condition checking for error added a check for a negative result\n> for failure, so that is now updated.\n\nSo did the callee think it is signalling an error, but the sole\ncaller was not taking that as an error, and that was one of the bugs\nthis change fixes?\n\nEven if that is the case, I still do not understand why we want to\nsay in the callee\n\n\terror(\"message\");\n\treturn 1;\n\nand adjust the caller to it, when we can just say\n\n\treturn error(\"message\");\n\nin the callee(), especially if the only caller is expecting to be\nsignalled by a negative return value for an error already.\n\n> 5. The call to open_pack_index() is removed, but this is needed later\n> in the case of a preferred pack. That call is moved to immediately\n> before its result is needed (checking for the object count).\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> midx-write.c | 38 ++++++++++++-------------------------\n> t/t5319-multi-pack-index.sh | 17 +++++++++++++++++\n> 2 files changed, 29 insertions(+), 26 deletions(-)\n\nThanks.\n\n> diff --git a/midx-write.c b/midx-write.c\n> index a0aceab5e0..d8f9679868 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -920,8 +920,7 @@ static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,\n> \treturn get_multi_pack_index(source);\n> }\n> \n> -static int fill_packs_from_midx(struct write_midx_context *ctx,\n> -\t\t\t\tconst char *preferred_pack_name, uint32_t flags)\n> +static int fill_packs_from_midx(struct write_midx_context *ctx)\n> {\n> \tstruct multi_pack_index *m;\n> \n> @@ -929,30 +928,13 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n> \t\tuint32_t i;\n> \n> \t\tfor (i = 0; i < m->num_packs; i++) {\n> -\t\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n> -\n> -\t\t\t/*\n> -\t\t\t * If generating a reverse index, need to have\n> -\t\t\t * packed_git's loaded to compare their\n> -\t\t\t * mtimes and object count.\n> -\t\t\t *\n> -\t\t\t * If a preferred pack is specified, need to\n> -\t\t\t * have packed_git's loaded to ensure the chosen\n> -\t\t\t * preferred pack has a non-zero object count.\n> -\t\t\t */\n> -\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n> -\t\t\t preferred_pack_name) {\n> -\t\t\t\tif (prepare_midx_pack(ctx->repo, m,\n> -\t\t\t\t\t\t m->num_packs_in_base + i)) {\n> -\t\t\t\t\terror(_(\"could not load pack\"));\n> -\t\t\t\t\treturn 1;\n> -\t\t\t\t}\n> -\n> -\t\t\t\tif (open_pack_index(m->packs[i]))\n> -\t\t\t\t\tdie(_(\"could not open index for %s\"),\n> -\t\t\t\t\t m->packs[i]->pack_name);\n> +\t\t\tif (prepare_midx_pack(ctx->repo, m,\n> +\t\t\t\t\t m->num_packs_in_base + i)) {\n> +\t\t\t\terror(_(\"could not load pack\"));\n> +\t\t\t\treturn 1;\n> \t\t\t}\n> \n> +\t\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n> \t\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n> \t\t\t\t m->pack_names[i],\n> \t\t\t\t m->num_packs_in_base + i);\n> @@ -1123,8 +1105,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n> \t\t\tctx.num_multi_pack_indexes_before++;\n> \t\t\tm = m->base_midx;\n> \t\t}\n> -\t} else if (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n> -\t\t\t\t\t\t flags) < 0) {\n> +\t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n> \t\tgoto cleanup;\n> \t}\n> \n> @@ -1223,6 +1204,11 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n> \n> \tif (ctx.preferred_pack_idx > -1) {\n> \t\tstruct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;\n> +\n> +\t\tif (open_pack_index(preferred))\n> +\t\t\tdie(_(\"failed to open preferred pack %s\"),\n> +\t\t\t ctx.info[ctx.preferred_pack_idx].pack_name);\n> +\n> \t\tif (!preferred->num_objects) {\n> \t\t\terror(_(\"cannot select preferred pack %s with no objects\"),\n> \t\t\t preferred->pack_name);\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index bd75dea950..49705c62a2 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -989,6 +989,23 @@ test_expect_success 'repack --batch-size=0 repacks everything' '\n> \t)\n> '\n> \n> +test_expect_success EXPENSIVE 'repack/expire with many packs' '\n> +\tcp -r dup many &&\n> +\t(\n> +\t\tcd many &&\n> +\n> +\t\tfor i in $(test_seq 1 100)\n> +\t\tdo\n> +\t\t\ttest_commit extra$i &&\n> +\t\t\tgit maintenance run --task=loose-objects || return 1\n> +\t\tdone &&\n> +\n> +\t\tgit multi-pack-index write &&\n> +\t\tgit multi-pack-index repack &&\n> +\t\tgit multi-pack-index expire\n> +\t)\n> +'\n> +\n> test_expect_success 'repack --batch-size= repacks everything' '\n> \t(\n> \t\tcd dup2 &&\n"},{"id":"525148","messageId":"xmqq1povuqdr.fsf@gitster.g","threadId":"64056","inReplyTo":"709555c5316014b2b4d6648db407d15160769146.1756402795.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/5] midx-write: put failing response value back","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-28T20:45:36Z","receivedAt":"2025-08-28T20:45:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> This instance of setting the result to 1 before going to cleanup was\n> accidentally removed in fcb2205b77 (midx: implement support for writing\n> incremental MIDX chains, 2024-08-06).\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> midx-write.c | 1 +\n> 1 file changed, 1 insertion(+)\n\nThe cover letter made it sound as if [1/5] was the only fix and the\nrest was clean-up, but unless all callers of write_midx_internal()\nignores the return value from it, this surely would change the\nbehaviour of the program, no?\n\nAnd the results from write-midx_file_only(), write_midx_file(), and\nexpire_midx_packs(), the three callers of this _internal() function,\nall seem to be used in builtin/multi-pack-index.c so wouldn't this\nalso be a fix?\n\nNot that I endorse \"0 is success and any non-zero value is an error\",\nbut this does not look like a mere clean-up to me.\n\n> diff --git a/midx-write.c b/midx-write.c\n> index d8f9679868..85b2d471ef 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1106,6 +1106,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n> \t\t\tm = m->base_midx;\n> \t\t}\n> \t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n> +\t\tresult = 1;\n> \t\tgoto cleanup;\n> \t}\n"},{"id":"525149","messageId":"xmqqwm6ntbjt.fsf@gitster.g","threadId":"64056","inReplyTo":"a5bee036019a044273f93434f9e382a4cfa6533c.1756402795.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/5] midx-write: use cleanup when incremental midx fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-28T20:51:18Z","receivedAt":"2025-08-28T20:51:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The incremental mode of writing a multi-pack-index has a few extra\n> conditions that could lead to failure, but these are currently\n> short-ciruiting with 'return -1' instead of setting the method's\n> 'result' variable and going to the cleanup tag.\n>\n> Replace these returns with gotos to avoid memory issues when exiting\n> early due to error conditions.\n>\n> Unfortunately, these error conditions are difficult to reproduce with\n> test cases, which is perhaps one reason why the memory loss was not\n> caught by existing test cases in memory tracking modes.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> midx-write.c | 18 ++++++++++++------\n> 1 file changed, 12 insertions(+), 6 deletions(-)\n\nGood thinking, but may I suggest us to go one more step to adopt\neven better convention if we were to do this?\n\nPessimistically initialize the \"result\" to -1 and let many \"goto\ncleanup\" just jump there. And have \"result = 0\" just before the\ncleanup label where the success code path joins the final cleanup\npart of the function.\n\nThis is often the right way to make the flow easier to see, because\noften the success code path is straight forward, and these error\nconditions are what employ the \"goto cleanup\" from many places. By\nstarting pessimistic, and declaring the success at the very end of\nthe straight-forward success case code path, all other flows to the\nclean-up labels do not have to set the \"ah I failed\" flag. It would\neliminate the need for patches like the previous step if the\noriginal were following that pattern.\n\n> diff --git a/midx-write.c b/midx-write.c\n> index 85b2d471ef..f2d9a990e6 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1321,13 +1321,15 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n> \t\tincr = mks_tempfile_m(midx_name.buf, 0444);\n> \t\tif (!incr) {\n> \t\t\terror(_(\"unable to create temporary MIDX layer\"));\n> -\t\t\treturn -1;\n> +\t\t\tresult = -1;\n> +\t\t\tgoto cleanup;\n> \t\t}\n> \n> \t\tif (adjust_shared_perm(r, get_tempfile_path(incr))) {\n> \t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n> \t\t\t get_tempfile_path(incr));\n> -\t\t\treturn -1;\n> +\t\t\tresult = -1;\n> +\t\t\tgoto cleanup;\n> \t\t}\n> \n> \t\tf = hashfd(r->hash_algo, get_tempfile_fd(incr),\n> @@ -1427,18 +1429,22 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n> \n> \t\tif (!chainf) {\n> \t\t\terror_errno(_(\"unable to open multi-pack-index chain file\"));\n> -\t\t\treturn -1;\n> +\t\t\tresult = -1;\n> +\t\t\tgoto cleanup;\n> \t\t}\n> \n> -\t\tif (link_midx_to_chain(ctx.base_midx) < 0)\n> -\t\t\treturn -1;\n> +\t\tif (link_midx_to_chain(ctx.base_midx) < 0) {\n> +\t\t\tresult = -1;\n> +\t\t\tgoto cleanup;\n> +\t\t}\n> \n> \t\tget_split_midx_filename_ext(r->hash_algo, &final_midx_name,\n> \t\t\t\t\t object_dir, midx_hash, MIDX_EXT_MIDX);\n> \n> \t\tif (rename_tempfile(&incr, final_midx_name.buf) < 0) {\n> \t\t\terror_errno(_(\"unable to rename new multi-pack-index layer\"));\n> -\t\t\treturn -1;\n> +\t\t\tresult = -1;\n> +\t\t\tgoto cleanup;\n> \t\t}\n> \n> \t\tstrbuf_release(&final_midx_name);\n"},{"id":"525150","messageId":"xmqqsehbtb7z.fsf@gitster.g","threadId":"64056","inReplyTo":"bd97db26f7f789315134dc796403b1ab9976135b.1756402795.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] midx-write: use uint32_t for preferred_pack_idx","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-28T20:58:24Z","receivedAt":"2025-08-28T20:58:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> For now, replace the use of -1 with a 'NO_PREFERRED_PACK' macro and an\n> equality check. The macro stores the max value of a uint32_t, so we\n> cannot store a preferred pack that appears last in a list of 2^32 total\n> packs, but that's expected to be unreasonable already. This improves the\n> range from 2^31 already.\n\n;-) \n\nI very much like this change. An obvious alternative may be to use\nint instead of uint32_t to number and index into in-core packs, as\nwe all know that not just 2^32 but 2^31 is still unreasonably too\nmany anyway.\n\n> There are some careful things to worry about with initializing the\n> preferred pack in the struct and using that value when searching for a\n> preferred pack that was already incorrect but accidentally working when\n> the index was initialized to zero.\n\nTrue.\n\n> -\tstruct write_midx_context ctx = { 0 };\n> +\tstruct write_midx_context ctx = {\n> +\t\t.preferred_pack_idx = NO_PREFERRED_PACK,\n> +\t };\n\nGood.\n\n> \tif (preferred_pack_name) {\n> -\t\tctx.preferred_pack_idx = -1;\n> +\t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n\nThis too.\n\nThanks.\n"},{"id":"525151","messageId":"xmqqo6rztb2h.fsf@gitster.g","threadId":"64056","inReplyTo":"eb1abdca32c64cdabbc92008bf318c7ed67b2c1f.1756402795.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] midx-write: reenable signed comparison errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-28T21:01:42Z","receivedAt":"2025-08-28T21:01:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> The strategy here involves defining the variable within the for loop\n> syntax to make sure we use the appropriate bitness for the loop\n> sentinel. This matters in at least one method where the variable was\n> compared to uint32_t in some loops and size_t in others.\n\nSounds like a fine way to avoid accidentally breaking these loops.\n\n"},{"id":"525177","messageId":"aLEAaNsm8LE2M3TE@nand.local","threadId":"64056","inReplyTo":"4a4b35c69413ff18f87930dd15335f018ec71910.1756402795.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] midx-write: only load initialized packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-08-29T01:20:40Z","receivedAt":"2025-08-29T01:20:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 28, 2025 at 05:39:51PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> The fill_packs_from_midx() method was refactored in fcb2205b77 (midx:\n> implement support for writing incremental MIDX chains, 2024-08-06) to\n> allow for preferred packfiles and incremental multi-pack-indexes.\n> However, this led to some conditions that can cause improperly\n> initialized memory in the context's list of packfiles.\n>\n> The conditions caring about the preferred pack name or the incremental\n> flag are currently necessary to load a packfile. But the context is\n> still being populated with pack_info structs based on the packfile array\n> for the existing multi-pack-index even if prepare_midx_pack() isn't\n> called.\n\nThanks for looking at this one. On the surface this looks not great, but\nI am having a hard time coming up with a smaller test case that\nexercises this behavior.\n\nI can get what you wrote below to fail on my machine pretty reliable\nwhen building with SANITIZE=address (even without --stress). All of the\nspots that read from the pack_info array and access the actual\npacked_git structs are guarded by either writing a MIDX bitmap or having\na non-empty preferred pack.\n\nSo I can see that we're definitely trying to close_pack() on a NULL\npointer, but I can't find a way to trigger that more directly.\n\n(I think the fact that we don't appear to be accessing the packed_git\nstruct because the spots that want to are guarded by the same conditions\nthat cause us to call prepare_midx_pack() in the first place is pure\nluck and not something that I am comfortable with us relying on. So we\nshould fix this up for sure, but I wish I understood the existing bug a\nlittle more clearly first.)\n\n> Add a new test that breaks under --stress when compiled with\n> SANITIZE=address. The chosen number of 100 packfiles was selected to get\n> the --stress output to fail about 50% of the time, while 50 packfiles\n> could not get a failure in most --stress runs. This test has a very\n> minor check at the end confirming only one packfile remaining. The\n> failing nature of this test actually relies on auto-GC cleaning up some\n> packfiles during the creation of the commits, as tests setting gc.auto\n> to zero make the packfile count match the number of added commits but\n> also avoids hitting the memory issue.\n\nHmm. Is this portion of the commit message out-of-date? I can't see the\ncheck you're referring to that ensures there is only one pack remaining,\nnor can I see the spot where we disable gc.auto.\n\n> The test case is marked as EXPENSIVE not only because of the number of\n> packfiles it creates, but because some CI environments were reporting\n> errors during the test that I could not reproduce, specifically around\n> being unable to open the packfiles or their pack-indexes.\n>\n> When it fails under SANITIZE=address, it provides the following error:\n>\n> AddressSanitizer:DEADLYSIGNAL\n> =================================================================\n> ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027\n> ==3263517==The signal is caused by a READ memory access.\n> ==3263517==Hint: address points to the zero page.\n> #0 0x562d5d82d1fb in close_pack_windows packfile.c:299\n> #1 0x562d5d82d3ab in close_pack packfile.c:354\n> #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490\n> #3 0x562d5d7c7aec in midx_repack midx-write.c:1795\n> #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305\n> ...\n>\n> This failure stack trace is disconnected from the real fix because it\n\ns/it// ?\n\n> the bad pointers are accessed later when closing the packfiles from the\n> context.\n>\n> There are a few different aspects to this fix that are worth noting:\n>\n> 1. We return to the previous behavior of fill_packs_from_midx to not\n> rely on the incremental flag or existence of a preferred pack.\n>\n> 2. The behavior to scan all layers of an incremental midx is kept, so\n> this is not a full revert of the change.\n>\n> 3. We skip allocating more room in the pack_info array if the pack\n> fails prepare_midx_pack().\n>\n> 4. The method has always returned 0 for success and 1 for failure, but\n> the condition checking for error added a check for a negative result\n> for failure, so that is now updated.\n\nOops ;-).\n\n> 5. The call to open_pack_index() is removed, but this is needed later\n> in the case of a preferred pack. That call is moved to immediately\n> before its result is needed (checking for the object count).\n\nI think we need to do this in at least one other spot, but see below.\n\n> +\t\t\tif (prepare_midx_pack(ctx->repo, m,\n> +\t\t\t\t\t m->num_packs_in_base + i)) {\n> +\t\t\t\terror(_(\"could not load pack\"));\n> +\t\t\t\treturn 1;\n\nLooks good, though I agree with Junio's comment in his separate reply\nthat we could probably just turn this into \"return error(...)\" while\nwe're at it.\n\n> @@ -1223,6 +1204,11 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n>\n> \tif (ctx.preferred_pack_idx > -1) {\n> \t\tstruct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;\n> +\n> +\t\tif (open_pack_index(preferred))\n> +\t\t\tdie(_(\"failed to open preferred pack %s\"),\n> +\t\t\t ctx.info[ctx.preferred_pack_idx].pack_name);\n\nThis makes sense, but I think we need to apply similar treatment in the\n\"else if\" arm of the if-statement immediately above this one too. That\nportion of the code handles the case where we're writing a MIDX bitmap\nbut didn't provide a preferred pack.\n\nWhen that's the case, we loop through to try and find the oldest pack\nthat contains at least one object. If we don't call open_pack_index()\nall of those ->num_objects fields will still be zero'd, so we'll only\nfind the oldest pack.\n\nThat may actually produce wrong behavior if we have duplicate objects\nthat aren't uniformly resolved in favor of the earliest pack in lex\norder. I'd have to think about it a little more to be sure, though.\n\nThanks,\nTaylor\n"},{"id":"525178","messageId":"aLEBwC9Oky+NV/Vk@nand.local","threadId":"64056","inReplyTo":"xmqq1povuqdr.fsf@gitster.g","subject":"Re: [PATCH 2/5] midx-write: put failing response value back","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-08-29T01:26:24Z","receivedAt":"2025-08-29T01:26:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 28, 2025 at 01:45:36PM -0700, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n>\n> > From: Derrick Stolee \n> >\n> > This instance of setting the result to 1 before going to cleanup was\n> > accidentally removed in fcb2205b77 (midx: implement support for writing\n> > incremental MIDX chains, 2024-08-06).\n> >\n> > Signed-off-by: Derrick Stolee \n> > ---\n> > midx-write.c | 1 +\n> > 1 file changed, 1 insertion(+)\n>\n> The cover letter made it sound as if [1/5] was the only fix and the\n> rest was clean-up, but unless all callers of write_midx_internal()\n> ignores the return value from it, this surely would change the\n> behaviour of the program, no?\n\nI think the cover letter is saying that only the first patch is required\nto fix the crash that was reported, and the rest are clean-ups.\n\nNot that this isn't a bug in its own right, but it's definitely distinct\nfrom the one that is addressed in the previous patch.\n\n> And the results from write-midx_file_only(), write_midx_file(), and\n> expire_midx_packs(), the three callers of this _internal() function,\n> all seem to be used in builtin/multi-pack-index.c so wouldn't this\n> also be a fix?\n>\n> Not that I endorse \"0 is success and any non-zero value is an error\",\n> but this does not look like a mere clean-up to me.\n>\n> > diff --git a/midx-write.c b/midx-write.c\n> > index d8f9679868..85b2d471ef 100644\n> > --- a/midx-write.c\n> > +++ b/midx-write.c\n> > @@ -1106,6 +1106,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n> > \t\t\tm = m->base_midx;\n> > \t\t}\n> > \t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n> > +\t\tresult = 1;\n> > \t\tgoto cleanup;\n\nIt might be nice to have a test that confirms this behavior (though the\nchanges look obviously correct to me). It should suffice to write a\nMIDX, drop one of its packs, and then try to write it again with a new\npack.\n\nThanks,\nTaylor\n"},{"id":"525179","messageId":"aLECkfDqFVgNA1xh@nand.local","threadId":"64056","inReplyTo":"xmqqwm6ntbjt.fsf@gitster.g","subject":"Re: [PATCH 3/5] midx-write: use cleanup when incremental midx fails","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-08-29T01:29:53Z","receivedAt":"2025-08-29T01:29:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 28, 2025 at 01:51:18PM -0700, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n>\n> > From: Derrick Stolee \n> >\n> > The incremental mode of writing a multi-pack-index has a few extra\n> > conditions that could lead to failure, but these are currently\n> > short-ciruiting with 'return -1' instead of setting the method's\n> > 'result' variable and going to the cleanup tag.\n> >\n> > Replace these returns with gotos to avoid memory issues when exiting\n> > early due to error conditions.\n> >\n> > Unfortunately, these error conditions are difficult to reproduce with\n> > test cases, which is perhaps one reason why the memory loss was not\n> > caught by existing test cases in memory tracking modes.\n> >\n> > Signed-off-by: Derrick Stolee \n> > ---\n> > midx-write.c | 18 ++++++++++++------\n> > 1 file changed, 12 insertions(+), 6 deletions(-)\n>\n> Good thinking, but may I suggest us to go one more step to adopt\n> even better convention if we were to do this?\n>\n> Pessimistically initialize the \"result\" to -1 and let many \"goto\n> cleanup\" just jump there. And have \"result = 0\" just before the\n> cleanup label where the success code path joins the final cleanup\n> part of the function.\n>\n> This is often the right way to make the flow easier to see, because\n> often the success code path is straight forward, and these error\n> conditions are what employ the \"goto cleanup\" from many places. By\n> starting pessimistic, and declaring the success at the very end of\n> the straight-forward success case code path, all other flows to the\n> clean-up labels do not have to set the \"ah I failed\" flag. It would\n> eliminate the need for patches like the previous step if the\n> original were following that pattern.\n\nAlternatively replacing something like:\n\n error(...);\n result = -1;\n goto cleanup;\n\nwith just\n\n result = error(...);\n goto cleanup;\n\nwould IMHO make the code easier to read, though I agree that nothing is\nforcing us to remember to assign result in the first place ;-). I am not\nsure the pessimistic initialization is better in all cases either, since\nwe have to remember to place it before any \"cleanup\" label, and make\nsure that that does not regress.\n\nSo, I dunno. I'm OK with what is written here, and I think we could\ncertainly have a separate discussion to perhaps have CodingGuidelines\ntake a stronger stance here.\n\nThanks,\nTaylor\n"},{"id":"525180","messageId":"aLEDzF3b6tax6Lnq@nand.local","threadId":"64056","inReplyTo":"bd97db26f7f789315134dc796403b1ab9976135b.1756402795.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] midx-write: use uint32_t for preferred_pack_idx","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-08-29T01:35:08Z","receivedAt":"2025-08-29T01:35:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 28, 2025 at 05:39:54PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> midx-write.c has the DISABLE_SIGN_COMPARE_WARNINGS macro defined for a\n> few reasons, but the biggest one is the use of a signed\n> preferred_pack_idx member inside the write_midx_context struct. The code\n> currently uses -1 to indicate an unset preferred pack but pack int ids\n> are normally handled as uint32_t. There are also a few loops that search\n> for the preferred pack by name and those iterators will need updates to\n> uint32_t in the next change.\n>\n> For now, replace the use of -1 with a 'NO_PREFERRED_PACK' macro and an\n> equality check. The macro stores the max value of a uint32_t, so we\n> cannot store a preferred pack that appears last in a list of 2^32 total\n> packs, but that's expected to be unreasonable already. This improves the\n> range from 2^31 already.\n\nNice ;-).\n\n> There are some careful things to worry about with initializing the\n> preferred pack in the struct and using that value when searching for a\n> preferred pack that was already incorrect but accidentally working when\n> the index was initialized to zero.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> midx-write.c | 26 +++++++++++++++-----------\n> 1 file changed, 15 insertions(+), 11 deletions(-)\n>\n> diff --git a/midx-write.c b/midx-write.c\n> index f2d9a990e6..ea1b3a199c 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -24,6 +24,7 @@\n> #define BITMAP_POS_UNKNOWN (~((uint32_t)0))\n> #define MIDX_CHUNK_FANOUT_SIZE (sizeof(uint32_t) * 256)\n> #define MIDX_CHUNK_LARGE_OFFSET_WIDTH (sizeof(uint64_t))\n> +#define NO_PREFERRED_PACK (~((uint32_t)0))\n\nI think that just (~(uint32_t)0) is necessary, but I don't mind the\nextra clarity.\n\n> @@ -274,7 +275,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n> \tend = m->num_objects_in_base + ntohl(m->chunk_oid_fanout[cur_fanout]);\n>\n> \tfor (cur_object = start; cur_object < end; cur_object++) {\n> -\t\tif ((preferred_pack > -1) &&\n> +\t\tif ((preferred_pack != NO_PREFERRED_PACK) &&\n> \t\t (preferred_pack == nth_midxed_pack_int_id(m, cur_object))) {\n\nOK, here we should make sure that the preferred pack was provided.\n\nLike you mentioned above, I guess that means we can't have the 2^32-1'st\npack, but before we couldn't have any pack greater than 2^31-1\npreferred, so this is a strict improvement ;-).\n\n> \t\t\t/*\n> \t\t\t * Objects from preferred packs are added\n> @@ -364,7 +365,8 @@ static void compute_sorted_entries(struct write_midx_context *ctx,\n> \t\t\t\t\t\t preferred, cur_fanout);\n> \t\t}\n>\n> -\t\tif (-1 < ctx->preferred_pack_idx && ctx->preferred_pack_idx < start_pack)\n> +\t\tif (ctx->preferred_pack_idx != NO_PREFERRED_PACK &&\n> +\t\t ctx->preferred_pack_idx < start_pack)\n\nLooks good. It's tempting to say that any value of preferred_pack_idx\nlower than start_pack is going to be OK, since start_pack is a uint32_t\nas well, but we need to make sure that the preferred pack was specified\nin the first place.\n\nThe rest all looks good as well, thanks.\n\nThanks,\nTaylor\n"},{"id":"525181","messageId":"aLED/YDNy9dZvkHO@nand.local","threadId":"64056","inReplyTo":"xmqqo6rztb2h.fsf@gitster.g","subject":"Re: [PATCH 5/5] midx-write: reenable signed comparison errors","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-08-29T01:35:57Z","receivedAt":"2025-08-29T01:35:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 28, 2025 at 02:01:42PM -0700, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n>\n> > The strategy here involves defining the variable within the for loop\n> > syntax to make sure we use the appropriate bitness for the loop\n> > sentinel. This matters in at least one method where the variable was\n> > compared to uint32_t in some loops and size_t in others.\n>\n> Sounds like a fine way to avoid accidentally breaking these loops.\n\nYup, all looks good to me.\n\nThanks,\nTaylor\n"},{"id":"525182","messageId":"aLEENQcWvfOC7Iyn@nand.local","threadId":"64056","inReplyTo":"pull.1965.git.1756402795.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] midx-write: fix segfault and do several cleanups","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-08-29T01:36:53Z","receivedAt":"2025-08-29T01:36:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 28, 2025 at 05:39:50PM +0000, Derrick Stolee via GitGitGadget wrote:\n> Derrick Stolee (5):\n> midx-write: only load initialized packs\n> midx-write: put failing response value back\n> midx-write: use cleanup when incremental midx fails\n> midx-write: use uint32_t for preferred_pack_idx\n> midx-write: reenable signed comparison errors\n\nThanks for looking at this, and my apologies for the regression plugged\nby the first patch.\n\nThe last four patches look good to me, and I left just some minor\ncomments throughout. The first patch does leave me feeling like there\nought to be a smaller reproduction, but I can't seem to find one easily.\n\nThanks,\nTaylor\n"},{"id":"525253","messageId":"f161727d-7688-4f62-9eb4-67d1b9033503@gmail.com","threadId":"64056","inReplyTo":"aLEAaNsm8LE2M3TE@nand.local","subject":"Re: [PATCH 1/5] midx-write: only load initialized packs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-08-30T14:33:39Z","receivedAt":"2025-08-30T14:33:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/28/25 9:20 PM, Taylor Blau wrote:\n> On Thu, Aug 28, 2025 at 05:39:51PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> The fill_packs_from_midx() method was refactored in fcb2205b77 (midx:\n>> implement support for writing incremental MIDX chains, 2024-08-06) to\n>> allow for preferred packfiles and incremental multi-pack-indexes.\n>> However, this led to some conditions that can cause improperly\n>> initialized memory in the context's list of packfiles.\n>>\n>> The conditions caring about the preferred pack name or the incremental\n>> flag are currently necessary to load a packfile. But the context is\n>> still being populated with pack_info structs based on the packfile array\n>> for the existing multi-pack-index even if prepare_midx_pack() isn't\n>> called.\n> \n> Thanks for looking at this one. On the surface this looks not great, but\n> I am having a hard time coming up with a smaller test case that\n> exercises this behavior.\n> \n> I can get what you wrote below to fail on my machine pretty reliable\n> when building with SANITIZE=address (even without --stress). All of the\n> spots that read from the pack_info array and access the actual\n> packed_git structs are guarded by either writing a MIDX bitmap or having\n> a non-empty preferred pack.\n\nI'm glad you're able to reproduce it. My --stress runs had about a\n50% hit rate.\n\n>> Add a new test that breaks under --stress when compiled with\n>> SANITIZE=address. The chosen number of 100 packfiles was selected to get\n>> the --stress output to fail about 50% of the time, while 50 packfiles\n>> could not get a failure in most --stress runs. This test has a very\n>> minor check at the end confirming only one packfile remaining. The\n>> failing nature of this test actually relies on auto-GC cleaning up some\n>> packfiles during the creation of the commits, as tests setting gc.auto\n>> to zero make the packfile count match the number of added commits but\n>> also avoids hitting the memory issue.\n> \n> Hmm. Is this portion of the commit message out-of-date? I can't see the\n> check you're referring to that ensures there is only one pack remaining,\n> nor can I see the spot where we disable gc.auto.\n\nYou're right. When I added more robustness around the packfile count\nby removing gc.auto, the test stopped failing pre-fix. Then, I forgot\nto remove mention of those test updates.\n\n>> The test case is marked as EXPENSIVE not only because of the number of\n>> packfiles it creates, but because some CI environments were reporting\n>> errors during the test that I could not reproduce, specifically around\n>> being unable to open the packfiles or their pack-indexes.\n>>\n>> When it fails under SANITIZE=address, it provides the following error:\n>>\n>> AddressSanitizer:DEADLYSIGNAL\n>> =================================================================\n>> ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027\n>> ==3263517==The signal is caused by a READ memory access.\n>> ==3263517==Hint: address points to the zero page.\n>> #0 0x562d5d82d1fb in close_pack_windows packfile.c:299\n>> #1 0x562d5d82d3ab in close_pack packfile.c:354\n>> #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490\n>> #3 0x562d5d7c7aec in midx_repack midx-write.c:1795\n>> #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305\n>> ...\n>>\n>> This failure stack trace is disconnected from the real fix because it\n> \n> s/it// ?\n\nThanks.\n\n>> the bad pointers are accessed later when closing the packfiles from the\n>> context.\n>>\n>> There are a few different aspects to this fix that are worth noting:\n>>\n>> 1. We return to the previous behavior of fill_packs_from_midx to not\n>> rely on the incremental flag or existence of a preferred pack.\n>>\n>> 2. The behavior to scan all layers of an incremental midx is kept, so\n>> this is not a full revert of the change.\n>>\n>> 3. We skip allocating more room in the pack_info array if the pack\n>> fails prepare_midx_pack().\n>>\n>> 4. The method has always returned 0 for success and 1 for failure, but\n>> the condition checking for error added a check for a negative result\n>> for failure, so that is now updated.\n> \n> Oops ;-).\n> \n>> 5. The call to open_pack_index() is removed, but this is needed later\n>> in the case of a preferred pack. That call is moved to immediately\n>> before its result is needed (checking for the object count).\n> \n> I think we need to do this in at least one other spot, but see below.\n\nInteresting!\n\n>> +\t\t\tif (prepare_midx_pack(ctx->repo, m,\n>> +\t\t\t\t\t m->num_packs_in_base + i)) {\n>> +\t\t\t\terror(_(\"could not load pack\"));\n>> +\t\t\t\treturn 1;\n> \n> Looks good, though I agree with Junio's comment in his separate reply\n> that we could probably just turn this into \"return error(...)\" while\n> we're at it.\n\nCan do.\n\n>> @@ -1223,6 +1204,11 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n>>\n>> \tif (ctx.preferred_pack_idx > -1) {\n>> \t\tstruct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;\n>> +\n>> +\t\tif (open_pack_index(preferred))\n>> +\t\t\tdie(_(\"failed to open preferred pack %s\"),\n>> +\t\t\t ctx.info[ctx.preferred_pack_idx].pack_name);\n> \n> This makes sense, but I think we need to apply similar treatment in the\n> \"else if\" arm of the if-statement immediately above this one too. That\n> portion of the code handles the case where we're writing a MIDX bitmap\n> but didn't provide a preferred pack.\n> \n> When that's the case, we loop through to try and find the oldest pack\n> that contains at least one object. If we don't call open_pack_index()\n> all of those ->num_objects fields will still be zero'd, so we'll only\n> find the oldest pack.\n> \n> That may actually produce wrong behavior if we have duplicate objects\n> that aren't uniformly resolved in favor of the earliest pack in lex\n> order. I'd have to think about it a little more to be sure, though.\n\nI see. In this case, we need to open_pack_index() before relying on\noldest->num_objects, which only needs to happen for the first pack\nand any packfile that wins via mtime preference. It also seems like\nwe can _warn_ on failures to open packfiles in those cases, since it\nisn't fatal if some packfiles fail to open.\n\nThanks,\n-Stolee\n\n"},{"id":"525254","messageId":"eac886a7-0f50-40fa-b10e-92c68d60bac7@gmail.com","threadId":"64056","inReplyTo":"aLECkfDqFVgNA1xh@nand.local","subject":"Re: [PATCH 3/5] midx-write: use cleanup when incremental midx fails","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-08-30T14:44:22Z","receivedAt":"2025-08-30T14:44:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/28/25 9:29 PM, Taylor Blau wrote:\n> On Thu, Aug 28, 2025 at 01:51:18PM -0700, Junio C Hamano wrote:\n>> \"Derrick Stolee via GitGitGadget\" writes:\n>>\n>>> From: Derrick Stolee \n>>>\n>>> The incremental mode of writing a multi-pack-index has a few extra\n>>> conditions that could lead to failure, but these are currently\n>>> short-ciruiting with 'return -1' instead of setting the method's\n>>> 'result' variable and going to the cleanup tag.\n>>>\n>>> Replace these returns with gotos to avoid memory issues when exiting\n>>> early due to error conditions.\n>>>\n>>> Unfortunately, these error conditions are difficult to reproduce with\n>>> test cases, which is perhaps one reason why the memory loss was not\n>>> caught by existing test cases in memory tracking modes.\n>>>\n>>> Signed-off-by: Derrick Stolee \n>>> ---\n>>> midx-write.c | 18 ++++++++++++------\n>>> 1 file changed, 12 insertions(+), 6 deletions(-)\n>>\n>> Good thinking, but may I suggest us to go one more step to adopt\n>> even better convention if we were to do this?\n>>\n>> Pessimistically initialize the \"result\" to -1 and let many \"goto\n>> cleanup\" just jump there. And have \"result = 0\" just before the\n>> cleanup label where the success code path joins the final cleanup\n>> part of the function.\n>>\n>> This is often the right way to make the flow easier to see, because\n>> often the success code path is straight forward, and these error\n>> conditions are what employ the \"goto cleanup\" from many places. By\n>> starting pessimistic, and declaring the success at the very end of\n>> the straight-forward success case code path, all other flows to the\n>> clean-up labels do not have to set the \"ah I failed\" flag. It would\n>> eliminate the need for patches like the previous step if the\n>> original were following that pattern.\n> \n> Alternatively replacing something like:\n> \n> error(...);\n> result = -1;\n> goto cleanup;\n> \n> with just\n> \n> result = error(...);\n> goto cleanup;\n> \n> would IMHO make the code easier to read, though I agree that nothing is\n> forcing us to remember to assign result in the first place ;-). I am not\n> sure the pessimistic initialization is better in all cases either, since\n> we have to remember to place it before any \"cleanup\" label, and make\n> sure that that does not regress.\n> \n> So, I dunno. I'm OK with what is written here, and I think we could\n> certainly have a separate discussion to perhaps have CodingGuidelines\n> take a stronger stance here.\n\nI'll look into adding this as a cleanup. This specific patch is\nabout adding the 'goto's where they were missing. I can make a new\npatch that unifies the result initialization to -1 (and thus making\nthe method unified in returning -1 on error). There are many more\n\"result = 1\" lines that need to change.\n\nThanks,\n-Stolee\n"},{"id":"525255","messageId":"pull.1965.v2.git.1756589007.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.git.1756402795.gitgitgadget@gmail.com","subject":"[PATCH v2 0/6] midx-write: fix segfault and do several cleanups","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-30T21:23:21Z","receivedAt":"2025-08-30T21:23:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I was motivated to start looking closely at midx-write.c due to multiple\nusers reporting Git crashes in their background maintenance, specifically\nduring git multi-pack-index repack calls. I was eventually able to reproduce\nit in git multi-pack-index expire as well.\n\nPatch 1 is the only change we need to fix this bug. It includes a test case\nthat will fail under --stress with SANITIZE=address. It requires creating\nmany packfiles (50 was not enough, but 100 is enough). As far as I can tell,\nthis bug has existed since Git 2.47.0 in October 2024, but I started hearing\nreports of this from users in July 2025 (and took a while to get a\ndump/repro).\n\nThe remaining patches are cleanups based on my careful rereading of\nmidx-write.c. There are some issues about error handling that needed some\ncleanup as well as a removal of the DISABLE_SIGN_COMPARE_WARNINGS macro.\n\n\nUpdates in V2\n=============\n\n * A stale comment to an unsubmitted version of the test is removed.\n * More cases needing open_pack_index() are patched.\n * Typos fixed.\n * A new patch assumes error and sets result to zero only on the few\n successful paths.\n\nThanks, -Stolee\n\nDerrick Stolee (6):\n midx-write: only load initialized packs\n midx-write: put failing response value back\n midx-write: use cleanup when incremental midx fails\n midx-write: use uint32_t for preferred_pack_idx\n midx-write: reenable signed comparison errors\n midx-write: simplify error cases\n\n midx-write.c | 134 +++++++++++++++++-------------------\n t/t5319-multi-pack-index.sh | 22 +++++-\n 2 files changed, 86 insertions(+), 70 deletions(-)\n\n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1965%2Fderrickstolee%2Fmidx-write-cleanup-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1965/derrickstolee/midx-write-cleanup-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1965\n\nRange-diff vs v1:\n\n 1: 4a4b35c694 ! 1: e02a444315 midx-write: only load initialized packs\n @@ Commit message\n Add a new test that breaks under --stress when compiled with\n SANITIZE=address. The chosen number of 100 packfiles was selected to get\n the --stress output to fail about 50% of the time, while 50 packfiles\n - could not get a failure in most --stress runs. This test has a very\n - minor check at the end confirming only one packfile remaining. The\n - failing nature of this test actually relies on auto-GC cleaning up some\n - packfiles during the creation of the commits, as tests setting gc.auto\n - to zero make the packfile count match the number of added commits but\n - also avoids hitting the memory issue.\n + could not get a failure in most --stress runs.\n \n The test case is marked as EXPENSIVE not only because of the number of\n packfiles it creates, but because some CI environments were reporting\n @@ Commit message\n #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305\n ...\n \n - This failure stack trace is disconnected from the real fix because it\n - the bad pointers are accessed later when closing the packfiles from the\n - context.\n + This failure stack trace is disconnected from the real fix because the bad\n + pointers are accessed later when closing the packfiles from the context.\n \n There are a few different aspects to this fix that are worth noting:\n \n @@ midx-write.c: static int fill_packs_from_midx(struct write_midx_context *ctx,\n -\t\t\t\tif (open_pack_index(m->packs[i]))\n -\t\t\t\t\tdie(_(\"could not open index for %s\"),\n -\t\t\t\t\t m->packs[i]->pack_name);\n +-\t\t\t}\n +\t\t\tif (prepare_midx_pack(ctx->repo, m,\n -+\t\t\t\t\t m->num_packs_in_base + i)) {\n -+\t\t\t\terror(_(\"could not load pack\"));\n -+\t\t\t\treturn 1;\n - \t\t\t}\n ++\t\t\t\t\t m->num_packs_in_base + i))\n ++\t\t\t\treturn error(_(\"could not load pack\"));\n \n +\t\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n \t\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n @@ midx-write.c: static int write_midx_internal(struct repository *r, const char *o\n \t\tgoto cleanup;\n \t}\n \n +@@ midx-write.c: static int write_midx_internal(struct repository *r, const char *object_dir,\n + \t\tstruct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p;\n + \t\tctx.preferred_pack_idx = 0;\n + \n ++\t\t/*\n ++\t\t * Attempt opening the pack index to populate num_objects.\n ++\t\t * Ignore failiures as they can be expected and are not\n ++\t\t * fatal during this selection time.\n ++\t\t */\n ++\t\topen_pack_index(oldest);\n ++\n + \t\tif (packs_to_drop && packs_to_drop->nr)\n + \t\t\tBUG(\"cannot write a MIDX bitmap during expiration\");\n + \n +@@ midx-write.c: static int write_midx_internal(struct repository *r, const char *object_dir,\n + \n + \t\t\tif (!oldest->num_objects || p->mtime < oldest->mtime) {\n + \t\t\t\toldest = p;\n ++\t\t\t\topen_pack_index(oldest);\n + \t\t\t\tctx.preferred_pack_idx = i;\n + \t\t\t}\n + \t\t}\n @@ midx-write.c: static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \tif (ctx.preferred_pack_idx > -1) {\n 2: 709555c531 ! 2: a1dd3ed874 midx-write: put failing response value back\n @@ Commit message\n \n This instance of setting the result to 1 before going to cleanup was\n accidentally removed in fcb2205b77 (midx: implement support for writing\n - incremental MIDX chains, 2024-08-06).\n + incremental MIDX chains, 2024-08-06). Build upon a test that already deletes\n + a packfile to verify that this error propagates to full command failure.\n \n Signed-off-by: Derrick Stolee \n \n @@ midx-write.c: static int write_midx_internal(struct repository *r, const char *o\n \t\tgoto cleanup;\n \t}\n \n +\n + ## t/t5319-multi-pack-index.sh ##\n +@@ t/t5319-multi-pack-index.sh: test_expect_success 'load reverse index when missing .idx, .pack' '\n + \t\tmv $idx.bak $idx &&\n + \n + \t\tmv $pack $pack.bak &&\n +-\t\tgit cat-file --batch-check=\"%(objectsize:disk)\" err &&\n ++\t\tgrep \"could not load pack\" err\n + \t)\n + '\n + \n 3: a5bee03601 = 3: c4f75cca09 midx-write: use cleanup when incremental midx fails\n 4: bd97db26f7 ! 4: 2290e27ded midx-write: use uint32_t for preferred_pack_idx\n @@ midx-write.c: static int write_midx_internal(struct repository *r, const char *o\n +\t\tstruct packed_git *oldest = ctx.info[0].p;\n \t\tctx.preferred_pack_idx = 0;\n \n - \t\tif (packs_to_drop && packs_to_drop->nr)\n + \t\t/*\n @@ midx-write.c: static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t * objects to resolve, so the preferred value doesn't\n \t\t\t * matter.\n 5: eb1abdca32 = 5: 35302f5228 midx-write: reenable signed comparison errors\n -: ---------- > 6: 7be25cf534 midx-write: simplify error cases\n\n-- \ngitgitgadget\n"},{"id":"525256","messageId":"e02a444315acbc638a3d31279c10a936f0adb7b4.1756589007.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v2.git.1756589007.gitgitgadget@gmail.com","subject":"[PATCH v2 1/6] midx-write: only load initialized packs","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-30T21:23:22Z","receivedAt":"2025-08-30T21:23:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe fill_packs_from_midx() method was refactored in fcb2205b77 (midx:\nimplement support for writing incremental MIDX chains, 2024-08-06) to\nallow for preferred packfiles and incremental multi-pack-indexes.\nHowever, this led to some conditions that can cause improperly\ninitialized memory in the context's list of packfiles.\n\nThe conditions caring about the preferred pack name or the incremental\nflag are currently necessary to load a packfile. But the context is\nstill being populated with pack_info structs based on the packfile array\nfor the existing multi-pack-index even if prepare_midx_pack() isn't\ncalled.\n\nAdd a new test that breaks under --stress when compiled with\nSANITIZE=address. The chosen number of 100 packfiles was selected to get\nthe --stress output to fail about 50% of the time, while 50 packfiles\ncould not get a failure in most --stress runs.\n\nThe test case is marked as EXPENSIVE not only because of the number of\npackfiles it creates, but because some CI environments were reporting\nerrors during the test that I could not reproduce, specifically around\nbeing unable to open the packfiles or their pack-indexes.\n\nWhen it fails under SANITIZE=address, it provides the following error:\n\nAddressSanitizer:DEADLYSIGNAL\n=================================================================\n==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027\n==3263517==The signal is caused by a READ memory access.\n==3263517==Hint: address points to the zero page.\n #0 0x562d5d82d1fb in close_pack_windows packfile.c:299\n #1 0x562d5d82d3ab in close_pack packfile.c:354\n #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490\n #3 0x562d5d7c7aec in midx_repack midx-write.c:1795\n #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305\n ...\n\nThis failure stack trace is disconnected from the real fix because the bad\npointers are accessed later when closing the packfiles from the context.\n\nThere are a few different aspects to this fix that are worth noting:\n\n 1. We return to the previous behavior of fill_packs_from_midx to not\n rely on the incremental flag or existence of a preferred pack.\n\n 2. The behavior to scan all layers of an incremental midx is kept, so\n this is not a full revert of the change.\n\n 3. We skip allocating more room in the pack_info array if the pack\n fails prepare_midx_pack().\n\n 4. The method has always returned 0 for success and 1 for failure, but\n the condition checking for error added a check for a negative result\n for failure, so that is now updated.\n\n 5. The call to open_pack_index() is removed, but this is needed later\n in the case of a preferred pack. That call is moved to immediately\n before its result is needed (checking for the object count).\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 46 +++++++++++++++----------------------\n t/t5319-multi-pack-index.sh | 17 ++++++++++++++\n 2 files changed, 36 insertions(+), 27 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex a0aceab5e0..070a7f61f4 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -920,8 +920,7 @@ static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,\n \treturn get_multi_pack_index(source);\n }\n \n-static int fill_packs_from_midx(struct write_midx_context *ctx,\n-\t\t\t\tconst char *preferred_pack_name, uint32_t flags)\n+static int fill_packs_from_midx(struct write_midx_context *ctx)\n {\n \tstruct multi_pack_index *m;\n \n@@ -929,30 +928,11 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n \t\tuint32_t i;\n \n \t\tfor (i = 0; i < m->num_packs; i++) {\n-\t\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n-\n-\t\t\t/*\n-\t\t\t * If generating a reverse index, need to have\n-\t\t\t * packed_git's loaded to compare their\n-\t\t\t * mtimes and object count.\n-\t\t\t *\n-\t\t\t * If a preferred pack is specified, need to\n-\t\t\t * have packed_git's loaded to ensure the chosen\n-\t\t\t * preferred pack has a non-zero object count.\n-\t\t\t */\n-\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n-\t\t\t preferred_pack_name) {\n-\t\t\t\tif (prepare_midx_pack(ctx->repo, m,\n-\t\t\t\t\t\t m->num_packs_in_base + i)) {\n-\t\t\t\t\terror(_(\"could not load pack\"));\n-\t\t\t\t\treturn 1;\n-\t\t\t\t}\n-\n-\t\t\t\tif (open_pack_index(m->packs[i]))\n-\t\t\t\t\tdie(_(\"could not open index for %s\"),\n-\t\t\t\t\t m->packs[i]->pack_name);\n-\t\t\t}\n+\t\t\tif (prepare_midx_pack(ctx->repo, m,\n+\t\t\t\t\t m->num_packs_in_base + i))\n+\t\t\t\treturn error(_(\"could not load pack\"));\n \n+\t\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n \t\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n \t\t\t\t m->pack_names[i],\n \t\t\t\t m->num_packs_in_base + i);\n@@ -1123,8 +1103,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\tctx.num_multi_pack_indexes_before++;\n \t\t\tm = m->base_midx;\n \t\t}\n-\t} else if (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n-\t\t\t\t\t\t flags) < 0) {\n+\t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n \t\tgoto cleanup;\n \t}\n \n@@ -1186,6 +1165,13 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tstruct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p;\n \t\tctx.preferred_pack_idx = 0;\n \n+\t\t/*\n+\t\t * Attempt opening the pack index to populate num_objects.\n+\t\t * Ignore failiures as they can be expected and are not\n+\t\t * fatal during this selection time.\n+\t\t */\n+\t\topen_pack_index(oldest);\n+\n \t\tif (packs_to_drop && packs_to_drop->nr)\n \t\t\tBUG(\"cannot write a MIDX bitmap during expiration\");\n \n@@ -1200,6 +1186,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \t\t\tif (!oldest->num_objects || p->mtime < oldest->mtime) {\n \t\t\t\toldest = p;\n+\t\t\t\topen_pack_index(oldest);\n \t\t\t\tctx.preferred_pack_idx = i;\n \t\t\t}\n \t\t}\n@@ -1223,6 +1210,11 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \tif (ctx.preferred_pack_idx > -1) {\n \t\tstruct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;\n+\n+\t\tif (open_pack_index(preferred))\n+\t\t\tdie(_(\"failed to open preferred pack %s\"),\n+\t\t\t ctx.info[ctx.preferred_pack_idx].pack_name);\n+\n \t\tif (!preferred->num_objects) {\n \t\t\terror(_(\"cannot select preferred pack %s with no objects\"),\n \t\t\t preferred->pack_name);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex bd75dea950..49705c62a2 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -989,6 +989,23 @@ test_expect_success 'repack --batch-size=0 repacks everything' '\n \t)\n '\n \n+test_expect_success EXPENSIVE 'repack/expire with many packs' '\n+\tcp -r dup many &&\n+\t(\n+\t\tcd many &&\n+\n+\t\tfor i in $(test_seq 1 100)\n+\t\tdo\n+\t\t\ttest_commit extra$i &&\n+\t\t\tgit maintenance run --task=loose-objects || return 1\n+\t\tdone &&\n+\n+\t\tgit multi-pack-index write &&\n+\t\tgit multi-pack-index repack &&\n+\t\tgit multi-pack-index expire\n+\t)\n+'\n+\n test_expect_success 'repack --batch-size= repacks everything' '\n \t(\n \t\tcd dup2 &&\n-- \ngitgitgadget\n\n"},{"id":"525257","messageId":"a1dd3ed87437322e298bb192fbfe4a9641d1356e.1756589007.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v2.git.1756589007.gitgitgadget@gmail.com","subject":"[PATCH v2 2/6] midx-write: put failing response value back","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-30T21:23:23Z","receivedAt":"2025-08-30T21:23:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThis instance of setting the result to 1 before going to cleanup was\naccidentally removed in fcb2205b77 (midx: implement support for writing\nincremental MIDX chains, 2024-08-06). Build upon a test that already deletes\na packfile to verify that this error propagates to full command failure.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 1 +\n t/t5319-multi-pack-index.sh | 5 ++++-\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 070a7f61f4..0f1d5653ab 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1104,6 +1104,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\tm = m->base_midx;\n \t\t}\n \t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n+\t\tresult = 1;\n \t\tgoto cleanup;\n \t}\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 49705c62a2..008e65c22e 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1100,7 +1100,10 @@ test_expect_success 'load reverse index when missing .idx, .pack' '\n \t\tmv $idx.bak $idx &&\n \n \t\tmv $pack $pack.bak &&\n-\t\tgit cat-file --batch-check=\"%(objectsize:disk)\" err &&\n+\t\tgrep \"could not load pack\" err\n \t)\n '\n \n-- \ngitgitgadget\n\n"},{"id":"525258","messageId":"c4f75cca094dd47c431de52e335ccaaf8b06355b.1756589007.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v2.git.1756589007.gitgitgadget@gmail.com","subject":"[PATCH v2 3/6] midx-write: use cleanup when incremental midx fails","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-30T21:23:24Z","receivedAt":"2025-08-30T21:23:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe incremental mode of writing a multi-pack-index has a few extra\nconditions that could lead to failure, but these are currently\nshort-ciruiting with 'return -1' instead of setting the method's\n'result' variable and going to the cleanup tag.\n\nReplace these returns with gotos to avoid memory issues when exiting\nearly due to error conditions.\n\nUnfortunately, these error conditions are difficult to reproduce with\ntest cases, which is perhaps one reason why the memory loss was not\ncaught by existing test cases in memory tracking modes.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 0f1d5653ab..cb0211289d 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1327,13 +1327,15 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tincr = mks_tempfile_m(midx_name.buf, 0444);\n \t\tif (!incr) {\n \t\t\terror(_(\"unable to create temporary MIDX layer\"));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n \t\tif (adjust_shared_perm(r, get_tempfile_path(incr))) {\n \t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n \t\t\t get_tempfile_path(incr));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n \t\tf = hashfd(r->hash_algo, get_tempfile_fd(incr),\n@@ -1433,18 +1435,22 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \t\tif (!chainf) {\n \t\t\terror_errno(_(\"unable to open multi-pack-index chain file\"));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n-\t\tif (link_midx_to_chain(ctx.base_midx) < 0)\n-\t\t\treturn -1;\n+\t\tif (link_midx_to_chain(ctx.base_midx) < 0) {\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \n \t\tget_split_midx_filename_ext(r->hash_algo, &final_midx_name,\n \t\t\t\t\t object_dir, midx_hash, MIDX_EXT_MIDX);\n \n \t\tif (rename_tempfile(&incr, final_midx_name.buf) < 0) {\n \t\t\terror_errno(_(\"unable to rename new multi-pack-index layer\"));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n \t\tstrbuf_release(&final_midx_name);\n-- \ngitgitgadget\n\n"},{"id":"525259","messageId":"2290e27ded03866cb816be5fb6e08b83a75a6f07.1756589007.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v2.git.1756589007.gitgitgadget@gmail.com","subject":"[PATCH v2 4/6] midx-write: use uint32_t for preferred_pack_idx","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-30T21:23:25Z","receivedAt":"2025-08-30T21:23:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nmidx-write.c has the DISABLE_SIGN_COMPARE_WARNINGS macro defined for a\nfew reasons, but the biggest one is the use of a signed\npreferred_pack_idx member inside the write_midx_context struct. The code\ncurrently uses -1 to indicate an unset preferred pack but pack int ids\nare normally handled as uint32_t. There are also a few loops that search\nfor the preferred pack by name and those iterators will need updates to\nuint32_t in the next change.\n\nFor now, replace the use of -1 with a 'NO_PREFERRED_PACK' macro and an\nequality check. The macro stores the max value of a uint32_t, so we\ncannot store a preferred pack that appears last in a list of 2^32 total\npacks, but that's expected to be unreasonable already. This improves the\nrange from 2^31 already.\n\nThere are some careful things to worry about with initializing the\npreferred pack in the struct and using that value when searching for a\npreferred pack that was already incorrect but accidentally working when\nthe index was initialized to zero.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 26 +++++++++++++++-----------\n 1 file changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex cb0211289d..1822268ce2 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -24,6 +24,7 @@\n #define BITMAP_POS_UNKNOWN (~((uint32_t)0))\n #define MIDX_CHUNK_FANOUT_SIZE (sizeof(uint32_t) * 256)\n #define MIDX_CHUNK_LARGE_OFFSET_WIDTH (sizeof(uint64_t))\n+#define NO_PREFERRED_PACK (~((uint32_t)0))\n \n extern int midx_checksum_valid(struct multi_pack_index *m);\n extern void clear_midx_files_ext(const char *object_dir, const char *ext,\n@@ -104,7 +105,7 @@ struct write_midx_context {\n \tunsigned large_offsets_needed:1;\n \tuint32_t num_large_offsets;\n \n-\tint preferred_pack_idx;\n+\tuint32_t preferred_pack_idx;\n \n \tint incremental;\n \tuint32_t num_multi_pack_indexes_before;\n@@ -260,7 +261,7 @@ static void midx_fanout_sort(struct midx_fanout *fanout)\n static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \t\t\t\t\tstruct multi_pack_index *m,\n \t\t\t\t\tuint32_t cur_fanout,\n-\t\t\t\t\tint preferred_pack)\n+\t\t\t\t\tuint32_t preferred_pack)\n {\n \tuint32_t start = m->num_objects_in_base, end;\n \tuint32_t cur_object;\n@@ -274,7 +275,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \tend = m->num_objects_in_base + ntohl(m->chunk_oid_fanout[cur_fanout]);\n \n \tfor (cur_object = start; cur_object < end; cur_object++) {\n-\t\tif ((preferred_pack > -1) &&\n+\t\tif ((preferred_pack != NO_PREFERRED_PACK) &&\n \t\t (preferred_pack == nth_midxed_pack_int_id(m, cur_object))) {\n \t\t\t/*\n \t\t\t * Objects from preferred packs are added\n@@ -364,7 +365,8 @@ static void compute_sorted_entries(struct write_midx_context *ctx,\n \t\t\t\t\t\t preferred, cur_fanout);\n \t\t}\n \n-\t\tif (-1 < ctx->preferred_pack_idx && ctx->preferred_pack_idx < start_pack)\n+\t\tif (ctx->preferred_pack_idx != NO_PREFERRED_PACK &&\n+\t\t ctx->preferred_pack_idx < start_pack)\n \t\t\tmidx_fanout_add_pack_fanout(&fanout, ctx->info,\n \t\t\t\t\t\t ctx->preferred_pack_idx, 1,\n \t\t\t\t\t\t cur_fanout);\n@@ -1040,7 +1042,9 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct tempfile *incr;\n-\tstruct write_midx_context ctx = { 0 };\n+\tstruct write_midx_context ctx = {\n+\t\t.preferred_pack_idx = NO_PREFERRED_PACK,\n+\t };\n \tint bitmapped_packs_concat_len = 0;\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n@@ -1148,7 +1152,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tgoto cleanup; /* nothing to do */\n \n \tif (preferred_pack_name) {\n-\t\tctx.preferred_pack_idx = -1;\n+\t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \n \t\tfor (i = 0; i < ctx.nr; i++) {\n \t\t\tif (!cmp_idx_or_pack_name(preferred_pack_name,\n@@ -1158,12 +1162,12 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t}\n \t\t}\n \n-\t\tif (ctx.preferred_pack_idx == -1)\n+\t\tif (ctx.preferred_pack_idx == NO_PREFERRED_PACK)\n \t\t\twarning(_(\"unknown preferred pack: '%s'\"),\n \t\t\t\tpreferred_pack_name);\n \t} else if (ctx.nr &&\n \t\t (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))) {\n-\t\tstruct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p;\n+\t\tstruct packed_git *oldest = ctx.info[0].p;\n \t\tctx.preferred_pack_idx = 0;\n \n \t\t/*\n@@ -1199,17 +1203,17 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t * objects to resolve, so the preferred value doesn't\n \t\t\t * matter.\n \t\t\t */\n-\t\t\tctx.preferred_pack_idx = -1;\n+\t\t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \t\t}\n \t} else {\n \t\t/*\n \t\t * otherwise don't mark any pack as preferred to avoid\n \t\t * interfering with expiration logic below\n \t\t */\n-\t\tctx.preferred_pack_idx = -1;\n+\t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \t}\n \n-\tif (ctx.preferred_pack_idx > -1) {\n+\tif (ctx.preferred_pack_idx != NO_PREFERRED_PACK) {\n \t\tstruct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;\n \n \t\tif (open_pack_index(preferred))\n-- \ngitgitgadget\n\n"},{"id":"525260","messageId":"35302f52285d98c3e7ed7ab4140b4721375bc8a3.1756589007.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v2.git.1756589007.gitgitgadget@gmail.com","subject":"[PATCH v2 5/6] midx-write: reenable signed comparison errors","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-30T21:23:26Z","receivedAt":"2025-08-30T21:23:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nRemove the remaining signed comparison warnings in midx-write.c so that\nthey can be enforced as errors in the future. After the previous change,\nthe remaining errors are due to iterator variables named 'i'.\n\nThe strategy here involves defining the variable within the for loop\nsyntax to make sure we use the appropriate bitness for the loop\nsentinel. This matters in at least one method where the variable was\ncompared to uint32_t in some loops and size_t in others.\n\nWhile adjusting these loops, there were some where the loop boundary was\nchecking against a uint32_t value _plus one_. These were replaced with\nnon-strict comparisons, but also the value is checked to not be\nUINT32_MAX. Since the value is the number of incremental multi-pack-\nindexes, this is not a meaningful restriction. The new die() is about\ndefensive programming more than it being realistically possible.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 35 ++++++++++++++++++-----------------\n 1 file changed, 18 insertions(+), 17 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 1822268ce2..14a0947c46 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1,5 +1,3 @@\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n #include \"abspath.h\"\n #include \"config.h\"\n@@ -845,7 +843,7 @@ static int write_midx_bitmap(struct write_midx_context *ctx,\n \t\t\t uint32_t commits_nr,\n \t\t\t unsigned flags)\n {\n-\tint ret, i;\n+\tint ret;\n \tuint16_t options = 0;\n \tstruct bitmap_writer writer;\n \tstruct pack_idx_entry **index;\n@@ -873,7 +871,7 @@ static int write_midx_bitmap(struct write_midx_context *ctx,\n \t * this order).\n \t */\n \tALLOC_ARRAY(index, pdata->nr_objects);\n-\tfor (i = 0; i < pdata->nr_objects; i++)\n+\tfor (uint32_t i = 0; i < pdata->nr_objects; i++)\n \t\tindex[i] = &pdata->objects[i].idx;\n \n \tbitmap_writer_init(&writer, ctx->repo, pdata,\n@@ -894,7 +892,7 @@ static int write_midx_bitmap(struct write_midx_context *ctx,\n \t * happens between bitmap_writer_build_type_index() and\n \t * bitmap_writer_finish().\n \t */\n-\tfor (i = 0; i < pdata->nr_objects; i++)\n+\tfor (uint32_t i = 0; i < pdata->nr_objects; i++)\n \t\tindex[ctx->pack_order[i]] = &pdata->objects[i].idx;\n \n \tbitmap_writer_select_commits(&writer, commits, commits_nr);\n@@ -1038,7 +1036,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n {\n \tstruct strbuf midx_name = STRBUF_INIT;\n \tunsigned char midx_hash[GIT_MAX_RAWSZ];\n-\tuint32_t i, start_pack;\n+\tuint32_t start_pack;\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct tempfile *incr;\n@@ -1154,7 +1152,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tif (preferred_pack_name) {\n \t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \n-\t\tfor (i = 0; i < ctx.nr; i++) {\n+\t\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\t\tif (!cmp_idx_or_pack_name(preferred_pack_name,\n \t\t\t\t\t\t ctx.info[i].pack_name)) {\n \t\t\t\tctx.preferred_pack_idx = i;\n@@ -1186,7 +1184,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t * pack-order has all of its objects selected from that pack\n \t\t * (and not another pack containing a duplicate)\n \t\t */\n-\t\tfor (i = 1; i < ctx.nr; i++) {\n+\t\tfor (size_t i = 1; i < ctx.nr; i++) {\n \t\t\tstruct packed_git *p = ctx.info[i].p;\n \n \t\t\tif (!oldest->num_objects || p->mtime < oldest->mtime) {\n@@ -1231,7 +1229,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tcompute_sorted_entries(&ctx, start_pack);\n \n \tctx.large_offsets_needed = 0;\n-\tfor (i = 0; i < ctx.entries_nr; i++) {\n+\tfor (size_t i = 0; i < ctx.entries_nr; i++) {\n \t\tif (ctx.entries[i].offset > 0x7fffffff)\n \t\t\tctx.num_large_offsets++;\n \t\tif (ctx.entries[i].offset > 0xffffffff)\n@@ -1241,10 +1239,10 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tQSORT(ctx.info, ctx.nr, pack_info_compare);\n \n \tif (packs_to_drop && packs_to_drop->nr) {\n-\t\tint drop_index = 0;\n+\t\tsize_t drop_index = 0;\n \t\tint missing_drops = 0;\n \n-\t\tfor (i = 0; i < ctx.nr && drop_index < packs_to_drop->nr; i++) {\n+\t\tfor (size_t i = 0; i < ctx.nr && drop_index < packs_to_drop->nr; i++) {\n \t\t\tint cmp = strcmp(ctx.info[i].pack_name,\n \t\t\t\t\t packs_to_drop->items[drop_index].string);\n \n@@ -1275,7 +1273,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t * pack_perm[old_id] = new_id\n \t */\n \tALLOC_ARRAY(ctx.pack_perm, ctx.nr);\n-\tfor (i = 0; i < ctx.nr; i++) {\n+\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\tif (ctx.info[i].expired) {\n \t\t\tdropped_packs++;\n \t\t\tctx.pack_perm[ctx.info[i].orig_pack_int_id] = PACK_EXPIRED;\n@@ -1284,7 +1282,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t}\n \t}\n \n-\tfor (i = 0; i < ctx.nr; i++) {\n+\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\tif (ctx.info[i].expired)\n \t\t\tcontinue;\n \t\tpack_name_concat_len += strlen(ctx.info[i].pack_name) + 1;\n@@ -1430,6 +1428,9 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t * have been freed in the previous if block.\n \t */\n \n+\tif (ctx.num_multi_pack_indexes_before == UINT32_MAX)\n+\t\tdie(\"too many multi-pack-indexes\");\n+\n \tCALLOC_ARRAY(keep_hashes, ctx.num_multi_pack_indexes_before + 1);\n \n \tif (ctx.incremental) {\n@@ -1462,7 +1463,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tkeep_hashes[ctx.num_multi_pack_indexes_before] =\n \t\t\txstrdup(hash_to_hex_algop(midx_hash, r->hash_algo));\n \n-\t\tfor (i = 0; i < ctx.num_multi_pack_indexes_before; i++) {\n+\t\tfor (uint32_t i = 0; i < ctx.num_multi_pack_indexes_before; i++) {\n \t\t\tuint32_t j = ctx.num_multi_pack_indexes_before - i - 1;\n \n \t\t\tkeep_hashes[j] = xstrdup(hash_to_hex_algop(get_midx_checksum(m),\n@@ -1470,7 +1471,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\tm = m->base_midx;\n \t\t}\n \n-\t\tfor (i = 0; i < ctx.num_multi_pack_indexes_before + 1; i++)\n+\t\tfor (uint32_t i = 0; i <= ctx.num_multi_pack_indexes_before; i++)\n \t\t\tfprintf(get_lock_file_fp(&lk), \"%s\\n\", keep_hashes[i]);\n \t} else {\n \t\tkeep_hashes[ctx.num_multi_pack_indexes_before] =\n@@ -1488,7 +1489,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t ctx.incremental);\n \n cleanup:\n-\tfor (i = 0; i < ctx.nr; i++) {\n+\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\tif (ctx.info[i].p) {\n \t\t\tclose_pack(ctx.info[i].p);\n \t\t\tfree(ctx.info[i].p);\n@@ -1501,7 +1502,7 @@ cleanup:\n \tfree(ctx.pack_perm);\n \tfree(ctx.pack_order);\n \tif (keep_hashes) {\n-\t\tfor (i = 0; i < ctx.num_multi_pack_indexes_before + 1; i++)\n+\t\tfor (uint32_t i = 0; i <= ctx.num_multi_pack_indexes_before; i++)\n \t\t\tfree((char *)keep_hashes[i]);\n \t\tfree(keep_hashes);\n \t}\n-- \ngitgitgadget\n\n"},{"id":"525261","messageId":"7be25cf5349c389cf2887ab5b852779fc364bd7e.1756589007.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v2.git.1756589007.gitgitgadget@gmail.com","subject":"[PATCH v2 6/6] midx-write: simplify error cases","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-30T21:23:27Z","receivedAt":"2025-08-30T21:23:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe write_midx_internal() method uses gotos to jump to a cleanup section to\nclear memory before returning 'result'. Since these jumps are more common\nfor error conditions, initialize 'result' to -1 and then only set it to 0\nbefore returning with success. There are a couple places where we return\nwith success via a jump.\n\nThis has the added benefit that the method now returns -1 on error instead\nof an inconsistent 1 or -1.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 26 +++++++++-----------------\n 1 file changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 14a0947c46..047ffdcdbf 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1046,7 +1046,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tint bitmapped_packs_concat_len = 0;\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n-\tint result = 0;\n+\tint result = -1;\n \tconst char **keep_hashes = NULL;\n \tstruct chunkfile *cf;\n \n@@ -1099,14 +1099,12 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t\terror(_(\"could not load reverse index for MIDX %s\"),\n \t\t\t\t hash_to_hex_algop(get_midx_checksum(m),\n \t\t\t\t\t\t\tm->repo->hash_algo));\n-\t\t\t\tresult = 1;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \t\t\tctx.num_multi_pack_indexes_before++;\n \t\t\tm = m->base_midx;\n \t\t}\n \t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n-\t\tresult = 1;\n \t\tgoto cleanup;\n \t}\n \n@@ -1142,12 +1140,16 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t */\n \t\t\tif (!want_bitmap)\n \t\t\t\tclear_midx_files_ext(object_dir, \"bitmap\", NULL);\n+\n+\t\t\tresult = 0;\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n \n-\tif (ctx.incremental && !ctx.nr)\n+\tif (ctx.incremental && !ctx.nr) {\n+\t\tresult = 0;\n \t\tgoto cleanup; /* nothing to do */\n+\t}\n \n \tif (preferred_pack_name) {\n \t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n@@ -1221,7 +1223,6 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tif (!preferred->num_objects) {\n \t\t\terror(_(\"cannot select preferred pack %s with no objects\"),\n \t\t\t preferred->pack_name);\n-\t\t\tresult = 1;\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n@@ -1260,10 +1261,8 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t}\n \t\t}\n \n-\t\tif (missing_drops) {\n-\t\t\tresult = 1;\n+\t\tif (missing_drops)\n \t\t\tgoto cleanup;\n-\t\t}\n \t}\n \n \t/*\n@@ -1309,7 +1308,6 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \tif (ctx.nr - dropped_packs == 0) {\n \t\terror(_(\"no pack files to index.\"));\n-\t\tresult = 1;\n \t\tgoto cleanup;\n \t}\n \n@@ -1329,14 +1327,12 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tincr = mks_tempfile_m(midx_name.buf, 0444);\n \t\tif (!incr) {\n \t\t\terror(_(\"unable to create temporary MIDX layer\"));\n-\t\t\tresult = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \n \t\tif (adjust_shared_perm(r, get_tempfile_path(incr))) {\n \t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n \t\t\t get_tempfile_path(incr));\n-\t\t\tresult = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \n@@ -1414,7 +1410,6 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t\t midx_hash, &pdata, commits, commits_nr,\n \t\t\t\t flags) < 0) {\n \t\t\terror(_(\"could not write multi-pack bitmap\"));\n-\t\t\tresult = 1;\n \t\t\tclear_packing_data(&pdata);\n \t\t\tfree(commits);\n \t\t\tgoto cleanup;\n@@ -1440,21 +1435,17 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \t\tif (!chainf) {\n \t\t\terror_errno(_(\"unable to open multi-pack-index chain file\"));\n-\t\t\tresult = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \n-\t\tif (link_midx_to_chain(ctx.base_midx) < 0) {\n-\t\t\tresult = -1;\n+\t\tif (link_midx_to_chain(ctx.base_midx) < 0)\n \t\t\tgoto cleanup;\n-\t\t}\n \n \t\tget_split_midx_filename_ext(r->hash_algo, &final_midx_name,\n \t\t\t\t\t object_dir, midx_hash, MIDX_EXT_MIDX);\n \n \t\tif (rename_tempfile(&incr, final_midx_name.buf) < 0) {\n \t\t\terror_errno(_(\"unable to rename new multi-pack-index layer\"));\n-\t\t\tresult = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \n@@ -1487,6 +1478,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tclear_midx_files(r, object_dir, keep_hashes,\n \t\t\t ctx.num_multi_pack_indexes_before + 1,\n \t\t\t ctx.incremental);\n+\tresult = 0;\n \n cleanup:\n \tfor (size_t i = 0; i < ctx.nr; i++) {\n-- \ngitgitgadget\n"},{"id":"525411","messageId":"aLgVIPpW9QBFmgb7@pks.im","threadId":"64056","inReplyTo":"e02a444315acbc638a3d31279c10a936f0adb7b4.1756589007.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/6] midx-write: only load initialized packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T10:14:56Z","receivedAt":"2025-09-03T10:15:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Aug 30, 2025 at 09:23:22PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The fill_packs_from_midx() method was refactored in fcb2205b77 (midx:\n> implement support for writing incremental MIDX chains, 2024-08-06) to\n> allow for preferred packfiles and incremental multi-pack-indexes.\n> However, this led to some conditions that can cause improperly\n> initialized memory in the context's list of packfiles.\n> \n> The conditions caring about the preferred pack name or the incremental\n> flag are currently necessary to load a packfile. But the context is\n> still being populated with pack_info structs based on the packfile array\n> for the existing multi-pack-index even if prepare_midx_pack() isn't\n> called.\n\nI honestly don't quite understand why the conditions are necessary here.\nIn other words, why do we need to be careful _not_ to open the\npackfiles?\n\n> Add a new test that breaks under --stress when compiled with\n> SANITIZE=address. The chosen number of 100 packfiles was selected to get\n> the --stress output to fail about 50% of the time, while 50 packfiles\n> could not get a failure in most --stress runs.\n> \n> The test case is marked as EXPENSIVE not only because of the number of\n> packfiles it creates, but because some CI environments were reporting\n> errors during the test that I could not reproduce, specifically around\n> being unable to open the packfiles or their pack-indexes.\n> \n> When it fails under SANITIZE=address, it provides the following error:\n> \n> AddressSanitizer:DEADLYSIGNAL\n> =================================================================\n> ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027\n> ==3263517==The signal is caused by a READ memory access.\n> ==3263517==Hint: address points to the zero page.\n> #0 0x562d5d82d1fb in close_pack_windows packfile.c:299\n> #1 0x562d5d82d3ab in close_pack packfile.c:354\n> #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490\n> #3 0x562d5d7c7aec in midx_repack midx-write.c:1795\n> #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305\n> ...\n> \n> This failure stack trace is disconnected from the real fix because the bad\n> pointers are accessed later when closing the packfiles from the context.\n\nOkay. So in other words we need to make sure to always prepare the\nMIDX'd packfiles, but we may not want to open them?\n\n> There are a few different aspects to this fix that are worth noting:\n> \n> 1. We return to the previous behavior of fill_packs_from_midx to not\n> rely on the incremental flag or existence of a preferred pack.\n> \n> 2. The behavior to scan all layers of an incremental midx is kept, so\n> this is not a full revert of the change.\n> \n> 3. We skip allocating more room in the pack_info array if the pack\n> fails prepare_midx_pack().\n> \n> 4. The method has always returned 0 for success and 1 for failure, but\n> the condition checking for error added a check for a negative result\n> for failure, so that is now updated.\n\nNit, feel free to ignore: this change feels like it would make for a\nnice separate commit.\n\nPatrick\n"},{"id":"525412","messageId":"aLgVLMnbGiUAviXZ@pks.im","threadId":"64056","inReplyTo":"a1dd3ed87437322e298bb192fbfe4a9641d1356e.1756589007.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/6] midx-write: put failing response value back","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T10:15:08Z","receivedAt":"2025-09-03T10:15:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Aug 30, 2025 at 09:23:23PM +0000, Derrick Stolee via GitGitGadget wrote:\n> diff --git a/midx-write.c b/midx-write.c\n> index 070a7f61f4..0f1d5653ab 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1104,6 +1104,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n> \t\t\tm = m->base_midx;\n> \t\t}\n> \t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n> +\t\tresult = 1;\n> \t\tgoto cleanup;\n> \t}\n\nWould it make sense to also convert this command to return negative\nerror codes?\n\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 49705c62a2..008e65c22e 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -1100,7 +1100,10 @@ test_expect_success 'load reverse index when missing .idx, .pack' '\n> \t\tmv $idx.bak $idx &&\n> \n> \t\tmv $pack $pack.bak &&\n> -\t\tgit cat-file --batch-check=\"%(objectsize:disk)\" +\t\tgit cat-file --batch-check=\"%(objectsize:disk)\" +\n> +\t\ttest_must_fail git multi-pack-index write 2>err &&\n> +\t\tgrep \"could not load pack\" err\n\nNit: this should probably use `test_grep`.\n\nPatrick\n"},{"id":"525413","messageId":"aLgVMxIeOH_goTG5@pks.im","threadId":"64056","inReplyTo":"c4f75cca094dd47c431de52e335ccaaf8b06355b.1756589007.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/6] midx-write: use cleanup when incremental midx fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T10:15:15Z","receivedAt":"2025-09-03T10:15:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Aug 30, 2025 at 09:23:24PM +0000, Derrick Stolee via GitGitGadget wrote:\n> diff --git a/midx-write.c b/midx-write.c\n> index 0f1d5653ab..cb0211289d 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1327,13 +1327,15 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n> \t\tincr = mks_tempfile_m(midx_name.buf, 0444);\n> \t\tif (!incr) {\n> \t\t\terror(_(\"unable to create temporary MIDX layer\"));\n> -\t\t\treturn -1;\n> +\t\t\tresult = -1;\n> +\t\t\tgoto cleanup;\n\nWe can instead use `result = error(...); goto cleanup;` here and in most\nother cases.\n\nPatrick\n"},{"id":"525414","messageId":"aLgVOa96grWC3G0Q@pks.im","threadId":"64056","inReplyTo":"2290e27ded03866cb816be5fb6e08b83a75a6f07.1756589007.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/6] midx-write: use uint32_t for preferred_pack_idx","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T10:15:21Z","receivedAt":"2025-09-03T10:15:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Aug 30, 2025 at 09:23:25PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> midx-write.c has the DISABLE_SIGN_COMPARE_WARNINGS macro defined for a\n> few reasons, but the biggest one is the use of a signed\n> preferred_pack_idx member inside the write_midx_context struct. The code\n> currently uses -1 to indicate an unset preferred pack but pack int ids\n> are normally handled as uint32_t. There are also a few loops that search\n> for the preferred pack by name and those iterators will need updates to\n> uint32_t in the next change.\n> \n> For now, replace the use of -1 with a 'NO_PREFERRED_PACK' macro and an\n> equality check. The macro stores the max value of a uint32_t, so we\n> cannot store a preferred pack that appears last in a list of 2^32 total\n> packs, but that's expected to be unreasonable already. This improves the\n> range from 2^31 already.\n\nTiny nit: the last sentence reads a bit funny. Maybe something like\nthis?\n\n Furthermore, with this change we end up extending the range from\n 2^31 possible packs to 2^32-1.\n\n> There are some careful things to worry about with initializing the\n> preferred pack in the struct and using that value when searching for a\n> preferred pack that was already incorrect but accidentally working when\n> the index was initialized to zero.\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> midx-write.c | 26 +++++++++++++++-----------\n> 1 file changed, 15 insertions(+), 11 deletions(-)\n> \n> diff --git a/midx-write.c b/midx-write.c\n> index cb0211289d..1822268ce2 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -274,7 +275,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n> \tend = m->num_objects_in_base + ntohl(m->chunk_oid_fanout[cur_fanout]);\n> \n> \tfor (cur_object = start; cur_object < end; cur_object++) {\n> -\t\tif ((preferred_pack > -1) &&\n> +\t\tif ((preferred_pack != NO_PREFERRED_PACK) &&\n> \t\t (preferred_pack == nth_midxed_pack_int_id(m, cur_object))) {\n> \t\t\t/*\n> \t\t\t * Objects from preferred packs are added\n\nNeither of these braces around comparisons are really needed, but feel\nfree to ignore as you simply piggy-back on existing style.\n\n> @@ -1040,7 +1042,9 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n> \tstruct hashfile *f = NULL;\n> \tstruct lock_file lk;\n> \tstruct tempfile *incr;\n> -\tstruct write_midx_context ctx = { 0 };\n> +\tstruct write_midx_context ctx = {\n> +\t\t.preferred_pack_idx = NO_PREFERRED_PACK,\n> +\t };\n> \tint bitmapped_packs_concat_len = 0;\n> \tint pack_name_concat_len = 0;\n> \tint dropped_packs = 0;\n\nWhy is this change needed? We didn't previously initialize\n`.preferred_pack_idx = -1` either.\n\nPatrick\n"},{"id":"525415","messageId":"aLgVPx5gOI9ZIRu_@pks.im","threadId":"64056","inReplyTo":"35302f52285d98c3e7ed7ab4140b4721375bc8a3.1756589007.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/6] midx-write: reenable signed comparison errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T10:15:27Z","receivedAt":"2025-09-03T10:15:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Aug 30, 2025 at 09:23:26PM +0000, Derrick Stolee via GitGitGadget wrote:\n> diff --git a/midx-write.c b/midx-write.c\n> index 1822268ce2..14a0947c46 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1430,6 +1428,9 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n> \t * have been freed in the previous if block.\n> \t */\n> \n> +\tif (ctx.num_multi_pack_indexes_before == UINT32_MAX)\n> +\t\tdie(\"too many multi-pack-indexes\");\n> +\n> \tCALLOC_ARRAY(keep_hashes, ctx.num_multi_pack_indexes_before + 1);\n> \n> \tif (ctx.incremental) {\n\nShould this error message be translated?\n\nEverything else in this commit looks good to me. Thanks for cleaning\nthese up.\n\nPatrick\n"},{"id":"525416","messageId":"aLgVRMxNUrdScrjk@pks.im","threadId":"64056","inReplyTo":"7be25cf5349c389cf2887ab5b852779fc364bd7e.1756589007.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 6/6] midx-write: simplify error cases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T10:15:32Z","receivedAt":"2025-09-03T10:15:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Aug 30, 2025 at 09:23:27PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The write_midx_internal() method uses gotos to jump to a cleanup section to\n> clear memory before returning 'result'. Since these jumps are more common\n> for error conditions, initialize 'result' to -1 and then only set it to 0\n> before returning with success. There are a couple places where we return\n> with success via a jump.\n> \n> This has the added benefit that the method now returns -1 on error instead\n> of an inconsistent 1 or -1.\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> midx-write.c | 26 +++++++++-----------------\n> 1 file changed, 9 insertions(+), 17 deletions(-)\n> \n> diff --git a/midx-write.c b/midx-write.c\n> index 14a0947c46..047ffdcdbf 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1046,7 +1046,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n> \tint bitmapped_packs_concat_len = 0;\n> \tint pack_name_concat_len = 0;\n> \tint dropped_packs = 0;\n> -\tint result = 0;\n> +\tint result = -1;\n> \tconst char **keep_hashes = NULL;\n> \tstruct chunkfile *cf;\n\nI personally prefer to keep the result uninitialized and then assign the\nresult of `error()` to it. It's almost the same lines of code as we have\nright now, but it has the advantage that the compiler will complain\nabout `result` being uninitialized if we ever forget to set it. So it's\noverall way more explicit, and the compiler protects us.\n\nBut seeing that Junio previously recommended to go into the direction of\nsetting it to `-1` I won't insist on such a refactoring. So please feel\nfree to ignore this comment.\n\nPatrick\n"},{"id":"525456","messageId":"xmqq8qivfkcj.fsf@gitster.g","threadId":"64056","inReplyTo":"aLgVRMxNUrdScrjk@pks.im","subject":"Re: [PATCH v2 6/6] midx-write: simplify error cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-03T18:43:08Z","receivedAt":"2025-09-03T18:43:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> -\tint result = 0;\n>> +\tint result = -1;\n>> \tconst char **keep_hashes = NULL;\n>> \tstruct chunkfile *cf;\n>\n> I personally prefer to keep the result uninitialized and then assign the\n> result of `error()` to it. It's almost the same lines of code as we have\n> right now, but it has the advantage that the compiler will complain\n> about `result` being uninitialized if we ever forget to set it. So it's\n> overall way more explicit, and the compiler protects us.\n>\n> But seeing that Junio previously recommended to go into the direction of\n> setting it to `-1` I won't insist on such a refactoring. So please feel\n> free to ignore this comment.\n\nI am equally fine with uninitialized one, as long as compilers are\ntrustworthy in all cases. But the code path I made a comment IIRC\ndid not necessarily have calls to error(), so the same number of\ncode argument does not apply. And initializing it to zero is worse\nthan leaving it uninitialized.\n\nThanks.\n"},{"id":"525658","messageId":"745f5241-b745-481b-85e7-8cfe6ef73844@gmail.com","threadId":"64056","inReplyTo":"aLgVIPpW9QBFmgb7@pks.im","subject":"Re: [PATCH v2 1/6] midx-write: only load initialized packs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-09-05T18:58:40Z","receivedAt":"2025-09-05T18:59:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/3/2025 6:14 AM, Patrick Steinhardt wrote:\n> On Sat, Aug 30, 2025 at 09:23:22PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> The fill_packs_from_midx() method was refactored in fcb2205b77 (midx:\n>> implement support for writing incremental MIDX chains, 2024-08-06) to\n>> allow for preferred packfiles and incremental multi-pack-indexes.\n>> However, this led to some conditions that can cause improperly\n>> initialized memory in the context's list of packfiles.\n>>\n>> The conditions caring about the preferred pack name or the incremental\n>> flag are currently necessary to load a packfile. But the context is\n>> still being populated with pack_info structs based on the packfile array\n>> for the existing multi-pack-index even if prepare_midx_pack() isn't\n>> called.\n> \n> I honestly don't quite understand why the conditions are necessary here.\n> In other words, why do we need to be careful _not_ to open the\n> packfiles?\n\nMy wording is poor. \"We don't load packfiles unless one of these\nconditions holds\" is more appropriate.\n\nThere are some test cases that want to keep things working even\nwhen a .idx file disappears, I think. This is a reason to be\ncareful about open_pack_index(), but prepare_midx_pack() is\nsomething we want to call always.\n \n>> Add a new test that breaks under --stress when compiled with\n>> SANITIZE=address. The chosen number of 100 packfiles was selected to get\n>> the --stress output to fail about 50% of the time, while 50 packfiles\n>> could not get a failure in most --stress runs.\n>>\n>> The test case is marked as EXPENSIVE not only because of the number of\n>> packfiles it creates, but because some CI environments were reporting\n>> errors during the test that I could not reproduce, specifically around\n>> being unable to open the packfiles or their pack-indexes.\n>>\n>> When it fails under SANITIZE=address, it provides the following error:\n>>\n>> AddressSanitizer:DEADLYSIGNAL\n>> =================================================================\n>> ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027\n>> ==3263517==The signal is caused by a READ memory access.\n>> ==3263517==Hint: address points to the zero page.\n>> #0 0x562d5d82d1fb in close_pack_windows packfile.c:299\n>> #1 0x562d5d82d3ab in close_pack packfile.c:354\n>> #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490\n>> #3 0x562d5d7c7aec in midx_repack midx-write.c:1795\n>> #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305\n>> ...\n>>\n>> This failure stack trace is disconnected from the real fix because the bad\n>> pointers are accessed later when closing the packfiles from the context.\n> \n> Okay. So in other words we need to make sure to always prepare the\n> MIDX'd packfiles, but we may not want to open them?\n\nYes. Always prepare. Don't always open (since that loads the .idx).\n\n>> There are a few different aspects to this fix that are worth noting:\n>>\n>> 1. We return to the previous behavior of fill_packs_from_midx to not\n>> rely on the incremental flag or existence of a preferred pack.\n>>\n>> 2. The behavior to scan all layers of an incremental midx is kept, so\n>> this is not a full revert of the change.\n>>\n>> 3. We skip allocating more room in the pack_info array if the pack\n>> fails prepare_midx_pack().\n>>\n>> 4. The method has always returned 0 for success and 1 for failure, but\n>> the condition checking for error added a check for a negative result\n>> for failure, so that is now updated.\n> \n> Nit, feel free to ignore: this change feels like it would make for a\n> nice separate commit.\n\nTrue. I only included it since I was modifying the call anyway due to\nthe changing parameters.\n\nThanks,\n-Stolee\n\n"},{"id":"525659","messageId":"18ff26d7-6e9d-4a8d-9d58-aff69ee8b2bb@gmail.com","threadId":"64056","inReplyTo":"aLgVLMnbGiUAviXZ@pks.im","subject":"Re: [PATCH v2 2/6] midx-write: put failing response value back","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-09-05T19:03:04Z","receivedAt":"2025-09-05T19:03:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/3/2025 6:15 AM, Patrick Steinhardt wrote:\n> On Sat, Aug 30, 2025 at 09:23:23PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> diff --git a/midx-write.c b/midx-write.c\n>> index 070a7f61f4..0f1d5653ab 100644\n>> --- a/midx-write.c\n>> +++ b/midx-write.c\n>> @@ -1104,6 +1104,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n>> \t\t\tm = m->base_midx;\n>> \t\t}\n>> \t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n>> +\t\tresult = 1;\n>> \t\tgoto cleanup;\n>> \t}\n> \n> Would it make sense to also convert this command to return negative\n> error codes?\n\n(Yes, in patch 6) \n>> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n>> index 49705c62a2..008e65c22e 100755\n>> --- a/t/t5319-multi-pack-index.sh\n>> +++ b/t/t5319-multi-pack-index.sh\n>> @@ -1100,7 +1100,10 @@ test_expect_success 'load reverse index when missing .idx, .pack' '\n>> \t\tmv $idx.bak $idx &&\n>> \n>> \t\tmv $pack $pack.bak &&\n>> -\t\tgit cat-file --batch-check=\"%(objectsize:disk)\" > +\t\tgit cat-file --batch-check=\"%(objectsize:disk)\" > +\n>> +\t\ttest_must_fail git multi-pack-index write 2>err &&\n>> +\t\tgrep \"could not load pack\" err\n> \n> Nit: this should probably use `test_grep`.\n\nYou're right. I think I misremembered the preferred use back when\ntest_i18ngrep was deprecated.\n\nThanks,\n-Stolee\n"},{"id":"525660","messageId":"4c6468d6-02c1-4c79-8ec7-4d250341ca81@gmail.com","threadId":"64056","inReplyTo":"aLgVOa96grWC3G0Q@pks.im","subject":"Re: [PATCH v2 4/6] midx-write: use uint32_t for preferred_pack_idx","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-09-05T19:05:42Z","receivedAt":"2025-09-05T19:05:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/3/2025 6:15 AM, Patrick Steinhardt wrote:\n> On Sat, Aug 30, 2025 at 09:23:25PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> midx-write.c has the DISABLE_SIGN_COMPARE_WARNINGS macro defined for a\n>> few reasons, but the biggest one is the use of a signed\n>> preferred_pack_idx member inside the write_midx_context struct. The code\n>> currently uses -1 to indicate an unset preferred pack but pack int ids\n>> are normally handled as uint32_t. There are also a few loops that search\n>> for the preferred pack by name and those iterators will need updates to\n>> uint32_t in the next change.\n>>\n>> For now, replace the use of -1 with a 'NO_PREFERRED_PACK' macro and an\n>> equality check. The macro stores the max value of a uint32_t, so we\n>> cannot store a preferred pack that appears last in a list of 2^32 total\n>> packs, but that's expected to be unreasonable already. This improves the\n>> range from 2^31 already.\n> \n> Tiny nit: the last sentence reads a bit funny. Maybe something like\n> this?\n> \n> Furthermore, with this change we end up extending the range from\n> 2^31 possible packs to 2^32-1.\n\nThat is better.\n\n>> @@ -1040,7 +1042,9 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n>> \tstruct hashfile *f = NULL;\n>> \tstruct lock_file lk;\n>> \tstruct tempfile *incr;\n>> -\tstruct write_midx_context ctx = { 0 };\n>> +\tstruct write_midx_context ctx = {\n>> +\t\t.preferred_pack_idx = NO_PREFERRED_PACK,\n>> +\t };\n>> \tint bitmapped_packs_concat_len = 0;\n>> \tint pack_name_concat_len = 0;\n>> \tint dropped_packs = 0;\n> \n> Why is this change needed? We didn't previously initialize\n> `.preferred_pack_idx = -1` either.\n\nI think the previous lack of initialization was incorrect. It happened\nto work because it became initialized to -1 later _or_ its value of\nzero was implicitly used when searching for a preferred pack.\n\nI thought it prudent to set this value as the default instead of\nimplying that the 0th packfile was preferred.\n\nThanks,\n-Stolee\n\n"},{"id":"525661","messageId":"pull.1965.v3.git.1757100378.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v2.git.1756589007.gitgitgadget@gmail.com","subject":"[PATCH v3 0/6] midx-write: fix segfault and do several cleanups","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-09-05T19:26:12Z","receivedAt":"2025-09-05T19:26:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I was motivated to start looking closely at midx-write.c due to multiple\nusers reporting Git crashes in their background maintenance, specifically\nduring git multi-pack-index repack calls. I was eventually able to reproduce\nit in git multi-pack-index expire as well.\n\nPatch 1 is the only change we need to fix this bug. It includes a test case\nthat will fail under --stress with SANITIZE=address. It requires creating\nmany packfiles (50 was not enough, but 100 is enough). As far as I can tell,\nthis bug has existed since Git 2.47.0 in October 2024, but I started hearing\nreports of this from users in July 2025 (and took a while to get a\ndump/repro).\n\nThe remaining patches are cleanups based on my careful rereading of\nmidx-write.c. There are some issues about error handling that needed some\ncleanup as well as a removal of the DISABLE_SIGN_COMPARE_WARNINGS macro.\n\n\nUpdates in V3\n=============\n\n * Use test_grep over grep.\n * Translate an error message.\n * Clarify a commit message.\n\n\nUpdates in V2\n=============\n\n * A stale comment to an unsubmitted version of the test is removed.\n * More cases needing open_pack_index() are patched.\n * Typos fixed.\n * A new patch assumes error and sets result to zero only on the few\n successful paths.\n\nThanks, -Stolee\n\nDerrick Stolee (6):\n midx-write: only load initialized packs\n midx-write: put failing response value back\n midx-write: use cleanup when incremental midx fails\n midx-write: use uint32_t for preferred_pack_idx\n midx-write: reenable signed comparison errors\n midx-write: simplify error cases\n\n midx-write.c | 134 +++++++++++++++++-------------------\n t/t5319-multi-pack-index.sh | 22 +++++-\n 2 files changed, 86 insertions(+), 70 deletions(-)\n\n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1965%2Fderrickstolee%2Fmidx-write-cleanup-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1965/derrickstolee/midx-write-cleanup-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1965\n\nRange-diff vs v2:\n\n 1: e02a444315 = 1: e02a444315 midx-write: only load initialized packs\n 2: a1dd3ed874 ! 2: 1e5f43a417 midx-write: put failing response value back\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'load reverse index when missin\n +\t\tgit cat-file --batch-check=\"%(objectsize:disk)\" err &&\n -+\t\tgrep \"could not load pack\" err\n ++\t\ttest_grep \"could not load pack\" err\n \t)\n '\n \n 3: c4f75cca09 = 3: 414ae51024 midx-write: use cleanup when incremental midx fails\n 4: 2290e27ded ! 4: b113b3f012 midx-write: use uint32_t for preferred_pack_idx\n @@ Commit message\n For now, replace the use of -1 with a 'NO_PREFERRED_PACK' macro and an\n equality check. The macro stores the max value of a uint32_t, so we\n cannot store a preferred pack that appears last in a list of 2^32 total\n - packs, but that's expected to be unreasonable already. This improves the\n - range from 2^31 already.\n + packs, but that's expected to be unreasonable already. Furthermore, with\n + this change we end up extending the range from 2^31 possible packs to\n + 2^32-1.\n \n There are some careful things to worry about with initializing the\n preferred pack in the struct and using that value when searching for a\n 5: 35302f5228 ! 5: 7c68f2535c midx-write: reenable signed comparison errors\n @@ midx-write.c: static int write_midx_internal(struct repository *r, const char *o\n \t */\n \n +\tif (ctx.num_multi_pack_indexes_before == UINT32_MAX)\n -+\t\tdie(\"too many multi-pack-indexes\");\n ++\t\tdie(_(\"too many multi-pack-indexes\"));\n +\n \tCALLOC_ARRAY(keep_hashes, ctx.num_multi_pack_indexes_before + 1);\n \n 6: 7be25cf534 = 6: 224be4ee5c midx-write: simplify error cases\n\n-- \ngitgitgadget\n"},{"id":"525662","messageId":"e02a444315acbc638a3d31279c10a936f0adb7b4.1757100378.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v3.git.1757100378.gitgitgadget@gmail.com","subject":"[PATCH v3 1/6] midx-write: only load initialized packs","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-09-05T19:26:13Z","receivedAt":"2025-09-05T19:26:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe fill_packs_from_midx() method was refactored in fcb2205b77 (midx:\nimplement support for writing incremental MIDX chains, 2024-08-06) to\nallow for preferred packfiles and incremental multi-pack-indexes.\nHowever, this led to some conditions that can cause improperly\ninitialized memory in the context's list of packfiles.\n\nThe conditions caring about the preferred pack name or the incremental\nflag are currently necessary to load a packfile. But the context is\nstill being populated with pack_info structs based on the packfile array\nfor the existing multi-pack-index even if prepare_midx_pack() isn't\ncalled.\n\nAdd a new test that breaks under --stress when compiled with\nSANITIZE=address. The chosen number of 100 packfiles was selected to get\nthe --stress output to fail about 50% of the time, while 50 packfiles\ncould not get a failure in most --stress runs.\n\nThe test case is marked as EXPENSIVE not only because of the number of\npackfiles it creates, but because some CI environments were reporting\nerrors during the test that I could not reproduce, specifically around\nbeing unable to open the packfiles or their pack-indexes.\n\nWhen it fails under SANITIZE=address, it provides the following error:\n\nAddressSanitizer:DEADLYSIGNAL\n=================================================================\n==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027\n==3263517==The signal is caused by a READ memory access.\n==3263517==Hint: address points to the zero page.\n #0 0x562d5d82d1fb in close_pack_windows packfile.c:299\n #1 0x562d5d82d3ab in close_pack packfile.c:354\n #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490\n #3 0x562d5d7c7aec in midx_repack midx-write.c:1795\n #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305\n ...\n\nThis failure stack trace is disconnected from the real fix because the bad\npointers are accessed later when closing the packfiles from the context.\n\nThere are a few different aspects to this fix that are worth noting:\n\n 1. We return to the previous behavior of fill_packs_from_midx to not\n rely on the incremental flag or existence of a preferred pack.\n\n 2. The behavior to scan all layers of an incremental midx is kept, so\n this is not a full revert of the change.\n\n 3. We skip allocating more room in the pack_info array if the pack\n fails prepare_midx_pack().\n\n 4. The method has always returned 0 for success and 1 for failure, but\n the condition checking for error added a check for a negative result\n for failure, so that is now updated.\n\n 5. The call to open_pack_index() is removed, but this is needed later\n in the case of a preferred pack. That call is moved to immediately\n before its result is needed (checking for the object count).\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 46 +++++++++++++++----------------------\n t/t5319-multi-pack-index.sh | 17 ++++++++++++++\n 2 files changed, 36 insertions(+), 27 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex a0aceab5e0..070a7f61f4 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -920,8 +920,7 @@ static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,\n \treturn get_multi_pack_index(source);\n }\n \n-static int fill_packs_from_midx(struct write_midx_context *ctx,\n-\t\t\t\tconst char *preferred_pack_name, uint32_t flags)\n+static int fill_packs_from_midx(struct write_midx_context *ctx)\n {\n \tstruct multi_pack_index *m;\n \n@@ -929,30 +928,11 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n \t\tuint32_t i;\n \n \t\tfor (i = 0; i < m->num_packs; i++) {\n-\t\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n-\n-\t\t\t/*\n-\t\t\t * If generating a reverse index, need to have\n-\t\t\t * packed_git's loaded to compare their\n-\t\t\t * mtimes and object count.\n-\t\t\t *\n-\t\t\t * If a preferred pack is specified, need to\n-\t\t\t * have packed_git's loaded to ensure the chosen\n-\t\t\t * preferred pack has a non-zero object count.\n-\t\t\t */\n-\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n-\t\t\t preferred_pack_name) {\n-\t\t\t\tif (prepare_midx_pack(ctx->repo, m,\n-\t\t\t\t\t\t m->num_packs_in_base + i)) {\n-\t\t\t\t\terror(_(\"could not load pack\"));\n-\t\t\t\t\treturn 1;\n-\t\t\t\t}\n-\n-\t\t\t\tif (open_pack_index(m->packs[i]))\n-\t\t\t\t\tdie(_(\"could not open index for %s\"),\n-\t\t\t\t\t m->packs[i]->pack_name);\n-\t\t\t}\n+\t\t\tif (prepare_midx_pack(ctx->repo, m,\n+\t\t\t\t\t m->num_packs_in_base + i))\n+\t\t\t\treturn error(_(\"could not load pack\"));\n \n+\t\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n \t\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n \t\t\t\t m->pack_names[i],\n \t\t\t\t m->num_packs_in_base + i);\n@@ -1123,8 +1103,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\tctx.num_multi_pack_indexes_before++;\n \t\t\tm = m->base_midx;\n \t\t}\n-\t} else if (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n-\t\t\t\t\t\t flags) < 0) {\n+\t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n \t\tgoto cleanup;\n \t}\n \n@@ -1186,6 +1165,13 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tstruct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p;\n \t\tctx.preferred_pack_idx = 0;\n \n+\t\t/*\n+\t\t * Attempt opening the pack index to populate num_objects.\n+\t\t * Ignore failiures as they can be expected and are not\n+\t\t * fatal during this selection time.\n+\t\t */\n+\t\topen_pack_index(oldest);\n+\n \t\tif (packs_to_drop && packs_to_drop->nr)\n \t\t\tBUG(\"cannot write a MIDX bitmap during expiration\");\n \n@@ -1200,6 +1186,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \t\t\tif (!oldest->num_objects || p->mtime < oldest->mtime) {\n \t\t\t\toldest = p;\n+\t\t\t\topen_pack_index(oldest);\n \t\t\t\tctx.preferred_pack_idx = i;\n \t\t\t}\n \t\t}\n@@ -1223,6 +1210,11 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \tif (ctx.preferred_pack_idx > -1) {\n \t\tstruct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;\n+\n+\t\tif (open_pack_index(preferred))\n+\t\t\tdie(_(\"failed to open preferred pack %s\"),\n+\t\t\t ctx.info[ctx.preferred_pack_idx].pack_name);\n+\n \t\tif (!preferred->num_objects) {\n \t\t\terror(_(\"cannot select preferred pack %s with no objects\"),\n \t\t\t preferred->pack_name);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex bd75dea950..49705c62a2 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -989,6 +989,23 @@ test_expect_success 'repack --batch-size=0 repacks everything' '\n \t)\n '\n \n+test_expect_success EXPENSIVE 'repack/expire with many packs' '\n+\tcp -r dup many &&\n+\t(\n+\t\tcd many &&\n+\n+\t\tfor i in $(test_seq 1 100)\n+\t\tdo\n+\t\t\ttest_commit extra$i &&\n+\t\t\tgit maintenance run --task=loose-objects || return 1\n+\t\tdone &&\n+\n+\t\tgit multi-pack-index write &&\n+\t\tgit multi-pack-index repack &&\n+\t\tgit multi-pack-index expire\n+\t)\n+'\n+\n test_expect_success 'repack --batch-size= repacks everything' '\n \t(\n \t\tcd dup2 &&\n-- \ngitgitgadget\n\n"},{"id":"525663","messageId":"1e5f43a417207e243a947d09255d71c26a52ef59.1757100378.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v3.git.1757100378.gitgitgadget@gmail.com","subject":"[PATCH v3 2/6] midx-write: put failing response value back","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-09-05T19:26:14Z","receivedAt":"2025-09-05T19:26:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThis instance of setting the result to 1 before going to cleanup was\naccidentally removed in fcb2205b77 (midx: implement support for writing\nincremental MIDX chains, 2024-08-06). Build upon a test that already deletes\na packfile to verify that this error propagates to full command failure.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 1 +\n t/t5319-multi-pack-index.sh | 5 ++++-\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 070a7f61f4..0f1d5653ab 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1104,6 +1104,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\tm = m->base_midx;\n \t\t}\n \t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n+\t\tresult = 1;\n \t\tgoto cleanup;\n \t}\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 49705c62a2..2c22fdb931 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1100,7 +1100,10 @@ test_expect_success 'load reverse index when missing .idx, .pack' '\n \t\tmv $idx.bak $idx &&\n \n \t\tmv $pack $pack.bak &&\n-\t\tgit cat-file --batch-check=\"%(objectsize:disk)\" err &&\n+\t\ttest_grep \"could not load pack\" err\n \t)\n '\n \n-- \ngitgitgadget\n\n"},{"id":"525664","messageId":"414ae51024ef04370180fd550b4cd51980cc7133.1757100378.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v3.git.1757100378.gitgitgadget@gmail.com","subject":"[PATCH v3 3/6] midx-write: use cleanup when incremental midx fails","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-09-05T19:26:15Z","receivedAt":"2025-09-05T19:26:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe incremental mode of writing a multi-pack-index has a few extra\nconditions that could lead to failure, but these are currently\nshort-ciruiting with 'return -1' instead of setting the method's\n'result' variable and going to the cleanup tag.\n\nReplace these returns with gotos to avoid memory issues when exiting\nearly due to error conditions.\n\nUnfortunately, these error conditions are difficult to reproduce with\ntest cases, which is perhaps one reason why the memory loss was not\ncaught by existing test cases in memory tracking modes.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 0f1d5653ab..cb0211289d 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1327,13 +1327,15 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tincr = mks_tempfile_m(midx_name.buf, 0444);\n \t\tif (!incr) {\n \t\t\terror(_(\"unable to create temporary MIDX layer\"));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n \t\tif (adjust_shared_perm(r, get_tempfile_path(incr))) {\n \t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n \t\t\t get_tempfile_path(incr));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n \t\tf = hashfd(r->hash_algo, get_tempfile_fd(incr),\n@@ -1433,18 +1435,22 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \t\tif (!chainf) {\n \t\t\terror_errno(_(\"unable to open multi-pack-index chain file\"));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n-\t\tif (link_midx_to_chain(ctx.base_midx) < 0)\n-\t\t\treturn -1;\n+\t\tif (link_midx_to_chain(ctx.base_midx) < 0) {\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \n \t\tget_split_midx_filename_ext(r->hash_algo, &final_midx_name,\n \t\t\t\t\t object_dir, midx_hash, MIDX_EXT_MIDX);\n \n \t\tif (rename_tempfile(&incr, final_midx_name.buf) < 0) {\n \t\t\terror_errno(_(\"unable to rename new multi-pack-index layer\"));\n-\t\t\treturn -1;\n+\t\t\tresult = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n \t\tstrbuf_release(&final_midx_name);\n-- \ngitgitgadget\n\n"},{"id":"525665","messageId":"b113b3f01238c393af647cf7718cbafa628209ea.1757100378.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v3.git.1757100378.gitgitgadget@gmail.com","subject":"[PATCH v3 4/6] midx-write: use uint32_t for preferred_pack_idx","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-09-05T19:26:16Z","receivedAt":"2025-09-05T19:26:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nmidx-write.c has the DISABLE_SIGN_COMPARE_WARNINGS macro defined for a\nfew reasons, but the biggest one is the use of a signed\npreferred_pack_idx member inside the write_midx_context struct. The code\ncurrently uses -1 to indicate an unset preferred pack but pack int ids\nare normally handled as uint32_t. There are also a few loops that search\nfor the preferred pack by name and those iterators will need updates to\nuint32_t in the next change.\n\nFor now, replace the use of -1 with a 'NO_PREFERRED_PACK' macro and an\nequality check. The macro stores the max value of a uint32_t, so we\ncannot store a preferred pack that appears last in a list of 2^32 total\npacks, but that's expected to be unreasonable already. Furthermore, with\nthis change we end up extending the range from 2^31 possible packs to\n2^32-1.\n\nThere are some careful things to worry about with initializing the\npreferred pack in the struct and using that value when searching for a\npreferred pack that was already incorrect but accidentally working when\nthe index was initialized to zero.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 26 +++++++++++++++-----------\n 1 file changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex cb0211289d..1822268ce2 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -24,6 +24,7 @@\n #define BITMAP_POS_UNKNOWN (~((uint32_t)0))\n #define MIDX_CHUNK_FANOUT_SIZE (sizeof(uint32_t) * 256)\n #define MIDX_CHUNK_LARGE_OFFSET_WIDTH (sizeof(uint64_t))\n+#define NO_PREFERRED_PACK (~((uint32_t)0))\n \n extern int midx_checksum_valid(struct multi_pack_index *m);\n extern void clear_midx_files_ext(const char *object_dir, const char *ext,\n@@ -104,7 +105,7 @@ struct write_midx_context {\n \tunsigned large_offsets_needed:1;\n \tuint32_t num_large_offsets;\n \n-\tint preferred_pack_idx;\n+\tuint32_t preferred_pack_idx;\n \n \tint incremental;\n \tuint32_t num_multi_pack_indexes_before;\n@@ -260,7 +261,7 @@ static void midx_fanout_sort(struct midx_fanout *fanout)\n static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \t\t\t\t\tstruct multi_pack_index *m,\n \t\t\t\t\tuint32_t cur_fanout,\n-\t\t\t\t\tint preferred_pack)\n+\t\t\t\t\tuint32_t preferred_pack)\n {\n \tuint32_t start = m->num_objects_in_base, end;\n \tuint32_t cur_object;\n@@ -274,7 +275,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,\n \tend = m->num_objects_in_base + ntohl(m->chunk_oid_fanout[cur_fanout]);\n \n \tfor (cur_object = start; cur_object < end; cur_object++) {\n-\t\tif ((preferred_pack > -1) &&\n+\t\tif ((preferred_pack != NO_PREFERRED_PACK) &&\n \t\t (preferred_pack == nth_midxed_pack_int_id(m, cur_object))) {\n \t\t\t/*\n \t\t\t * Objects from preferred packs are added\n@@ -364,7 +365,8 @@ static void compute_sorted_entries(struct write_midx_context *ctx,\n \t\t\t\t\t\t preferred, cur_fanout);\n \t\t}\n \n-\t\tif (-1 < ctx->preferred_pack_idx && ctx->preferred_pack_idx < start_pack)\n+\t\tif (ctx->preferred_pack_idx != NO_PREFERRED_PACK &&\n+\t\t ctx->preferred_pack_idx < start_pack)\n \t\t\tmidx_fanout_add_pack_fanout(&fanout, ctx->info,\n \t\t\t\t\t\t ctx->preferred_pack_idx, 1,\n \t\t\t\t\t\t cur_fanout);\n@@ -1040,7 +1042,9 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct tempfile *incr;\n-\tstruct write_midx_context ctx = { 0 };\n+\tstruct write_midx_context ctx = {\n+\t\t.preferred_pack_idx = NO_PREFERRED_PACK,\n+\t };\n \tint bitmapped_packs_concat_len = 0;\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n@@ -1148,7 +1152,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tgoto cleanup; /* nothing to do */\n \n \tif (preferred_pack_name) {\n-\t\tctx.preferred_pack_idx = -1;\n+\t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \n \t\tfor (i = 0; i < ctx.nr; i++) {\n \t\t\tif (!cmp_idx_or_pack_name(preferred_pack_name,\n@@ -1158,12 +1162,12 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t}\n \t\t}\n \n-\t\tif (ctx.preferred_pack_idx == -1)\n+\t\tif (ctx.preferred_pack_idx == NO_PREFERRED_PACK)\n \t\t\twarning(_(\"unknown preferred pack: '%s'\"),\n \t\t\t\tpreferred_pack_name);\n \t} else if (ctx.nr &&\n \t\t (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))) {\n-\t\tstruct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p;\n+\t\tstruct packed_git *oldest = ctx.info[0].p;\n \t\tctx.preferred_pack_idx = 0;\n \n \t\t/*\n@@ -1199,17 +1203,17 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t * objects to resolve, so the preferred value doesn't\n \t\t\t * matter.\n \t\t\t */\n-\t\t\tctx.preferred_pack_idx = -1;\n+\t\t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \t\t}\n \t} else {\n \t\t/*\n \t\t * otherwise don't mark any pack as preferred to avoid\n \t\t * interfering with expiration logic below\n \t\t */\n-\t\tctx.preferred_pack_idx = -1;\n+\t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \t}\n \n-\tif (ctx.preferred_pack_idx > -1) {\n+\tif (ctx.preferred_pack_idx != NO_PREFERRED_PACK) {\n \t\tstruct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;\n \n \t\tif (open_pack_index(preferred))\n-- \ngitgitgadget\n\n"},{"id":"525666","messageId":"7c68f2535cd5084ae796ef0e8767caecaf1c7c3f.1757100378.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v3.git.1757100378.gitgitgadget@gmail.com","subject":"[PATCH v3 5/6] midx-write: reenable signed comparison errors","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-09-05T19:26:17Z","receivedAt":"2025-09-05T19:26:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nRemove the remaining signed comparison warnings in midx-write.c so that\nthey can be enforced as errors in the future. After the previous change,\nthe remaining errors are due to iterator variables named 'i'.\n\nThe strategy here involves defining the variable within the for loop\nsyntax to make sure we use the appropriate bitness for the loop\nsentinel. This matters in at least one method where the variable was\ncompared to uint32_t in some loops and size_t in others.\n\nWhile adjusting these loops, there were some where the loop boundary was\nchecking against a uint32_t value _plus one_. These were replaced with\nnon-strict comparisons, but also the value is checked to not be\nUINT32_MAX. Since the value is the number of incremental multi-pack-\nindexes, this is not a meaningful restriction. The new die() is about\ndefensive programming more than it being realistically possible.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 35 ++++++++++++++++++-----------------\n 1 file changed, 18 insertions(+), 17 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 1822268ce2..cd7bf7554a 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1,5 +1,3 @@\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n #include \"abspath.h\"\n #include \"config.h\"\n@@ -845,7 +843,7 @@ static int write_midx_bitmap(struct write_midx_context *ctx,\n \t\t\t uint32_t commits_nr,\n \t\t\t unsigned flags)\n {\n-\tint ret, i;\n+\tint ret;\n \tuint16_t options = 0;\n \tstruct bitmap_writer writer;\n \tstruct pack_idx_entry **index;\n@@ -873,7 +871,7 @@ static int write_midx_bitmap(struct write_midx_context *ctx,\n \t * this order).\n \t */\n \tALLOC_ARRAY(index, pdata->nr_objects);\n-\tfor (i = 0; i < pdata->nr_objects; i++)\n+\tfor (uint32_t i = 0; i < pdata->nr_objects; i++)\n \t\tindex[i] = &pdata->objects[i].idx;\n \n \tbitmap_writer_init(&writer, ctx->repo, pdata,\n@@ -894,7 +892,7 @@ static int write_midx_bitmap(struct write_midx_context *ctx,\n \t * happens between bitmap_writer_build_type_index() and\n \t * bitmap_writer_finish().\n \t */\n-\tfor (i = 0; i < pdata->nr_objects; i++)\n+\tfor (uint32_t i = 0; i < pdata->nr_objects; i++)\n \t\tindex[ctx->pack_order[i]] = &pdata->objects[i].idx;\n \n \tbitmap_writer_select_commits(&writer, commits, commits_nr);\n@@ -1038,7 +1036,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n {\n \tstruct strbuf midx_name = STRBUF_INIT;\n \tunsigned char midx_hash[GIT_MAX_RAWSZ];\n-\tuint32_t i, start_pack;\n+\tuint32_t start_pack;\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct tempfile *incr;\n@@ -1154,7 +1152,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tif (preferred_pack_name) {\n \t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n \n-\t\tfor (i = 0; i < ctx.nr; i++) {\n+\t\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\t\tif (!cmp_idx_or_pack_name(preferred_pack_name,\n \t\t\t\t\t\t ctx.info[i].pack_name)) {\n \t\t\t\tctx.preferred_pack_idx = i;\n@@ -1186,7 +1184,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t * pack-order has all of its objects selected from that pack\n \t\t * (and not another pack containing a duplicate)\n \t\t */\n-\t\tfor (i = 1; i < ctx.nr; i++) {\n+\t\tfor (size_t i = 1; i < ctx.nr; i++) {\n \t\t\tstruct packed_git *p = ctx.info[i].p;\n \n \t\t\tif (!oldest->num_objects || p->mtime < oldest->mtime) {\n@@ -1231,7 +1229,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tcompute_sorted_entries(&ctx, start_pack);\n \n \tctx.large_offsets_needed = 0;\n-\tfor (i = 0; i < ctx.entries_nr; i++) {\n+\tfor (size_t i = 0; i < ctx.entries_nr; i++) {\n \t\tif (ctx.entries[i].offset > 0x7fffffff)\n \t\t\tctx.num_large_offsets++;\n \t\tif (ctx.entries[i].offset > 0xffffffff)\n@@ -1241,10 +1239,10 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tQSORT(ctx.info, ctx.nr, pack_info_compare);\n \n \tif (packs_to_drop && packs_to_drop->nr) {\n-\t\tint drop_index = 0;\n+\t\tsize_t drop_index = 0;\n \t\tint missing_drops = 0;\n \n-\t\tfor (i = 0; i < ctx.nr && drop_index < packs_to_drop->nr; i++) {\n+\t\tfor (size_t i = 0; i < ctx.nr && drop_index < packs_to_drop->nr; i++) {\n \t\t\tint cmp = strcmp(ctx.info[i].pack_name,\n \t\t\t\t\t packs_to_drop->items[drop_index].string);\n \n@@ -1275,7 +1273,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t * pack_perm[old_id] = new_id\n \t */\n \tALLOC_ARRAY(ctx.pack_perm, ctx.nr);\n-\tfor (i = 0; i < ctx.nr; i++) {\n+\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\tif (ctx.info[i].expired) {\n \t\t\tdropped_packs++;\n \t\t\tctx.pack_perm[ctx.info[i].orig_pack_int_id] = PACK_EXPIRED;\n@@ -1284,7 +1282,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t}\n \t}\n \n-\tfor (i = 0; i < ctx.nr; i++) {\n+\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\tif (ctx.info[i].expired)\n \t\t\tcontinue;\n \t\tpack_name_concat_len += strlen(ctx.info[i].pack_name) + 1;\n@@ -1430,6 +1428,9 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t * have been freed in the previous if block.\n \t */\n \n+\tif (ctx.num_multi_pack_indexes_before == UINT32_MAX)\n+\t\tdie(_(\"too many multi-pack-indexes\"));\n+\n \tCALLOC_ARRAY(keep_hashes, ctx.num_multi_pack_indexes_before + 1);\n \n \tif (ctx.incremental) {\n@@ -1462,7 +1463,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tkeep_hashes[ctx.num_multi_pack_indexes_before] =\n \t\t\txstrdup(hash_to_hex_algop(midx_hash, r->hash_algo));\n \n-\t\tfor (i = 0; i < ctx.num_multi_pack_indexes_before; i++) {\n+\t\tfor (uint32_t i = 0; i < ctx.num_multi_pack_indexes_before; i++) {\n \t\t\tuint32_t j = ctx.num_multi_pack_indexes_before - i - 1;\n \n \t\t\tkeep_hashes[j] = xstrdup(hash_to_hex_algop(get_midx_checksum(m),\n@@ -1470,7 +1471,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\tm = m->base_midx;\n \t\t}\n \n-\t\tfor (i = 0; i < ctx.num_multi_pack_indexes_before + 1; i++)\n+\t\tfor (uint32_t i = 0; i <= ctx.num_multi_pack_indexes_before; i++)\n \t\t\tfprintf(get_lock_file_fp(&lk), \"%s\\n\", keep_hashes[i]);\n \t} else {\n \t\tkeep_hashes[ctx.num_multi_pack_indexes_before] =\n@@ -1488,7 +1489,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t ctx.incremental);\n \n cleanup:\n-\tfor (i = 0; i < ctx.nr; i++) {\n+\tfor (size_t i = 0; i < ctx.nr; i++) {\n \t\tif (ctx.info[i].p) {\n \t\t\tclose_pack(ctx.info[i].p);\n \t\t\tfree(ctx.info[i].p);\n@@ -1501,7 +1502,7 @@ cleanup:\n \tfree(ctx.pack_perm);\n \tfree(ctx.pack_order);\n \tif (keep_hashes) {\n-\t\tfor (i = 0; i < ctx.num_multi_pack_indexes_before + 1; i++)\n+\t\tfor (uint32_t i = 0; i <= ctx.num_multi_pack_indexes_before; i++)\n \t\t\tfree((char *)keep_hashes[i]);\n \t\tfree(keep_hashes);\n \t}\n-- \ngitgitgadget\n\n"},{"id":"525667","messageId":"224be4ee5c66a5d161ba35754dd324b8ee53d07c.1757100378.git.gitgitgadget@gmail.com","threadId":"64056","inReplyTo":"pull.1965.v3.git.1757100378.gitgitgadget@gmail.com","subject":"[PATCH v3 6/6] midx-write: simplify error cases","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-09-05T19:26:18Z","receivedAt":"2025-09-05T19:26:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe write_midx_internal() method uses gotos to jump to a cleanup section to\nclear memory before returning 'result'. Since these jumps are more common\nfor error conditions, initialize 'result' to -1 and then only set it to 0\nbefore returning with success. There are a couple places where we return\nwith success via a jump.\n\nThis has the added benefit that the method now returns -1 on error instead\nof an inconsistent 1 or -1.\n\nSigned-off-by: Derrick Stolee \n---\n midx-write.c | 26 +++++++++-----------------\n 1 file changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex cd7bf7554a..72189f74bb 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1046,7 +1046,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tint bitmapped_packs_concat_len = 0;\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n-\tint result = 0;\n+\tint result = -1;\n \tconst char **keep_hashes = NULL;\n \tstruct chunkfile *cf;\n \n@@ -1099,14 +1099,12 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t\terror(_(\"could not load reverse index for MIDX %s\"),\n \t\t\t\t hash_to_hex_algop(get_midx_checksum(m),\n \t\t\t\t\t\t\tm->repo->hash_algo));\n-\t\t\t\tresult = 1;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \t\t\tctx.num_multi_pack_indexes_before++;\n \t\t\tm = m->base_midx;\n \t\t}\n \t} else if (ctx.m && fill_packs_from_midx(&ctx)) {\n-\t\tresult = 1;\n \t\tgoto cleanup;\n \t}\n \n@@ -1142,12 +1140,16 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t */\n \t\t\tif (!want_bitmap)\n \t\t\t\tclear_midx_files_ext(object_dir, \"bitmap\", NULL);\n+\n+\t\t\tresult = 0;\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n \n-\tif (ctx.incremental && !ctx.nr)\n+\tif (ctx.incremental && !ctx.nr) {\n+\t\tresult = 0;\n \t\tgoto cleanup; /* nothing to do */\n+\t}\n \n \tif (preferred_pack_name) {\n \t\tctx.preferred_pack_idx = NO_PREFERRED_PACK;\n@@ -1221,7 +1223,6 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tif (!preferred->num_objects) {\n \t\t\terror(_(\"cannot select preferred pack %s with no objects\"),\n \t\t\t preferred->pack_name);\n-\t\t\tresult = 1;\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n@@ -1260,10 +1261,8 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t}\n \t\t}\n \n-\t\tif (missing_drops) {\n-\t\t\tresult = 1;\n+\t\tif (missing_drops)\n \t\t\tgoto cleanup;\n-\t\t}\n \t}\n \n \t/*\n@@ -1309,7 +1308,6 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \tif (ctx.nr - dropped_packs == 0) {\n \t\terror(_(\"no pack files to index.\"));\n-\t\tresult = 1;\n \t\tgoto cleanup;\n \t}\n \n@@ -1329,14 +1327,12 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\tincr = mks_tempfile_m(midx_name.buf, 0444);\n \t\tif (!incr) {\n \t\t\terror(_(\"unable to create temporary MIDX layer\"));\n-\t\t\tresult = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \n \t\tif (adjust_shared_perm(r, get_tempfile_path(incr))) {\n \t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n \t\t\t get_tempfile_path(incr));\n-\t\t\tresult = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \n@@ -1414,7 +1410,6 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\t\t midx_hash, &pdata, commits, commits_nr,\n \t\t\t\t flags) < 0) {\n \t\t\terror(_(\"could not write multi-pack bitmap\"));\n-\t\t\tresult = 1;\n \t\t\tclear_packing_data(&pdata);\n \t\t\tfree(commits);\n \t\t\tgoto cleanup;\n@@ -1440,21 +1435,17 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \n \t\tif (!chainf) {\n \t\t\terror_errno(_(\"unable to open multi-pack-index chain file\"));\n-\t\t\tresult = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \n-\t\tif (link_midx_to_chain(ctx.base_midx) < 0) {\n-\t\t\tresult = -1;\n+\t\tif (link_midx_to_chain(ctx.base_midx) < 0)\n \t\t\tgoto cleanup;\n-\t\t}\n \n \t\tget_split_midx_filename_ext(r->hash_algo, &final_midx_name,\n \t\t\t\t\t object_dir, midx_hash, MIDX_EXT_MIDX);\n \n \t\tif (rename_tempfile(&incr, final_midx_name.buf) < 0) {\n \t\t\terror_errno(_(\"unable to rename new multi-pack-index layer\"));\n-\t\t\tresult = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \n@@ -1487,6 +1478,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \tclear_midx_files(r, object_dir, keep_hashes,\n \t\t\t ctx.num_multi_pack_indexes_before + 1,\n \t\t\t ctx.incremental);\n+\tresult = 0;\n \n cleanup:\n \tfor (size_t i = 0; i < ctx.nr; i++) {\n-- \ngitgitgadget\n"},{"id":"525670","messageId":"xmqqcy843d27.fsf@gitster.g","threadId":"64056","inReplyTo":"pull.1965.v3.git.1757100378.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/6] midx-write: fix segfault and do several cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-05T19:38:08Z","receivedAt":"2025-09-05T19:38:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> I was motivated to start looking closely at midx-write.c due to multiple\n> users reporting Git crashes in their background maintenance, specifically\n> during git multi-pack-index repack calls. I was eventually able to reproduce\n> it in git multi-pack-index expire as well.\n>\n> Patch 1 is the only change we need to fix this bug. It includes a test case\n> that will fail under --stress with SANITIZE=address. It requires creating\n> many packfiles (50 was not enough, but 100 is enough). As far as I can tell,\n> this bug has existed since Git 2.47.0 in October 2024, but I started hearing\n> reports of this from users in July 2025 (and took a while to get a\n> dump/repro).\n>\n> The remaining patches are cleanups based on my careful rereading of\n> midx-write.c. There are some issues about error handling that needed some\n> cleanup as well as a removal of the DISABLE_SIGN_COMPARE_WARNINGS macro.\n>\n>\n> Updates in V3\n> =============\n>\n> * Use test_grep over grep.\n> * Translate an error message.\n> * Clarify a commit message.\n\nAll incremental changes made sense to me. Will replace.\n\nShall we mark the topic ready for 'next' by now?\n\nThanks.\n"},{"id":"525673","messageId":"cc0c30f4-5e03-423e-a6d5-f8effb953f68@gmail.com","threadId":"64056","inReplyTo":"xmqqcy843d27.fsf@gitster.g","subject":"Re: [PATCH v3 0/6] midx-write: fix segfault and do several cleanups","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2025-09-05T19:57:10Z","receivedAt":"2025-09-05T19:57:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/5/2025 3:38 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> I was motivated to start looking closely at midx-write.c due to multiple\n>> users reporting Git crashes in their background maintenance, specifically\n>> during git multi-pack-index repack calls. I was eventually able to reproduce\n>> it in git multi-pack-index expire as well.\n>>\n>> Patch 1 is the only change we need to fix this bug. It includes a test case\n>> that will fail under --stress with SANITIZE=address. It requires creating\n>> many packfiles (50 was not enough, but 100 is enough). As far as I can tell,\n>> this bug has existed since Git 2.47.0 in October 2024, but I started hearing\n>> reports of this from users in July 2025 (and took a while to get a\n>> dump/repro).\n>>\n>> The remaining patches are cleanups based on my careful rereading of\n>> midx-write.c. There are some issues about error handling that needed some\n>> cleanup as well as a removal of the DISABLE_SIGN_COMPARE_WARNINGS macro.\n>>\n>>\n>> Updates in V3\n>> =============\n>>\n>> * Use test_grep over grep.\n>> * Translate an error message.\n>> * Clarify a commit message.\n> \n> All incremental changes made sense to me. Will replace.\n> \n> Shall we mark the topic ready for 'next' by now?\n\nI believe it's ready. Thanks.\n-Stolee\n"},{"id":"526158","messageId":"aMNXhDn9O5+swQ2u@nand.local","threadId":"64056","inReplyTo":"cc0c30f4-5e03-423e-a6d5-f8effb953f68@gmail.com","subject":"Re: [PATCH v3 0/6] midx-write: fix segfault and do several cleanups","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-09-11T23:13:08Z","receivedAt":"2025-09-11T23:13:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 05, 2025 at 03:57:10PM -0400, Derrick Stolee wrote:\n> > All incremental changes made sense to me. Will replace.\n> >\n> > Shall we mark the topic ready for 'next' by now?\n>\n> I believe it's ready. Thanks.\n\nAgreed. I still have this nagging feeling that the reproduction case can\nbe made smaller, and I wish it weren't flaky (as I understand it to be\non Stolee's machine). But I don't see any reason that should hold up\nthis series.\n\nThanks,\nTaylor\n"},{"id":"526166","messageId":"xmqqjz24o8p5.fsf@gitster.g","threadId":"64056","inReplyTo":"aMNXhDn9O5+swQ2u@nand.local","subject":"Re: [PATCH v3 0/6] midx-write: fix segfault and do several cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-11T23:44:54Z","receivedAt":"2025-09-11T23:44:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Fri, Sep 05, 2025 at 03:57:10PM -0400, Derrick Stolee wrote:\n>> > All incremental changes made sense to me. Will replace.\n>> >\n>> > Shall we mark the topic ready for 'next' by now?\n>>\n>> I believe it's ready. Thanks.\n>\n> Agreed. I still have this nagging feeling that the reproduction case can\n> be made smaller, and I wish it weren't flaky (as I understand it to be\n> on Stolee's machine). But I don't see any reason that should hold up\n> this series.\n\nThanks, all.\n"}]}