Re: [PATCH 3/5] midx-write.c: simplify fill_packs_from_midx()
- From
Taylor Blau <me@ttaylorr.com>
- Date
- May 28, 2025, 02:15 UTC
- Message-ID
- <aDZxtK+I0sWNCUVl@nand.local>
- In-Reply-To
- <aDQXD8EHD163Mijo@pks.im>
On Mon, May 26, 2025 at 09:23:59AM +0200, Patrick Steinhardt wrote:
Show 30 quoted lines
> 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