Re: [PATCH 3/8] midx: stop using linked list when closing MIDX

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 10, 2025, 23:22 UTC
Message-ID: <aHBLUg7Y1cgf8i7k@nand.local>
In-Reply-To: <20250709-b4-pks-midx-via-odb-alternate-v1-3-f31150d21331@pks.im>

On Wed, Jul 09, 2025 at 09:54:51AM +0200, Patrick Steinhardt wrote:

Show 22 quoted lines

> 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@pks.im>
> ---
>  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...

Show 16 quoted lines

>  	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?

Show 17 quoted lines

>  	}
>
>  	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