Re: [PATCH 1/3] sparse-index: sparse index is disallowed when split index is active

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

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 6e773527b6b (sparse-index: convert from full to sparse, 2021-03-30),
> we introduced initial support for a sparse index, and were careful to
> avoid converting to a sparse index in the presence of a split index.
>
> However, when we _just_ read a freshly-initialized index, it might not
> contain a split index even if _writing_ it will add one by virtue of
> being asked for via the `GIT_TEST_SPLIT_INDEX` variable.
>
> We did not notice any problems with checking _only_ for `split_index`
> (and not `GIT_TEST_SPLIT_INDEX`) right until both
> `vd/sparse-sparsity-fix-on-read` _and_ `vd/sparse-reset` were merged.
>
> Those two topics' interplay triggers a bug in conjunction with running
> t1091.15 when `GIT_TEST_SPLIT_INDEX=true` in the following way:
> `vd/sparse-sparsity-fix-on-read` ensures that the index is made sparse
> right after reading, and `vd/sparse-reset` ensures that the index is
> made non-sparse again unless running in the `--soft` mode. Since the
> split index feature is incompatible with the sparse index feature, we
> see a symptom like this:
>
> 	fatal: position for replacement 4 exceeds base index size 4
>
> Let's fix this by avoiding the conversion to a sparse index when
> `GIT_TEST_SPLIT_INDEX=true`.

Does [2/3] allow you to sidestep that issue entirely by skipping
1091 altogether?

There are 4 hits of "if (istate->split_index" in the codebase, and
this patch makes me wonder why it is suffice to patch only one of
them.

I also wondered why we test both split_index and environment
separately, instead of splitting the index very early when the
environment variable is set, so that the rest of the runtime does
not have to worry about the environment, but is the reason why such
an approach was not taken was because GIT_TEST_SPLIT_INDEX can later
allow the index to be splitted, even if istate->split_index is still
NULL right now when this function is called?

If that is the reason, it leads to another question.  Even if we
ignore GIT_TEST_SPLIT_INDEX and concentrate only on real workload,
if the in-core index can be NULL when this function is called and
then later can become split, there still must be somebody who
notices that sparse index is unallowed when (or after) such a split
happens, no?  If there is no such code, then this patch would not be
the whole fix and there needs more change to do so, no?

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sparse-index.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sparse-index.c b/sparse-index.c
> index a1d505d50e9..08f54747bb4 100644
> --- a/sparse-index.c
> +++ b/sparse-index.c
> @@ -136,7 +136,7 @@ static int is_sparse_index_allowed(struct index_state *istate, int flags)
>  		/*
>  		 * The sparse index is not (yet) integrated with a split index.
>  		 */
> -		if (istate->split_index)
> +		if (istate->split_index || git_env_bool("GIT_TEST_SPLIT_INDEX", 0))
>  			return 0;
>  		/*
>  		 * The GIT_TEST_SPARSE_INDEX environment variable triggers the