[PATCH 0/3] Fix misuse of `refs_for_each_ref_in()`

[PATCH 0/3] Fix misuse of `refs_for_each_ref_in()`

[Patrick Steinhardt]

Hi,

this small patch series fixes a bug I have discovered where configuring
"pack.preferBitmapTips" to an exact branch will cause Git to `BUG()`.

The root cause of this bug is misuse of `refs_for_each_ref_in()`: this
function accepts a prefix to yield refs for, and then strips the prefix
for each ref. Consequently, if passed an exact refname, then stripping
the prefix would make us end up with an empty refname, and that is not
supposed to happen.

There was one other caller that got it wrong, too, and which is also
fixed in this patch series.

Thanks!

Patrick

---
Patrick Steinhardt (3):
      pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
      pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
      bisect: fix misuse of `refs_for_each_ref_in()`

 bisect.c                    |  8 ++++----
 builtin/pack-objects.c      | 19 ++-----------------
 pack-bitmap.c               | 18 +++++++++++++++++-
 pack-bitmap.h               |  9 ++++++++-
 repack-midx.c               | 14 +++-----------
 t/t5310-pack-bitmaps.sh     | 35 +++++++++++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh | 36 ++++++++++++++++++++++++++++++++++++
 7 files changed, 105 insertions(+), 34 deletions(-)


---
base-commit: ea717645d199f6f1b66058886475db3e8c9330e9
change-id: 20260128-b4-pks-fix-for-each-ref-in-misuse-96ae62e313a5

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

[Patrick Steinhardt]

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

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;
+
+	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;
 
 	refs_for_each_ref(get_main_ref_store(repo),
 			  midx_snapshot_ref_one, &data);

-- 
2.53.0.rc2.206.g60c1bca835.dirty

[PATCH 2/3] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Patrick Steinhardt]

The "pack.preferBitmapTips" configuration allows the user to specify
which references should be preferred when generating bitmaps. This
option is typically expected to be set to a reference prefix, like for
example "refs/heads/".

It's not unreasonable though for a user to configure one specific
reference as preferred. But if they do, they'll hit a `BUG()`:

    $ git -c pack.preferBitmapTips=refs/heads/main repack -adb
    BUG: ../refs/iterator.c:366: attempt to trim too many characters
    error: pack-objects died of signal 6

The root cause for this bug is how we enumerate these references. We
call `refs_for_each_ref_in()`, which will:

  - Yield all references that have a user-specified prefix.

  - Trim each of these references so that the prefix is removed.

Typically, this function is called with a trailing slash, like
"refs/heads/", and in that case things work alright. But if the function
is called with the name of an existing reference then we'll try to trim
the full reference name, which would leave us with an empty name. And as
this would not really leave us with anything sensible, we call `BUG()`
instead of yielding this reference.

One could argue that this is a bug in `refs_for_each_ref_in()`. But the
question then becomes what the correct behaviour would be:

  - Do we want to skip exact matches? In our case we certainly don't
    want that, as the user has asked us to generate a bitmap for it.

  - Do we want to yield the reference with the empty refname? That would
    lead to a somewhat weird result.

Neither of these feel like viable options, so calling `BUG()` feels like
a sensible way out.

The root cause really is that we try to trim the whole refname. We can
thus easily fix the bug itself by calling `refs_for_each_fullref_in()`
instead. This function behaves the same as `refs_for_each_ref_in()`,
except that it doesn't strip the prefix. Consequently, it correctly
yields also exact refnames.

One resulting weirdness is that two refs "refs/heads/base" and
"refs/heads/base-something" would now match if the user configured
"refs/heads/base" as bitmap tips. One could arguably change the
semantics of the configuration such that a string without a trailing
slash needs to be an exact reference match, whereas a string with a
trailing slash indicates a directory hierarchy. But such a change would
potentially cause regressions with dubious benefits, so this issue is
ignored for now.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c               |  4 ++--
 t/t5310-pack-bitmaps.sh     | 35 +++++++++++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 2f5cb34009..8d3b5ac037 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -3334,8 +3334,8 @@ void for_each_preferred_bitmap_tip(struct repository *repo,
 		return;
 
 	for_each_string_list_item(item, preferred_tips) {
-		refs_for_each_ref_in(get_main_ref_store(repo),
-				     item->string, cb, cb_data);
+		refs_for_each_fullref_in(get_main_ref_store(repo),
+					 item->string, NULL, cb, cb_data);
 	}
 }
 
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 6718fb98c0..7ef91b502c 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -466,6 +466,41 @@ test_bitmap_cases () {
 		)
 	'
 
+	test_expect_success 'pack.preferBitmapTips can use direct refname' '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+
+			# Create enough commits that not all will receive bitmap
+			# coverage even if they are all at the tip of some reference.
+			test_commit_bulk --message="%s" 103 &&
+			git log --format="create refs/tags/%s %H" HEAD >refs &&
+			git update-ref --stdin <refs &&
+
+			# Create the bitmap.
+			git repack -adb &&
+			test-tool bitmap list-commits | sort >commits-with-bitmap &&
+
+			# Verify that we have at least one commit that did not
+			# receive a bitmap.
+			git rev-list HEAD >commits.raw &&
+			sort <commits.raw >commits &&
+			comm -13 commits-with-bitmap commits >commits-wo-bitmap &&
+			test_file_not_empty commits-wo-bitmap &&
+			commit_id=$(head commits-wo-bitmap) &&
+
+			# We now create a reference for this commit and repack
+			# with "preferBitmapTips" pointing to that exact
+			# reference. The expectation is that it will now be
+			# covered by a bitmap.
+			git update-ref refs/heads/cover-me "$commit_id" &&
+			git -c pack.preferBitmapTips=refs/heads/cover-me repack -adb &&
+			test-tool bitmap list-commits >after &&
+			test_grep "$commit_id" after
+		)
+	'
+
 	test_expect_success 'complains about multiple pack bitmaps' '
 		rm -fr repo &&
 		git init repo &&
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index faae98c7e7..40d36118bd 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1345,4 +1345,40 @@ test_expect_success 'bitmapped packs are stored via the BTMP chunk' '
 	)
 '
 
+test_expect_success 'pack.preferBitmapTips can use direct refname' '
+	git init repo &&
+	test_when_finished "rm -fr repo" &&
+	(
+		cd repo &&
+
+		# Create enough commits that not all will receive bitmap
+		# coverage even if they are all at the tip of some reference.
+		test_commit_bulk --message="%s" 103 &&
+		git log --format="create refs/tags/%s %H" HEAD >refs &&
+		git update-ref --stdin <refs &&
+
+		# Create the bitmap via the MIDX.
+		git repack -adb --write-midx &&
+		test-tool bitmap list-commits | sort >commits-with-bitmap &&
+
+		# Verify that we have at least one commit that did not
+		# receive a bitmap.
+		git rev-list HEAD >commits.raw &&
+		sort <commits.raw >commits &&
+		comm -13 commits-with-bitmap commits >commits-wo-bitmap &&
+		test_file_not_empty commits-wo-bitmap &&
+		commit_id=$(head commits-wo-bitmap) &&
+
+		# We now create a reference for this commit and repack
+		# with "preferBitmapTips" pointing to that exact
+		# reference. The expectation is that it will now be
+		# covered by a bitmap.
+		git update-ref refs/heads/cover-me "$commit_id" &&
+		rm .git/objects/pack/multi-pack-index* &&
+		git -c pack.preferBitmapTips=refs/heads/cover-me repack -adb --write-midx &&
+		test-tool bitmap list-commits >after &&
+		test_grep "$commit_id" after
+	)
+'
+
 test_done

-- 
2.53.0.rc2.206.g60c1bca835.dirty

[PATCH 3/3] bisect: fix misuse of `refs_for_each_ref_in()`

[Patrick Steinhardt]

All callers of `refs_for_each_ref_in()` pass in a string that is
terminated with a trailing slash to indicate that they only want to see
refs in that specific ref hierarchy. This is in fact a requirement if
one wants to use this function, as the function trims the prefix from
each yielded ref. So if there was a reference that was called
"refs/bisect" as in our example, the result after trimming would be the
empty string, and that's something we disallow.

Fix this by adding the trailing slash.

Furthermore, taking a closer look, we strip the prefix only to re-add it
in `mark_for_removal()`. This is somewhat roundabout, as we can instead
call `refs_for_each_fullref_in()` to not do any stripping at all. Do so
to simplify the code a bit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index 326b59c0dc..4f0d1a1853 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1180,7 +1180,7 @@ int estimate_bisect_steps(int all)
 static int mark_for_removal(const struct reference *ref, void *cb_data)
 {
 	struct string_list *refs = cb_data;
-	char *bisect_ref = xstrfmt("refs/bisect%s", ref->name);
+	char *bisect_ref = xstrdup(ref->name);
 	string_list_append(refs, bisect_ref);
 	return 0;
 }
@@ -1191,9 +1191,9 @@ int bisect_clean_state(void)
 
 	/* There may be some refs packed during bisection */
 	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
-	refs_for_each_ref_in(get_main_ref_store(the_repository),
-			     "refs/bisect", mark_for_removal,
-			     (void *) &refs_for_removal);
+	refs_for_each_fullref_in(get_main_ref_store(the_repository),
+				 "refs/bisect/", NULL, mark_for_removal,
+				 &refs_for_removal);
 	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
 	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
 	result = refs_delete_refs(get_main_ref_store(the_repository),

-- 
2.53.0.rc2.206.g60c1bca835.dirty

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 5752 bytes --]

Patrick Steinhardt <ps@pks.im> 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 <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(-)
>
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 2/3] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 5542 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> The "pack.preferBitmapTips" configuration allows the user to specify
> which references should be preferred when generating bitmaps. This
> option is typically expected to be set to a reference prefix, like for
> example "refs/heads/".
>
> It's not unreasonable though for a user to configure one specific
> reference as preferred. But if they do, they'll hit a `BUG()`:
>
>     $ git -c pack.preferBitmapTips=refs/heads/main repack -adb
>     BUG: ../refs/iterator.c:366: attempt to trim too many characters
>     error: pack-objects died of signal 6
>
> The root cause for this bug is how we enumerate these references. We
> call `refs_for_each_ref_in()`, which will:
>
>   - Yield all references that have a user-specified prefix.
>
>   - Trim each of these references so that the prefix is removed.
>
> Typically, this function is called with a trailing slash, like
> "refs/heads/", and in that case things work alright. But if the function
> is called with the name of an existing reference then we'll try to trim
> the full reference name, which would leave us with an empty name. And as
> this would not really leave us with anything sensible, we call `BUG()`
> instead of yielding this reference.
>
> One could argue that this is a bug in `refs_for_each_ref_in()`. But the
> question then becomes what the correct behaviour would be:
>
>   - Do we want to skip exact matches? In our case we certainly don't
>     want that, as the user has asked us to generate a bitmap for it.
>
>   - Do we want to yield the reference with the empty refname? That would
>     lead to a somewhat weird result.
>
> Neither of these feel like viable options, so calling `BUG()` feels like
> a sensible way out.
>
> The root cause really is that we try to trim the whole refname. We can
> thus easily fix the bug itself by calling `refs_for_each_fullref_in()`
> instead. This function behaves the same as `refs_for_each_ref_in()`,
> except that it doesn't strip the prefix. Consequently, it correctly
> yields also exact refnames.
>
> One resulting weirdness is that two refs "refs/heads/base" and
> "refs/heads/base-something" would now match if the user configured
> "refs/heads/base" as bitmap tips. One could arguably change the
> semantics of the configuration such that a string without a trailing
> slash needs to be an exact reference match, whereas a string with a
> trailing slash indicates a directory hierarchy. But such a change would
> potentially cause regressions with dubious benefits, so this issue is
> ignored for now.
>

