From: Taylor Blau <me@ttaylorr.com>
Date: Tue, 09 Dec 2025 02:04:25 GMT
Subject: Re: [PATCH 08/17] midx-write.c: introduce `struct write_midx_opts`
Message-ID: <aTeDqfOlDK9terAD@nand.local>
In-Reply-To: <aTcYXJr_-IxPmC65@pks.im>

On Mon, Dec 08, 2025 at 07:26:36PM +0100, Patrick Steinhardt wrote:
> One might argue that parameters which _must_ be passed could be moved
> out of the structure and into the function signature, and as far as I
> understand, that would only be the `struct odb_source`. After all, we
> are talking about options, and a mandatory field is not really an option
> in my book. It also makes the interface at least a tiny bit more self
> documenting.
>
> Other than that this patch looks like a nice improvement to me.

I think that's a reasonable consideration. My preference would be to
keep everything contained within the 'struct write_midx_opts' since it
makes it really easy to pass everything you might need around to other
sub-routines by just passing a single pointer.

So I'm inclined to keep the new API as-is presented here, but I'm happy
to discuss changing it around if you feel strongly about it.

As a reasonable middle-ground, I added a "/* non-optional */" next to
the "source" member within the new structure.

> > @@ -1566,8 +1586,11 @@ int expire_midx_packs(struct odb_source *source, unsigned flags)
> >  	free(count);
> >
> >  	if (packs_to_drop.nr)
> > -		result = write_midx_internal(source, NULL,
> > -					     &packs_to_drop, NULL, NULL, flags);
> > +		result = write_midx_internal(&(struct write_midx_opts) {
> > +					     .source = source,
> > +					     .packs_to_drop = &packs_to_drop,
> > +					     .flags = flags & MIDX_PROGRESS,
> > +					     });
> >
> >  	string_list_clear(&packs_to_drop, 0);
> >
>
> I think this syntax is not allowed in our codebase except for a test
> balloon just yet. See aso 9b2527caa4 (CodingGuidelines: document test
> balloons in flight, 2025-07-23):
>
>     since late 2024 with v2.48.0-rc0~20, we have test balloons for
>     compound literal syntax, e.g., (struct foo){ .member = value };
>     our hope is that no platforms we care about have trouble using
>     them, and officially adopt its wider use in mid 2026.  Do not add
>     more use of the syntax until that happens.
>
> > @@ -1774,8 +1797,10 @@ int midx_repack(struct odb_source *source, size_t batch_size, unsigned flags)
> >  		goto cleanup;
> >  	}
> >
> > -	result = write_midx_internal(source, NULL, NULL, NULL, NULL,
> > -				     flags);
> > +	result = write_midx_internal(&(struct write_midx_opts) {
> > +				     .source = source,
> > +				     .flags = flags,
> > +				     });
>
> Same here.

Hah, I even remember checking to make sure there wasn't such a test
balloon and then thinking that I'd need to adjust before sending. That
must have been just before I got up from my desk, and I must have
forgotten about it until now. Fixed up locally, thanks for spotting!

Thanks,
Taylor