Re: [PATCH v4 2/2] pack-objects: Teach it to use reachability bitmap index when generating non-stdout pack too

[Jeff King <peff@peff.net>]

On Fri, Jul 29, 2016 at 10:47:46AM +0300, Kirill Smelkov wrote:

> @@ -2527,7 +2528,7 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
>  	if (prepare_bitmap_walk(revs) < 0)
>  		return -1;
>  
> -	if (pack_options_allow_reuse() &&
> +	if (pack_options_allow_reuse() && pack_to_stdout &&
>  	    !reuse_partial_packfile_from_bitmap(

Should pack_to_stdout just be part of pack_options_allow_reuse()?

> @@ -2812,7 +2813,23 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  	if (!rev_list_all || !rev_list_reflog || !rev_list_index)
>  		unpack_unreachable_expiration = 0;
>  
> -	if (!use_internal_rev_list || !pack_to_stdout || is_repository_shallow())
> +	/*
> +	 * "soft" reasons not to use bitmaps - for on-disk repack by default we want
> +	 *
> +	 * - to produce good pack (with bitmap index not-yet-packed objects are
> +	 *   packed in suboptimal order).
> +	 *
> +	 * - to use more robust pack-generation codepath (avoiding possible
> +	 *   bugs in bitmap code and possible bitmap index corruption).
> +	 */
> +	if (!pack_to_stdout)
> +		use_bitmap_index_default = 0;
> +
> +	if (use_bitmap_index < 0)
> +		use_bitmap_index = use_bitmap_index_default;
> +
> +	/* "hard" reasons not to use bitmaps; these just won't work at all */
> +	if (!use_internal_rev_list || (!pack_to_stdout && write_bitmap_index) || is_repository_shallow())
>  		use_bitmap_index = 0;

This all makes sense and looks good.

> +test_expect_success 'pack-objects to file can use bitmap' '
> +	# make sure we still have 1 bitmap index from previous tests
> +	ls .git/objects/pack/ | grep bitmap >output &&
> +	test_line_count = 1 output &&
> +	# verify equivalent packs are generated with/without using bitmap index
> +	packasha1=$(git pack-objects --no-use-bitmap-index --all packa </dev/null) &&
> +	packbsha1=$(git pack-objects --use-bitmap-index --all packb </dev/null) &&
> +	git verify-pack -v packa-$packasha1.pack >packa.verify &&
> +	git verify-pack -v packb-$packbsha1.pack >packb.verify &&
> +	grep -o "^$_x40" packa.verify |sort >packa.objects &&
> +	grep -o "^$_x40" packb.verify |sort >packb.objects &&
> +	test_cmp packa.objects packb.objects
> +'

I don't think "grep -o" is portable. However, an easier way to do this
is probably:

  # these are already in sorted order
  git show-index <packa-$packasha1.pack | cut -d' ' -f2 >packa.objects &&
  git show-index <packb-$packbsha1.pack | cut -d' ' -f2 >packb.objects &&
  test_cmp packa.objects packb.objects

-Peff