When using `refs_for_each_ref_in()` this would yield just
'refs/heads/base-something'. That too feels like a BUG(), so I would
think this is the better solution.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  pack-bitmap.c               |  4 ++--
>  t/t5310-pack-bitmaps.sh     | 35 +++++++++++++++++++++++++++++++++++
>  t/t5319-multi-pack-index.sh | 36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 73 insertions(+), 2 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 2f5cb34009..8d3b5ac037 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -3334,8 +3334,8 @@ void for_each_preferred_bitmap_tip(struct repository *repo,
>  		return;
>
>  	for_each_string_list_item(item, preferred_tips) {
> -		refs_for_each_ref_in(get_main_ref_store(repo),
> -				     item->string, cb, cb_data);
> +		refs_for_each_fullref_in(get_main_ref_store(repo),
> +					 item->string, NULL, cb, cb_data);
>  	}
>  }
>
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 6718fb98c0..7ef91b502c 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -466,6 +466,41 @@ test_bitmap_cases () {
>  		)
>  	'
>
> +	test_expect_success 'pack.preferBitmapTips can use direct refname' '
> +		git init repo &&
> +		test_when_finished "rm -fr repo" &&
> +		(
> +			cd repo &&
> +
> +			# Create enough commits that not all will receive bitmap
> +			# coverage even if they are all at the tip of some reference.
> +			test_commit_bulk --message="%s" 103 &&
> +			git log --format="create refs/tags/%s %H" HEAD >refs &&
> +			git update-ref --stdin <refs &&
> +

We create a bunch of commits. Nit: is '--message="%s"' even needed here?
Seems to be the default behavior anyways. Since we don't provide a ref,
it uses HEAD, so finally we'll only have one commit being referenced.

But then we also create individual tags for each of them.

> +			# Create the bitmap.
> +			git repack -adb &&
> +			test-tool bitmap list-commits | sort >commits-with-bitmap &&
> +
> +			# Verify that we have at least one commit that did not
> +			# receive a bitmap.
> +			git rev-list HEAD >commits.raw &&
> +			sort <commits.raw >commits &&
> +			comm -13 commits-with-bitmap commits >commits-wo-bitmap &&
> +			test_file_not_empty commits-wo-bitmap &&
> +			commit_id=$(head commits-wo-bitmap) &&
> +

Alright, so of all the commits we have, some of them won't have a bitmap
and we pick the first one.

> +			# We now create a reference for this commit and repack
> +			# with "preferBitmapTips" pointing to that exact
> +			# reference. The expectation is that it will now be
> +			# covered by a bitmap.
> +			git update-ref refs/heads/cover-me "$commit_id" &&
> +			git -c pack.preferBitmapTips=refs/heads/cover-me repack -adb &&
> +			test-tool bitmap list-commits >after &&
> +			test_grep "$commit_id" after

Alright makes sense, since we fixed the prefix issue, providing the full
refname appears to work now.

[skip]

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 0/3] Fix misuse of `refs_for_each_ref_in()`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this small patch series fixes a bug I have discovered where configuring
> "pack.preferBitmapTips" to an exact branch will cause Git to `BUG()`.
>
> The root cause of this bug is misuse of `refs_for_each_ref_in()`: this
> function accepts a prefix to yield refs for, and then strips the prefix
> for each ref. Consequently, if passed an exact refname, then stripping
> the prefix would make us end up with an empty refname, and that is not
> supposed to happen.
>
> There was one other caller that got it wrong, too, and which is also
> fixed in this patch series.
>
> Thanks!
>
> Patrick
>

The patches look good, thanks for the fix!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Taylor Blau]

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

Re: [PATCH 2/3] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Taylor Blau]

On Wed, Jan 28, 2026 at 09:49:21AM +0100, Patrick Steinhardt wrote:
> The "pack.preferBitmapTips" configuration allows the user to specify
> which references should be preferred when generating bitmaps. This
> option is typically expected to be set to a reference prefix, like for
> example "refs/heads/".
>
> It's not unreasonable though for a user to configure one specific
> reference as preferred. But if they do, they'll hit a `BUG()`:
>
>     $ git -c pack.preferBitmapTips=refs/heads/main repack -adb
>     BUG: ../refs/iterator.c:366: attempt to trim too many characters
>     error: pack-objects died of signal 6

Oops. While we should definitely not BUG() here, I am not sure I
understand the desired use-case of specifying a single reference as a
value for pack.preferBitmapTips.

Looking at the implementation of bitmap_writer_select_commits(), we do
not guarantee that *any* reference specified by pack.preferBitmapTips
will receive a bitmap. That's because we don't necessarily enumerate the
entire set of commits when determining which ones to bitmap.

For example, if I do something like (assuming that the bug described
here is fixed):

    $ git -c pack.preferBitmapTips=refs/heads/foo \
          -c pack.preferBitmapTips=refs/heads/bar repack -adb

, and suppose "indexed_commits" list has the commits pointed to by "foo"
and "bar" next to each other. We'll look at the next batch of bitmap
candidates, realize that commit "foo" has the NEEDS_BITMAP flag set,
mark it as chosen, and then skip ahead to the next chunk, all without
having looked at "bar".

Looking at the code, I *think* it's the case that specifying a single
preferred bitmap tip with an exact reference name will guarantee that we
select it for bitmapping, but it's not the case in general.

> One resulting weirdness is that two refs "refs/heads/base" and
> "refs/heads/base-something" would now match if the user configured
> "refs/heads/base" as bitmap tips. One could arguably change the
> semantics of the configuration such that a string without a trailing
> slash needs to be an exact reference match, whereas a string with a
> trailing slash indicates a directory hierarchy. But such a change would
> potentially cause regressions with dubious benefits, so this issue is
> ignored for now.

(Setting aside the for_each_ref vs. for_each_fullref issue for a
moment...)

Am I understanding this change correctly that doing something like -c
pack.preferBitmapTips=refs/heads/foo would match both foo and foobar?

If so, I am not sure that that is a desirable interface, especially
since we went the opposite direction in 10e8a9352bc (refs.c: stop
matching non-directory prefixes in exclude patterns, 2025-03-06). Having
the two behave inconsistently from one another feels somewhat awkward to
me and may lead to unexpected results.

At the very least, if we do end up going in this direction (and I am not
necessarily advocating that we do, since I would prefer a more
consistent set of behavior), we should at minimum document it in
git-config(1).

Thanks,
Taylor

Re: [PATCH 3/3] bisect: fix misuse of `refs_for_each_ref_in()`

[Jeff King]

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

> Furthermore, taking a closer look, we strip the prefix only to re-add it
> in `mark_for_removal()`. This is somewhat roundabout, as we can instead
> call `refs_for_each_fullref_in()` to not do any stripping at all. Do so
> to simplify the code a bit.

Yeah, I think the result is much better.

We might also want this simplification on top:

-- >8 --
Subject: [PATCH] bisect: simplify string_list memory handling

We declare the refs_for_removal string_list as NODUP, forcing us to
manually allocate strings we insert. And then when it comes time to
clean up, we set strdup_strings so that string_list_clear() will free
them for us.

This is a confusing pattern, and can be done much more simply by just
declaring the list with the DUP initializer in the first place.

It was written this way originally because one of the callsites
generated the item using xstrfmt(). But that spot switched to a plain
xstrdup() in cf01f617b9 (bisect: fix misuse of `refs_for_each_ref_in()`,
2026-01-28). That means we can now just let the string_list code handle
allocation itself.

Signed-off-by: Jeff King <peff@peff.net>
---
Even before cf01f617b9 we could have done:

  string_list_append_nodup(&refs, xstrfmt(...));

to get a similar simplification, but after that commit it is even
easier.

 bisect.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/bisect.c b/bisect.c
index 2cd97bc9fe..6b9e9d81c3 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1183,8 +1183,7 @@ int estimate_bisect_steps(int all)
 static int mark_for_removal(const struct reference *ref, void *cb_data)
 {
 	struct string_list *refs = cb_data;
-	char *bisect_ref = xstrdup(ref->name);
-	string_list_append(refs, bisect_ref);
+	string_list_append(refs, ref->name);
 	return 0;
 }
 
@@ -1193,16 +1192,15 @@ int bisect_clean_state(void)
 	int result = 0;
 
 	/* There may be some refs packed during bisection */
-	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
+	struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
 	refs_for_each_fullref_in(get_main_ref_store(the_repository),
 				 "refs/bisect/", NULL, mark_for_removal,
 				 &refs_for_removal);
-	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
-	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
+	string_list_append(&refs_for_removal, "BISECT_HEAD");
+	string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV");
 	result = refs_delete_refs(get_main_ref_store(the_repository),
 				  "bisect: remove", &refs_for_removal,
 				  REF_NO_DEREF);
-	refs_for_removal.strdup_strings = 1;
 	string_list_clear(&refs_for_removal, 0);
 	unlink_or_warn(git_path_bisect_ancestors_ok());
 	unlink_or_warn(git_path_bisect_log());
-- 
2.53.0.rc2.296.g6748ed0491

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

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

True.  I found it easier to see from the way the patch was written
that this is a pure refactoring patch, though.  IOW, we may want to
do that on top as a further rewrite, but I am not sure if it makes
the result easier to reason about.  Such helper functions that are
file-scope static often help reading the logic flow of the program,
and compilers would inline them when it is more beneficial anyway.

Re: [PATCH 2/3] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Looking at the implementation of bitmap_writer_select_commits(), we do
> not guarantee that *any* reference specified by pack.preferBitmapTips
> will receive a bitmap. That's because we don't necessarily enumerate the
> entire set of commits when determining which ones to bitmap.

Hmph.  Is this documented?

	... Goes and looks ...

Yes, it is documented.  We say "This is because ..." but it just
explains it as what the chosen design of the implementation happens
to do, without saying for what benefit the implementation was chosen,
so it is unclear if this is designed behaviour, or more importantly,
even if this were designed, what the rationale of choosing that
design was.

"When they are so close to fall into the same chunk, there is no
point having bitmaps individually for them, as their bitmaps will be
very similar anyway, so this design saves space without sacrificing
the quality of the resulting set of bitmaps" or something?


> At the very least, if we do end up going in this direction (and I am not
> necessarily advocating that we do, since I would prefer a more
> consistent set of behavior), we should at minimum document it in
> git-config(1).

The documentation says "... reference that is a suffix of any value
of this configuration".  Is "refs/heads/foobar" a "suffix" of
"refs/heads/foo"?  I actually find this phrasing fairly strange, as
I do not think of "refs/heads/main" be a "suffix" of "refs/heads/".

Re: [PATCH 3/3] bisect: fix misuse of `refs_for_each_ref_in()`

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 28, 2026 at 09:49:22AM +0100, Patrick Steinhardt wrote:
>
>> Furthermore, taking a closer look, we strip the prefix only to re-add it
>> in `mark_for_removal()`. This is somewhat roundabout, as we can instead
>> call `refs_for_each_fullref_in()` to not do any stripping at all. Do so
>> to simplify the code a bit.
>
> Yeah, I think the result is much better.
>
> We might also want this simplification on top:

Very good.  Thanks, both, for improvements.

> -- >8 --
> Subject: [PATCH] bisect: simplify string_list memory handling
>
> We declare the refs_for_removal string_list as NODUP, forcing us to
> manually allocate strings we insert. And then when it comes time to
> clean up, we set strdup_strings so that string_list_clear() will free
> them for us.
>
> This is a confusing pattern, and can be done much more simply by just
> declaring the list with the DUP initializer in the first place.
>
> It was written this way originally because one of the callsites
> generated the item using xstrfmt(). But that spot switched to a plain
> xstrdup() in cf01f617b9 (bisect: fix misuse of `refs_for_each_ref_in()`,
> 2026-01-28). That means we can now just let the string_list code handle
> allocation itself.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Even before cf01f617b9 we could have done:
>
>   string_list_append_nodup(&refs, xstrfmt(...));
>
> to get a similar simplification, but after that commit it is even
> easier.
>
>  bisect.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 2cd97bc9fe..6b9e9d81c3 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -1183,8 +1183,7 @@ int estimate_bisect_steps(int all)
>  static int mark_for_removal(const struct reference *ref, void *cb_data)
>  {
>  	struct string_list *refs = cb_data;
> -	char *bisect_ref = xstrdup(ref->name);
> -	string_list_append(refs, bisect_ref);
> +	string_list_append(refs, ref->name);
>  	return 0;
>  }
>  
> @@ -1193,16 +1192,15 @@ int bisect_clean_state(void)
>  	int result = 0;
>  
>  	/* There may be some refs packed during bisection */
> -	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> +	struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
>  	refs_for_each_fullref_in(get_main_ref_store(the_repository),
>  				 "refs/bisect/", NULL, mark_for_removal,
>  				 &refs_for_removal);
> -	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
> -	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
> +	string_list_append(&refs_for_removal, "BISECT_HEAD");
> +	string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV");
>  	result = refs_delete_refs(get_main_ref_store(the_repository),
>  				  "bisect: remove", &refs_for_removal,
>  				  REF_NO_DEREF);
> -	refs_for_removal.strdup_strings = 1;
>  	string_list_clear(&refs_for_removal, 0);
>  	unlink_or_warn(git_path_bisect_ancestors_ok());
>  	unlink_or_warn(git_path_bisect_log());

