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

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

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Victoria Dye <vdye@github.com>
>
> Use the index.sparse config setting to expand or collapse the index when
> read. Previously, index.sparse would determine how the index would be
> written to disk, but would not enforce whether the index is read into memory
> as full or sparse. Now, the index is expanded when a sparse index is read
> with `index.sparse=false` and is collapsed to sparse when a full index is
> read with `index.sparse=true` (and the command does not require a full
> index).

Instead of calling both in-core index and on-disk index, perhaps use
"the in-core index" as appropriately in the above description and
the result would become much less ambigous.

My knee-jerk reaction was that it is of dubious value to spend
cycles to make the in-core index sparse after reading a non-sparse
index from the disk to give to the caller, but this hurts only the
commands that are not yet sparse-aware and call ensure_full_index()
as the first thing they do.  To them, we are wasting cycles to
shrink and expand for no good reason, and after they are done, the
final writeout would create a sparse on-disk index.

Besides, the on-disk index is expected to be sparse most of the time
when index.sparse is true, so it is hopefully a one-time waste that
corrects by itself.

For all commands that are sparse-aware, especially when asked to
perform their operation on the paths that are not hidden by a
tree-like index entry, it may or may not be a win, but the downside
would be much smaller.  The cost to shrink a full in-core index
before writing out as a sparse one should be comparable to the cost
to shrink a full in-core index just read from the disk before the
sparse-index-aware caller works on it and leaves a still mostly
sparse in-core index to be written out without much extra work to
re-shrink it to the disk.

> This makes the behavior of `index.sparse` more intuitive, as it now clearly
> enables/disables usage of a sparse index.

It is a minor thing, so I am willing to let it pass, but I am not
sure about this claim.  The write-out codepath ensures, independent
of this change, that a full on-disk index is corrected to become
sparse when the configuration is set to true, and a sparse on-disk
index is corrected to become full when the configuration is set to
false, no?

So the only "intuitive"-ness we may be gaining is that the codepaths
that are sparse-aware would work in their "sparse" (non-"sparse")
mode when index.sparse is set to true (false), respectively, no
matter how sparse (or not sparse) the on-disk index they work on is
initially.  That might help debuggability (assuming that converting
between the full and sparse forms are working correctly), but I am
not sure if that is something end users would even care about.

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

Quite straight-forward.  Looking good.