Skip to content

pr-2167/derrickstolee/ita-segfault-v1

tagged this 06 Jul 13:50
From: Derrick Stolee <stolee@gmail.com>

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 gone undetected by real users, 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. The
test in t1092 demonstrates how some sparse directories could be collapsed
further with a more involved fix, if so desired in the future.

Signed-off-by: Derrick Stolee <stolee@gmail.com>

Submitted-As: https://lore.kernel.org/git/pull.2167.git.1783345853272.gitgitgadget@gmail.com
Assets 2
Loading