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

From: Patrick Steinhardt <ps@pks.im>
Date: May 26, 2025, 07:23 UTC
Message-ID: <aDQXD8EHD163Mijo@pks.im>
In-Reply-To: <5d97b706e17117217bf6e90e5cbca9e6e408c5b4.1748198489.git.me@ttaylorr.com>

On Sun, May 25, 2025 at 02:41:57PM -0400, Taylor Blau wrote:

Show 20 quoted lines

> 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?

Patrick