Re: [PATCH 1/3] pack-bitmap: deduplicate logic to iterate over preferred bitmap tips

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 29, 2026, 02:16 UTC
Message-ID: <aXrDD1H4lvBR1sF8@nand.local>
In-Reply-To: <20260128-b4-pks-fix-for-each-ref-in-misuse-v1-1-deccae3ea725@pks.im>

On Wed, Jan 28, 2026 at 09:49:20AM +0100, Patrick Steinhardt wrote:

> We have two locations that iterate over the preferred bitmap tips as
> configured by the user via "pack.preferBitmapTips". Both of these
> callsites are subtly wrong and can lead to a `BUG()`, which we'll fix in
> a subsequent commit.

OK, so there is some bug here that is shared by both call-sites (one in the pack-objects case for single-pack bitmaps, and another in the MIDX code for multi-pack bitmaps). That bug is yet unspecified, but that makes sense since the point of this patch appears to be unifying the two implementations together so that both may be fixed at once.

As of yet, it's not totally clear to me what that bug is having just read the cover letter. I don't know how much detail it's worth getting into here since you'll end up covering it in much greater detail in the following patch, though it might be nice to include at least a taste of what's to come beyond just "[they] are subtly wrong".

Show 6 quoted lines

> Prepare for this fix by unifying the two callsites into a new
> `for_each_preferred_bitmap_tip()` function.
>
> This removes the last callsite of `bitmap_preferred_tips()` outside of
> "pack-bitmap.c". As such, convert the function to be local to that file
> only.

OK, I think hiding this implementation from outside of the compilation unit makes sense, however I am not sure that we should keep it as a separate function.

Show 7 quoted lines

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/pack-objects.c | 19 ++-----------------
>  pack-bitmap.c          | 18 +++++++++++++++++-
>  pack-bitmap.h          |  9 ++++++++-
>  repack-midx.c          | 14 +++-----------
>  4 files changed, 30 insertions(+), 30 deletions(-)

Show 7 quoted lines

> @@ -4710,7 +4694,8 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
>  		load_delta_islands(the_repository, progress);
>
>  	if (write_bitmap_index)
> -		mark_bitmap_preferred_tips();
> +		for_each_preferred_bitmap_tip(the_repository, mark_bitmap_preferred_tip,
> +					      NULL);

This one looks good to me. The function mark_bitmap_preferred_tips() here is identical in its implementation to the new one introduced in pack-bitmap.c, and the callback is reused. Good.

Show 24 quoted lines

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 972203f12b..2f5cb34009 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -3314,7 +3314,7 @@ int bitmap_is_midx(struct bitmap_index *bitmap_git)
>  	return !!bitmap_git->midx;
>  }
>
> -const struct string_list *bitmap_preferred_tips(struct repository *r)
> +static const struct string_list *bitmap_preferred_tips(struct repository *r)
>  {
>  	const struct string_list *dest;
>
> @@ -3323,6 +3323,22 @@ const struct string_list *bitmap_preferred_tips(struct repository *r)
>  	return NULL;
>  }
>
> +void for_each_preferred_bitmap_tip(struct repository *repo,
> +				   each_ref_fn cb, void *cb_data)
> +{
> +	struct string_list_item *item;
> +	const struct string_list *preferred_tips;
> +
> +	preferred_tips = bitmap_preferred_tips(repo);

OK, so this is the sole caller of bitmap_preferred_tips() you were referring to earlier. That function's implementation is hidden from the diff context, but it's effectively a thin wrapper around repo_config_get_string_multi().

I wonder if we should just inline the implementation here into its sole caller. Is there a reason to keep them separate? I do not feel strongly here, just thinking aloud...

Show 7 quoted lines

> +	if (!preferred_tips)
> +		return;
> +
> +	for_each_string_list_item(item, preferred_tips) {
> +		refs_for_each_ref_in(get_main_ref_store(repo),
> +				     item->string, cb, cb_data);
> +	}

The rest of this function looks identical to the one from pack-objects.

Show 14 quoted lines

> +}
> +
>  int bitmap_is_preferred_refname(struct repository *r, const char *refname)
>  {
>  	const struct string_list *preferred_tips = bitmap_preferred_tips(r);
> diff --git a/pack-bitmap.h b/pack-bitmap.h
> index 1bd7a791e2..d0611d0481 100644
> --- a/pack-bitmap.h
> +++ b/pack-bitmap.h
> @@ -5,6 +5,7 @@
>  #include "khash.h"
>  #include "pack.h"
>  #include "pack-objects.h"
> +#include "refs.h"

Oof. I wish that there was a way to forward-declare the each_ref_fn type, but there is not AFAIK.

The rest looks good to me.

Thanks, Taylor