From: Karthik Nayak Date: Wed, 28 Jan 2026 10:37:55 GMT Subject: Re: [PATCH 1/3] pack-bitmap: deduplicate logic to iterate over preferred bitmap tips Message-ID: In-Reply-To: <20260128-b4-pks-fix-for-each-ref-in-misuse-v1-1-deccae3ea725@pks.im> Patrick Steinhardt writes: > 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. > > 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. > > Signed-off-by: Patrick Steinhardt > --- > builtin/pack-objects.c | 19 ++----------------- > pack-bitmap.c | 18 +++++++++++++++++- > pack-bitmap.h | 9 ++++++++- > repack-midx.c | 14 +++----------- > 4 files changed, 30 insertions(+), 30 deletions(-) > > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index 5846b6a293..979470e402 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -4554,22 +4554,6 @@ static int mark_bitmap_preferred_tip(const struct reference *ref, void *data UNU > return 0; > } > > -static void mark_bitmap_preferred_tips(void) > -{ > - struct string_list_item *item; > - const struct string_list *preferred_tips; > - > - preferred_tips = bitmap_preferred_tips(the_repository); > - if (!preferred_tips) > - return; > - > - for_each_string_list_item(item, preferred_tips) { > - refs_for_each_ref_in(get_main_ref_store(the_repository), > - item->string, mark_bitmap_preferred_tip, > - NULL); > - } > -} > - > static inline int is_oid_uninteresting(struct repository *repo, > struct object_id *oid) > { > @@ -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); > > if (!fn_show_object) > fn_show_object = show_object; > 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); > + if (!preferred_tips) > + return; > + So we move the config check here. Instead of individually checking it. Makes sense. > + for_each_string_list_item(item, preferred_tips) { > + refs_for_each_ref_in(get_main_ref_store(repo), > + item->string, cb, cb_data); > + } > +} > + > 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" > #include "string-list.h" > > struct commit; > @@ -99,6 +100,13 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git, > show_reachable_fn show_reach, > void *payload); > > +/* > + * Iterate over all references that are configured as preferred bitmap tips via > + * "pack.preferBitmapTips" and invoke the callback on each function. > + */ > +void for_each_preferred_bitmap_tip(struct repository *repo, > + each_ref_fn cb, void *cb_data); > + > #define GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL \ > "GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL" > > @@ -182,7 +190,6 @@ char *pack_bitmap_filename(struct packed_git *p); > > int bitmap_is_midx(struct bitmap_index *bitmap_git); > > -const struct string_list *bitmap_preferred_tips(struct repository *r); > int bitmap_is_preferred_refname(struct repository *r, const char *refname); > > int verify_bitmap_files(struct repository *r); > diff --git a/repack-midx.c b/repack-midx.c > index 74bdfa3a6e..0682b80c42 100644 > --- a/repack-midx.c > +++ b/repack-midx.c > @@ -40,7 +40,6 @@ static int midx_snapshot_ref_one(const struct reference *ref, void *_data) > void midx_snapshot_refs(struct repository *repo, struct tempfile *f) > { > struct midx_snapshot_ref_data data; > - const struct string_list *preferred = bitmap_preferred_tips(repo); > > data.repo = repo; > data.f = f; > @@ -51,16 +50,9 @@ void midx_snapshot_refs(struct repository *repo, struct tempfile *f) > die(_("could not open tempfile %s for writing"), > get_tempfile_path(f)); > > - if (preferred) { > - struct string_list_item *item; > - > - data.preferred = 1; > - for_each_string_list_item(item, preferred) > - refs_for_each_ref_in(get_main_ref_store(repo), > - item->string, > - midx_snapshot_ref_one, &data); > - data.preferred = 0; > - } > + data.preferred = 1; > + for_each_preferred_bitmap_tip(repo, midx_snapshot_ref_one, &data); > + data.preferred = 0; > So we no longer need to check for the config, since `for_each_preferred_bitmap_tip()` does that. Looks good. > refs_for_each_ref(get_main_ref_store(repo), > midx_snapshot_ref_one, &data); > > -- > 2.53.0.rc2.206.g60c1bca835.dirty