Re: [PATCH 1/2] builtin/repack.c: only repack `.pack`s that exist

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

Taylor Blau <me@ttaylorr.com> writes:

> Add a check to see if the .pack file exists before adding it to the list
> for repacking. This will stop a number of maintenance failures seen in
> production but fixed by deleting the .idx files.

When we are adding we'd eventually add both and something that lack
one of them will eventually become complete and will be part of the
repacking once that happens.  When we are removing (because another
repack has about to finish), removing either one of them will make
the pack ineligible, which is OK.  If somebody crashed while adding
or removing and ended up leaving only one, not both, on the
filesystem, ignoring such a leftover stuff would be the least
disruptive for automation.  Makes sense.

> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index af79266c58..284954791d 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -213,16 +213,16 @@ test_expect_success 'repack --keep-pack' '
>  	test_create_repo keep-pack &&
>  	(
>  		cd keep-pack &&
> -		# avoid producing difference packs to delta/base choices
> +		# avoid producing different packs due to delta/base choices

OK.

>  		git config pack.window 0 &&
>  		P1=$(commit_and_pack 1) &&
>  		P2=$(commit_and_pack 2) &&
>  		P3=$(commit_and_pack 3) &&
>  		P4=$(commit_and_pack 4) &&
> -		ls .git/objects/pack/*.pack >old-counts &&
> +		find .git/objects/pack -name "*.pack" -type f | sort >old-counts &&
>  		test_line_count = 4 old-counts &&
>  		git repack -a -d --keep-pack $P1 --keep-pack $P4 &&
> -		ls .git/objects/pack/*.pack >new-counts &&
> +		find .git/objects/pack -name "*.pack" -type f | sort >new-counts &&
>  		grep -q $P1 new-counts &&
>  		grep -q $P4 new-counts &&
>  		test_line_count = 3 new-counts &&

I do not think "sort" in both of these added lines is doing anything
useful.  Does the test break without this hunk and if so how?

> @@ -239,14 +239,51 @@ test_expect_success 'repack --keep-pack' '
>  			mv "$from" "$to" || return 1
>  		done &&
>  
> +		# A .idx file without a .pack should not stop us from
> +		# repacking what we can.
> +		touch .git/objects/pack/pack-does-not-exist.idx &&

	>.git/objects/pack/pack-does-not-exist.idx &&

> +		find .git/objects/pack -name "*.pack" -type f | sort >packs.before &&
>  		git repack --cruft -d --keep-pack $P1 --keep-pack $P4 &&
> +		find .git/objects/pack -name "*.pack" -type f | sort >packs.after &&
>  
> -		ls .git/objects/pack/*.pack >newer-counts &&
> -		test_cmp new-counts newer-counts &&
> +		test_cmp packs.before packs.after &&


I'd say the changes from "ls" to "find" in the earlier hunk is
excusable in the name of consistency with this updated on, if the
use of "sort" matters in this hunk, but as "ls" gives a consistent
output unless you say "ls (-U|--sort=none)", I am not sure if we
need "sort" in this hunk, either.  So, I dunno.

"before vs after" does look like an improvement over "new vs newer".

> +test_expect_success 'repacking fails when missing .pack actually means missing objects' '
> +	test_create_repo idx-without-pack &&
> +	(
> +		cd idx-without-pack &&
> +
> +		# Avoid producing different packs due to delta/base choices
> +		git config pack.window 0 &&
> +		P1=$(commit_and_pack 1) &&
> +		P2=$(commit_and_pack 2) &&
> +		P3=$(commit_and_pack 3) &&
> +		P4=$(commit_and_pack 4) &&
> +		find .git/objects/pack -name "*.pack" -type f | sort >old-counts &&
> +		test_line_count = 4 old-counts &&
> +
> +		# Remove one .pack file
> +		rm .git/objects/pack/$P2 &&
> +
> +		find .git/objects/pack -name "*.pack" -type f |
> +			sort >before-pack-dir &&
> +
> +		test_must_fail git fsck &&
> +		test_must_fail git repack --cruft -d 2>err &&
> +		grep "bad object" err &&
> +
> +		# Before failing, the repack did not modify the
> +		# pack directory.
> +		find .git/objects/pack -name "*.pack" -type f |
> +			sort >after-pack-dir &&
> +		test_cmp before-pack-dir after-pack-dir
> +	)
> +'

Ditto for the use of "find | sort" vs "ls", but otherwise it looks
like it is testing the right thing.

Thanks.