Re: [PATCH 2/3] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Taylor Blau]

On Thu, Jan 29, 2026 at 08:52:43AM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Looking at the implementation of bitmap_writer_select_commits(), we do
> > not guarantee that *any* reference specified by pack.preferBitmapTips
> > will receive a bitmap. That's because we don't necessarily enumerate the
> > entire set of commits when determining which ones to bitmap.
>
> Hmph.  Is this documented?
>
> 	... Goes and looks ...
>
> Yes, it is documented.  We say "This is because ..." but it just
> explains it as what the chosen design of the implementation happens
> to do, without saying for what benefit the implementation was chosen,
> so it is unclear if this is designed behaviour, or more importantly,
> even if this were designed, what the rationale of choosing that
> design was.
>
> "When they are so close to fall into the same chunk, there is no
> point having bitmaps individually for them, as their bitmaps will be
> very similar anyway, so this design saves space without sacrificing
> the quality of the resulting set of bitmaps" or something?

The commits are generally presented in the order they are traversed
(regardless of whether we are generating single- or multi-pack bitmaps).
That makes it likely that commits within the same window are likely to
generate very similar bitmaps, but it is not guaranteed.

When looking at the documentation, I ended up with the following:

--- 8< ---
diff --git a/Documentation/config/pack.adoc b/Documentation/config/pack.adoc
index 75402d5579d..b65cbaaebb4 100644
--- a/Documentation/config/pack.adoc
+++ b/Documentation/config/pack.adoc
@@ -168,7 +168,10 @@ pack.preferBitmapTips::
 Note that setting this configuration to `refs/foo` does not mean that
 the commits at the tips of `refs/foo/bar` and `refs/foo/baz` will
 necessarily be selected. This is because commits are selected for
-bitmaps from within a series of windows of variable length.
+bitmaps from within a series of windows of variable length (in order to
+space bitmaps out throughout history), and we only select one commit per
+window. Thus if multiple preferred commits appear in the same window,
+only one will be selected.
 +
 If a commit at the tip of any reference which is a suffix of any value
 of this configuration is seen in a window, it is immediately given
--- >8 ---

> > At the very least, if we do end up going in this direction (and I am not
> > necessarily advocating that we do, since I would prefer a more
> > consistent set of behavior), we should at minimum document it in
> > git-config(1).
>
> The documentation says "... reference that is a suffix of any value
> of this configuration".  Is "refs/heads/foobar" a "suffix" of
> "refs/heads/foo"?  I actually find this phrasing fairly strange, as
> I do not think of "refs/heads/main" be a "suffix" of "refs/heads/".

I agree, the use of "suffix" is confusing at best. I think if/how we
change this section depends on the outcome of this series, but the
original intent was to say that preferring "refs/heads/foo" would make
the commits at the tips of "refs/heads/foo/bar" and "refs/heads/foo/baz"
preferred, but not "refs/heads/foobar".

Thanks,
Taylor

Re: [PATCH 2/3] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> When looking at the documentation, I ended up with the following:
>
> --- 8< ---
> diff --git a/Documentation/config/pack.adoc b/Documentation/config/pack.adoc
> index 75402d5579d..b65cbaaebb4 100644
> --- a/Documentation/config/pack.adoc
> +++ b/Documentation/config/pack.adoc
> @@ -168,7 +168,10 @@ pack.preferBitmapTips::
>  Note that setting this configuration to `refs/foo` does not mean that
>  the commits at the tips of `refs/foo/bar` and `refs/foo/baz` will
>  necessarily be selected. This is because commits are selected for
> -bitmaps from within a series of windows of variable length.
> +bitmaps from within a series of windows of variable length (in order to
> +space bitmaps out throughout history), and we only select one commit per
> +window. Thus if multiple preferred commits appear in the same window,
> +only one will be selected.

That's certainly better.

>> The documentation says "... reference that is a suffix of any value
>> of this configuration".  Is "refs/heads/foobar" a "suffix" of
>> "refs/heads/foo"?  I actually find this phrasing fairly strange, as
>> I do not think of "refs/heads/main" be a "suffix" of "refs/heads/".
>
> I agree, the use of "suffix" is confusing at best. I think if/how we
> change this section depends on the outcome of this series, but the
> original intent was to say that preferring "refs/heads/foo" would make
> the commits at the tips of "refs/heads/foo/bar" and "refs/heads/foo/baz"
> preferred, but not "refs/heads/foobar".

I am still not sure if naming an individual ref is an intended use,
but I assume that the original intent of this part of the document
was to specify the leading hierarchies and commits at the tip of
refs that appear in one of the listed hiearchies are used as
preferred candidates to give bitmaps.

    pack.preferBitmapTips::

	Specifies a ref hierarchy (e.g., "refs/heads/"); can be
	given multiple times to specify more than one hierarchies.

	When selecting which commits will receive bitmaps, prefer a
        commmit at the tip of a reference that appears in one of the
        hierarchies specified over any other commits ...


or something?

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

[Patrick Steinhardt]

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

Re: [PATCH 3/3] bisect: fix misuse of `refs_for_each_ref_in()`

[Patrick Steinhardt]

On Thu, Jan 29, 2026 at 09:28:58AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Jan 28, 2026 at 09:49:22AM +0100, Patrick Steinhardt wrote:
> >
> >> Furthermore, taking a closer look, we strip the prefix only to re-add it
> >> in `mark_for_removal()`. This is somewhat roundabout, as we can instead
> >> call `refs_for_each_fullref_in()` to not do any stripping at all. Do so
> >> to simplify the code a bit.
> >
> > Yeah, I think the result is much better.
> >
> > We might also want this simplification on top:
> 
> Very good.  Thanks, both, for improvements.

Indeed, I'll apply your patch on top of what I have. Thanks!

Patrick

Re: [PATCH 2/3] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Patrick Steinhardt]

On Wed, Jan 28, 2026 at 09:31:24PM -0500, Taylor Blau wrote:
> On Wed, Jan 28, 2026 at 09:49:21AM +0100, Patrick Steinhardt wrote:
> > The "pack.preferBitmapTips" configuration allows the user to specify
> > which references should be preferred when generating bitmaps. This
> > option is typically expected to be set to a reference prefix, like for
> > example "refs/heads/".
> >
> > It's not unreasonable though for a user to configure one specific
> > reference as preferred. But if they do, they'll hit a `BUG()`:
> >
> >     $ git -c pack.preferBitmapTips=refs/heads/main repack -adb
> >     BUG: ../refs/iterator.c:366: attempt to trim too many characters
> >     error: pack-objects died of signal 6
> 
> Oops. While we should definitely not BUG() here, I am not sure I
> understand the desired use-case of specifying a single reference as a
> value for pack.preferBitmapTips.

There is not really a desired use case here, I just happened to stumble
over this bug due to playing around with the feature.

> Looking at the implementation of bitmap_writer_select_commits(), we do
> not guarantee that *any* reference specified by pack.preferBitmapTips
> will receive a bitmap. That's because we don't necessarily enumerate the
> entire set of commits when determining which ones to bitmap.

Yeah, we don't indeed.

[snip]
> > One resulting weirdness is that two refs "refs/heads/base" and
> > "refs/heads/base-something" would now match if the user configured
> > "refs/heads/base" as bitmap tips. One could arguably change the
> > semantics of the configuration such that a string without a trailing
> > slash needs to be an exact reference match, whereas a string with a
> > trailing slash indicates a directory hierarchy. But such a change would
> > potentially cause regressions with dubious benefits, so this issue is
> > ignored for now.
> 
> (Setting aside the for_each_ref vs. for_each_fullref issue for a
> moment...)
> 
> Am I understanding this change correctly that doing something like -c
> pack.preferBitmapTips=refs/heads/foo would match both foo and foobar?
> 
> If so, I am not sure that that is a desirable interface, especially
> since we went the opposite direction in 10e8a9352bc (refs.c: stop
> matching non-directory prefixes in exclude patterns, 2025-03-06). Having
> the two behave inconsistently from one another feels somewhat awkward to
> me and may lead to unexpected results.

I don't have too much skin in the game, and as I wrote I agree that the
things are a bit nuanced here. I think there's two major ways to go from
here:

  - We can either fix the bug and say that we accept all references that
    start with the configured prefix. "refs/heads/main" _does_ start
    with the prefix "refs/heads/main", so it should match.

  - Or we can fix the bug by appending a slash to the configured prefix
    if it doesn't already have one.

The reason I picked the first option here is mostly because it allows
for more options rather than restricting options. The user has the
ability to both match hierarchies by appending a "/", and they can have
ref-prefix-matches by not doing so.

I'll adapt the commit message accordingly to document my thought
process, and...

> At the very least, if we do end up going in this direction (and I am not
> necessarily advocating that we do, since I would prefer a more
> consistent set of behavior), we should at minimum document it in
> git-config(1).

... will also adapt the documentation accordingly.

That being said, I'm also open to adapt my approach here.

Thanks!

Patrick

[PATCH v2 0/4] Fix misuse of `refs_for_each_ref_in()`

[Patrick Steinhardt]

Hi,

this small patch series fixes a bug I have discovered where configuring
"pack.preferBitmapTips" to an exact branch will cause Git to `BUG()`.

The root cause of this bug is misuse of `refs_for_each_ref_in()`: this
function accepts a prefix to yield refs for, and then strips the prefix
for each ref. Consequently, if passed an exact refname, then stripping
the prefix would make us end up with an empty refname, and that is not
supposed to happen.

There was one other caller that got it wrong, too, and which is also
fixed in this patch series.

Changes in v2:
  - Explain my thought process against why I chose to also allow exact
    ref matches in the second commit and clarify the documentation a
    bit. As said, I'm very open to changing this if my spelled-out
    thoughts are not convincing.
  - Apply Peff's patch to further simplify code.
  - Link to v1: https://lore.kernel.org/r/20260128-b4-pks-fix-for-each-ref-in-misuse-v1-0-deccae3ea725@pks.im

Thanks!

Patrick

---
Jeff King (1):
      bisect: simplify string_list memory handling

Patrick Steinhardt (3):
      pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
      pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
      bisect: fix misuse of `refs_for_each_ref_in()`

 Documentation/config/pack.adoc |  7 +++----
 bisect.c                       | 16 +++++++---------
 builtin/pack-objects.c         | 19 ++-----------------
 pack-bitmap.c                  | 18 +++++++++++++++++-
 pack-bitmap.h                  |  9 ++++++++-
 repack-midx.c                  | 14 +++-----------
 t/t5310-pack-bitmaps.sh        | 35 +++++++++++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh    | 36 ++++++++++++++++++++++++++++++++++++
 8 files changed, 111 insertions(+), 43 deletions(-)

Range-diff versus v1:

1:  dcdc4a5aa2 ! 1:  2a2558bb22 pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
    @@ Commit message
     
         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.
    +    callsites are subtly wrong: when the preferred bitmap tips contain an
    +    exact refname match, then we will hit a `BUG()`.
     
    -    Prepare for this fix by unifying the two callsites into a new
    +    Prepare for the 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.
    +    only. Note that the function is still used by a second caller, so we
    +    cannot just inline it.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
