Re: [PATCH v3 2/3] sparse-index: add ensure_correct_sparsity function
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 27, 2021, 21:32 UTC
- Message-ID
- <xmqqbl3aupbh.fsf@gitster.g>
- In-Reply-To
- <bf73e2d2-a2ab-8364-d505-1bd5bba3efd2@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
Show 9 quoted lines
>> +int convert_to_sparse(struct index_state *istate, int flags)
>> +{
>> + /*
>> + * If the index is already sparse, empty, or otherwise
>> + * cannot be converted to sparse, do not convert.
>> + */
>> + if (istate->sparse_index || !istate->cache_nr ||
>> + !is_sparse_index_allowed(istate, flags))
>> + return 0;Shouldn't we also at least do this? Blindly blowing away the entire cache-tree and rebuilding it from scratch may be hiding a latent bug somewhere else, but is never supposed to be needed, and is a huge waste of computational resources.
I say "at least" here, because a cache tree that is partially valid should be safely salvageable---at least that was the intention back when I designed the subsystem.
sparse-index.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)
diff --git c/sparse-index.c w/sparse-index.c index bc3ee358c6..a95c3386f3 100644 --- c/sparse-index.c +++ w/sparse-index.c @@ -188,17 +188,19 @@ int convert_to_sparse(struct index_state *istate, int flags) if (index_has_unmerged_entries(istate)) return 0; - /* Clear and recompute the cache-tree */ - cache_tree_free(&istate->cache_tree); - /* - * Silently return if there is a problem with the cache tree update, - * which might just be due to a conflict state in some entry. - * - * This might create new tree objects, so be sure to use - * WRITE_TREE_MISSING_OK. - */ - if (cache_tree_update(istate, WRITE_TREE_MISSING_OK)) - return 0; + if (!cache_tree_fully_valid(&istate->cache_tree)) { + /* Clear and recompute the cache-tree */ + cache_tree_free(&istate->cache_tree); + /* + * Silently return if there is a problem with the cache tree update, + * which might just be due to a conflict state in some entry. + * + * This might create new tree objects, so be sure to use + * WRITE_TREE_MISSING_OK. + */ + if (cache_tree_update(istate, WRITE_TREE_MISSING_OK)) + return 0; + } remove_fsmonitor(istate);