Re: [PATCH] sparse-index: avoid crash on intent-to-add entry outside the cone
Derrick Stolee <[email protected]> Fri, 31 Jul 2026 09:23:41 -0400
| Newsgroups | org.kernel.vger.git |
|---|---|
| Message-ID | <[email protected]> |
On 7/29/2026 5:51 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <[email protected]> writes: > >> From: Derrick Stolee <[email protected]> >> >> When collapsing a full index to a sparse index, the recursive >> convert_to_sparse_rec() walks the cache tree to determine if any >> of the cache tree entries can be used to represent a sparse directory. >> >> As it goes, the method tracks how many cache entries are being represented >> by the cache tree entry. The cache tree node's 'entry_count' represents how >> many cache entries are covered by the node. >> >> However, this value can be negative, representing that a node is invalid, >> and is no longer reflecting the number of cache entries fit within. This can >> happen when the user uses 'git add --intent-to-add' to mark an untracked >> file with the intent-to-add bit to avoid committing without finishing the >> add. > > Yes. If the code were not anticipating this, I can understand how a > bug can arise ;-) > >> Theoretically, we could scan to find the range of entries that match this >> directory and determine if they truly do have an intent-to-add bit and then >> collapse as many child trees as possible (the ones with valid cache tree >> nodes). That would be a non-trivial change for performance-only benefit. >> Since this combination of the intent-to-add and sparse index features has so >> far gone undetected by real users, this scenario is unlikely to be worth >> such a change. > > I tend to agree. That does sound nasty. >> + span = ct->down[pos]->cache_tree->entry_count; >> + if (span < 0) { >> + /* cache-tree entry is invalidated, cannot collapse. */ >> + istate->cache[num_converted++] = ce; >> + i++; >> + continue; >> + } > > OK. That is an easy and safe cop-out that is much better than segfaulting. > > Shall we mark the topic for 'next'? Thanks for taking a look. yes, this should be a pretty safe change that can merge. Thanks, -Stolee