Re: [PATCH] cache-tree: fix inverted object existence check in cache_tree_fully_valid

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

Derrick Stolee <stolee@gmail.com> writes:

> On 4/6/2026 11:14 AM, David Lin wrote:
>> cache_tree_fully_valid() is supposed to return 0 (not valid) when a
>> tree object is missing from the object database. The condition
>> currently returns 0 when odb_has_object() succeeds, which is the
>> opposite of what is intended: the cache tree should be considered
>> invalid when the object does not exist.
>
> It looks like this negation occurred two refactors ago in
> 062b914c84 (treewide: convert users of `repo_has_object_file()` to
> `has_object()`, 2025-04-29) which had this diff:
>
> diff --git a/cache-tree.c b/cache-tree.c
> index c0e1e9ee1d..fa3858e282 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -238,7 +238,9 @@ int cache_tree_fully_valid(struct cache_tree *it)
>         int i;
>         if (!it)
>                 return 0;
> -       if (it->entry_count < 0 || !repo_has_object_file(the_repository, &it->oid))
> +       if (it->entry_count < 0 ||
> +           has_object(the_repository, &it->oid,
> +                      HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))
>                 return 0;
>         for (i = 0; i < it->subtree_nr; i++) {
>                 if (!cache_tree_fully_valid(it->down[i]->cache_tree))
>  
> This was one hunk among many, so it is easy to miss that the ! was
> lost as the function was renamed and moved to another line.

Thanks for archaeology.  It deserves to be recorded in the proposed
log message.  David, would you mind updating the message and repost
a v2 of the patch?

I wonder what the practical effect of this breakage was.  The added
test only checks what happens in the trace, but what was the effect
externally observable?   We did not rebuild the cache-tree when we
should have, causing "write-tree" to record a set of tree objects
that do not match what is in the index?


>>  	if (it->entry_count < 0 ||
>> -	    odb_has_object(the_repository->objects, &it->oid,
>> +	    !odb_has_object(the_repository->objects, &it->oid,
>>  			   HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))
>>  		return 0;
>
> This fix (respecting the has_object to odb_has_object refactor)
> is the correct one.
>
>> +test_expect_success 'cache-tree is used by write-tree when valid' '
>> +	test_commit use-valid &&
>> +
>> +	# write-tree with a valid cache-tree should skip cache_tree_update
>> +	GIT_TRACE2_PERF="$(pwd)/trace.output" git write-tree &&
>> +	! grep region_enter.*cache_tree.*update trace.output
>
> nit: I think this would be better as "test_grep ! ..."
>
> Thanks,
> -Stolee