Re: [PATCH 2/4] builtin/pack-objects.c: support `--max-pack-size` with `--cruft`

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> When pack-objects learned the `--cruft` option back in b757353676
> (builtin/pack-objects.c: --cruft without expiration, 2022-05-20), we
> explicitly forbade `--cruft` with `--max-pack-size`.
>
> At the time, there was no specific rationale given in the patch for not
> supporting the `--max-pack-size` option with `--cruft`. (As best I can
> remember, it's because we were trying to push users towards only ever
> having a single cruft pack, but I cannot be sure).

I am reasonably sure it was the case but then I do not recall we
ever discussing how the second cruft pack gets consolidated into one
by combining it with the existing one.

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 868efe7e0f..a046994a43 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1190,8 +1190,7 @@ static void write_pack_file(void)
>  		unsigned char hash[GIT_MAX_RAWSZ];
>  		char *pack_tmp_name = NULL;
>  
> -		if (pack_to_stdout)
> -			f = hashfd_throughput(1, "<stdout>", progress_state);
> +		if (pack_to_stdout) f = hashfd_throughput(1, "<stdout>", progress_state);
>  		else
>  			f = create_tmp_packfile(&pack_tmp_name);

An unintended change, I am sure ;-)

It is very surprising that absolutely no real change is needed to
allow cruft packs to honor the settings, other than removing the
seemingly artificial inter-option-compatibility roadblocks (all
hunks for it omitted above as they were trivially obvious).  I am
sure the first hunk to fold an "if" statement onto a single line is
not what makes the feature to actually work ;-)

> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
> index 45667d4999..fc5fedbe9b 100755
> --- a/t/t5329-pack-objects-cruft.sh
> +++ b/t/t5329-pack-objects-cruft.sh
> @@ -573,23 +573,54 @@ test_expect_success 'cruft repack with no reachable objects' '
>  	)
>  '
>  
> -test_expect_success 'cruft repack ignores --max-pack-size' '
> +write_blob () {
> +	test-tool genrandom "$@" >in &&
> +	git hash-object -w -t blob in
> +}
> +
> +find_pack () {
> +	for idx in $(ls $packdir/pack-*.idx)
> +	do
> +		git show-index <$idx >out &&
> +		if grep -q "$1" out
> +		then
> +			echo $idx
> +		fi || return 1
> +	done
> +}
> +
> +test_expect_success 'cruft repack with --max-pack-size' '
>  	git init max-pack-size &&
>  	(
>  		cd max-pack-size &&
>  		test_commit base &&
> +
>  		# two cruft objects which exceed the maximum pack size
> -		test-tool genrandom foo 1048576 | git hash-object --stdin -w &&
> -		test-tool genrandom bar 1048576 | git hash-object --stdin -w &&
> +		foo=$(write_blob foo 1048576) &&
> +		bar=$(write_blob bar 1048576) &&
> +		test-tool chmtime --get -1000 \
> +			"$objdir/$(test_oid_to_path $foo)" >foo.mtime &&
> +		test-tool chmtime --get -2000 \
> +			"$objdir/$(test_oid_to_path $bar)" >bar.mtime &&
>  		git repack --cruft --max-pack-size=1M &&
>  		find $packdir -name "*.mtimes" >cruft &&
> -		test_line_count = 1 cruft &&
> -		test-tool pack-mtimes "$(basename "$(cat cruft)")" >objects &&
> -		test_line_count = 2 objects
> +		test_line_count = 2 cruft &&
> +
> +		foo_mtimes="$(basename $(find_pack $foo) .idx).mtimes" &&
> +		bar_mtimes="$(basename $(find_pack $bar) .idx).mtimes" &&
> +		test-tool pack-mtimes $foo_mtimes >foo.actual &&
> +		test-tool pack-mtimes $bar_mtimes >bar.actual &&
> +
> +		echo "$foo $(cat foo.mtime)" >foo.expect &&
> +		echo "$bar $(cat bar.mtime)" >bar.expect &&
> +
> +		test_cmp foo.expect foo.actual &&
> +		test_cmp bar.expect bar.actual &&
> +		test "$foo_mtimes" != "$bar_mtimes"
>  	)
>  '
>  
> -test_expect_success 'cruft repack ignores pack.packSizeLimit' '
> +test_expect_success 'cruft repack with pack.packSizeLimit' '
>  	(
>  		cd max-pack-size &&
>  		# repack everything back together to remove the existing cruft
> @@ -599,9 +630,12 @@ test_expect_success 'cruft repack ignores pack.packSizeLimit' '
>  		# ensure the same post condition is met when --max-pack-size
>  		# would otherwise be inferred from the configuration
>  		find $packdir -name "*.mtimes" >cruft &&
> -		test_line_count = 1 cruft &&
> -		test-tool pack-mtimes "$(basename "$(cat cruft)")" >objects &&
> -		test_line_count = 2 objects
> +		test_line_count = 2 cruft &&
> +		for pack in $(cat cruft)
> +		do
> +			test-tool pack-mtimes "$(basename $pack)" >objects &&
> +			test_line_count = 1 objects || return 1
> +		done
>  	)
>  '