2:  5b14a8a680 ! 2:  5b4a33a0bd pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
    @@ Commit message
             lead to a somewhat weird result.
     
         Neither of these feel like viable options, so calling `BUG()` feels like
    -    a sensible way out.
    -
    -    The root cause really is that we try to trim the whole refname. We can
    -    thus easily fix the bug itself by calling `refs_for_each_fullref_in()`
    -    instead. This function behaves the same as `refs_for_each_ref_in()`,
    -    except that it doesn't strip the prefix. Consequently, it correctly
    -    yields also exact refnames.
    -
    -    One resulting weirdness is that two refs "refs/heads/base" and
    -    "refs/heads/base-something" would now match if the user configured
    -    "refs/heads/base" as bitmap tips. One could arguably change the
    -    semantics of the configuration such that a string without a trailing
    -    slash needs to be an exact reference match, whereas a string with a
    -    trailing slash indicates a directory hierarchy. But such a change would
    -    potentially cause regressions with dubious benefits, so this issue is
    -    ignored for now.
    +    a sensible way out. The root cause ultimately is that we even try to
    +    trim the whole refname in the first place. There are two possible ways
    +    to fix this issue:
    +
    +      - We can fix the bug by using `refs_for_each_fullref_in()` instead,
    +        which does not strip the prefix at all. Consequently, we would now
    +        start to accept all references that start with the configured
    +        prefix, including exact matches. So if we had "refs/heads/main", we
    +        would both match "refs/heads/main" and "refs/heads/main-branch".
    +
    +      - Or we can fix the bug by appending a slash to the prefix if it
    +        doesn't already have one. This would mean that we only match
    +        ref hierarchies that start with this prefix.
    +
    +    The first fix leaves the user with strictly _more_ configuration
    +    options: they can have prefix matches by not appending a slash to the
    +    configuration, and they can have ref hierarchy matches by appending one.
    +
    +    Apply this fix and clarify the documentation accordingly.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    + ## Documentation/config/pack.adoc ##
    +@@ Documentation/config/pack.adoc: pack.usePathWalk::
    + 
    + pack.preferBitmapTips::
    + 	When selecting which commits will receive bitmaps, prefer a
    +-	commit at the tip of any reference that is a suffix of any value
    +-	of this configuration over any other commits in the "selection
    +-	window".
    ++	commmit at the tip of a reference that matches any of the
    ++	configured prefixes.
    + +
    +-Note that setting this configuration to `refs/foo` does not mean that
    ++Note that setting this configuration to `refs/foo/` does not mean that
    + the commits at the tips of `refs/foo/bar` and `refs/foo/baz` will
    + necessarily be selected. This is because commits are selected for
    + bitmaps from within a series of windows of variable length.
    +
      ## pack-bitmap.c ##
     @@ pack-bitmap.c: void for_each_preferred_bitmap_tip(struct repository *repo,
      		return;
3:  2e1bfb0894 = 3:  1579608b04 bisect: fix misuse of `refs_for_each_ref_in()`
-:  ---------- > 4:  99a80bdb37 bisect: simplify string_list memory handling

---
base-commit: ea717645d199f6f1b66058886475db3e8c9330e9
change-id: 20260128-b4-pks-fix-for-each-ref-in-misuse-96ae62e313a5

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

[Patrick Steinhardt]

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: when the preferred bitmap tips contain an
exact refname match, then we will hit a `BUG()`.

Prepare for the 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. Note that the function is still used by a second caller, so we
cannot just inline it.

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

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;
+
+	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;
 
 	refs_for_each_ref(get_main_ref_store(repo),
 			  midx_snapshot_ref_one, &data);

-- 
2.53.0.rc2.206.g60c1bca835.dirty

[PATCH v2 2/4] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Patrick Steinhardt]

The "pack.preferBitmapTips" configuration allows the user to specify
which references should be preferred when generating bitmaps. This
option is typically expected to be set to a reference prefix, like for
example "refs/heads/".

It's not unreasonable though for a user to configure one specific
reference as preferred. But if they do, they'll hit a `BUG()`:

    $ git -c pack.preferBitmapTips=refs/heads/main repack -adb
    BUG: ../refs/iterator.c:366: attempt to trim too many characters
    error: pack-objects died of signal 6

The root cause for this bug is how we enumerate these references. We
call `refs_for_each_ref_in()`, which will:

  - Yield all references that have a user-specified prefix.

  - Trim each of these references so that the prefix is removed.

Typically, this function is called with a trailing slash, like
"refs/heads/", and in that case things work alright. But if the function
is called with the name of an existing reference then we'll try to trim
the full reference name, which would leave us with an empty name. And as
this would not really leave us with anything sensible, we call `BUG()`
instead of yielding this reference.

One could argue that this is a bug in `refs_for_each_ref_in()`. But the
question then becomes what the correct behaviour would be:

  - Do we want to skip exact matches? In our case we certainly don't
    want that, as the user has asked us to generate a bitmap for it.

  - Do we want to yield the reference with the empty refname? That would
    lead to a somewhat weird result.

Neither of these feel like viable options, so calling `BUG()` feels like
a sensible way out. The root cause ultimately is that we even try to
trim the whole refname in the first place. There are two possible ways
to fix this issue:

  - We can fix the bug by using `refs_for_each_fullref_in()` instead,
    which does not strip the prefix at all. Consequently, we would now
    start to accept all references that start with the configured
    prefix, including exact matches. So if we had "refs/heads/main", we
    would both match "refs/heads/main" and "refs/heads/main-branch".

  - Or we can fix the bug by appending a slash to the prefix if it
    doesn't already have one. This would mean that we only match
    ref hierarchies that start with this prefix.

The first fix leaves the user with strictly _more_ configuration
options: they can have prefix matches by not appending a slash to the
configuration, and they can have ref hierarchy matches by appending one.

Apply this fix and clarify the documentation accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/config/pack.adoc |  7 +++----
 pack-bitmap.c                  |  4 ++--
 t/t5310-pack-bitmaps.sh        | 35 +++++++++++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh    | 36 ++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/Documentation/config/pack.adoc b/Documentation/config/pack.adoc
index 75402d5579..929d781552 100644
--- a/Documentation/config/pack.adoc
+++ b/Documentation/config/pack.adoc
@@ -161,11 +161,10 @@ pack.usePathWalk::
 
 pack.preferBitmapTips::
 	When selecting which commits will receive bitmaps, prefer a
-	commit at the tip of any reference that is a suffix of any value
-	of this configuration over any other commits in the "selection
-	window".
+	commmit at the tip of a reference that matches any of the
+	configured prefixes.
 +
-Note that setting this configuration to `refs/foo` does not mean that
+Note that setting this configuration to `refs/foo/` does not mean that
 the commits at the tips of `refs/foo/bar` and `refs/foo/baz` will
 necessarily be selected. This is because commits are selected for
 bitmaps from within a series of windows of variable length.
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 2f5cb34009..8d3b5ac037 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -3334,8 +3334,8 @@ void for_each_preferred_bitmap_tip(struct repository *repo,
 		return;
 
 	for_each_string_list_item(item, preferred_tips) {
-		refs_for_each_ref_in(get_main_ref_store(repo),
-				     item->string, cb, cb_data);
+		refs_for_each_fullref_in(get_main_ref_store(repo),
+					 item->string, NULL, cb, cb_data);
 	}
 }
 
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 6718fb98c0..7ef91b502c 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -466,6 +466,41 @@ test_bitmap_cases () {
 		)
 	'
 
+	test_expect_success 'pack.preferBitmapTips can use direct refname' '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+
+			# Create enough commits that not all will receive bitmap
+			# coverage even if they are all at the tip of some reference.
+			test_commit_bulk --message="%s" 103 &&
+			git log --format="create refs/tags/%s %H" HEAD >refs &&
+			git update-ref --stdin <refs &&
+
+			# Create the bitmap.
+			git repack -adb &&
+			test-tool bitmap list-commits | sort >commits-with-bitmap &&
+
+			# Verify that we have at least one commit that did not
+			# receive a bitmap.
+			git rev-list HEAD >commits.raw &&
+			sort <commits.raw >commits &&
+			comm -13 commits-with-bitmap commits >commits-wo-bitmap &&
+			test_file_not_empty commits-wo-bitmap &&
+			commit_id=$(head commits-wo-bitmap) &&
+
+			# We now create a reference for this commit and repack
+			# with "preferBitmapTips" pointing to that exact
+			# reference. The expectation is that it will now be
+			# covered by a bitmap.
+			git update-ref refs/heads/cover-me "$commit_id" &&
+			git -c pack.preferBitmapTips=refs/heads/cover-me repack -adb &&
+			test-tool bitmap list-commits >after &&
+			test_grep "$commit_id" after
+		)
+	'
+
 	test_expect_success 'complains about multiple pack bitmaps' '
 		rm -fr repo &&
 		git init repo &&
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index faae98c7e7..40d36118bd 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1345,4 +1345,40 @@ test_expect_success 'bitmapped packs are stored via the BTMP chunk' '
 	)
 '
 
+test_expect_success 'pack.preferBitmapTips can use direct refname' '
+	git init repo &&
+	test_when_finished "rm -fr repo" &&
+	(
+		cd repo &&
+
+		# Create enough commits that not all will receive bitmap
+		# coverage even if they are all at the tip of some reference.
+		test_commit_bulk --message="%s" 103 &&
+		git log --format="create refs/tags/%s %H" HEAD >refs &&
+		git update-ref --stdin <refs &&
+
+		# Create the bitmap via the MIDX.
+		git repack -adb --write-midx &&
+		test-tool bitmap list-commits | sort >commits-with-bitmap &&
+
+		# Verify that we have at least one commit that did not
+		# receive a bitmap.
+		git rev-list HEAD >commits.raw &&
+		sort <commits.raw >commits &&
+		comm -13 commits-with-bitmap commits >commits-wo-bitmap &&
+		test_file_not_empty commits-wo-bitmap &&
+		commit_id=$(head commits-wo-bitmap) &&
+
+		# We now create a reference for this commit and repack
+		# with "preferBitmapTips" pointing to that exact
+		# reference. The expectation is that it will now be
+		# covered by a bitmap.
+		git update-ref refs/heads/cover-me "$commit_id" &&
+		rm .git/objects/pack/multi-pack-index* &&
+		git -c pack.preferBitmapTips=refs/heads/cover-me repack -adb --write-midx &&
+		test-tool bitmap list-commits >after &&
+		test_grep "$commit_id" after
+	)
+'
+
 test_done

-- 
2.53.0.rc2.206.g60c1bca835.dirty

[PATCH v2 3/4] bisect: fix misuse of `refs_for_each_ref_in()`

[Patrick Steinhardt]

All callers of `refs_for_each_ref_in()` pass in a string that is
terminated with a trailing slash to indicate that they only want to see
refs in that specific ref hierarchy. This is in fact a requirement if
one wants to use this function, as the function trims the prefix from
each yielded ref. So if there was a reference that was called
"refs/bisect" as in our example, the result after trimming would be the
empty string, and that's something we disallow.

Fix this by adding the trailing slash.

Furthermore, taking a closer look, we strip the prefix only to re-add it
in `mark_for_removal()`. This is somewhat roundabout, as we can instead
call `refs_for_each_fullref_in()` to not do any stripping at all. Do so
to simplify the code a bit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index 326b59c0dc..4f0d1a1853 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1180,7 +1180,7 @@ int estimate_bisect_steps(int all)
 static int mark_for_removal(const struct reference *ref, void *cb_data)
 {
 	struct string_list *refs = cb_data;
-	char *bisect_ref = xstrfmt("refs/bisect%s", ref->name);
+	char *bisect_ref = xstrdup(ref->name);
 	string_list_append(refs, bisect_ref);
 	return 0;
 }
@@ -1191,9 +1191,9 @@ int bisect_clean_state(void)
 
 	/* There may be some refs packed during bisection */
 	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
-	refs_for_each_ref_in(get_main_ref_store(the_repository),
-			     "refs/bisect", mark_for_removal,
-			     (void *) &refs_for_removal);
+	refs_for_each_fullref_in(get_main_ref_store(the_repository),
+				 "refs/bisect/", NULL, mark_for_removal,
+				 &refs_for_removal);
 	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
 	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
 	result = refs_delete_refs(get_main_ref_store(the_repository),

-- 
2.53.0.rc2.206.g60c1bca835.dirty

[PATCH v2 4/4] bisect: simplify string_list memory handling

[Patrick Steinhardt]

From: Jeff King <peff@peff.net>

We declare the refs_for_removal string_list as NODUP, forcing us to
manually allocate strings we insert. And then when it comes time to
clean up, we set strdup_strings so that string_list_clear() will free
them for us.

This is a confusing pattern, and can be done much more simply by just
declaring the list with the DUP initializer in the first place.

It was written this way originally because one of the callsites
generated the item using xstrfmt(). But that spot switched to a plain
xstrdup() in the preceding commit. That means we can now just let the
string_list code handle allocation itself.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/bisect.c b/bisect.c
index 4f0d1a1853..268f5e36f8 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1180,8 +1180,7 @@ int estimate_bisect_steps(int all)
 static int mark_for_removal(const struct reference *ref, void *cb_data)
 {
 	struct string_list *refs = cb_data;
-	char *bisect_ref = xstrdup(ref->name);
-	string_list_append(refs, bisect_ref);
+	string_list_append(refs, ref->name);
 	return 0;
 }
 
@@ -1190,16 +1189,15 @@ int bisect_clean_state(void)
 	int result = 0;
 
 	/* There may be some refs packed during bisection */
-	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
+	struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
 	refs_for_each_fullref_in(get_main_ref_store(the_repository),
 				 "refs/bisect/", NULL, mark_for_removal,
 				 &refs_for_removal);
-	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
-	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
+	string_list_append(&refs_for_removal, "BISECT_HEAD");
+	string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV");
 	result = refs_delete_refs(get_main_ref_store(the_repository),
 				  "bisect: remove", &refs_for_removal,
 				  REF_NO_DEREF);
-	refs_for_removal.strdup_strings = 1;
 	string_list_clear(&refs_for_removal, 0);
 	unlink_or_warn(git_path_bisect_ancestors_ok());
 	unlink_or_warn(git_path_bisect_log());

-- 
2.53.0.rc2.206.g60c1bca835.dirty

Re: [PATCH v2 4/4] bisect: simplify string_list memory handling

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> It was written this way originally because one of the callsites
> generated the item using xstrfmt(). But that spot switched to a plain
> xstrdup() in the preceding commit. That means we can now just let the
> string_list code handle allocation itself.

Thanks for an extra attention to the detail of the way to refer the
previous change ;-).

