Derrick Stolee <stolee@xxxxxxxxx> 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.
