[PATCH v3 0/4] Fix misuse of `refs_for_each_ref_in()`
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Feb 6, 2026, 07:49 UTC
- Message-ID
- <20260206-b4-pks-fix-for-each-ref-in-misuse-v3-0-1e050c3d6a50@pks.im>
- In-Reply-To
- <20260128-b4-pks-fix-for-each-ref-in-misuse-v1-0-deccae3ea725@pks.im>
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.imChanges 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.imThanks!
Patrick
---
Jeff King (1):
bisect: simplify string_list memory handlingPatrick 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