midx-write: fix segfault and do several cleanups #1965

gitgitgadget · 2025-08-28T20:34:37Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes: > The fill_packs_from_midx() method was refactored in fcb2205b77 (midx: > implement support for writing incremental MIDX chains, 2024-08-06) to > allow for preferred packfiles and incremental multi-pack-indexes. > However, this led to some conditions that can cause improperly > initialized memory in the context's list of packfiles. > > The conditions caring about the preferred pack name or the incremental > flag are currently necessary to load a packfile. But the context is > still being populated with pack_info structs based on the packfile array > for the existing multi-pack-index even if prepare_midx_pack() isn't > called. > > Add a new test that breaks under --stress when compiled with > SANITIZE=address. The chosen number of 100 packfiles was selected to get > the --stress output to fail about 50% of the time, while 50 packfiles > could not get a failure in most --stress runs. This test has a very > minor check at the end confirming only one packfile remaining. The > failing nature of this test actually relies on auto-GC cleaning up some > packfiles during the creation of the commits, as tests setting gc.auto > to zero make the packfile count match the number of added commits but > also avoids hitting the memory issue. > > The test case is marked as EXPENSIVE not only because of the number of > packfiles it creates, but because some CI environments were reporting > errors during the test that I could not reproduce, specifically around > being unable to open the packfiles or their pack-indexes. > > When it fails under SANITIZE=address, it provides the following error: > > AddressSanitizer:DEADLYSIGNAL > ================================================================= > ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027 > ==3263517==The signal is caused by a READ memory access. > ==3263517==Hint: address points to the zero page. > #0 0x562d5d82d1fb in close_pack_windows packfile.c:299 > #1 0x562d5d82d3ab in close_pack packfile.c:354 > #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490 > #3 0x562d5d7c7aec in midx_repack midx-write.c:1795 > #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305 > ... > > This failure stack trace is disconnected from the real fix because it > the bad pointers are accessed later when closing the packfiles from the > context. "because it the bad pointers" -> ??? Perhaps just drop "it"? > There are a few different aspects to this fix that are worth noting: > > 1. We return to the previous behavior of fill_packs_from_midx to not > rely on the incremental flag or existence of a preferred pack. > > 2. The behavior to scan all layers of an incremental midx is kept, so > this is not a full revert of the change. > > 3. We skip allocating more room in the pack_info array if the pack > fails prepare_midx_pack(). > > 4. The method has always returned 0 for success and 1 for failure, but > the condition checking for error added a check for a negative result > for failure, so that is now updated. So did the callee think it is signalling an error, but the sole caller was not taking that as an error, and that was one of the bugs this change fixes? Even if that is the case, I still do not understand why we want to say in the callee error("message"); return 1; and adjust the caller to it, when we can just say return error("message"); in the callee(), especially if the only caller is expecting to be signalled by a negative return value for an error already. > 5. The call to open_pack_index() is removed, but this is needed later > in the case of a preferred pack. That call is moved to immediately > before its result is needed (checking for the object count). > > Signed-off-by: Derrick Stolee <[email protected]> > --- > midx-write.c | 38 ++++++++++++------------------------- > t/t5319-multi-pack-index.sh | 17 +++++++++++++++++ > 2 files changed, 29 insertions(+), 26 deletions(-) Thanks. > diff --git a/midx-write.c b/midx-write.c > index a0aceab5e0..d8f9679868 100644 > --- a/midx-write.c > +++ b/midx-write.c > @@ -920,8 +920,7 @@ static struct multi_pack_index *lookup_multi_pack_index(struct repository *r, > return get_multi_pack_index(source); > } > > -static int fill_packs_from_midx(struct write_midx_context *ctx, > - const char *preferred_pack_name, uint32_t flags) > +static int fill_packs_from_midx(struct write_midx_context *ctx) > { > struct multi_pack_index *m; > > @@ -929,30 +928,13 @@ static int fill_packs_from_midx(struct write_midx_context *ctx, > uint32_t i; > > for (i = 0; i < m->num_packs; i++) { > - ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc); > - > - /* > - * If generating a reverse index, need to have > - * packed_git's loaded to compare their > - * mtimes and object count. > - * > - * If a preferred pack is specified, need to > - * have packed_git's loaded to ensure the chosen > - * preferred pack has a non-zero object count. > - */ > - if (flags & MIDX_WRITE_REV_INDEX || > - preferred_pack_name) { > - if (prepare_midx_pack(ctx->repo, m, > - m->num_packs_in_base + i)) { > - error(_("could not load pack")); > - return 1; > - } > - > - if (open_pack_index(m->packs[i])) > - die(_("could not open index for %s"), > - m->packs[i]->pack_name); > + if (prepare_midx_pack(ctx->repo, m, > + m->num_packs_in_base + i)) { > + error(_("could not load pack")); > + return 1; > } > > + ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc); > fill_pack_info(&ctx->info[ctx->nr++], m->packs[i], > m->pack_names[i], > m->num_packs_in_base + i); > @@ -1123,8 +1105,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir, > ctx.num_multi_pack_indexes_before++; > m = m->base_midx; > } > - } else if (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name, > - flags) < 0) { > + } else if (ctx.m && fill_packs_from_midx(&ctx)) { > goto cleanup; > } > > @@ -1223,6 +1204,11 @@ static int write_midx_internal(struct repository *r, const char *object_dir, > > if (ctx.preferred_pack_idx > -1) { > struct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p; > + > + if (open_pack_index(preferred)) > + die(_("failed to open preferred pack %s"), > + ctx.info[ctx.preferred_pack_idx].pack_name); > + > if (!preferred->num_objects) { > error(_("cannot select preferred pack %s with no objects"), > preferred->pack_name); > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh > index bd75dea950..49705c62a2 100755 > --- a/t/t5319-multi-pack-index.sh > +++ b/t/t5319-multi-pack-index.sh > @@ -989,6 +989,23 @@ test_expect_success 'repack --batch-size=0 repacks everything' ' > ) > ' > > +test_expect_success EXPENSIVE 'repack/expire with many packs' ' > + cp -r dup many && > + ( > + cd many && > + > + for i in $(test_seq 1 100) > + do > + test_commit extra$i && > + git maintenance run --task=loose-objects || return 1 > + done && > + > + git multi-pack-index write && > + git multi-pack-index repack && > + git multi-pack-index expire > + ) > +' > + > test_expect_success 'repack --batch-size=<large> repacks everything' ' > ( > cd dup2 &&

