Re: [PATCH 04/12] packfile: check midx coverage with .idx rather than .pack

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Thu, Apr 04, 2019 at 07:25:46PM -0400, Jeff King wrote:
> When we have a .midx that covers many packfiles, we try to avoid opening
> the .idx for those packfiles. However, there are a few problems with the
> filename comparison we use:
> 
>   - we ask midx_contains_pack() about the .pack name, not the .idx name.
>     But it compares to the latter.
> 
>   - we compute the basename of the pack using strrchr() to find the
>     final slash. But that leaves an extra "/" at the start of our
>     string; we need to advance past it.
> 
>     That also raises the question of what to do when the name does not
>     have a slash at all. This should generally not happen (we always
>     find files in "pack/"), but it doesn't hurt to be defensive here.
> 
> The tests don't notice because there's nothing about opening those .idx
> files that would cause us to give incorrect output. It's just a little
> slower. The new test checks this case by corrupting the covered .idx,
> and then making sure we don't complain about it.

When the test suite is run with GIT_TEST_MULTI_PACK_INDEX=1, the
following tests fail with this patch:

  t1006-cat-file.sh                                (Wstat: 256 Tests: 111 Failed: 1)
    Failed test:  87
    Non-zero exit status: 1
  t5320-delta-islands.sh                           (Wstat: 256 Tests: 15 Failed: 10)
    Failed tests:  2-4, 6-10, 12-13
    Non-zero exit status: 1
  t5310-pack-bitmaps.sh                            (Wstat: 256 Tests: 46 Failed: 1)
    Failed test:  44
    Non-zero exit status: 1
  t5570-git-daemon.sh                              (Wstat: 256 Tests: 21 Failed: 1)
    Failed test:  10
    Non-zero exit status: 1


> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  packfile.c                  | 17 ++++++++++++++---
>  t/t5319-multi-pack-index.sh | 14 ++++++++++++++
>  2 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/packfile.c b/packfile.c
> index 054269ae5d..e7ca135ed5 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -472,6 +472,16 @@ static unsigned int get_max_fd_limit(void)
>  #endif
>  }
>  
> +static const char *pack_basename(struct packed_git *p)
> +{
> +	const char *ret = strrchr(p->pack_name, '/');
> +	if (ret)
> +		ret = ret + 1; /* skip past slash */
> +	else
> +		ret = p->pack_name; /* we only have a base */
> +	return ret;
> +}
> +
>  /*
>   * Do not call this directly as this leaks p->pack_fd on error return;
>   * call open_packed_git() instead.
> @@ -486,15 +496,16 @@ 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) {
> +	if (!p->index_data && the_repository->objects->multi_pack_index) {
>  		struct multi_pack_index *m;
> -		const char *pack_name = strrchr(p->pack_name, '/');
> +		char *idx_name = pack_name_to_idx(pack_basename(p));
>  
>  		for (m = the_repository->objects->multi_pack_index;
>  		     m; m = m->next) {
> -			if (midx_contains_pack(m, pack_name))
> +			if (midx_contains_pack(m, idx_name))
>  				break;
>  		}
> +		free(idx_name);
>  
>  		if (!m && open_pack_index(p))
>  			return error("packfile %s index unavailable", p->pack_name);
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 8c4d2bd849..1ebf19ec3c 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -117,6 +117,20 @@ test_expect_success 'write midx with one v2 pack' '
>  
>  compare_results_with_midx "one v2 pack"
>  
> +test_expect_success 'corrupt idx not opened' '
> +	idx=$(test-tool read-midx $objdir | grep "\.idx\$") &&
> +	mv $objdir/pack/$idx backup-$idx &&
> +	test_when_finished "mv backup-\$idx \$objdir/pack/\$idx" &&
> +
> +	# This is the minimum size for a sha-1 based .idx; this lets
> +	# us pass perfunctory tests, but anything that actually opens and reads
> +	# the idx file will complain.
> +	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
> +'
> +
>  test_expect_success 'add more objects' '
>  	for i in $(test_seq 6 10)
>  	do
> -- 
> 2.21.0.714.gd1be1d035b
>