Re: [PATCH 3/5] midx-write.c: simplify fill_packs_from_midx()

[Taylor Blau <me@ttaylorr.com>]

On Mon, May 26, 2025 at 09:23:59AM +0200, Patrick Steinhardt wrote:
> On Sun, May 25, 2025 at 02:41:57PM -0400, Taylor Blau wrote:
> > diff --git a/midx-write.c b/midx-write.c
> > index e4a3830d45..7802a4b694 100644
> > --- a/midx-write.c
> > +++ b/midx-write.c
> > @@ -943,6 +943,19 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,
> >  {
> >  	struct multi_pack_index *m;
> >
> > +	if (preferred_pack_name) {
> > +		/*
> > +		 * If a preferred pack is specified, need to have
> > +		 * packed_git's loaded to ensure the chosen preferred
> > +		 * pack has a non-zero object count.
> > +		 *
> > +		 * Trick ourselves into thinking that we're writing a
> > +		 * reverse index in this case in order to open up the
> > +		 * pack index file.
> > +		 */
> > +		flags |= MIDX_WRITE_REV_INDEX;
> > +	}
>
> This change feels a bit weird to me. Sure, it does allow us to pull out
> the loop in the subsequent patch. But honestly, that makes this
> workaround even weirder in that we now set unrelated flags in some
> function and expect a different function to only honor it in order to
> open the packfile.
>
> Shouldn't we instead have a separate flag for the new function that
> tells it whether or not it is supposed to prepare the pack?

Yeah, I like that much better, thanks!

Thanks,
Taylor