gitgitgadget · 2025-08-29T02:18:07Z

On the Git mailing list, Taylor Blau wrote (reply to this):

On Thu, Aug 28, 2025 at 05:39:51PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <[email protected]> > > The fill_packs_from_midx() method was refactored in fcb2205b77 (midx: > implement support for writing incremental MIDX chains, 2024-08-06) to > allow for preferred packfiles and incremental multi-pack-indexes. > However, this led to some conditions that can cause improperly > initialized memory in the context's list of packfiles. > > The conditions caring about the preferred pack name or the incremental > flag are currently necessary to load a packfile. But the context is > still being populated with pack_info structs based on the packfile array > for the existing multi-pack-index even if prepare_midx_pack() isn't > called. Thanks for looking at this one. On the surface this looks not great, but I am having a hard time coming up with a smaller test case that exercises this behavior. I can get what you wrote below to fail on my machine pretty reliable when building with SANITIZE=address (even without --stress). All of the spots that read from the pack_info array and access the actual packed_git structs are guarded by either writing a MIDX bitmap or having a non-empty preferred pack. So I can see that we're definitely trying to close_pack() on a NULL pointer, but I can't find a way to trigger that more directly. (I think the fact that we don't appear to be accessing the packed_git struct because the spots that want to are guarded by the same conditions that cause us to call prepare_midx_pack() in the first place is pure luck and not something that I am comfortable with us relying on. So we should fix this up for sure, but I wish I understood the existing bug a little more clearly first.) > Add a new test that breaks under --stress when compiled with > SANITIZE=address. The chosen number of 100 packfiles was selected to get > the --stress output to fail about 50% of the time, while 50 packfiles > could not get a failure in most --stress runs. This test has a very > minor check at the end confirming only one packfile remaining. The > failing nature of this test actually relies on auto-GC cleaning up some > packfiles during the creation of the commits, as tests setting gc.auto > to zero make the packfile count match the number of added commits but > also avoids hitting the memory issue. Hmm. Is this portion of the commit message out-of-date? I can't see the check you're referring to that ensures there is only one pack remaining, nor can I see the spot where we disable gc.auto. > The test case is marked as EXPENSIVE not only because of the number of > packfiles it creates, but because some CI environments were reporting > errors during the test that I could not reproduce, specifically around > being unable to open the packfiles or their pack-indexes. > > When it fails under SANITIZE=address, it provides the following error: > > AddressSanitizer:DEADLYSIGNAL > ================================================================= > ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027 > ==3263517==The signal is caused by a READ memory access. > ==3263517==Hint: address points to the zero page. > #0 0x562d5d82d1fb in close_pack_windows packfile.c:299 > #1 0x562d5d82d3ab in close_pack packfile.c:354 > #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490 > #3 0x562d5d7c7aec in midx_repack midx-write.c:1795 > #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305 > ... > > This failure stack trace is disconnected from the real fix because it s/it// ? > the bad pointers are accessed later when closing the packfiles from the > context. > > There are a few different aspects to this fix that are worth noting: > > 1. We return to the previous behavior of fill_packs_from_midx to not > rely on the incremental flag or existence of a preferred pack. > > 2. The behavior to scan all layers of an incremental midx is kept, so > this is not a full revert of the change. > > 3. We skip allocating more room in the pack_info array if the pack > fails prepare_midx_pack(). > > 4. The method has always returned 0 for success and 1 for failure, but > the condition checking for error added a check for a negative result > for failure, so that is now updated. Oops ;-). > 5. The call to open_pack_index() is removed, but this is needed later > in the case of a preferred pack. That call is moved to immediately > before its result is needed (checking for the object count). I think we need to do this in at least one other spot, but see below. > + if (prepare_midx_pack(ctx->repo, m, > + m->num_packs_in_base + i)) { > + error(_("could not load pack")); > + return 1; Looks good, though I agree with Junio's comment in his separate reply that we could probably just turn this into "return error(...)" while we're at it. > @@ -1223,6 +1204,11 @@ static int write_midx_internal(struct repository *r, const char *object_dir, > > if (ctx.preferred_pack_idx > -1) { > struct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p; > + > + if (open_pack_index(preferred)) > + die(_("failed to open preferred pack %s"), > + ctx.info[ctx.preferred_pack_idx].pack_name); This makes sense, but I think we need to apply similar treatment in the "else if" arm of the if-statement immediately above this one too. That portion of the code handles the case where we're writing a MIDX bitmap but didn't provide a preferred pack. When that's the case, we loop through to try and find the oldest pack that contains at least one object. If we don't call open_pack_index() all of those ->num_objects fields will still be zero'd, so we'll only find the oldest pack. That may actually produce wrong behavior if we have duplicate objects that aren't uniformly resolved in favor of the earliest pack in lex order. I'd have to think about it a little more to be sure, though. Thanks, Taylor

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 8/28/25 9:20 PM, Taylor Blau wrote: > On Thu, Aug 28, 2025 at 05:39:51PM +0000, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <[email protected]> >> >> The fill_packs_from_midx() method was refactored in fcb2205b77 (midx: >> implement support for writing incremental MIDX chains, 2024-08-06) to >> allow for preferred packfiles and incremental multi-pack-indexes. >> However, this led to some conditions that can cause improperly >> initialized memory in the context's list of packfiles. >> >> The conditions caring about the preferred pack name or the incremental >> flag are currently necessary to load a packfile. But the context is >> still being populated with pack_info structs based on the packfile array >> for the existing multi-pack-index even if prepare_midx_pack() isn't >> called. > > Thanks for looking at this one. On the surface this looks not great, but > I am having a hard time coming up with a smaller test case that > exercises this behavior. > > I can get what you wrote below to fail on my machine pretty reliable > when building with SANITIZE=address (even without --stress). All of the > spots that read from the pack_info array and access the actual > packed_git structs are guarded by either writing a MIDX bitmap or having > a non-empty preferred pack. I'm glad you're able to reproduce it. My --stress runs had about a 50% hit rate. >> Add a new test that breaks under --stress when compiled with >> SANITIZE=address. The chosen number of 100 packfiles was selected to get >> the --stress output to fail about 50% of the time, while 50 packfiles >> could not get a failure in most --stress runs. This test has a very >> minor check at the end confirming only one packfile remaining. The >> failing nature of this test actually relies on auto-GC cleaning up some >> packfiles during the creation of the commits, as tests setting gc.auto >> to zero make the packfile count match the number of added commits but >> also avoids hitting the memory issue. > > Hmm. Is this portion of the commit message out-of-date? I can't see the > check you're referring to that ensures there is only one pack remaining, > nor can I see the spot where we disable gc.auto. You're right. When I added more robustness around the packfile count by removing gc.auto, the test stopped failing pre-fix. Then, I forgot to remove mention of those test updates. >> The test case is marked as EXPENSIVE not only because of the number of >> packfiles it creates, but because some CI environments were reporting >> errors during the test that I could not reproduce, specifically around >> being unable to open the packfiles or their pack-indexes. >> >> When it fails under SANITIZE=address, it provides the following error: >> >> AddressSanitizer:DEADLYSIGNAL >> ================================================================= >> ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027 >> ==3263517==The signal is caused by a READ memory access. >> ==3263517==Hint: address points to the zero page. >> #0 0x562d5d82d1fb in close_pack_windows packfile.c:299 >> #1 0x562d5d82d3ab in close_pack packfile.c:354 >> #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490 >> #3 0x562d5d7c7aec in midx_repack midx-write.c:1795 >> #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305 >> ... >> >> This failure stack trace is disconnected from the real fix because it > > s/it// ? Thanks. >> the bad pointers are accessed later when closing the packfiles from the >> context. >> >> There are a few different aspects to this fix that are worth noting: >> >> 1. We return to the previous behavior of fill_packs_from_midx to not >> rely on the incremental flag or existence of a preferred pack. >> >> 2. The behavior to scan all layers of an incremental midx is kept, so >> this is not a full revert of the change. >> >> 3. We skip allocating more room in the pack_info array if the pack >> fails prepare_midx_pack(). >> >> 4. The method has always returned 0 for success and 1 for failure, but >> the condition checking for error added a check for a negative result >> for failure, so that is now updated. > > Oops ;-). > >> 5. The call to open_pack_index() is removed, but this is needed later >> in the case of a preferred pack. That call is moved to immediately >> before its result is needed (checking for the object count). > > I think we need to do this in at least one other spot, but see below. Interesting! >> + if (prepare_midx_pack(ctx->repo, m, >> + m->num_packs_in_base + i)) { >> + error(_("could not load pack")); >> + return 1; > > Looks good, though I agree with Junio's comment in his separate reply > that we could probably just turn this into "return error(...)" while > we're at it. Can do. >> @@ -1223,6 +1204,11 @@ static int write_midx_internal(struct repository *r, const char *object_dir, >> >> if (ctx.preferred_pack_idx > -1) { >> struct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p; >> + >> + if (open_pack_index(preferred)) >> + die(_("failed to open preferred pack %s"), >> + ctx.info[ctx.preferred_pack_idx].pack_name); > > This makes sense, but I think we need to apply similar treatment in the > "else if" arm of the if-statement immediately above this one too. That > portion of the code handles the case where we're writing a MIDX bitmap > but didn't provide a preferred pack. > > When that's the case, we loop through to try and find the oldest pack > that contains at least one object. If we don't call open_pack_index() > all of those ->num_objects fields will still be zero'd, so we'll only > find the oldest pack. > > That may actually produce wrong behavior if we have duplicate objects > that aren't uniformly resolved in favor of the earliest pack in lex > order. I'd have to think about it a little more to be sure, though. I see. In this case, we need to open_pack_index() before relying on oldest->num_objects, which only needs to happen for the first pack and any packfile that wins via mtime preference. It also seems like we can _warn_ on failures to open packfiles in those cases, since it isn't fatal if some packfiles fail to open. Thanks, -Stolee

gitgitgadget · 2025-09-03T10:32:34Z

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Sat, Aug 30, 2025 at 09:23:22PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <[email protected]> > > The fill_packs_from_midx() method was refactored in fcb2205b77 (midx: > implement support for writing incremental MIDX chains, 2024-08-06) to > allow for preferred packfiles and incremental multi-pack-indexes. > However, this led to some conditions that can cause improperly > initialized memory in the context's list of packfiles. > > The conditions caring about the preferred pack name or the incremental > flag are currently necessary to load a packfile. But the context is > still being populated with pack_info structs based on the packfile array > for the existing multi-pack-index even if prepare_midx_pack() isn't > called. I honestly don't quite understand why the conditions are necessary here. In other words, why do we need to be careful _not_ to open the packfiles? > Add a new test that breaks under --stress when compiled with > SANITIZE=address. The chosen number of 100 packfiles was selected to get > the --stress output to fail about 50% of the time, while 50 packfiles > could not get a failure in most --stress runs. > > The test case is marked as EXPENSIVE not only because of the number of > packfiles it creates, but because some CI environments were reporting > errors during the test that I could not reproduce, specifically around > being unable to open the packfiles or their pack-indexes. > > When it fails under SANITIZE=address, it provides the following error: > > AddressSanitizer:DEADLYSIGNAL > ================================================================= > ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027 > ==3263517==The signal is caused by a READ memory access. > ==3263517==Hint: address points to the zero page. > #0 0x562d5d82d1fb in close_pack_windows packfile.c:299 > #1 0x562d5d82d3ab in close_pack packfile.c:354 > #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490 > #3 0x562d5d7c7aec in midx_repack midx-write.c:1795 > #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305 > ... > > This failure stack trace is disconnected from the real fix because the bad > pointers are accessed later when closing the packfiles from the context. Okay. So in other words we need to make sure to always prepare the MIDX'd packfiles, but we may not want to open them? > There are a few different aspects to this fix that are worth noting: > > 1. We return to the previous behavior of fill_packs_from_midx to not > rely on the incremental flag or existence of a preferred pack. > > 2. The behavior to scan all layers of an incremental midx is kept, so > this is not a full revert of the change. > > 3. We skip allocating more room in the pack_info array if the pack > fails prepare_midx_pack(). > > 4. The method has always returned 0 for success and 1 for failure, but > the condition checking for error added a check for a negative result > for failure, so that is now updated. Nit, feel free to ignore: this change feels like it would make for a nice separate commit. Patrick

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 9/3/2025 6:14 AM, Patrick Steinhardt wrote: > On Sat, Aug 30, 2025 at 09:23:22PM +0000, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <[email protected]> >> >> The fill_packs_from_midx() method was refactored in fcb2205b77 (midx: >> implement support for writing incremental MIDX chains, 2024-08-06) to >> allow for preferred packfiles and incremental multi-pack-indexes. >> However, this led to some conditions that can cause improperly >> initialized memory in the context's list of packfiles. >> >> The conditions caring about the preferred pack name or the incremental >> flag are currently necessary to load a packfile. But the context is >> still being populated with pack_info structs based on the packfile array >> for the existing multi-pack-index even if prepare_midx_pack() isn't >> called. > > I honestly don't quite understand why the conditions are necessary here. > In other words, why do we need to be careful _not_ to open the > packfiles? My wording is poor. "We don't load packfiles unless one of these conditions holds" is more appropriate. There are some test cases that want to keep things working even when a .idx file disappears, I think. This is a reason to be careful about open_pack_index(), but prepare_midx_pack() is something we want to call always. >> Add a new test that breaks under --stress when compiled with >> SANITIZE=address. The chosen number of 100 packfiles was selected to get >> the --stress output to fail about 50% of the time, while 50 packfiles >> could not get a failure in most --stress runs. >> >> The test case is marked as EXPENSIVE not only because of the number of >> packfiles it creates, but because some CI environments were reporting >> errors during the test that I could not reproduce, specifically around >> being unable to open the packfiles or their pack-indexes. >> >> When it fails under SANITIZE=address, it provides the following error: >> >> AddressSanitizer:DEADLYSIGNAL >> ================================================================= >> ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027 >> ==3263517==The signal is caused by a READ memory access. >> ==3263517==Hint: address points to the zero page. >> #0 0x562d5d82d1fb in close_pack_windows packfile.c:299 >> #1 0x562d5d82d3ab in close_pack packfile.c:354 >> #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490 >> #3 0x562d5d7c7aec in midx_repack midx-write.c:1795 >> #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305 >> ... >> >> This failure stack trace is disconnected from the real fix because the bad >> pointers are accessed later when closing the packfiles from the context. > > Okay. So in other words we need to make sure to always prepare the > MIDX'd packfiles, but we may not want to open them? Yes. Always prepare. Don't always open (since that loads the .idx). >> There are a few different aspects to this fix that are worth noting: >> >> 1. We return to the previous behavior of fill_packs_from_midx to not >> rely on the incremental flag or existence of a preferred pack. >> >> 2. The behavior to scan all layers of an incremental midx is kept, so >> this is not a full revert of the change. >> >> 3. We skip allocating more room in the pack_info array if the pack >> fails prepare_midx_pack(). >> >> 4. The method has always returned 0 for success and 1 for failure, but >> the condition checking for error added a check for a negative result >> for failure, so that is now updated. > > Nit, feel free to ignore: this change feels like it would make for a > nice separate commit. True. I only included it since I was modifying the call anyway due to the changing parameters. Thanks, -Stolee

gitgitgadget · 2025-08-28T20:52:50Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes: > From: Derrick Stolee <[email protected]> > > This instance of setting the result to 1 before going to cleanup was > accidentally removed in fcb2205b77 (midx: implement support for writing > incremental MIDX chains, 2024-08-06). > > Signed-off-by: Derrick Stolee <[email protected]> > --- > midx-write.c | 1 + > 1 file changed, 1 insertion(+) The cover letter made it sound as if [1/5] was the only fix and the rest was clean-up, but unless all callers of write_midx_internal() ignores the return value from it, this surely would change the behaviour of the program, no? And the results from write-midx_file_only(), write_midx_file(), and expire_midx_packs(), the three callers of this _internal() function, all seem to be used in builtin/multi-pack-index.c so wouldn't this also be a fix? Not that I endorse "0 is success and any non-zero value is an error", but this does not look like a mere clean-up to me. > diff --git a/midx-write.c b/midx-write.c > index d8f9679868..85b2d471ef 100644 > --- a/midx-write.c > +++ b/midx-write.c > @@ -1106,6 +1106,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir, > m = m->base_midx; > } > } else if (ctx.m && fill_packs_from_midx(&ctx)) { > + result = 1; > goto cleanup; > }

On the Git mailing list, Taylor Blau wrote (reply to this):

On Thu, Aug 28, 2025 at 01:45:36PM -0700, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <[email protected]> writes: > > > From: Derrick Stolee <[email protected]> > > > > This instance of setting the result to 1 before going to cleanup was > > accidentally removed in fcb2205b77 (midx: implement support for writing > > incremental MIDX chains, 2024-08-06). > > > > Signed-off-by: Derrick Stolee <[email protected]> > > --- > > midx-write.c | 1 + > > 1 file changed, 1 insertion(+) > > The cover letter made it sound as if [1/5] was the only fix and the > rest was clean-up, but unless all callers of write_midx_internal() > ignores the return value from it, this surely would change the > behaviour of the program, no? I think the cover letter is saying that only the first patch is required to fix the crash that was reported, and the rest are clean-ups. Not that this isn't a bug in its own right, but it's definitely distinct from the one that is addressed in the previous patch. > And the results from write-midx_file_only(), write_midx_file(), and > expire_midx_packs(), the three callers of this _internal() function, > all seem to be used in builtin/multi-pack-index.c so wouldn't this > also be a fix? > > Not that I endorse "0 is success and any non-zero value is an error", > but this does not look like a mere clean-up to me. > > > diff --git a/midx-write.c b/midx-write.c > > index d8f9679868..85b2d471ef 100644 > > --- a/midx-write.c > > +++ b/midx-write.c > > @@ -1106,6 +1106,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir, > > m = m->base_midx; > > } > > } else if (ctx.m && fill_packs_from_midx(&ctx)) { > > + result = 1; > > goto cleanup; It might be nice to have a test that confirms this behavior (though the changes look obviously correct to me). It should suffice to write a MIDX, drop one of its packs, and then try to write it again with a new pack. Thanks, Taylor

gitgitgadget · 2025-09-03T10:33:00Z

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Sat, Aug 30, 2025 at 09:23:23PM +0000, Derrick Stolee via GitGitGadget wrote: > diff --git a/midx-write.c b/midx-write.c > index 070a7f61f4..0f1d5653ab 100644 > --- a/midx-write.c > +++ b/midx-write.c > @@ -1104,6 +1104,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir, > m = m->base_midx; > } > } else if (ctx.m && fill_packs_from_midx(&ctx)) { > + result = 1; > goto cleanup; > } Would it make sense to also convert this command to return negative error codes? > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh > index 49705c62a2..008e65c22e 100755 > --- a/t/t5319-multi-pack-index.sh > +++ b/t/t5319-multi-pack-index.sh > @@ -1100,7 +1100,10 @@ test_expect_success 'load reverse index when missing .idx, .pack' ' > mv $idx.bak $idx && > > mv $pack $pack.bak && > - git cat-file --batch-check="%(objectsize:disk)" <tip > + git cat-file --batch-check="%(objectsize:disk)" <tip && > + > + test_must_fail git multi-pack-index write 2>err && > + grep "could not load pack" err Nit: this should probably use `test_grep`. Patrick

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 9/3/2025 6:15 AM, Patrick Steinhardt wrote: > On Sat, Aug 30, 2025 at 09:23:23PM +0000, Derrick Stolee via GitGitGadget wrote: >> diff --git a/midx-write.c b/midx-write.c >> index 070a7f61f4..0f1d5653ab 100644 >> --- a/midx-write.c >> +++ b/midx-write.c >> @@ -1104,6 +1104,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir, >> m = m->base_midx; >> } >> } else if (ctx.m && fill_packs_from_midx(&ctx)) { >> + result = 1; >> goto cleanup; >> } > > Would it make sense to also convert this command to return negative > error codes? (Yes, in patch 6) >> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh >> index 49705c62a2..008e65c22e 100755 >> --- a/t/t5319-multi-pack-index.sh >> +++ b/t/t5319-multi-pack-index.sh >> @@ -1100,7 +1100,10 @@ test_expect_success 'load reverse index when missing .idx, .pack' ' >> mv $idx.bak $idx && >> >> mv $pack $pack.bak && >> - git cat-file --batch-check="%(objectsize:disk)" <tip >> + git cat-file --batch-check="%(objectsize:disk)" <tip && >> + >> + test_must_fail git multi-pack-index write 2>err && >> + grep "could not load pack" err > > Nit: this should probably use `test_grep`. You're right. I think I misremembered the preferred use back when test_i18ngrep was deprecated. Thanks, -Stolee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

