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

> 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