Re: [PATCH 2/3] cache-tree: detect mismatching number of index entries

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 19, 2024, 01:35 UTC
Message-ID: <xmqqttec8ly0.fsf@gitster.g>
In-Reply-To: <595693a6420b2571aabd51ed989bedfa0cfa62e2.1726556195.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> +	if (it->entry_count + pos > istate->cache_nr) {
> +		ret = error(_("corrupted cache-tree has entries not present in index"));
> +		goto out;
> +	}

Is it a safe assumption that the if() condition always indicates an error? When sparse-index is in effect, istate->cache_nr may be a number that is smaller than the true number of paths in the index (because all paths under a subdirectory we are not interested in are folded into a single tree-ish entry), and I am not sure how it should interact with it->entry_count (i.e. the number of paths under the current directory we are looking at, which obviously cannot be a sparsified entry) and pos (i.e. the index into active_cache[] that represend the first path under the current directory)?

I guess as long as "it" is not folded, it does not matter how other paths from different directories in active_cache[] are sparsified or expanded, as long as "pos" keeps track of the current position correctly.

Show 15 quoted lines

> diff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh
> index 2501c89c1c9..3f602adb055 100755
> --- a/t/t4058-diff-duplicates.sh
> +++ b/t/t4058-diff-duplicates.sh
> @@ -132,15 +132,15 @@ test_expect_success 'create a few commits' '
>  	rm commit_id up final
>  '
>  
> -test_expect_failure 'git read-tree does not segfault' '
> -	test_when_finished rm .git/index.lock &&
> -	test_might_fail git read-tree --reset base
> +test_expect_success 'git read-tree does not segfault' '
> +	test_must_fail git read-tree --reset base 2>err &&
> +	test_grep "error: corrupted cache-tree has entries not present in index" err
>  '

Very good. test_might_fail is a sign of trouble, and this gives us a lot more predictability.