midx-write: fix segfault and do several cleanups #1965

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gitgitgadget bot Aug 28, 2025

Uh oh!

gitgitgadget bot Aug 29, 2025

Uh oh!

gitgitgadget bot Aug 30, 2025

Uh oh!

gitgitgadget bot Sep 3, 2025

Uh oh!

gitgitgadget bot Sep 5, 2025

Uh oh!

gitgitgadget bot Aug 28, 2025

Uh oh!

gitgitgadget bot Aug 29, 2025

Uh oh!

gitgitgadget bot Sep 3, 2025

Uh oh!

gitgitgadget bot Sep 5, 2025

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up @@
     	return get_multi_pack_index(source);
     }
-    static int fill_packs_from_midx(struct write_midx_context *ctx,
-    				const char *preferred_pack_name, uint32_t flags)
+    static int fill_packs_from_midx(struct write_midx_context *ctx)
     {
     	struct multi_pack_index *m;
     	for (m = ctx->m; m; m = m->base_midx) {
     		uint32_t i;
     		for (i = 0; i < m->num_packs; i++) {
-    			ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);
-    			/*
-    			 * If generating a reverse index, need to have
-    			 * packed_git's loaded to compare their
-    			 * mtimes and object count.
-    			 *
-    			 * If a preferred pack is specified, need to
-    			 * have packed_git's loaded to ensure the chosen
-    			 * preferred pack has a non-zero object count.
-    			 */
-    			if (flags & MIDX_WRITE_REV_INDEX ||
-    			    preferred_pack_name) {
-    				if (prepare_midx_pack(ctx->repo, m,
-    						      m->num_packs_in_base + i)) {
-    					error(_("could not load pack"));
-    					return 1;
-    				}
-    				if (open_pack_index(m->packs[i]))
-    					die(_("could not open index for %s"),
-    					    m->packs[i]->pack_name);
-    			}
+    			if (prepare_midx_pack(ctx->repo, m,
+    					      m->num_packs_in_base + i))
+    				return error(_("could not load pack"));
+    			ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);
     			fill_pack_info(&ctx->info[ctx->nr++], m->packs[i],
     				       m->pack_names[i],
     				       m->num_packs_in_base + i);
@@ Expand Down Expand Up @@
     			ctx.num_multi_pack_indexes_before++;
     			m = m->base_midx;
     		}
