From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 30 Jan 2026 12:58:03 GMT
Subject: Re: [PATCH 1/3] pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
Message-ID: <aXyq2_MByjxf3TBX@pks.im>
In-Reply-To: <aXrDD1H4lvBR1sF8@nand.local>

On Wed, Jan 28, 2026 at 09:16:47PM -0500, Taylor Blau wrote:
> 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".

Sure, can do.

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

I originally though the same, but there still is a second callsite of
this function. So...

> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index 972203f12b..2f5cb34009 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -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...

... I actually tried this, only to realize that the function is still
called by `bitmap_is_preferred_refname()`, too.

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

We could get around this by simply open-coding the function signature
and having a forward-declaration for `struct reference`. Not sure
though whether that's really worth it.

Patrick