Re: [PATCH 3/3] builtin/pack-objects.c: respect 'pack.preferBitmapTips'

[Derrick Stolee <stolee@gmail.com>]

On 3/31/2021 9:32 PM, Taylor Blau wrote:
> When writing a new pack with a bitmap, it is sometimes convenient to
> indicate some reference prefixes which should receive priority when
> selecting which commits to receive bitmaps.
> 
> A truly motivated caller could accomplish this by setting
> 'pack.islandCore', (since all commits in the core island are similarly
> marked as preferred) but this requires callers to opt into using delta
> islands, which they may or may not want to do.
> 
> Introduce a new multi-valued configuration, 'pack.preferBitmapTips' to
> allow callers to specify a list of reference prefixes. All references
> which have a prefix contained in 'pack.preferBitmapTips' will mark their
> tips as "preferred" in the same way as commits are marked as preferred
> for selection by 'pack.islandCore'.

I almost hit send on an email saying "this doesn't seem to work with
prefixes" but I have discovered the subtle reason why. I'll point it
out below.

> +static int mark_bitmap_preferred_tip(const char *refname,
> +				     const struct object_id *oid, int flags,
> +				     void *_data)
> +{
> +	struct object_id peeled;
> +	struct object *object;
> +
> +	if (!peel_iterated_oid(oid, &peeled))
> +		oid = &peeled;
> +
> +	object = parse_object_or_die(oid, refname);
> +	if (object->type == OBJ_COMMIT)
> +		object->flags |= NEEDS_BITMAP;
> +
> +	return 0;
> +}

This takes a refname and peels it (in case of a tag) but doesn't
do any prefix matching.

> +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) {
> +		for_each_ref_in(item->string, mark_bitmap_preferred_tip, NULL);
> +	}
> +}

This iterates on the list returned by bitmap_preferred_tips(), but
specifically for_each_ref_in() does the prefix matching for us. I
missed that point on my first read.

> +const struct string_list *bitmap_preferred_tips(struct repository *r)
> +{
> +	return repo_config_get_value_multi(r, "pack.preferbitmaptips");
> +}

And this just returns the config values, which are fed into
for_each_ref_in().

> +test_expect_success 'pack.preferBitmapTips' '
> +	git init repo &&
> +	test_when_finished "rm -fr repo" &&
> +	(
> +		cd repo &&
> +
> +		# create enough commits that not all are receive bitmap
> +		# coverage even if they are all at the tip of some reference.
> +		test_commit_bulk --message="%s" 103 &&
> +
> +		git rev-list HEAD >commits.raw &&
> +		sort <commits.raw >commits &&
> +
> +		git log --format="create refs/tags/%s %H" HEAD >refs &&
> +		git update-ref --stdin <refs &&
> +
> +		git repack -adb &&
> +		test-tool bitmap list-commits | sort >bitmaps &&
> +
> +		# remember which commits did not receive bitmaps
> +		comm -13 bitmaps commits >before &&
> +		test_file_not_empty before &&
> +
> +		# mark the commits which did not receive bitmaps as preferred,
> +		# and generate the bitmap again
> +		perl -pe "s{^}{create refs/tags/include/$. }" <before |
> +			git update-ref --stdin &&
> +		git -c pack.preferBitmapTips=refs/tags/include repack -adb &&

And this test is a clever way of creating a new ref category to
check the prefix matching. Since you created refs pointing to
every commit earlier, this verifies that the config is the reason
they are picked now. Good.

Sorry for all of my thinking out loud. These are good patches and
I have no recommended changes.

Thanks,
-Stolee