-    	} else if (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,
-    						 flags) < 0) {
+    	} else if (ctx.m && fill_packs_from_midx(&ctx)) {
     		goto cleanup;
     	}
@@ Expand Down Expand Up @@
     		struct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p;
     		ctx.preferred_pack_idx = 0;
+    		/*
+    		 * Attempt opening the pack index to populate num_objects.
+    		 * Ignore failiures as they can be expected and are not
+    		 * fatal during this selection time.
+    		 */
+    		open_pack_index(oldest);
     		if (packs_to_drop && packs_to_drop->nr)
     			BUG("cannot write a MIDX bitmap during expiration");
@@ Expand All @@
     			if (!oldest->num_objects || p->mtime < oldest->mtime) {
     				oldest = p;
+    				open_pack_index(oldest);
     				ctx.preferred_pack_idx = i;
     			}
     		}
@@ Expand All @@
     	if (ctx.preferred_pack_idx > -1) {
     		struct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;
+    		if (open_pack_index(preferred))
+    			die(_("failed to open preferred pack %s"),
+    			    ctx.info[ctx.preferred_pack_idx].pack_name);
     		if (!preferred->num_objects) {
     			error(_("cannot select preferred pack %s with no objects"),
     			      preferred->pack_name);
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     	)
     '
+    test_expect_success EXPENSIVE 'repack/expire with many packs' '
+    	cp -r dup many &&
+    	(
+    		cd many &&
+    		for i in $(test_seq 1 100)
+    		do
+    			test_commit extra$i &&
+    			git maintenance run --task=loose-objects || return 1
+    		done &&
+    		git multi-pack-index write &&
+    		git multi-pack-index repack &&
+    		git multi-pack-index expire
+    	)
+    '
     test_expect_success 'repack --batch-size=<large> repacks everything' '
     	(
     		cd dup2 &&
@@ Expand Down @@

midx-write: fix segfault and do several cleanups #1965

Uh oh!

midx-write: fix segfault and do several cleanups #1965

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gitgitgadget bot Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Aug 30, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!