{"thread":{"id":"65445","subject":"[PATCH] cache-tree: fix inverted object existence check in cache_tree_fully_valid","startedAt":"2026-04-06T15:15:25Z","lastAt":"2026-04-07T05:15:19Z","messageCount":5,"participants":["David Lin","Derrick Stolee","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"540976","messageId":"20260406151456.63620-1-davidlin@stripe.com","threadId":"65445","inReplyTo":null,"subject":"[PATCH] cache-tree: fix inverted object existence check in cache_tree_fully_valid","fromName":"David Lin","fromEmail":"davidzylin@gmail.com","sentAt":"2026-04-06T15:14:56Z","receivedAt":"2026-04-06T15:15:25Z","isPatch":true,"body":"cache_tree_fully_valid() is supposed to return 0 (not valid) when a\ntree object is missing from the object database. The condition\ncurrently returns 0 when odb_has_object() succeeds, which is the\nopposite of what is intended: the cache tree should be considered\ninvalid when the object does not exist.\n\nAdd the missing negation so the function correctly invalidates cache\ntree nodes whose objects are absent.\n\nSigned-off-by: David Lin \n---\n cache-tree.c | 2 +-\n t/t0090-cache-tree.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 60bcc07c3b..9fe057355c 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -238,7 +238,7 @@ int cache_tree_fully_valid(struct cache_tree *it)\n \tif (!it)\n \t\treturn 0;\n \tif (it->entry_count < 0 ||\n-\t odb_has_object(the_repository->objects, &it->oid,\n+\t !odb_has_object(the_repository->objects, &it->oid,\n \t\t\t HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))\n \t\treturn 0;\n \tfor (i = 0; i < it->subtree_nr; i++) {\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex d901588294..2c6b7a0899 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -278,4 +278,12 @@ test_expect_success 'switching trees does not invalidate shared index' '\n \t)\n '\n \n+test_expect_success 'cache-tree is used by write-tree when valid' '\n+\ttest_commit use-valid &&\n+\n+\t# write-tree with a valid cache-tree should skip cache_tree_update\n+\tGIT_TRACE2_PERF=\"$(pwd)/trace.output\" git write-tree &&\n+\t! grep region_enter.*cache_tree.*update trace.output\n+'\n+\n test_done\n\nbase-commit: 2855562ca6a9c6b0e7bc780b050c1e83c9fcfbd0\n-- \n2.52.0.ge17bebe515.stripe\n\n"},{"id":"541006","messageId":"b0ee86fb-2fb8-4c7f-904d-66140b58164d@gmail.com","threadId":"65445","inReplyTo":"20260406151456.63620-1-davidlin@stripe.com","subject":"Re: [PATCH] cache-tree: fix inverted object existence check in cache_tree_fully_valid","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-04-06T18:10:16Z","receivedAt":"2026-04-06T18:10:18Z","isPatch":true,"body":"On 4/6/2026 11:14 AM, David Lin wrote:\n> cache_tree_fully_valid() is supposed to return 0 (not valid) when a\n> tree object is missing from the object database. The condition\n> currently returns 0 when odb_has_object() succeeds, which is the\n> opposite of what is intended: the cache tree should be considered\n> invalid when the object does not exist.\n\nIt looks like this negation occurred two refactors ago in\n062b914c84 (treewide: convert users of `repo_has_object_file()` to\n`has_object()`, 2025-04-29) which had this diff:\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex c0e1e9ee1d..fa3858e282 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -238,7 +238,9 @@ int cache_tree_fully_valid(struct cache_tree *it)\n int i;\n if (!it)\n return 0;\n- if (it->entry_count < 0 || !repo_has_object_file(the_repository, &it->oid))\n+ if (it->entry_count < 0 ||\n+ has_object(the_repository, &it->oid,\n+ HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))\n return 0;\n for (i = 0; i < it->subtree_nr; i++) {\n if (!cache_tree_fully_valid(it->down[i]->cache_tree))\n \nThis was one hunk among many, so it is easy to miss that the ! was\nlost as the function was renamed and moved to another line.\n\n> \tif (it->entry_count < 0 ||\n> -\t odb_has_object(the_repository->objects, &it->oid,\n> +\t !odb_has_object(the_repository->objects, &it->oid,\n> \t\t\t HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))\n> \t\treturn 0;\n\nThis fix (respecting the has_object to odb_has_object refactor)\nis the correct one.\n\n> +test_expect_success 'cache-tree is used by write-tree when valid' '\n> +\ttest_commit use-valid &&\n> +\n> +\t# write-tree with a valid cache-tree should skip cache_tree_update\n> +\tGIT_TRACE2_PERF=\"$(pwd)/trace.output\" git write-tree &&\n> +\t! grep region_enter.*cache_tree.*update trace.output\n\nnit: I think this would be better as \"test_grep ! ...\"\n\nThanks,\n-Stolee\n\n"},{"id":"541012","messageId":"xmqqcy0bc4bf.fsf@gitster.g","threadId":"65445","inReplyTo":"b0ee86fb-2fb8-4c7f-904d-66140b58164d@gmail.com","subject":"Re: [PATCH] cache-tree: fix inverted object existence check in cache_tree_fully_valid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-06T18:49:08Z","receivedAt":"2026-04-06T18:49:11Z","isPatch":true,"body":"Derrick Stolee writes:\n\n> On 4/6/2026 11:14 AM, David Lin wrote:\n>> cache_tree_fully_valid() is supposed to return 0 (not valid) when a\n>> tree object is missing from the object database. The condition\n>> currently returns 0 when odb_has_object() succeeds, which is the\n>> opposite of what is intended: the cache tree should be considered\n>> invalid when the object does not exist.\n>\n> It looks like this negation occurred two refactors ago in\n> 062b914c84 (treewide: convert users of `repo_has_object_file()` to\n> `has_object()`, 2025-04-29) which had this diff:\n>\n> diff --git a/cache-tree.c b/cache-tree.c\n> index c0e1e9ee1d..fa3858e282 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -238,7 +238,9 @@ int cache_tree_fully_valid(struct cache_tree *it)\n> int i;\n> if (!it)\n> return 0;\n> - if (it->entry_count < 0 || !repo_has_object_file(the_repository, &it->oid))\n> + if (it->entry_count < 0 ||\n> + has_object(the_repository, &it->oid,\n> + HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))\n> return 0;\n> for (i = 0; i < it->subtree_nr; i++) {\n> if (!cache_tree_fully_valid(it->down[i]->cache_tree))\n> \n> This was one hunk among many, so it is easy to miss that the ! was\n> lost as the function was renamed and moved to another line.\n\nThanks for archaeology. It deserves to be recorded in the proposed\nlog message. David, would you mind updating the message and repost\na v2 of the patch?\n\nI wonder what the practical effect of this breakage was. The added\ntest only checks what happens in the trace, but what was the effect\nexternally observable? We did not rebuild the cache-tree when we\nshould have, causing \"write-tree\" to record a set of tree objects\nthat do not match what is in the index?\n\n\n>> \tif (it->entry_count < 0 ||\n>> -\t odb_has_object(the_repository->objects, &it->oid,\n>> +\t !odb_has_object(the_repository->objects, &it->oid,\n>> \t\t\t HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))\n>> \t\treturn 0;\n>\n> This fix (respecting the has_object to odb_has_object refactor)\n> is the correct one.\n>\n>> +test_expect_success 'cache-tree is used by write-tree when valid' '\n>> +\ttest_commit use-valid &&\n>> +\n>> +\t# write-tree with a valid cache-tree should skip cache_tree_update\n>> +\tGIT_TRACE2_PERF=\"$(pwd)/trace.output\" git write-tree &&\n>> +\t! grep region_enter.*cache_tree.*update trace.output\n>\n> nit: I think this would be better as \"test_grep ! ...\"\n>\n> Thanks,\n> -Stolee\n"},{"id":"541014","messageId":"20260406192711.68870-1-davidlin@stripe.com","threadId":"65445","inReplyTo":"xmqqcy0bc4bf.fsf@gitster.g","subject":"[PATCH v2] cache-tree: fix inverted object existence check in cache_tree_fully_valid","fromName":"David Lin","fromEmail":"davidzylin@gmail.com","sentAt":"2026-04-06T19:27:11Z","receivedAt":"2026-04-06T19:28:04Z","isPatch":true,"body":"The negation in front of the object existence check in\ncache_tree_fully_valid() was lost in 062b914c84 (treewide: convert\nusers of `repo_has_object_file()` to `has_object()`, 2025-04-29),\nturning `!repo_has_object_file(...)` into `has_object(...)` instead\nof `!has_object(...)`.\n\nThis makes cache_tree_fully_valid() always report the cache tree as\ninvalid when objects exist (the common case), forcing callers like\nwrite_index_as_tree() to call cache_tree_update() on every\ninvocation. An odb_has_object() check inside update_one() avoids a\nfull tree rebuild, but the unnecessary call still pays the cost of\nopening an ODB transaction and, in partial clones, a promisor remote\ncheck.\n\nRestore the missing negation and add a test that verifies write-tree\ntakes the cache-tree shortcut when the cache tree is valid.\n\nHelped-by: Derrick Stolee \nSigned-off-by: David Lin \n---\n\nNotes:\n Changes since v1:\n - Use test_grep instead of bare grep (Stolee)\n\n cache-tree.c | 2 +-\n t/t0090-cache-tree.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 60bcc07c3b..9fe057355c 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -238,7 +238,7 @@ int cache_tree_fully_valid(struct cache_tree *it)\n \tif (!it)\n \t\treturn 0;\n \tif (it->entry_count < 0 ||\n-\t odb_has_object(the_repository->objects, &it->oid,\n+\t !odb_has_object(the_repository->objects, &it->oid,\n \t\t\t HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))\n \t\treturn 0;\n \tfor (i = 0; i < it->subtree_nr; i++) {\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex d901588294..0964718d7f 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -278,4 +278,12 @@ test_expect_success 'switching trees does not invalidate shared index' '\n \t)\n '\n \n+test_expect_success 'cache-tree is used by write-tree when valid' '\n+\ttest_commit use-valid &&\n+\n+\t# write-tree with a valid cache-tree should skip cache_tree_update\n+\tGIT_TRACE2_PERF=\"$(pwd)/trace.output\" git write-tree &&\n+\ttest_grep ! region_enter.*cache_tree.*update trace.output\n+'\n+\n test_done\n\nbase-commit: 2855562ca6a9c6b0e7bc780b050c1e83c9fcfbd0\n-- \n2.52.0.ge17bebe515.stripe\n\n"},{"id":"541037","messageId":"adSS4GJyBoB2rY4s@pks.im","threadId":"65445","inReplyTo":"20260406192711.68870-1-davidlin@stripe.com","subject":"Re: [PATCH v2] cache-tree: fix inverted object existence check in cache_tree_fully_valid","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-07T05:15:12Z","receivedAt":"2026-04-07T05:15:19Z","isPatch":true,"body":"On Mon, Apr 06, 2026 at 03:27:11PM -0400, David Lin wrote:\n> The negation in front of the object existence check in\n> cache_tree_fully_valid() was lost in 062b914c84 (treewide: convert\n> users of `repo_has_object_file()` to `has_object()`, 2025-04-29),\n> turning `!repo_has_object_file(...)` into `has_object(...)` instead\n> of `!has_object(...)`.\n> \n> This makes cache_tree_fully_valid() always report the cache tree as\n> invalid when objects exist (the common case), forcing callers like\n> write_index_as_tree() to call cache_tree_update() on every\n> invocation. An odb_has_object() check inside update_one() avoids a\n> full tree rebuild, but the unnecessary call still pays the cost of\n> opening an ODB transaction and, in partial clones, a promisor remote\n> check.\n> \n> Restore the missing negation and add a test that verifies write-tree\n> takes the cache-tree shortcut when the cache tree is valid.\n\nOh, indeed, thanks for the fix.\n\nI also checked whether there's any other such case in the commit in\nquestion, but didn't spot any.\n\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 60bcc07c3b..9fe057355c 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -238,7 +238,7 @@ int cache_tree_fully_valid(struct cache_tree *it)\n> \tif (!it)\n> \t\treturn 0;\n> \tif (it->entry_count < 0 ||\n> -\t odb_has_object(the_repository->objects, &it->oid,\n> +\t !odb_has_object(the_repository->objects, &it->oid,\n> \t\t\t HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))\n> \t\treturn 0;\n> \tfor (i = 0; i < it->subtree_nr; i++) {\n\nYup, this looks obviously good to me.\n\nPatrick\n"}]}