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

[Junio C Hamano <gitster@pobox.com>]

Derrick Stolee <stolee@gmail.com> writes:

> I think the 'else if' should be removed, which would match my
> expectations.
> ...
> 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.

Heh ;-) It was exactly the fact that convert_to_sparse() has many
checks inside that confused me into thinking that the new "else" may
turn into a no-op and the behaviour of the new code matches your
description.  It is confusing, and it being one way street does not
help readers, either.

Which in turn led me into worrying that our unconditional calls to
convert_to_sparse(), which are not accompanied by corresponding
calls to ensure_full_index() on the write code paths, were
problematic.  With the patch under discussion, in a repository that
does not want to see a sparse index, if we did not have "otherwise,
do sparsify the in-core index", lack of ensure_full_index() may not
be a problem, because we are guaranteeing that the in-core index is
fully expanded by the time the control reaches the write code path.

But if we by default shrunk the in-core index upon reading
(i.e. with the "else" clause), its correctness becomes muddy.
Convert_to_sparse() may not turn a non-sparse in-core index into
sparse with these checks, but it would not expand a sparse in-core
index before writing out.

Which in turn led me to suggest, instead of unconditional calls to
convert_to_sparse(), make unconditional calls to a helper function
that fixes the sparseness.  I knew convert_to_sparse() refrains from
touching the in-core index when the repository does not want a
sparse index, but I also am reasonably sure that it lacks support to
expanding the sparse index into full, so it cannot be the "fix to
the correct sparseness" function.

If our promise to a repository that does not want a sparse index is
that the read code path ensures that the in-core index is fully
expanded, and that nobody (including convert_to_sparse()) places a
sparse entry in the in-core index, then the promise would not be
broken, but I still wonder if the whole code is human-reader
friendly.  It wasn't to me ;-)

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

"-if-able" does not make much sense to me, but "-as-needed" may.  By
not calling it "fix-sparseness", but saying "convert-to-sparse", we
are making it clear that it is a one-way street that may make it
sparse but never removes sparseness.

I do not know if that is the guarantee we want to keep giving the
callers (not saying that I have a hunch that it would be better to
allow us to optionally expand to full in that function---I genuinely
do not know), or we may want to call it a neutral "fix sparseness"
to keep the door open for a future where we may expand an already
sparse in-core index into full based on the setting.

Thanks.