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

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jul 10, 2025 at 07:22:58PM -0400, Taylor Blau wrote:
> On Wed, Jul 09, 2025 at 09:54:51AM +0200, Patrick Steinhardt wrote:
> > diff --git a/midx.c b/midx.c
> > index a91231bfcdf..416b3e8b54f 100644
> > --- a/midx.c
> > +++ b/midx.c
> > @@ -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 ;-).

Fair, can adapt.

> > +		}
> > +		r->objects->multi_pack_index = NULL;
> 
> Presumably this pointer will go away at some point in the future as
> well?

Yup, exactly, it's removed at the end of this series.

Patrick