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

On 10/17/2021 9:15 PM, Junio C Hamano wrote:

Derrick Stolee [off-list ref] writes:

quoted

quoted

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

quoted

-	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