I think [2/4] is a good direction myself, but I'd prefer to hear
Taylor's opinion as well.

Thanks.

Re: [PATCH v2 2/4] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Taylor Blau]

On Fri, Jan 30, 2026 at 02:27:43PM +0100, Patrick Steinhardt wrote:
> [...] There are two possible ways to fix this issue:
>
>   - We can fix the bug by using `refs_for_each_fullref_in()` instead,
>     which does not strip the prefix at all. Consequently, we would now
>     start to accept all references that start with the configured
>     prefix, including exact matches. So if we had "refs/heads/main", we
>     would both match "refs/heads/main" and "refs/heads/main-branch".
>
>   - Or we can fix the bug by appending a slash to the prefix if it
>     doesn't already have one. This would mean that we only match
>     ref hierarchies that start with this prefix.
>
> The first fix leaves the user with strictly _more_ configuration
> options: they can have prefix matches by not appending a slash to the
> configuration, and they can have ref hierarchy matches by appending one.

I would definitely like to err on the side of more flexible
configuration options, but I am still concerned that this change would
lead to somewhat surprising behavior.

A couple of thoughts:

 - Like I mentioned in the earlier round, 10e8a9352bc (refs.c: stop
   matching non-directory prefixes in exclude patterns, 2025-03-06)
   takes the opposite approach as what is being proposed here. I worry
   that users will find the difference in behavior between
   pack.preferBitmapTips and for-each-ref's --exclude patterns to be
   confusing.

 - If a user wants to list all references that start with
   "refs/heads/ma" in the string prefix sense (that is, matching
   "refs/heads/ma", "refs/heads/main", "refs/heads/master" and so on),
   then they would do

     $ git for-each-ref 'refs/heads/ma*'

   , not 'refs/heads/ma'. In fact, enumerating 'refs/heads/ma' when
   there exist references "refs/heads/ma/foo", "refs/heads/ma/bar",
   etc., for-each-ref will output those three references (but only
   "refs/heads/ma" itself if it exists).

I suppose there is an argument to be made that we are dealing with
"patterns" vs. "prefixes" here, but TBH I am not sure that is a
distinction that is well-understood by users (nor should we expect it to
be).

The original intent of this configuration was that "suffix" in this
context meant directory suffix or exact match, not string suffix. The
implementation does not match that intent, but I think there is enough
ambiguity here that I wouldn't consider the change I'm suggesting to be
a breaking one.

Overall, I think interpreting the pack.preferBitmapTips configuration as
a reference pattern gives the user both (a) more flexibility in which
references to match, and (b) does so in a way that is consistent with
10e8a9352bc and the existing behavior of for-each-ref.

Thanks,
Taylor

Re: [PATCH v2 4/4] bisect: simplify string_list memory handling

[Taylor Blau]

On Fri, Jan 30, 2026 at 08:56:36AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > It was written this way originally because one of the callsites
> > generated the item using xstrfmt(). But that spot switched to a plain
> > xstrdup() in the preceding commit. That means we can now just let the
> > string_list code handle allocation itself.
>
> Thanks for an extra attention to the detail of the way to refer the
> previous change ;-).
>
> I think [2/4] is a good direction myself, but I'd prefer to hear
> Taylor's opinion as well.

After thinking it over and re-reading the second round, I am still not
quite convinced that this is the right approach. I left some more
thoughts on possible alternatives in my response to [2/4].

Thanks,
Taylor

Re: [PATCH v2 2/4] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Patrick Steinhardt]

On Sun, Feb 01, 2026 at 09:13:43PM -0500, Taylor Blau wrote:
> On Fri, Jan 30, 2026 at 02:27:43PM +0100, Patrick Steinhardt wrote:
> > [...] There are two possible ways to fix this issue:
> >
> >   - We can fix the bug by using `refs_for_each_fullref_in()` instead,
> >     which does not strip the prefix at all. Consequently, we would now
> >     start to accept all references that start with the configured
> >     prefix, including exact matches. So if we had "refs/heads/main", we
> >     would both match "refs/heads/main" and "refs/heads/main-branch".
> >
> >   - Or we can fix the bug by appending a slash to the prefix if it
> >     doesn't already have one. This would mean that we only match
> >     ref hierarchies that start with this prefix.
> >
> > The first fix leaves the user with strictly _more_ configuration
> > options: they can have prefix matches by not appending a slash to the
> > configuration, and they can have ref hierarchy matches by appending one.
> 
> I would definitely like to err on the side of more flexible
> configuration options, but I am still concerned that this change would
> lead to somewhat surprising behavior.

I think my point is mostly that this somewhat surprising behaviour
already exists right now.

> A couple of thoughts:
> 
>  - Like I mentioned in the earlier round, 10e8a9352bc (refs.c: stop
>    matching non-directory prefixes in exclude patterns, 2025-03-06)
>    takes the opposite approach as what is being proposed here. I worry
>    that users will find the difference in behavior between
>    pack.preferBitmapTips and for-each-ref's --exclude patterns to be
>    confusing.
> 
>  - If a user wants to list all references that start with
>    "refs/heads/ma" in the string prefix sense (that is, matching
>    "refs/heads/ma", "refs/heads/main", "refs/heads/master" and so on),
>    then they would do
> 
>      $ git for-each-ref 'refs/heads/ma*'
> 
>    , not 'refs/heads/ma'. In fact, enumerating 'refs/heads/ma' when
>    there exist references "refs/heads/ma/foo", "refs/heads/ma/bar",
>    etc., for-each-ref will output those three references (but only
>    "refs/heads/ma" itself if it exists).
> 
> I suppose there is an argument to be made that we are dealing with
> "patterns" vs. "prefixes" here, but TBH I am not sure that is a
> distinction that is well-understood by users (nor should we expect it to
> be).
> 
> The original intent of this configuration was that "suffix" in this
> context meant directory suffix or exact match, not string suffix. The
> implementation does not match that intent, but I think there is enough
> ambiguity here that I wouldn't consider the change I'm suggesting to be
> a breaking one.

I wouldn't quite frame it in the way of ambiguity, but rather in the way
of it being very niche. I would argue that almost nobody out there will
use this configuration outside of hosting providers. GitHub will
probably use it correctly, GitLab doesn't use it at all.

So I guess it's fine overall if we introduce a breaking change here.

> Overall, I think interpreting the pack.preferBitmapTips configuration as
> a reference pattern gives the user both (a) more flexibility in which
> references to match, and (b) does so in a way that is consistent with
> 10e8a9352bc and the existing behavior of for-each-ref.

I disagree with (a) as you can do strictly less, but being constistent
with (b) might be a good thing.

At the end I'm not entirely convinced by the arguments, but as I said I
don't have too much skin in the game, either. So let's take your
approach.

Thanks!

Patrick

[PATCH v3 0/4] Fix misuse of `refs_for_each_ref_in()`

[Patrick Steinhardt]

Hi,

this small patch series fixes a bug I have discovered where configuring
"pack.preferBitmapTips" to an exact branch will cause Git to `BUG()`.

The root cause of this bug is misuse of `refs_for_each_ref_in()`: this
function accepts a prefix to yield refs for, and then strips the prefix
for each ref. Consequently, if passed an exact refname, then stripping
the prefix would make us end up with an empty refname, and that is not
supposed to happen.

There was one other caller that got it wrong, too, and which is also
fixed in this patch series.

Changes in v3:
  - Switch the approach to perform ref hierarchy matches instead, which
    is in line with the changes in 10e8a9352b (refs.c: stop matching
    non-directory prefixes in exclude patterns, 2025-03-06).
  - Link to v2: https://lore.kernel.org/r/20260130-b4-pks-fix-for-each-ref-in-misuse-v2-0-0449b198a681@pks.im

Changes in v2:
  - Explain my thought process against why I chose to also allow exact
    ref matches in the second commit and clarify the documentation a
    bit. As said, I'm very open to changing this if my spelled-out
    thoughts are not convincing.
  - Apply Peff's patch to further simplify code.
  - Link to v1: https://lore.kernel.org/r/20260128-b4-pks-fix-for-each-ref-in-misuse-v1-0-deccae3ea725@pks.im

Thanks!

Patrick

---
Jeff King (1):
      bisect: simplify string_list memory handling

Patrick Steinhardt (3):
      pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
      pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
      bisect: fix misuse of `refs_for_each_ref_in()`

 Documentation/config/pack.adoc |  9 +++++----
 bisect.c                       | 16 +++++++---------
 builtin/pack-objects.c         | 19 ++-----------------
 pack-bitmap.c                  | 29 +++++++++++++++++++++++++++-
 pack-bitmap.h                  |  9 ++++++++-
 repack-midx.c                  | 14 +++-----------
 t/t5310-pack-bitmaps.sh        | 41 ++++++++++++++++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh    | 43 ++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 137 insertions(+), 43 deletions(-)

Range-diff versus v2:

1:  276b593c0d = 1:  927e8bf4ae pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
2:  e7a5e5e447 ! 2:  3e4ae331dd pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
    @@ Commit message
             doesn't already have one. This would mean that we only match
             ref hierarchies that start with this prefix.
     
    -    The first fix leaves the user with strictly _more_ configuration
    -    options: they can have prefix matches by not appending a slash to the
    -    configuration, and they can have ref hierarchy matches by appending one.
    +    While the first fix leaves the user with strictly _more_ configuration
    +    options, we have already fixed a similar case in 10e8a9352b (refs.c:
    +    stop matching non-directory prefixes in exclude patterns, 2025-03-06) by
    +    using the second option. So for the sake of consistency, let's apply the
    +    same fix here.
     
    -    Apply this fix and clarify the documentation accordingly.
    +    Clarify the documentation accordingly.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## Documentation/config/pack.adoc ##
     @@ Documentation/config/pack.adoc: pack.usePathWalk::
    + 	processes. See linkgit:git-pack-objects[1] for full details.
      
      pack.preferBitmapTips::
    ++	Specifies a ref hierarchy (e.g., "refs/heads/"); can be
    ++	given multiple times to specify more than one hierarchies.
      	When selecting which commits will receive bitmaps, prefer a
     -	commit at the tip of any reference that is a suffix of any value
     -	of this configuration over any other commits in the "selection
     -	window".
    -+	commmit at the tip of a reference that matches any of the
    -+	configured prefixes.
    ++	commmit at the tip of a reference that is contained in any of
    ++	the configured hierarchies.
      +
     -Note that setting this configuration to `refs/foo` does not mean that
     +Note that setting this configuration to `refs/foo/` does not mean that
    @@ Documentation/config/pack.adoc: pack.usePathWalk::
     
      ## pack-bitmap.c ##
     @@ pack-bitmap.c: void for_each_preferred_bitmap_tip(struct repository *repo,
    + {
    + 	struct string_list_item *item;
    + 	const struct string_list *preferred_tips;
    ++	struct strbuf buf = STRBUF_INIT;
    + 
    + 	preferred_tips = bitmap_preferred_tips(repo);
    + 	if (!preferred_tips)
      		return;
      
      	for_each_string_list_item(item, preferred_tips) {
    --		refs_for_each_ref_in(get_main_ref_store(repo),
    ++		const char *pattern = item->string;
    ++
    ++		if (!ends_with(pattern, "/")) {
    ++			strbuf_reset(&buf);
    ++			strbuf_addf(&buf, "%s/", pattern);
    ++			pattern = buf.buf;
    ++		}
    ++
    + 		refs_for_each_ref_in(get_main_ref_store(repo),
     -				     item->string, cb, cb_data);
    -+		refs_for_each_fullref_in(get_main_ref_store(repo),
    -+					 item->string, NULL, cb, cb_data);
    ++				     pattern, cb, cb_data);
      	}
    ++
    ++	strbuf_release(&buf);
      }
      
    + int bitmap_is_preferred_refname(struct repository *r, const char *refname)
     
      ## t/t5310-pack-bitmaps.sh ##
     @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () {
      		)
      	'
      
    -+	test_expect_success 'pack.preferBitmapTips can use direct refname' '
    ++	test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
     +		git init repo &&
     +		test_when_finished "rm -fr repo" &&
     +		(
    @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () {
     +			# Create enough commits that not all will receive bitmap
     +			# coverage even if they are all at the tip of some reference.
     +			test_commit_bulk --message="%s" 103 &&
    -+			git log --format="create refs/tags/%s %H" HEAD >refs &&
    ++			git log --format="create refs/tags/%s/tag %H" HEAD >refs &&
     +			git update-ref --stdin <refs &&
     +
     +			# Create the bitmap.
    @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () {
     +			comm -13 commits-with-bitmap commits >commits-wo-bitmap &&
     +			test_file_not_empty commits-wo-bitmap &&
     +			commit_id=$(head commits-wo-bitmap) &&
    ++			ref_without_bitmap=$(git for-each-ref --points-at="$commit_id" --format="%(refname)") &&
     +
    -+			# We now create a reference for this commit and repack
    -+			# with "preferBitmapTips" pointing to that exact
    -+			# reference. The expectation is that it will now be
    -+			# covered by a bitmap.
    -+			git update-ref refs/heads/cover-me "$commit_id" &&
    -+			git -c pack.preferBitmapTips=refs/heads/cover-me repack -adb &&
    ++			# When passing the full refname we do not expect a
    ++			# bitmap to be generated, as it should be interpreted
    ++			# as if a slash was appended to the pattern.
    ++			git -c pack.preferBitmapTips="$ref_without_bitmap" repack -adb &&
    ++			test-tool bitmap list-commits >after &&
    ++			test_grep ! "$commit_id" after &&
    ++
    ++			# But if we pass the parent directory of the ref we
    ++			# should see a bitmap.
    ++			ref_namespace=$(dirname "$ref_without_bitmap") &&
    ++			git -c pack.preferBitmapTips="$ref_namespace" repack -adb &&
     +			test-tool bitmap list-commits >after &&
     +			test_grep "$commit_id" after
     +		)
    @@ t/t5319-multi-pack-index.sh: test_expect_success 'bitmapped packs are stored via
      	)
      '
      
    -+test_expect_success 'pack.preferBitmapTips can use direct refname' '
    ++test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
     +	git init repo &&
     +	test_when_finished "rm -fr repo" &&
     +	(
    @@ t/t5319-multi-pack-index.sh: test_expect_success 'bitmapped packs are stored via
     +		comm -13 commits-with-bitmap commits >commits-wo-bitmap &&
     +		test_file_not_empty commits-wo-bitmap &&
     +		commit_id=$(head commits-wo-bitmap) &&
    ++		ref_without_bitmap=$(git for-each-ref --points-at="$commit_id" --format="%(refname)") &&
    ++
    ++		# When passing the full refname we do not expect a bitmap to be
    ++		# generated, as it should be interpreted as if a slash was
    ++		# appended to the pattern.
    ++		rm .git/objects/pack/multi-pack-index* &&
    ++		git -c pack.preferBitmapTips="$ref_without_bitmap" repack -adb --write-midx &&
    ++		test-tool bitmap list-commits >after &&
    ++		test_grep ! "$commit_id" after &&
     +
    -+		# We now create a reference for this commit and repack
    -+		# with "preferBitmapTips" pointing to that exact
    -+		# reference. The expectation is that it will now be
    -+		# covered by a bitmap.
    -+		git update-ref refs/heads/cover-me "$commit_id" &&
    ++		# But if we pass the parent directory of the ref we should see
    ++		# a bitmap.
    ++		ref_namespace=$(dirname "$ref_without_bitmap") &&
     +		rm .git/objects/pack/multi-pack-index* &&
    -+		git -c pack.preferBitmapTips=refs/heads/cover-me repack -adb --write-midx &&
    ++		git -c pack.preferBitmapTips="$ref_namespace" repack -adb --write-midx &&
     +		test-tool bitmap list-commits >after &&
     +		test_grep "$commit_id" after
     +	)
3:  261523862b = 3:  8d82966f8f bisect: fix misuse of `refs_for_each_ref_in()`
4:  e9b0fd0d5f = 4:  7f68687165 bisect: simplify string_list memory handling

---
base-commit: ea717645d199f6f1b66058886475db3e8c9330e9
change-id: 20260128-b4-pks-fix-for-each-ref-in-misuse-96ae62e313a5

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

[Patrick Steinhardt]

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: when the preferred bitmap tips contain an
exact refname match, then we will hit a `BUG()`.

Prepare for the 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. Note that the function is still used by a second caller, so we
cannot just inline it.

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

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;
+
+	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;
 
 	refs_for_each_ref(get_main_ref_store(repo),
 			  midx_snapshot_ref_one, &data);

-- 
2.53.0.239.g8d8fc8a987.dirty

[PATCH v3 2/4] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Patrick Steinhardt]

The "pack.preferBitmapTips" configuration allows the user to specify
which references should be preferred when generating bitmaps. This
option is typically expected to be set to a reference prefix, like for
example "refs/heads/".

It's not unreasonable though for a user to configure one specific
reference as preferred. But if they do, they'll hit a `BUG()`:

    $ git -c pack.preferBitmapTips=refs/heads/main repack -adb
    BUG: ../refs/iterator.c:366: attempt to trim too many characters
    error: pack-objects died of signal 6

The root cause for this bug is how we enumerate these references. We
call `refs_for_each_ref_in()`, which will:

  - Yield all references that have a user-specified prefix.

  - Trim each of these references so that the prefix is removed.

Typically, this function is called with a trailing slash, like
"refs/heads/", and in that case things work alright. But if the function
is called with the name of an existing reference then we'll try to trim
the full reference name, which would leave us with an empty name. And as
this would not really leave us with anything sensible, we call `BUG()`
instead of yielding this reference.

One could argue that this is a bug in `refs_for_each_ref_in()`. But the
question then becomes what the correct behaviour would be:

  - Do we want to skip exact matches? In our case we certainly don't
    want that, as the user has asked us to generate a bitmap for it.

  - Do we want to yield the reference with the empty refname? That would
    lead to a somewhat weird result.

Neither of these feel like viable options, so calling `BUG()` feels like
a sensible way out. The root cause ultimately is that we even try to
trim the whole refname in the first place. There are two possible ways
to fix this issue:

  - We can fix the bug by using `refs_for_each_fullref_in()` instead,
    which does not strip the prefix at all. Consequently, we would now
    start to accept all references that start with the configured
    prefix, including exact matches. So if we had "refs/heads/main", we
    would both match "refs/heads/main" and "refs/heads/main-branch".

  - Or we can fix the bug by appending a slash to the prefix if it
    doesn't already have one. This would mean that we only match
    ref hierarchies that start with this prefix.

While the first fix leaves the user with strictly _more_ configuration
options, we have already fixed a similar case in 10e8a9352b (refs.c:
stop matching non-directory prefixes in exclude patterns, 2025-03-06) by
using the second option. So for the sake of consistency, let's apply the
same fix here.

Clarify the documentation accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/config/pack.adoc |  9 +++++----
 pack-bitmap.c                  | 13 ++++++++++++-
 t/t5310-pack-bitmaps.sh        | 41 ++++++++++++++++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh    | 43 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 101 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/pack.adoc b/Documentation/config/pack.adoc
index 75402d5579..dd2de30f0c 100644
--- a/Documentation/config/pack.adoc
+++ b/Documentation/config/pack.adoc
@@ -160,12 +160,13 @@ pack.usePathWalk::
 	processes. See linkgit:git-pack-objects[1] for full details.
 
 pack.preferBitmapTips::
+	Specifies a ref hierarchy (e.g., "refs/heads/"); can be
+	given multiple times to specify more than one hierarchies.
 	When selecting which commits will receive bitmaps, prefer a
-	commit at the tip of any reference that is a suffix of any value
-	of this configuration over any other commits in the "selection
-	window".
+	commmit at the tip of a reference that is contained in any of
+	the configured hierarchies.
 +
-Note that setting this configuration to `refs/foo` does not mean that
+Note that setting this configuration to `refs/foo/` does not mean that
 the commits at the tips of `refs/foo/bar` and `refs/foo/baz` will
 necessarily be selected. This is because commits are selected for
 bitmaps from within a series of windows of variable length.
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 2f5cb34009..1c93871484 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -3328,15 +3328,26 @@ void for_each_preferred_bitmap_tip(struct repository *repo,
 {
 	struct string_list_item *item;
 	const struct string_list *preferred_tips;
+	struct strbuf buf = STRBUF_INIT;
 
 	preferred_tips = bitmap_preferred_tips(repo);
 	if (!preferred_tips)
 		return;
 
 	for_each_string_list_item(item, preferred_tips) {
+		const char *pattern = item->string;
+
+		if (!ends_with(pattern, "/")) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "%s/", pattern);
+			pattern = buf.buf;
+		}
+
 		refs_for_each_ref_in(get_main_ref_store(repo),
-				     item->string, cb, cb_data);
+				     pattern, cb, cb_data);
 	}
+
+	strbuf_release(&buf);
 }
 
 int bitmap_is_preferred_refname(struct repository *r, const char *refname)
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 6718fb98c0..310b708c5c 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -466,6 +466,47 @@ test_bitmap_cases () {
 		)
 	'
 
+	test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+
+			# Create enough commits that not all will receive bitmap
+			# coverage even if they are all at the tip of some reference.
+			test_commit_bulk --message="%s" 103 &&
+			git log --format="create refs/tags/%s/tag %H" HEAD >refs &&
+			git update-ref --stdin <refs &&
+
+			# Create the bitmap.
+			git repack -adb &&
+			test-tool bitmap list-commits | sort >commits-with-bitmap &&
+
+			# Verify that we have at least one commit that did not
+			# receive a bitmap.
+			git rev-list HEAD >commits.raw &&
+			sort <commits.raw >commits &&
+			comm -13 commits-with-bitmap commits >commits-wo-bitmap &&
+			test_file_not_empty commits-wo-bitmap &&
+			commit_id=$(head commits-wo-bitmap) &&
+			ref_without_bitmap=$(git for-each-ref --points-at="$commit_id" --format="%(refname)") &&
+
+			# When passing the full refname we do not expect a
+			# bitmap to be generated, as it should be interpreted
+			# as if a slash was appended to the pattern.
+			git -c pack.preferBitmapTips="$ref_without_bitmap" repack -adb &&
+			test-tool bitmap list-commits >after &&
+			test_grep ! "$commit_id" after &&
+
+			# But if we pass the parent directory of the ref we
+			# should see a bitmap.
+			ref_namespace=$(dirname "$ref_without_bitmap") &&
+			git -c pack.preferBitmapTips="$ref_namespace" repack -adb &&
+			test-tool bitmap list-commits >after &&
+			test_grep "$commit_id" after
+		)
+	'
+
 	test_expect_success 'complains about multiple pack bitmaps' '
 		rm -fr repo &&
 		git init repo &&
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index faae98c7e7..449353416f 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1345,4 +1345,47 @@ test_expect_success 'bitmapped packs are stored via the BTMP chunk' '
 	)
 '
 
