patchcache-tree: fix inverted object existence check in cache_tree_fully_valid

5 messages between Apr 6, 2026 and Apr 7, 2026, from David Lin, Derrick Stolee, Junio C Hamano, Patrick Steinhardt.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

David LinApr 6, 2026, 15:14 UTC on lore

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(-)

Show changes to 2 files +9 −1

cache-tree.c, t/t0090-cache-tree.sh

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

Derrick StoleeApr 6, 2026, 18:10 UTC in reply to David Lin on lore

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

On 4/6/2026 11:14 AM, David Lin wrote:

Show 5 quoted lines

> 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:

Show changes to cache-tree.c +3 −2

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

Junio C HamanoApr 6, 2026, 18:49 UTC in reply to Derrick Stolee on lore

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

Derrick Stolee <stolee@gmail.com> writes:

Show 29 quoted lines

> 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?

Show 20 quoted lines

>>  	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

David LinApr 6, 2026, 19:27 UTC in reply to Junio C Hamano on lore

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

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(-)

Show changes to 2 files +9 −1

cache-tree.c, t/t0090-cache-tree.sh

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

Patrick SteinhardtApr 7, 2026, 05:15 UTC in reply to David Lin on lore

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

On Mon, Apr 06, 2026 at 03:27:11PM -0400, David Lin wrote:

Show 16 quoted lines

> 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.

Show 13 quoted lines

> 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