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

[Derrick Stolee <stolee@gmail.com>]

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.

>  	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