{"thread":{"id":"65930","subject":"[PATCH] sparse-index: avoid crash on intent-to-add entry outside the cone","startedAt":"2026-07-06T13:50:55Z","lastAt":"2026-07-31T15:56:49Z","messageCount":4,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"547232","messageId":"pull.2167.git.1783345853272.gitgitgadget@gmail.com","threadId":"65930","inReplyTo":null,"subject":"[PATCH] sparse-index: avoid crash on intent-to-add entry outside the cone","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-06T13:50:52Z","receivedAt":"2026-07-06T13:50:55Z","isPatch":true,"body":"From: Derrick Stolee <stolee@gmail.com>\n\nWhen collapsing a full index to a sparse index, the recursive\nconvert_to_sparse_rec() walks the cache tree to determine if any\nof the cache tree entries can be used to represent a sparse directory.\n\nAs it goes, the method tracks how many cache entries are being represented\nby the cache tree entry. The cache tree node's 'entry_count' represents how\nmany cache entries are covered by the node.\n\nHowever, this value can be negative, representing that a node is invalid,\nand is no longer reflecting the number of cache entries fit within. This can\nhappen when the user uses 'git add --intent-to-add' to mark an untracked\nfile with the intent-to-add bit to avoid committing without finishing the\nadd.\n\nWhen such an intent-to-add file exists and the sparse-checkout changes to no\nlonger contain its parent directory, this leads to a segfault. Two tests are\nadded to demonstrate this fault:\n\n* One test is added to t3705-add-sparse-checkout.sh to demonstrate\n  how 'git add' behaves with sparse-checkout.\n\n* One test is added to t1092-sparse-checkout-compatibility.sh to demonstrate\n  the interaction with the sparse index and to compare it directly to how\n  the commands behave with a full index or no sparse-checkout.\n\nThe fix involves engaging with the loop that iterates over all cache entries\nwithin the parent cache tree node (from 'start' to 'end') and to set the\n'span' variable slightly earlier. At this point, the cache entry is for a\nfile that is at least one directory deeper than the current cache tree node.\nThe path is also not in the sparse-checkout because of an earlier\npath_in_sparse_checkout() check above the loop. So we are trying to collapse\nthis directory by recursively calling convert_to_sparse_rec() over that span\nof entries, but the negative value prevents us from predicting that number\nwithout scanning.\n\nTheoretically, we could scan to find the range of entries that match this\ndirectory and determine if they truly do have an intent-to-add bit and then\ncollapse as many child trees as possible (the ones with valid cache tree\nnodes). That would be a non-trivial change for performance-only benefit.\nSince this combination of the intent-to-add and sparse index features has so\nfar gone undetected by real users, this scenario is unlikely to be worth\nsuch a change.\n\nWe settle for the simplest change that prevents a bug: don't try to collapse\na node that is invalid for this reason. The tests that would demonstrate a\nsegfault now pass. Further, they demonstrate that the intent-to-add bit\npersists in the index file after changing the sparse-checkout scope. The\ntest in t1092 demonstrates how some sparse directories could be collapsed\nfurther with a more involved fix, if so desired in the future.\n\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n    sparse-index: avoid crash on intent-to-add entry outside the cone\n    \n    I discovered this while taking inventory of the un-audited\n    ensure_full_index() calls, finding this block:\n    \n    /* TODO: audit for interaction with sparse-index. */\n    ensure_full_index(the_repository->index);\n    for (i = 0; i < the_repository->index->cache_nr; i++)\n    \tif (ce_intent_to_add(the_repository->index->cache[i]))\n    \t\tita_nr++;\n    committable = the_repository->index->cache_nr > ita_nr;\n    \n    \n    This led me to realize that the sparse-index collapse algorithm didn't\n    take intent-to-add into account for avoiding a collapse. We already\n    avoid collapse to a sparse directory if there exists a submodule\n    somewhere, but we don't do the same for intent-to-add.\n    \n    I thought I'd just find a normal bug, not a segfault, but that made the\n    fix somewhat simpler though less efficient in the final result.\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2167%2Fderrickstolee%2Fita-segfault-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2167/derrickstolee/ita-segfault-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2167\n\n sparse-index.c                           |  9 ++++-\n t/t1092-sparse-checkout-compatibility.sh | 48 ++++++++++++++++++++++++\n t/t3705-add-sparse-checkout.sh           | 26 +++++++++++++\n 3 files changed, 82 insertions(+), 1 deletion(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 1ed769b78d..c1fa231a89 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -113,10 +113,17 @@ static int convert_to_sparse_rec(struct index_state *istate,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tspan = ct->down[pos]->cache_tree->entry_count;\n+\t\tif (span < 0) {\n+\t\t\t/* cache-tree entry is invalidated, cannot collapse. */\n+\t\t\tistate->cache[num_converted++] = ce;\n+\t\t\ti++;\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tstrbuf_setlen(&child_path, 0);\n \t\tstrbuf_add(&child_path, ce->name, slash - ce->name + 1);\n \n-\t\tspan = ct->down[pos]->cache_tree->entry_count;\n \t\tcount = convert_to_sparse_rec(istate,\n \t\t\t\t\t      num_converted, i, i + span,\n \t\t\t\t\t      child_path.buf, child_path.len,\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 8186da5c88..c433de2c1e 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -384,6 +384,54 @@ test_expect_success 'add, commit, checkout' '\n \ttest_all_match git checkout -\n '\n \n+test_expect_success 'intent-to-add entries outside sparse-checkout' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\n+\ttest_sparse_match git sparse-checkout set deep folder1 &&\n+\trun_on_sparse mkdir -p folder1 &&\n+\trun_on_all ../edit-contents folder1/newita &&\n+\ttest_sparse_match git add -N folder1/newita &&\n+\n+\ttest_sparse_match git sparse-checkout set deep &&\n+\ttest_sparse_match git status --porcelain=v2 &&\n+\ttest_sparse_match git ls-files --stage\n+'\n+\n+test_expect_success 'intent-to-add with --sparse outside sparse-checkout' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\n+\trun_on_all mkdir -p folder1 &&\n+\trun_on_all ../edit-contents folder1/newita &&\n+\ttest_all_match git add --sparse --intent-to-add folder1/newita &&\n+\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git ls-files --stage &&\n+\ttest_all_match git diff --cached --stat &&\n+\n+\t# Ensure sparse index stores correct sparse directories and\n+\t# intent-to-add path.\n+\tgit -C sparse-index ls-files --format=\"%(path)\" --sparse >out &&\n+\n+\t# These paths should be present in index as-is.\n+\ttest_grep \"^before/\\$\" out &&\n+\ttest_grep \"^folder1/newita\\$\" out &&\n+\ttest_grep \"^folder2/\\$\" out &&\n+\ttest_grep \"^x/\\$\" out &&\n+\n+\t# folder/0/ could theoretically be collapsed to a sparse\n+\t# directory entry, but the current implementation avoids the\n+\t# reduction because of folder1/newita\n+\ttest_grep \"^folder1/0/0/0\\$\" out\n+'\n+\n test_expect_success 'git add, checkout, and reset with -p' '\n \tinit_repos &&\n \ndiff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh\nindex 53a4782267..cf3f42a353 100755\n--- a/t/t3705-add-sparse-checkout.sh\n+++ b/t/t3705-add-sparse-checkout.sh\n@@ -233,4 +233,30 @@ test_expect_success 'refuse to add non-skip-worktree file from sparse dir' '\n \ttest_cmp expect stderr\n '\n \n+test_expect_success 'intent-to-add entry and sparse index' '\n+\ttest_when_finished \"git sparse-checkout disable\" &&\n+\ttest_when_finished \"git reset --hard\" &&\n+\n+\tgit sparse-checkout disable &&\n+\tmkdir -p in out &&\n+\techo base >in/file &&\n+\techo base >out/file &&\n+\tgit add in/file out/file &&\n+\tgit commit -m \"in and out directories\" &&\n+\n+\t# enable sparse-checkout, but with all child directories.\n+\tgit config index.sparse true &&\n+\tgit sparse-checkout set in out &&\n+\n+\t# create a new path and set intent-to-add bit\n+\techo new >out/newita &&\n+\tgit add -N out/newita &&\n+\n+\t# collapse sparse-checkout, and make sure that the sparse index\n+\t# maintains the intent-to-add bit.\n+\tgit sparse-checkout set in &&\n+\tgit ls-files --error-unmatch out/newita &&\n+\tgit status --porcelain\n+'\n+\n test_done\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\n-- \ngitgitgadget\n"},{"id":"549239","messageId":"xmqq33x1o465.fsf@gitster.g","threadId":"65930","inReplyTo":"pull.2167.git.1783345853272.gitgitgadget@gmail.com","subject":"Re: [PATCH] sparse-index: avoid crash on intent-to-add entry outside the cone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-29T21:51:30Z","receivedAt":"2026-07-29T21:51:33Z","isPatch":true,"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <stolee@gmail.com>\n>\n> When collapsing a full index to a sparse index, the recursive\n> convert_to_sparse_rec() walks the cache tree to determine if any\n> of the cache tree entries can be used to represent a sparse directory.\n>\n> As it goes, the method tracks how many cache entries are being represented\n> by the cache tree entry. The cache tree node's 'entry_count' represents how\n> many cache entries are covered by the node.\n>\n> However, this value can be negative, representing that a node is invalid,\n> and is no longer reflecting the number of cache entries fit within. This can\n> happen when the user uses 'git add --intent-to-add' to mark an untracked\n> file with the intent-to-add bit to avoid committing without finishing the\n> add.\n\nYes.  If the code were not anticipating this, I can understand how a\nbug can arise ;-)\n\n> Theoretically, we could scan to find the range of entries that match this\n> directory and determine if they truly do have an intent-to-add bit and then\n> collapse as many child trees as possible (the ones with valid cache tree\n> nodes). That would be a non-trivial change for performance-only benefit.\n> Since this combination of the intent-to-add and sparse index features has so\n> far gone undetected by real users, this scenario is unlikely to be worth\n> such a change.\n\nI tend to agree.  That does sound nasty.\n\n> diff --git a/sparse-index.c b/sparse-index.c\n> index 1ed769b78d..c1fa231a89 100644\n> --- a/sparse-index.c\n> +++ b/sparse-index.c\n> @@ -113,10 +113,17 @@ static int convert_to_sparse_rec(struct index_state *istate,\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n> +\t\tspan = ct->down[pos]->cache_tree->entry_count;\n> +\t\tif (span < 0) {\n> +\t\t\t/* cache-tree entry is invalidated, cannot collapse. */\n> +\t\t\tistate->cache[num_converted++] = ce;\n> +\t\t\ti++;\n> +\t\t\tcontinue;\n> +\t\t}\n\nOK.  That is an easy and safe cop-out that is much better than segfaulting.\n\nShall we mark the topic for 'next'?\n\nThanks.\n"},{"id":"549351","messageId":"e4cce4e2-4287-4e1a-8833-d37ee48ff7d6@gmail.com","threadId":"65930","inReplyTo":"xmqq33x1o465.fsf@gitster.g","subject":"Re: [PATCH] sparse-index: avoid crash on intent-to-add entry outside the cone","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-07-31T13:23:41Z","receivedAt":"2026-07-31T13:23:43Z","isPatch":true,"body":"On 7/29/2026 5:51 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Derrick Stolee <stolee@gmail.com>\n>>\n>> When collapsing a full index to a sparse index, the recursive\n>> convert_to_sparse_rec() walks the cache tree to determine if any\n>> of the cache tree entries can be used to represent a sparse directory.\n>>\n>> As it goes, the method tracks how many cache entries are being represented\n>> by the cache tree entry. The cache tree node's 'entry_count' represents how\n>> many cache entries are covered by the node.\n>>\n>> However, this value can be negative, representing that a node is invalid,\n>> and is no longer reflecting the number of cache entries fit within. This can\n>> happen when the user uses 'git add --intent-to-add' to mark an untracked\n>> file with the intent-to-add bit to avoid committing without finishing the\n>> add.\n> \n> Yes.  If the code were not anticipating this, I can understand how a\n> bug can arise ;-)\n> \n>> Theoretically, we could scan to find the range of entries that match this\n>> directory and determine if they truly do have an intent-to-add bit and then\n>> collapse as many child trees as possible (the ones with valid cache tree\n>> nodes). That would be a non-trivial change for performance-only benefit.\n>> Since this combination of the intent-to-add and sparse index features has so\n>> far gone undetected by real users, this scenario is unlikely to be worth\n>> such a change.\n> \n> I tend to agree.  That does sound nasty.\n\n>> +\t\tspan = ct->down[pos]->cache_tree->entry_count;\n>> +\t\tif (span < 0) {\n>> +\t\t\t/* cache-tree entry is invalidated, cannot collapse. */\n>> +\t\t\tistate->cache[num_converted++] = ce;\n>> +\t\t\ti++;\n>> +\t\t\tcontinue;\n>> +\t\t}\n> \n> OK.  That is an easy and safe cop-out that is much better than segfaulting.\n> \n> Shall we mark the topic for 'next'?\nThanks for taking a look. yes, this should be a pretty safe change that\ncan merge.\n\nThanks,\n-Stolee\n\n"},{"id":"549357","messageId":"xmqqtspfcfup.fsf@gitster.g","threadId":"65930","inReplyTo":"e4cce4e2-4287-4e1a-8833-d37ee48ff7d6@gmail.com","subject":"Re: [PATCH] sparse-index: avoid crash on intent-to-add entry outside the cone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-31T15:56:46Z","receivedAt":"2026-07-31T15:56:49Z","isPatch":true,"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>> OK.  That is an easy and safe cop-out that is much better than segfaulting.\n>> \n>> Shall we mark the topic for 'next'?\n> Thanks for taking a look. yes, this should be a pretty safe change that\n> can merge.\n\nThanks.\n"}]}