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

[Derrick Stolee <stolee@gmail.com>]

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