# [PATCH] cache-tree: fix inverted object existence check in cache_tree_fully_valid 5 messages from 2026-04-06 to 2026-04-07. Participants: David Lin, Derrick Stolee, Junio C Hamano, Patrick Steinhardt. Thread: https://gitlist.dev/t/65445 ## David Lin, 2026-04-06 15:14 Subject: [PATCH] cache-tree: fix inverted object existence check in cache_tree_fully_valid Message-ID: <20260406151456.63620-1-davidlin@stripe.com> ``` 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 --- 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 ``` ## Derrick Stolee, 2026-04-06 18:10 Subject: Re: [PATCH] cache-tree: fix inverted object existence check in cache_tree_fully_valid Message-ID: In-Reply-To: <20260406151456.63620-1-davidlin@stripe.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 ``` ## Junio C Hamano, 2026-04-06 18:49 Subject: Re: [PATCH] cache-tree: fix inverted object existence check in cache_tree_fully_valid Message-ID: In-Reply-To: ``` Derrick Stolee 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 ``` ## David Lin, 2026-04-06 19:27 Subject: [PATCH v2] cache-tree: fix inverted object existence check in cache_tree_fully_valid Message-ID: <20260406192711.68870-1-davidlin@stripe.com> In-Reply-To: ``` 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 Signed-off-by: David Lin --- 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 ``` ## Patrick Steinhardt, 2026-04-07 05:15 Subject: Re: [PATCH v2] cache-tree: fix inverted object existence check in cache_tree_fully_valid Message-ID: In-Reply-To: <20260406192711.68870-1-davidlin@stripe.com> ``` 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 ```