+test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
+	git init repo &&
+	test_when_finished "rm -fr repo" &&
+	(
+		cd repo &&
+
+		# Create enough commits that not all will receive bitmap
+		# coverage even if they are all at the tip of some reference.
+		test_commit_bulk --message="%s" 103 &&
+		git log --format="create refs/tags/%s %H" HEAD >refs &&
+		git update-ref --stdin <refs &&
+
+		# Create the bitmap via the MIDX.
+		git repack -adb --write-midx &&
+		test-tool bitmap list-commits | sort >commits-with-bitmap &&
+
+		# Verify that we have at least one commit that did not
+		# receive a bitmap.
+		git rev-list HEAD >commits.raw &&
+		sort <commits.raw >commits &&
+		comm -13 commits-with-bitmap commits >commits-wo-bitmap &&
+		test_file_not_empty commits-wo-bitmap &&
+		commit_id=$(head commits-wo-bitmap) &&
+		ref_without_bitmap=$(git for-each-ref --points-at="$commit_id" --format="%(refname)") &&
+
+		# When passing the full refname we do not expect a bitmap to be
+		# generated, as it should be interpreted as if a slash was
+		# appended to the pattern.
+		rm .git/objects/pack/multi-pack-index* &&
+		git -c pack.preferBitmapTips="$ref_without_bitmap" repack -adb --write-midx &&
+		test-tool bitmap list-commits >after &&
+		test_grep ! "$commit_id" after &&
+
+		# But if we pass the parent directory of the ref we should see
+		# a bitmap.
+		ref_namespace=$(dirname "$ref_without_bitmap") &&
+		rm .git/objects/pack/multi-pack-index* &&
+		git -c pack.preferBitmapTips="$ref_namespace" repack -adb --write-midx &&
+		test-tool bitmap list-commits >after &&
+		test_grep "$commit_id" after
+	)
+'
+
 test_done

-- 
2.53.0.239.g8d8fc8a987.dirty

[PATCH v3 3/4] bisect: fix misuse of `refs_for_each_ref_in()`

[Patrick Steinhardt]

All callers of `refs_for_each_ref_in()` pass in a string that is
terminated with a trailing slash to indicate that they only want to see
refs in that specific ref hierarchy. This is in fact a requirement if
one wants to use this function, as the function trims the prefix from
each yielded ref. So if there was a reference that was called
"refs/bisect" as in our example, the result after trimming would be the
empty string, and that's something we disallow.

Fix this by adding the trailing slash.

Furthermore, taking a closer look, we strip the prefix only to re-add it
in `mark_for_removal()`. This is somewhat roundabout, as we can instead
call `refs_for_each_fullref_in()` to not do any stripping at all. Do so
to simplify the code a bit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index 326b59c0dc..4f0d1a1853 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1180,7 +1180,7 @@ int estimate_bisect_steps(int all)
 static int mark_for_removal(const struct reference *ref, void *cb_data)
 {
 	struct string_list *refs = cb_data;
-	char *bisect_ref = xstrfmt("refs/bisect%s", ref->name);
+	char *bisect_ref = xstrdup(ref->name);
 	string_list_append(refs, bisect_ref);
 	return 0;
 }
@@ -1191,9 +1191,9 @@ int bisect_clean_state(void)
 
 	/* There may be some refs packed during bisection */
 	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
-	refs_for_each_ref_in(get_main_ref_store(the_repository),
-			     "refs/bisect", mark_for_removal,
-			     (void *) &refs_for_removal);
+	refs_for_each_fullref_in(get_main_ref_store(the_repository),
+				 "refs/bisect/", NULL, mark_for_removal,
+				 &refs_for_removal);
 	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
 	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
 	result = refs_delete_refs(get_main_ref_store(the_repository),

-- 
2.53.0.239.g8d8fc8a987.dirty

[PATCH v3 4/4] bisect: simplify string_list memory handling

[Patrick Steinhardt]

From: Jeff King <peff@peff.net>

We declare the refs_for_removal string_list as NODUP, forcing us to
manually allocate strings we insert. And then when it comes time to
clean up, we set strdup_strings so that string_list_clear() will free
them for us.

This is a confusing pattern, and can be done much more simply by just
declaring the list with the DUP initializer in the first place.

It was written this way originally because one of the callsites
generated the item using xstrfmt(). But that spot switched to a plain
xstrdup() in the preceding commit. That means we can now just let the
string_list code handle allocation itself.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/bisect.c b/bisect.c
index 4f0d1a1853..268f5e36f8 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1180,8 +1180,7 @@ int estimate_bisect_steps(int all)
 static int mark_for_removal(const struct reference *ref, void *cb_data)
 {
 	struct string_list *refs = cb_data;
-	char *bisect_ref = xstrdup(ref->name);
-	string_list_append(refs, bisect_ref);
+	string_list_append(refs, ref->name);
 	return 0;
 }
 
@@ -1190,16 +1189,15 @@ int bisect_clean_state(void)
 	int result = 0;
 
 	/* There may be some refs packed during bisection */
-	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
+	struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
 	refs_for_each_fullref_in(get_main_ref_store(the_repository),
 				 "refs/bisect/", NULL, mark_for_removal,
 				 &refs_for_removal);
-	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
-	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
+	string_list_append(&refs_for_removal, "BISECT_HEAD");
+	string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV");
 	result = refs_delete_refs(get_main_ref_store(the_repository),
 				  "bisect: remove", &refs_for_removal,
 				  REF_NO_DEREF);
-	refs_for_removal.strdup_strings = 1;
 	string_list_clear(&refs_for_removal, 0);
 	unlink_or_warn(git_path_bisect_ancestors_ok());
 	unlink_or_warn(git_path_bisect_log());

-- 
2.53.0.239.g8d8fc8a987.dirty

Re: [PATCH v3 2/4] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> @@ -3328,15 +3328,26 @@ void for_each_preferred_bitmap_tip(struct repository *repo,
>  {
>  	struct string_list_item *item;
>  	const struct string_list *preferred_tips;
> +	struct strbuf buf = STRBUF_INIT;
>  
>  	preferred_tips = bitmap_preferred_tips(repo);
>  	if (!preferred_tips)
>  		return;
>  
>  	for_each_string_list_item(item, preferred_tips) {
> +		const char *pattern = item->string;
> +
> +		if (!ends_with(pattern, "/")) {
> +			strbuf_reset(&buf);
> +			strbuf_addf(&buf, "%s/", pattern);
> +			pattern = buf.buf;
> +		}
> +

I briefly wondered if a possible alternative solution is to update
bitmap_preferred_tips() that reads the configuration so that it
gives the callers a string-list with "corrected" strings.  If it can
have many other callers, such an approach would force everybody to
adopt the same worldview, i.e., the configuration is meant to specify
hierarchy prefixes and never individual refs.

But when I noticed that nobody bothers to release resources held by
preferred_Tips, I realized that the above implementation is far
simpler and much better.  This iterator is the only caller of the
file-scope static bitmap_preferred_tips() helper, and what the
helper returns is a borrowed piece of string_list that is owned by
the config subsystem, so such a change will force us to make a copy
and then release the copy in the caller.

So, I think the above change is much better than such an alternative.

Thanks.

Re: [PATCH v3 0/4] Fix misuse of `refs_for_each_ref_in()`

[Taylor Blau]

Hi Patrick,

On Fri, Feb 06, 2026 at 08:49:55AM +0100, Patrick Steinhardt wrote:
> Jeff King (1):
>       bisect: simplify string_list memory handling
>
> Patrick Steinhardt (3):
>       pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
>       pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
>       bisect: fix misuse of `refs_for_each_ref_in()`

Thanks, this version looks good to me.

Thanks,
Taylor

[PATCH v4 0/4] Fix misuse of `refs_for_each_ref_in()`

[Patrick Steinhardt]

Hi,

this small patch series fixes a bug I have discovered where configuring
"pack.preferBitmapTips" to an exact branch will cause Git to `BUG()`.

The root cause of this bug is misuse of `refs_for_each_ref_in()`: this
function accepts a prefix to yield refs for, and then strips the prefix
for each ref. Consequently, if passed an exact refname, then stripping
the prefix would make us end up with an empty refname, and that is not
supposed to happen.

There was one other caller that got it wrong, too, and which is also
fixed in this patch series.

Changes in v4:
  - Fix a typo in the documentation.
  - Link to v3: https://lore.kernel.org/r/20260206-b4-pks-fix-for-each-ref-in-misuse-v3-0-1e050c3d6a50@pks.im

Changes in v3:
  - Switch the approach to perform ref hierarchy matches instead, which
    is in line with the changes in 10e8a9352b (refs.c: stop matching
    non-directory prefixes in exclude patterns, 2025-03-06).
  - Link to v2: https://lore.kernel.org/r/20260130-b4-pks-fix-for-each-ref-in-misuse-v2-0-0449b198a681@pks.im

Changes in v2:
  - Explain my thought process against why I chose to also allow exact
    ref matches in the second commit and clarify the documentation a
    bit. As said, I'm very open to changing this if my spelled-out
    thoughts are not convincing.
  - Apply Peff's patch to further simplify code.
  - Link to v1: https://lore.kernel.org/r/20260128-b4-pks-fix-for-each-ref-in-misuse-v1-0-deccae3ea725@pks.im

Thanks!

Patrick

---
Jeff King (1):
      bisect: simplify string_list memory handling

Patrick Steinhardt (3):
      pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
      pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
      bisect: fix misuse of `refs_for_each_ref_in()`

 Documentation/config/pack.adoc |  9 +++++----
 bisect.c                       | 16 +++++++---------
 builtin/pack-objects.c         | 19 ++-----------------
 pack-bitmap.c                  | 29 +++++++++++++++++++++++++++-
 pack-bitmap.h                  |  9 ++++++++-
 repack-midx.c                  | 14 +++-----------
 t/t5310-pack-bitmaps.sh        | 41 ++++++++++++++++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh    | 43 ++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 137 insertions(+), 43 deletions(-)

Range-diff versus v3:

1:  aa71011718 = 1:  0c2a2a122e pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
2:  d15aa04be6 ! 2:  a923e92ddd pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
    @@ Documentation/config/pack.adoc: pack.usePathWalk::
     -	commit at the tip of any reference that is a suffix of any value
     -	of this configuration over any other commits in the "selection
     -	window".
    -+	commmit at the tip of a reference that is contained in any of
    ++	commit at the tip of a reference that is contained in any of
     +	the configured hierarchies.
      +
     -Note that setting this configuration to `refs/foo` does not mean that
3:  2f1b73dd08 = 3:  7e1dbf1b4e bisect: fix misuse of `refs_for_each_ref_in()`
4:  f579e31633 = 4:  094aa2345c bisect: simplify string_list memory handling

---
base-commit: ea717645d199f6f1b66058886475db3e8c9330e9
change-id: 20260128-b4-pks-fix-for-each-ref-in-misuse-96ae62e313a5

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

[Patrick Steinhardt]

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: when the preferred bitmap tips contain an
exact refname match, then we will hit a `BUG()`.

Prepare for the 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. Note that the function is still used by a second caller, so we
cannot just inline it.

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

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;
+
+	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;
 
 	refs_for_each_ref(get_main_ref_store(repo),
 			  midx_snapshot_ref_one, &data);

-- 
2.53.0.414.gf7e9f6c205.dirty

[PATCH v4 2/4] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"

[Patrick Steinhardt]

The "pack.preferBitmapTips" configuration allows the user to specify
which references should be preferred when generating bitmaps. This
option is typically expected to be set to a reference prefix, like for
example "refs/heads/".

It's not unreasonable though for a user to configure one specific
reference as preferred. But if they do, they'll hit a `BUG()`:

    $ git -c pack.preferBitmapTips=refs/heads/main repack -adb
    BUG: ../refs/iterator.c:366: attempt to trim too many characters
    error: pack-objects died of signal 6

The root cause for this bug is how we enumerate these references. We
call `refs_for_each_ref_in()`, which will:

  - Yield all references that have a user-specified prefix.

  - Trim each of these references so that the prefix is removed.

Typically, this function is called with a trailing slash, like
"refs/heads/", and in that case things work alright. But if the function
is called with the name of an existing reference then we'll try to trim
the full reference name, which would leave us with an empty name. And as
this would not really leave us with anything sensible, we call `BUG()`
instead of yielding this reference.

One could argue that this is a bug in `refs_for_each_ref_in()`. But the
question then becomes what the correct behaviour would be:

  - Do we want to skip exact matches? In our case we certainly don't
    want that, as the user has asked us to generate a bitmap for it.

  - Do we want to yield the reference with the empty refname? That would
    lead to a somewhat weird result.

Neither of these feel like viable options, so calling `BUG()` feels like
a sensible way out. The root cause ultimately is that we even try to
trim the whole refname in the first place. There are two possible ways
to fix this issue:

  - We can fix the bug by using `refs_for_each_fullref_in()` instead,
    which does not strip the prefix at all. Consequently, we would now
    start to accept all references that start with the configured
    prefix, including exact matches. So if we had "refs/heads/main", we
    would both match "refs/heads/main" and "refs/heads/main-branch".

  - Or we can fix the bug by appending a slash to the prefix if it
    doesn't already have one. This would mean that we only match
    ref hierarchies that start with this prefix.

While the first fix leaves the user with strictly _more_ configuration
options, we have already fixed a similar case in 10e8a9352b (refs.c:
stop matching non-directory prefixes in exclude patterns, 2025-03-06) by
using the second option. So for the sake of consistency, let's apply the
same fix here.

Clarify the documentation accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/config/pack.adoc |  9 +++++----
 pack-bitmap.c                  | 13 ++++++++++++-
 t/t5310-pack-bitmaps.sh        | 41 ++++++++++++++++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh    | 43 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 101 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/pack.adoc b/Documentation/config/pack.adoc
index 75402d5579..fa997c8597 100644
--- a/Documentation/config/pack.adoc
+++ b/Documentation/config/pack.adoc
@@ -160,12 +160,13 @@ pack.usePathWalk::
 	processes. See linkgit:git-pack-objects[1] for full details.
 
 pack.preferBitmapTips::
+	Specifies a ref hierarchy (e.g., "refs/heads/"); can be
+	given multiple times to specify more than one hierarchies.
 	When selecting which commits will receive bitmaps, prefer a
-	commit at the tip of any reference that is a suffix of any value
-	of this configuration over any other commits in the "selection
-	window".
+	commit at the tip of a reference that is contained in any of
+	the configured hierarchies.
 +
-Note that setting this configuration to `refs/foo` does not mean that
+Note that setting this configuration to `refs/foo/` does not mean that
 the commits at the tips of `refs/foo/bar` and `refs/foo/baz` will
 necessarily be selected. This is because commits are selected for
 bitmaps from within a series of windows of variable length.
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 2f5cb34009..1c93871484 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -3328,15 +3328,26 @@ void for_each_preferred_bitmap_tip(struct repository *repo,
 {
 	struct string_list_item *item;
 	const struct string_list *preferred_tips;
+	struct strbuf buf = STRBUF_INIT;
 
 	preferred_tips = bitmap_preferred_tips(repo);
 	if (!preferred_tips)
 		return;
 
 	for_each_string_list_item(item, preferred_tips) {
+		const char *pattern = item->string;
+
+		if (!ends_with(pattern, "/")) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "%s/", pattern);
+			pattern = buf.buf;
+		}
+
 		refs_for_each_ref_in(get_main_ref_store(repo),
-				     item->string, cb, cb_data);
+				     pattern, cb, cb_data);
 	}
+
+	strbuf_release(&buf);
 }
 
 int bitmap_is_preferred_refname(struct repository *r, const char *refname)
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 6718fb98c0..310b708c5c 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -466,6 +466,47 @@ test_bitmap_cases () {
 		)
 	'
 
+	test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+
+			# Create enough commits that not all will receive bitmap
+			# coverage even if they are all at the tip of some reference.
+			test_commit_bulk --message="%s" 103 &&
+			git log --format="create refs/tags/%s/tag %H" HEAD >refs &&
+			git update-ref --stdin <refs &&
+
+			# Create the bitmap.
+			git repack -adb &&
+			test-tool bitmap list-commits | sort >commits-with-bitmap &&
+
+			# Verify that we have at least one commit that did not
+			# receive a bitmap.
+			git rev-list HEAD >commits.raw &&
+			sort <commits.raw >commits &&
+			comm -13 commits-with-bitmap commits >commits-wo-bitmap &&
+			test_file_not_empty commits-wo-bitmap &&
+			commit_id=$(head commits-wo-bitmap) &&
+			ref_without_bitmap=$(git for-each-ref --points-at="$commit_id" --format="%(refname)") &&
+
+			# When passing the full refname we do not expect a
+			# bitmap to be generated, as it should be interpreted
+			# as if a slash was appended to the pattern.
+			git -c pack.preferBitmapTips="$ref_without_bitmap" repack -adb &&
+			test-tool bitmap list-commits >after &&
+			test_grep ! "$commit_id" after &&
+
+			# But if we pass the parent directory of the ref we
+			# should see a bitmap.
+			ref_namespace=$(dirname "$ref_without_bitmap") &&
+			git -c pack.preferBitmapTips="$ref_namespace" repack -adb &&
+			test-tool bitmap list-commits >after &&
+			test_grep "$commit_id" after
+		)
+	'
+
 	test_expect_success 'complains about multiple pack bitmaps' '
 		rm -fr repo &&
 		git init repo &&
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index faae98c7e7..449353416f 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1345,4 +1345,47 @@ test_expect_success 'bitmapped packs are stored via the BTMP chunk' '
 	)
 '
 
