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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 18, 2021, 01:15 UTC
Message-ID: <xmqqczo3yvz3.fsf@gitster.g>
In-Reply-To: <8c148f7d-f175-7dc5-51a0-eef48aa98c3d@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 8 quoted lines

>>     * 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():

Show 8 quoted lines

> -	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?

It internally checks the repo->settings.sparse_index again and refrains from shrinking the entries, but we are talking about the case where that repo settings is set to "true", so wouldn't we end up getting the originally full in-core index turned into sparse?

It is a confusing piece of code.

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?

Still puzzled...

Thanks.