Re: [PATCH 4/7] pack-bitmap: iterate object sources when opening bitmaps

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jul 09, 2026 at 04:08:31PM -0500, Justin Tobler wrote:
> On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> > When opening a bitmap for a repository we perform two steps:
> > 
> >   - We first look for a multi-pack index bitmap in any of the object
> >     sources connected to the repository.
> > 
> >   - We then look for a packfile bitmap in any of the packfiles of any of
> >     the object sources.
> 
> So IIUC, we generally stop searching for a bitmap once we find one.

Except that we continue searching so that we can print a warning, but
later results are simply being ignored.

> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index eda38a5433..0e3e18a557 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -680,60 +680,53 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,
> >  	return 0;
> >  }
> >  
> > -static int open_pack_bitmap(struct repository *r,
> > -			    struct bitmap_index *bitmap_git)
> > +static int open_bitmap_for_source(struct odb_source_packed *source,
> > +				  struct bitmap_index *bitmap_git)
> >  {
> > -	struct packed_git *p;
> > +	struct multi_pack_index *midx = get_multi_pack_index(source);
> > +	struct packfile_list_entry *e;
> >  	int ret = -1;
> >  
> > -	repo_for_each_pack(r, p) {
> > -		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
> > -			ret = 0;
> > -			/*
> > -			 * The only reason to keep looking is to report
> > -			 * duplicates.
> > -			 */
> > -			if (!trace2_is_enabled())
> > -				break;
> > -		}
> > +	if (midx && !open_midx_bitmap_1(bitmap_git, midx))
> > +		ret = 0;
> 
> Ok, open_midx_bitmap_1() returns 0 if it find a MIDX and -1 otherwise.
> Probably just a matter of preference, but I think writing out like below
> is a little bit easier on the eyes:
> 
>   if (midx)
>     ret = open_midx_bitmap_1(bitmap_git, midx);
> 
> it might just be that I find the return values a bit confusing though.
> Maybe we could instead use `found` like a bit later in this patch.

That's fair, and the result would be both easier to reason about and
more consistent indeed. I'll adapt this accordingly.

Patrick