Re: [PATCH 1/2] packfile.c: protect against disappearing indexes

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

Taylor Blau <me@ttaylorr.com> writes:

> In 17c35c8969 (packfile: skip loading index if in multi-pack-index,
> 2018-07-12) we stopped loading the .idx file for packs that are
> contained within a multi-pack index.
>
> This saves us the effort of loading an .idx and doing some lightweight
> validity checks by way of 'packfile.c:load_idx()', but introduces a race
> between processes that need to load the index (e.g., to generate a
> reverse index) and processes that can delete the index.

Ah, OK.  And demonstration using %(objectsize:disk) makes perfect
sense given the above explanation.

> These two together effectively revert 17c35c8969, and avoid the race
> explained above.
>
> Co-authored-by: Jeff King <peff@peff.net>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  packfile.c                  | 19 ++-----------------
>  t/t5319-multi-pack-index.sh | 24 ++++++++++++++++++++++--
>  2 files changed, 24 insertions(+), 19 deletions(-)

Makes sense.  Thanks.

> diff --git a/packfile.c b/packfile.c
> index 0929ebe4fc..8d7f37a5f6 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -514,19 +514,8 @@ static int open_packed_git_1(struct packed_git *p)
>  	ssize_t read_result;
>  	const unsigned hashsz = the_hash_algo->rawsz;
>  
> -	if (!p->index_data) {
> -		struct multi_pack_index *m;
> -		const char *pack_name = pack_basename(p);
> -
> -		for (m = the_repository->objects->multi_pack_index;
> -		     m; m = m->next) {
> -			if (midx_contains_pack(m, pack_name))
> -				break;
> -		}
> -
> -		if (!m && open_pack_index(p))
> -			return error("packfile %s index unavailable", p->pack_name);
> -	}
> +	if (open_pack_index(p))
> +		return error("packfile %s index unavailable", p->pack_name);
>  
>  	if (!pack_max_fds) {
>  		unsigned int max_fds = get_max_fd_limit();
> @@ -567,10 +556,6 @@ static int open_packed_git_1(struct packed_git *p)
>  			" supported (try upgrading GIT to a newer version)",
>  			p->pack_name, ntohl(hdr.hdr_version));
>  
> -	/* Skip index checking if in multi-pack-index */
> -	if (!p->index_data)
> -		return 0;
> -
>  	/* Verify the pack matches its index. */
>  	if (p->num_objects != ntohl(hdr.hdr_entries))
>  		return error("packfile %s claims to have %"PRIu32" objects"
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index ace469c95c..d4607daec1 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -138,7 +138,7 @@ test_expect_success 'write midx with one v2 pack' '
>  
>  compare_results_with_midx "one v2 pack"
>  
> -test_expect_success 'corrupt idx not opened' '
> +test_expect_success 'corrupt idx reports errors' '
>  	idx=$(test-tool read-midx $objdir | grep "\.idx\$") &&
>  	mv $objdir/pack/$idx backup-$idx &&
>  	test_when_finished "mv backup-\$idx \$objdir/pack/\$idx" &&
> @@ -149,7 +149,7 @@ test_expect_success 'corrupt idx not opened' '
>  	test_copy_bytes 1064 <backup-$idx >$objdir/pack/$idx &&
>  
>  	git -c core.multiPackIndex=true rev-list --objects --all 2>err &&
> -	test_must_be_empty err
> +	grep "index unavailable" err
>  '
>  
>  test_expect_success 'add more objects' '
> @@ -755,4 +755,24 @@ test_expect_success 'repack --batch-size=<large> repacks everything' '
>  	)
>  '
>  
> +test_expect_success 'load reverse index when missing .idx' '
> +	git init repo &&
> +	test_when_finished "rm -fr repo" &&
> +	(
> +		cd repo &&
> +
> +		git config core.multiPackIndex true &&
> +
> +		test_commit base &&
> +		git repack -ad &&
> +		git multi-pack-index write &&
> +
> +		git rev-parse HEAD >tip &&
> +		idx=$(ls .git/objects/pack/pack-*.idx) &&
> +
> +		mv $idx $idx.bak &&
> +		git cat-file --batch-check="%(objectsize:disk)" <tip
> +	)
> +'
> +
>  test_done