sparse-index: avoid crash on intent-to-add entry outside the cone#2167
Open
derrickstolee wants to merge 1 commit into
Open
sparse-index: avoid crash on intent-to-add entry outside the cone#2167derrickstolee wants to merge 1 commit into
derrickstolee wants to merge 1 commit into
Conversation
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. When such an intent-to-add file exists and the sparse-checkout changes to no longer contain its parent directory, this leads to a segfault. Two tests are added to demonstrate this fault: * One test is added to t3705-add-sparse-checkout.sh to demonstrate how 'git add' behaves with sparse-checkout. * One test is added to t1092-sparse-checkout-compatibility.sh to demonstrate the interaction with the sparse index and to compare it directly to how the commands behave with a full index or no sparse-checkout. The fix involves engaging with the loop that iterates over all cache entries within the parent cache tree node (from 'start' to 'end') and to set the 'span' variable slightly earlier. At this point, the cache entry is for a file that is at least one directory deeper than the current cache tree node. The path is also not in the sparse-checkout because of an earlier path_in_sparse_checkout() check above the loop. So we are trying to collapse this directory by recursively calling convert_to_sparse_rec() over that span of entries, but the negative value prevents us from predicting that number without scanning. 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 continued without issue, this scenario is unlikely to be worth such a change. We settle for the simplest change that prevents a bug: don't try to collapse a node that is invalid for this reason. The tests that would demonstrate a segfault now pass. Further, they demonstrate that the intent-to-add bit persists in the index file after changing the sparse-checkout scope. Signed-off-by: Derrick Stolee <stolee@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I discovered this while taking inventory of the un-audited ensure_full_index() calls, finding this block:
This led me to realize that the sparse-index collapse algorithm didn't take intent-to-add into account for avoiding a collapse. We already avoid collapse to a sparse directory if there exists a submodule somewhere, but we don't do the same for intent-to-add.
I thought I'd just find a normal bug, not a segfault, but that made the fix somewhat simpler though less efficient in the final result.
Thanks,
-Stolee
cc: gitster@pobox.com