Re: [PATCH 3/8] midx: stop using linked list when closing MIDX
From: Taylor Blau <hidden>
Date: 2025-07-10 23:23:00
On Wed, Jul 09, 2025 at 09:54:51AM +0200, Patrick Steinhardt wrote:
quoted hunk ↗ jump to hunk
When calling `close_midx()` we not only close the multi-pack index for one object source, but instead we iterate through the whole linked list of MIDXs to close all of them. This linked list is about to go away in favor of using the new per-source pointer to its respective MIDX. Refactor the function to iterate through sources instead. Signed-off-by: Patrick Steinhardt <redacted> --- midx.c | 11 ++++++----- packfile.c | 10 +++++----- 2 files changed, 11 insertions(+), 10 deletions(-)diff --git a/midx.c b/midx.c index a91231bfcdf..416b3e8b54f 100644 --- a/midx.c +++ b/midx.c@@ -401,7 +401,6 @@ void close_midx(struct multi_pack_index *m) if (!m) return; - close_midx(m->next);
OK, so previously this recursive call to "close_midx()" would have freed up the `m->next` MIDX...
quoted hunk ↗ jump to hunk
close_midx(m->base_midx); munmap((unsigned char *)m->data, m->data_len);@@ -835,11 +834,13 @@ void clear_midx_file(struct repository *r) get_midx_filename(r->hash_algo, &midx, r->objects->sources->path); - if (r->objects && r->objects->multi_pack_index) { - close_midx(r->objects->multi_pack_index); - r->objects->multi_pack_index = NULL; - for (struct odb_source *source = r->objects->sources; source; source = source->next) + if (r->objects) { + for (struct odb_source *source = r->objects->sources; source; source = source->next) { + if (source->multi_pack_index) + close_midx(source->multi_pack_index); source->multi_pack_index = NULL;
...and then this line would NULL the now-free()'d memory out.
But instead we are directly iterating through the sources and both
closing and NULL-ing out their respective MIDXs (if any).
As an aside: I know we do the C99-style for loop with declarations in
many places, but in this instance it seems to have produced an awfully
long line. I wonder if in this instance it would be better to write:
struct odb_source *source;
for (source = r->objects->sources; source; source = source->next) {
/* ... */
}
That's still a little lengthy, but it's fewer than 80 characters ;-).
+ } + r->objects->multi_pack_index = NULL;
Presumably this pointer will go away at some point in the future as well?
quoted hunk ↗ jump to hunk
} if (remove_path(midx.buf))diff --git a/packfile.c b/packfile.c index b43dd2fe6cb..546c161d0c1 100644 --- a/packfile.c +++ b/packfile.c@@ -369,12 +369,12 @@ void close_object_store(struct object_database *o) else close_pack(p); - if (o->multi_pack_index) { - close_midx(o->multi_pack_index); - o->multi_pack_index = NULL; - for (struct odb_source *source = o->sources; source; source = source->next) - source->multi_pack_index = NULL; + for (struct odb_source *source = o->sources; source; source = source->next) {
Same comment here as well. Thanks, Taylor