Re: [PATCH 2/2] sparse-index: update index read to consider index.sparse config

[Derrick Stolee <stolee@gmail.com>]

On 10/17/2021 9:15 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>>>     * In addition, with these patches, if index.sparse=false, a
>>>       sparse index will be expaned to full upon reading, and if it
>>>       is true, a non-sparse index will be shrunk to sparse upon
>>>       reading
>>
>> This is only half true. If "index.sparse=false", then a sparse
>> index will be expanded to full upon reading. If "index.sparse=true"
>> then nothing special will happen to an index on reading.
> 
> OK.  I somehow got the impression that we convert in both ways from
> the patch text under discussion, specifically this part in
> do_read_index():
> 
>> -	if (istate->repo->settings.command_requires_full_index)
>> +	if (!istate->repo->settings.sparse_index ||
>> +	    istate->repo->settings.command_requires_full_index)
>>  		ensure_full_index(istate);
>> +	else if (!istate->sparse_index)
>> +		convert_to_sparse(istate, 0);
>>  
>>  	return istate->cache_nr;
> 
> We used to say "when we know we are running a command that is not
> sparse ready, expand it here" and nothing else.
> 
> We now added a bit more condition for expansion, i.e. "when we are
> told that the repository does not want sparse index, or when the
> command is not sparse ready".
> 
> But the patch goes one more step.  "If we didn't find a reason to
> expand to full, and if the in-core index we read is not sparse, then
> call convert_to_sparse() on it".  So if the repo settings tells us
> that the repository wants a sparse index, and the index we read was
> not sparse, what does convert_to_sparse() without MEM_ONLY flag bit
> do?  Nothing special?

You are absolutely right. I've been talking about what I _thought_
the code does (and should do) but I missed this 'else if' which is
in fact doing what you have been claiming the entire time. I should
have done a better job double-checking the code before doubling
down on my claims.

I think the 'else if' should be removed, which would match my
expectations.

> I see many unconditional calls to convert_to_sparse(istate, 0) on
> the write code paths, where I instead would expect "if the repo
> wants sparse, call convert_to_sparse(), and if the repo does not
> want sparse, call ensure_full_index()", before actualy writing the
> entries out.  These also are setting traps to confuse readers.
> 
> Perhaps we want a helper function "ensure_right_sparsity(istate)"
> that would be called where we have
> 
> 	if (condition)
> 		convert_to_sparse();
> 	else
> 		ensure_full_index();
> 
> or an unconditonal call to convert_to_sparse() to unconfuse readers?

convert_to_sparse() has several checks that prevent the conversion
from happening, such as having a split index. In particular, it will
skip the conversion if the_repository->settings.sparse_index is false.
Thus, calling it unconditionally in the write code is correct.

Doing these conditional checks within convert_to_sparse() make sense
to avoid repeating the conditionals in all callers. ensure_full_index()
doesn't do the same because we absolutely want a full index at the end
of that method.

Perhaps a rename to something like "convert_to_sparse_if_able()" would
make sense? But it might be best to leave that one lie.

Thanks,
-Stolee