Subject: Re: [PATCH v3 2/3] sparse-index: add ensure_correct_sparsity function

From: Junio C Hamano <[email-masked]>
Date: Wed, 27 Oct 2021 14:32:02 -0700

Derrick Stolee <[email-masked]> writes:

>> +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);