On Wed, Jul 09, 2025 at 09:54:51AM +0200, Patrick Steinhardt wrote:
> 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 <ps@xxxxxx>
> ---
> 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...
> 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?
> }
>
> 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
