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

[Taylor Blau <me@ttaylorr.com>]

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".

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

> 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(-)

> @@ -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.

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

> +	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.

> +}
> +
>  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