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

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

[David Lin]

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.

Add the missing negation so the function correctly invalidates cache
tree nodes whose objects are absent.

Signed-off-by: David Lin <davidlin@stripe.com>
---
 cache-tree.c          | 2 +-
 t/t0090-cache-tree.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/cache-tree.c b/cache-tree.c
index 60bcc07c3b..9fe057355c 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -238,7 +238,7 @@ int cache_tree_fully_valid(struct cache_tree *it)
 	if (!it)
 		return 0;
 	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;
 	for (i = 0; i < it->subtree_nr; i++) {
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index d901588294..2c6b7a0899 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -278,4 +278,12 @@ test_expect_success 'switching trees does not invalidate shared index' '
 	)
 '
 
+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
+'
+
 test_done

base-commit: 2855562ca6a9c6b0e7bc780b050c1e83c9fcfbd0
-- 
2.52.0.ge17bebe515.stripe

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

[Derrick Stolee]

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

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

[Junio C Hamano]

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

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

[David Lin]

The negation in front of the object existence check in
cache_tree_fully_valid() was lost in 062b914c84 (treewide: convert
users of `repo_has_object_file()` to `has_object()`, 2025-04-29),
turning `!repo_has_object_file(...)` into `has_object(...)` instead
of `!has_object(...)`.

This makes cache_tree_fully_valid() always report the cache tree as
invalid when objects exist (the common case), forcing callers like
write_index_as_tree() to call cache_tree_update() on every
invocation.  An odb_has_object() check inside update_one() avoids a
full tree rebuild, but the unnecessary call still pays the cost of
opening an ODB transaction and, in partial clones, a promisor remote
check.

Restore the missing negation and add a test that verifies write-tree
takes the cache-tree shortcut when the cache tree is valid.

Helped-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: David Lin <davidlin@stripe.com>
---

Notes:
    Changes since v1:
      - Use test_grep instead of bare grep (Stolee)

 cache-tree.c          | 2 +-
 t/t0090-cache-tree.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/cache-tree.c b/cache-tree.c
index 60bcc07c3b..9fe057355c 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -238,7 +238,7 @@ int cache_tree_fully_valid(struct cache_tree *it)
 	if (!it)
 		return 0;
 	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;
 	for (i = 0; i < it->subtree_nr; i++) {
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index d901588294..0964718d7f 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -278,4 +278,12 @@ test_expect_success 'switching trees does not invalidate shared index' '
 	)
 '
 
+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 &&
+	test_grep ! region_enter.*cache_tree.*update trace.output
+'
+
 test_done

base-commit: 2855562ca6a9c6b0e7bc780b050c1e83c9fcfbd0
-- 
2.52.0.ge17bebe515.stripe

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

[Patrick Steinhardt]

On Mon, Apr 06, 2026 at 03:27:11PM -0400, David Lin wrote:
> The negation in front of the object existence check in
> cache_tree_fully_valid() was lost in 062b914c84 (treewide: convert
> users of `repo_has_object_file()` to `has_object()`, 2025-04-29),
> turning `!repo_has_object_file(...)` into `has_object(...)` instead
> of `!has_object(...)`.
> 
> This makes cache_tree_fully_valid() always report the cache tree as
> invalid when objects exist (the common case), forcing callers like
> write_index_as_tree() to call cache_tree_update() on every
> invocation.  An odb_has_object() check inside update_one() avoids a
> full tree rebuild, but the unnecessary call still pays the cost of
> opening an ODB transaction and, in partial clones, a promisor remote
> check.
> 
> Restore the missing negation and add a test that verifies write-tree
> takes the cache-tree shortcut when the cache tree is valid.

Oh, indeed, thanks for the fix.

I also checked whether there's any other such case in the commit in
question, but didn't spot any.

> diff --git a/cache-tree.c b/cache-tree.c
> index 60bcc07c3b..9fe057355c 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -238,7 +238,7 @@ int cache_tree_fully_valid(struct cache_tree *it)
>  	if (!it)
>  		return 0;
>  	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;
>  	for (i = 0; i < it->subtree_nr; i++) {

Yup, this looks obviously good to me.

Patrick