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

[Victoria Dye <vdye@github.com>]

Derrick Stolee wrote:
> On 10/27/2021 5:32 PM, Junio C Hamano wrote:
>> Derrick Stolee <stolee@gmail.com> 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.
> 
> I think you are right, what you propose below. It certainly seems
> like it would work, and even speed up the conversion from full to
> sparse. I think I erred on the side of extreme caution and used
> a hope that converting to sparse would be rare.
> 
>>  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;
>> +	}
> 
> I think at this point we have enough tests that check the sparse index
> and its different conversion points that the test suite might catch if
> this is a bad idea. Note that this is only a change of behavior if the
> cache-tree is valid, which I expect to be the case most of the time in
> the tests.
> 
> Thanks,
> -Stolee
> 

This change doesn't appear to introduce any test failures or other unwanted
behavior, so I don't see a reason not to include it. I'll add it in a
re-roll - thanks!