+test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
+	git init repo &&
+	test_when_finished "rm -fr repo" &&
+	(
+		cd repo &&
+
+		# Create enough commits that not all will receive bitmap
+		# coverage even if they are all at the tip of some reference.
+		test_commit_bulk --message="%s" 103 &&
+		git log --format="create refs/tags/%s %H" HEAD >refs &&
+		git update-ref --stdin <refs &&
+
+		# Create the bitmap via the MIDX.
+		git repack -adb --write-midx &&
+		test-tool bitmap list-commits | sort >commits-with-bitmap &&
+
+		# Verify that we have at least one commit that did not
+		# receive a bitmap.
+		git rev-list HEAD >commits.raw &&
+		sort <commits.raw >commits &&
+		comm -13 commits-with-bitmap commits >commits-wo-bitmap &&
+		test_file_not_empty commits-wo-bitmap &&
+		commit_id=$(head commits-wo-bitmap) &&
+		ref_without_bitmap=$(git for-each-ref --points-at="$commit_id" --format="%(refname)") &&
+
+		# When passing the full refname we do not expect a bitmap to be
+		# generated, as it should be interpreted as if a slash was
+		# appended to the pattern.
+		rm .git/objects/pack/multi-pack-index* &&
+		git -c pack.preferBitmapTips="$ref_without_bitmap" repack -adb --write-midx &&
+		test-tool bitmap list-commits >after &&
+		test_grep ! "$commit_id" after &&
+
+		# But if we pass the parent directory of the ref we should see
+		# a bitmap.
+		ref_namespace=$(dirname "$ref_without_bitmap") &&
+		rm .git/objects/pack/multi-pack-index* &&
+		git -c pack.preferBitmapTips="$ref_namespace" repack -adb --write-midx &&
+		test-tool bitmap list-commits >after &&
+		test_grep "$commit_id" after
+	)
+'
+
 test_done

-- 
2.53.0.414.gf7e9f6c205.dirty

[PATCH v4 3/4] bisect: fix misuse of `refs_for_each_ref_in()`

[Patrick Steinhardt]

All callers of `refs_for_each_ref_in()` pass in a string that is
terminated with a trailing slash to indicate that they only want to see
refs in that specific ref hierarchy. This is in fact a requirement if
one wants to use this function, as the function trims the prefix from
each yielded ref. So if there was a reference that was called
"refs/bisect" as in our example, the result after trimming would be the
empty string, and that's something we disallow.

Fix this by adding the trailing slash.

Furthermore, taking a closer look, we strip the prefix only to re-add it
in `mark_for_removal()`. This is somewhat roundabout, as we can instead
call `refs_for_each_fullref_in()` to not do any stripping at all. Do so
to simplify the code a bit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index 326b59c0dc..4f0d1a1853 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1180,7 +1180,7 @@ int estimate_bisect_steps(int all)
 static int mark_for_removal(const struct reference *ref, void *cb_data)
 {
 	struct string_list *refs = cb_data;
-	char *bisect_ref = xstrfmt("refs/bisect%s", ref->name);
+	char *bisect_ref = xstrdup(ref->name);
 	string_list_append(refs, bisect_ref);
 	return 0;
 }
@@ -1191,9 +1191,9 @@ int bisect_clean_state(void)
 
 	/* There may be some refs packed during bisection */
 	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
-	refs_for_each_ref_in(get_main_ref_store(the_repository),
-			     "refs/bisect", mark_for_removal,
-			     (void *) &refs_for_removal);
+	refs_for_each_fullref_in(get_main_ref_store(the_repository),
+				 "refs/bisect/", NULL, mark_for_removal,
+				 &refs_for_removal);
 	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
 	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
 	result = refs_delete_refs(get_main_ref_store(the_repository),

-- 
2.53.0.414.gf7e9f6c205.dirty

[PATCH v4 4/4] bisect: simplify string_list memory handling

[Patrick Steinhardt]

From: Jeff King <peff@peff.net>

We declare the refs_for_removal string_list as NODUP, forcing us to
manually allocate strings we insert. And then when it comes time to
clean up, we set strdup_strings so that string_list_clear() will free
them for us.

This is a confusing pattern, and can be done much more simply by just
declaring the list with the DUP initializer in the first place.

It was written this way originally because one of the callsites
generated the item using xstrfmt(). But that spot switched to a plain
xstrdup() in the preceding commit. That means we can now just let the
string_list code handle allocation itself.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/bisect.c b/bisect.c
index 4f0d1a1853..268f5e36f8 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1180,8 +1180,7 @@ int estimate_bisect_steps(int all)
 static int mark_for_removal(const struct reference *ref, void *cb_data)
 {
 	struct string_list *refs = cb_data;
-	char *bisect_ref = xstrdup(ref->name);
-	string_list_append(refs, bisect_ref);
+	string_list_append(refs, ref->name);
 	return 0;
 }
 
@@ -1190,16 +1189,15 @@ int bisect_clean_state(void)
 	int result = 0;
 
 	/* There may be some refs packed during bisection */
-	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
+	struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
 	refs_for_each_fullref_in(get_main_ref_store(the_repository),
 				 "refs/bisect/", NULL, mark_for_removal,
 				 &refs_for_removal);
-	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
-	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
+	string_list_append(&refs_for_removal, "BISECT_HEAD");
+	string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV");
 	result = refs_delete_refs(get_main_ref_store(the_repository),
 				  "bisect: remove", &refs_for_removal,
 				  REF_NO_DEREF);
-	refs_for_removal.strdup_strings = 1;
 	string_list_clear(&refs_for_removal, 0);
 	unlink_or_warn(git_path_bisect_ancestors_ok());
 	unlink_or_warn(git_path_bisect_log());

-- 
2.53.0.414.gf7e9f6c205.dirty

Re: [PATCH v3 0/4] Fix misuse of `refs_for_each_ref_in()`

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Hi Patrick,
>
> On Fri, Feb 06, 2026 at 08:49:55AM +0100, Patrick Steinhardt wrote:
>> Jeff King (1):
>>       bisect: simplify string_list memory handling
>>
>> Patrick Steinhardt (3):
>>       pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
>>       pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
>>       bisect: fix misuse of `refs_for_each_ref_in()`
>
> Thanks, this version looks good to me.
>
> Thanks,
> Taylor

Thanks, let's mark the topic for 'next' then.

Re: [PATCH v4 0/4] Fix misuse of `refs_for_each_ref_in()`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Changes in v4:
>   - Fix a typo in the documentation.
>   - Link to v3: https://lore.kernel.org/r/20260206-b4-pks-fix-for-each-ref-in-misuse-v3-0-1e050c3d6a50@pks.im
>
> Changes in v3:
>   - Switch the approach to perform ref hierarchy matches instead, which
>     is in line with the changes in 10e8a9352b (refs.c: stop matching
>     non-directory prefixes in exclude patterns, 2025-03-06).
>   - Link to v2: https://lore.kernel.org/r/20260130-b4-pks-fix-for-each-ref-in-misuse-v2-0-0449b198a681@pks.im

This round saws no activities, but I just re-read all and found them
quite nice.  Let's mark it for 'next'.

Thanks.

Re: [PATCH v4 0/4] Fix misuse of `refs_for_each_ref_in()`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Changes in v4:
>   - Fix a typo in the documentation.
>   - Link to v3: https://lore.kernel.org/r/20260206-b4-pks-fix-for-each-ref-in-misuse-v3-0-1e050c3d6a50@pks.im
>
> Changes in v3:
>   - Switch the approach to perform ref hierarchy matches instead, which
>     is in line with the changes in 10e8a9352b (refs.c: stop matching
>     non-directory prefixes in exclude patterns, 2025-03-06).
>   - Link to v2: https://lore.kernel.org/r/20260130-b4-pks-fix-for-each-ref-in-misuse-v2-0-0449b198a681@pks.im

This round saw no activities, but I just re-read all four patches
and found them quite nice.  Let's mark it for 'next'.

Thanks.