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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 10, 2023, 20:09 UTC
Message-ID: <xmqqedlf4jg1.fsf@gitster.g>
In-Reply-To: <f14a88f1075093c870cd1d53b4e0cea10d5ab67d.1689017830.git.me@ttaylorr.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.

Show 10 quoted lines

> 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.

Show 14 quoted lines

>  		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?

Show 7 quoted lines

> @@ -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 &&

Show 7 quoted lines

> +		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".

Show 31 quoted lines

> +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.