From: Karthik Nayak Date: Wed, 28 Jan 2026 11:04:09 GMT Subject: Re: [PATCH 2/3] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips" Message-ID: In-Reply-To: <20260128-b4-pks-fix-for-each-ref-in-misuse-v1-2-deccae3ea725@pks.im> Patrick Steinhardt 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 > --- > 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 + 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 && > + 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