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

From: Justin Tobler <jltobler@gmail.com>
Date: Jul 10, 2025, 21:31 UTC
Message-ID: <lb7rv7wkpdbekciz4astmtay3u2t7os56jvxpfa5x5vx6ao4vc@jvvnav6ybqzl>
In-Reply-To: <20250709-b4-pks-midx-via-odb-alternate-v1-3-f31150d21331@pks.im>

On 25/07/09 09:54AM, Patrick Steinhardt wrote:

Show 6 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.

The `close_midx()` function itself is not iterating though the sources. Rather each of the callsites are now resposible to ensure `close_midx()` is called separately for each source. It might be nice to clarify this a bit in the message.

I also noticed that there are several other existing `close_midx()` callsites that we leave as-is. Each of these sites though looks like they don't care about globally closing all MIDXs so they should be fine. This might also we worth mentioning.

Show 15 quoted lines

> 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);

By dropping this recursive call, now only the single MIDX chain is closed. Other entries in the list are no longer recursively interated through and will have to `close_midx()` called on them explicitly.

Show 18 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;
> +		}
> +		r->objects->multi_pack_index = NULL;

At this callsite we want to close all of the loaded MIDX and do so my iterating through each of the sources. Since we still have the global MIDX for now, we explicitly unset it.

Show 22 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) {
> +		if (source->multi_pack_index)
> +			close_midx(source->multi_pack_index);
> +		source->multi_pack_index = NULL;
>  	}
> +	o->multi_pack_index = NULL;

Same here. Looking good :)

-Justin