: sparse index vs split index fixes
cover : sparse index vs split index fixes
We noticed split/sparse index-related issues while rebasing Microsoft's fork of Git. These fixes are necessary for that fork's test suite to pass, but they might not be super critical to get into upstream v2.35.0 (especially this close to -rc2). However, the team felt that the decision should be left to the Git maintainer whether to take these patches into v2.35.0 or not.
Johannes Schindelin (3):
sparse-index: sparse index is disallowed when split index is active
t1091: disable split index
split-index: it really is incompatible with the sparse index
read-cache.c | 3 ++ sparse-index.c | 2 +- split-index.c | 3 ++ t/t1091-sparse-checkout-builtin.sh | 54 ++++++++++++++---------------- 4 files changed, 33 insertions(+), 29 deletions(-)
base-commit: af4e5f569bc89f356eb34a9373d7f82aca6faa8a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1119%2Fdscho%2Fsparse-index-vs-split-index-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1119/dscho/sparse-index-vs-split-index-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1119
Signature · 1 line
gitgitgadget
PATCH 1/3 sparse-index: sparse index is disallowed when split index is active
From: Johannes Schindelin <[email protected]>
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.
--- sparse-index.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
sparse-index.c +1 -1
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH 1/3] sparse-index: sparse index is disallowed when split index is active
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 22 lines · writes:
From: Johannes Schindelin <[email protected]>
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_INDEXvariable.We did not notice any problems with checking _only_ for
split_index(and notGIT_TEST_SPLIT_INDEX) right until bothvd/sparse-sparsity-fix-on-read_and_vd/sparse-resetwere merged.Those two topics' interplay triggers a bug in conjunction with running t1091.15 when
GIT_TEST_SPLIT_INDEX=truein the following way:vd/sparse-sparsity-fix-on-readensures that the index is made sparse right after reading, andvd/sparse-resetensures that the index is made non-sparse again unless running in the--softmode. Since the split index feature is incompatible with the sparse index feature, we see a symptom like this:
Quoted 4 lines
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?
Quoted 18 lines
Signed-off-by Johannes Schindelin <[email protected]>--- sparse-index.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)sparse-index.c +1
-1@@ -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
Re: [PATCH 1/3] sparse-index: sparse index is disallowed when split index is active
Hi Junio,
Quoted 25 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 22 lines · writes:
From: Johannes Schindelin <[email protected]>
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_INDEXvariable.We did not notice any problems with checking _only_ for
split_index(and notGIT_TEST_SPLIT_INDEX) right until bothvd/sparse-sparsity-fix-on-read_and_vd/sparse-resetwere merged.Those two topics' interplay triggers a bug in conjunction with running t1091.15 when
GIT_TEST_SPLIT_INDEX=truein the following way:vd/sparse-sparsity-fix-on-readensures that the index is made sparse right after reading, andvd/sparse-resetensures that the index is made non-sparse again unless running in the--softmode. Since the split index feature is incompatible with the sparse index feature, we see a symptom like this:
Quoted 7 lines
Quoted 4 lines
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?
You are right that reverting this patch after applying the next patch _still_ works around the issue. That's because currently only t1091 exposes the bug where GIT_TEST_SPLIT_INDEX writes a split index that was initially non-split (because it was just created).
My intention here was to avoid any problems in case _another_ test script triggers the same condition. After all, we do have an entire CI job that forces GIT_TEST_SPLIT_INDEX=1, and in microsoft/git we enable sparse index by default. The entire test suite is therefore surface for the bug to show.
Quoted 3 lines
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 tried to catch only those locations where we run the danger of trying to make a split index also sparse, or a sparse index also split.
Quoted 7 lines
I also wondered why we test both
split_indexand 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 becauseGIT_TEST_SPLIT_INDEXcan later allow the index to be splitted, even ifistate->split_indexis still NULL right now when this function is called?
Indeed. A freshly created index will never be split, even if GIT_TEST_SPLIT_INDEX is set. Only when we _write_ this to disk will it be turned into a split index. At this point, we might have mistakenly converted the index into a sparse one, though, and this here patch wants to prevent that from happening.
Quoted 7 lines
If that is the reason, it leads to another question. Even if we ignore
GIT_TEST_SPLIT_INDEXand 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?
Indeed. Hence 3/3 which tries specifically to prevent the user from turning an already-sparse index into a split index. You will note that 3/3 calls die() instead BUG() in such a case because it would constitute a pilot error, not a bug in Git's code.
Ciao,
Dscho
PATCH 2/3 t1091: disable split index
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In 61feddcdf28 (tests: disable GIT_TEST_SPLIT_INDEX for sparse index tests, 2021-08-26), it was already called out that the split index feature is incompatible with the sparse index feature, and its commit message wondered aloud whether more checks would be required to ensure that the split index and sparse index features aren't enabled at the same time.
We are about to introduce such additional checks, and indeed, t1091 would utterly fail with them. Therefore, let's preemptively disable the split index for the entirety of t1091.
This partially reverts above-mentioned patch because it covered only one test case whereas we want to cover the entire test script.
--- t/t1091-sparse-checkout-builtin.sh | 54 ++++++++++++++---------------- 1 file changed, 26 insertions(+), 28 deletions(-)
t/t1091-sparse-checkout-builtin.sh +26 -28
@@ -5,6 +5,9 @@ test_description='sparse checkout builtin tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +GIT_TEST_SPLIT_INDEX=false +export GIT_TEST_SPLIT_INDEX + . ./test-lib.sh list_files() {
@@ -228,36 +231,31 @@ test_expect_success 'sparse-checkout disable' ' ' test_expect_success 'sparse-index enabled and disabled' ' - ( - sane_unset GIT_TEST_SPLIT_INDEX && - git -C repo update-index --no-split-index && - - git -C repo sparse-checkout init --cone --sparse-index && - test_cmp_config -C repo true index.sparse && - git -C repo ls-files --sparse >sparse && - git -C repo sparse-checkout disable && - git -C repo ls-files --sparse >full && - - cat >expect <<-\EOF && - @@ -1,4 +1,7 @@ - a - -deep/ - -folder1/ - -folder2/ - +deep/a - +deep/deeper1/a - +deep/deeper1/deepest/a - +deep/deeper2/a - +folder1/a - +folder2/a - EOF + git -C repo sparse-checkout init --cone --sparse-index && + test_cmp_config -C repo true index.sparse && + git -C repo ls-files --sparse >sparse && + git -C repo sparse-checkout disable && + git -C repo ls-files --sparse >full && - diff -u sparse full | tail -n +3 >actual && - test_cmp expect actual && + cat >expect <<-\EOF && + @@ -1,4 +1,7 @@ + a + -deep/ + -folder1/ + -folder2/ + +deep/a + +deep/deeper1/a + +deep/deeper1/deepest/a + +deep/deeper2/a + +folder1/a + +folder2/a + EOF + + diff -u sparse full | tail -n +3 >actual && + test_cmp expect actual && - git -C repo config --list >config && - ! grep index.sparse config - ) + git -C repo config --list >config && + ! grep index.sparse config ' test_expect_success 'cone mode: init and set' '
Signature · 1 line
gitgitgadget
PATCH 3/3 split-index: it really is incompatible with the sparse index
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
... at least for now. So let's error out if we are even trying to initialize the split index when the index is sparse, or when trying to write the split index extension for a sparse index.
--- read-cache.c | 3 +++ split-index.c | 3 +++ 2 files changed, 6 insertions(+)
read-cache.c +3 -0
@@ -3009,6 +3009,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, !is_null_oid(&istate->split_index->base_oid)) {struct strbuf sb =STRBUF_INIT ; +if (istate->sparse_index) +die (_ ("cannot write split index for a sparse index" )); + err =write_link_extension (&sb, istate) <0 ||write_index_ext_header (f, eoie_c,CACHE_EXT_LINK , sb.len) <0 ;
split-index.c +3 -0
@@ -5,6 +5,9 @@struct split_index *init_split_index (struct index_state *istate) {if (!istate->split_index) { +if (istate->sparse_index) +die (_ ("cannot use split index with a sparse index" )); +CALLOC_ARRAY (istate->split_index,1 ); istate->split_index->refcount =1 ; }
Signature · 1 line
gitgitgadget
Re: [PATCH 3/3] split-index: it really is incompatible with the sparse index
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 11 lines · writes:
From: Johannes Schindelin <[email protected]>
... at least for now. So let's error out if we are even trying to initialize the split index when the index is sparse, or when trying to write the split index extension for a sparse index.
Signed-off-by Johannes Schindelin <[email protected]>--- read-cache.c | 3 +++ split-index.c | 3 +++ 2 files changed, 6 insertions(+)
Good. Will queue.
Quoted 28 lines
read-cache.c +3
-0@@ -3009,6 +3009,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, !is_null_oid(&istate->split_index->base_oid)) {struct strbuf sb =STRBUF_INIT ; +if (istate->sparse_index) +die (_ ("cannot write split index for a sparse index" )); + err =write_link_extension (&sb, istate) <0 ||write_index_ext_header (f, eoie_c,CACHE_EXT_LINK , sb.len) <0 ;split-index.c +3
-0@@ -5,6 +5,9 @@struct split_index *init_split_index (struct index_state *istate) {if (!istate->split_index) { +if (istate->sparse_index) +die (_ ("cannot use split index with a sparse index" )); +CALLOC_ARRAY (istate->split_index,1 ); istate->split_index->refcount =1 ; }
Re: [PATCH 3/3] split-index: it really is incompatible with the sparse index
Hi Junio,
Quoted 9 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 6 lines · writes:
From: Johannes Schindelin <[email protected]>
... at least for now. So let's error out if we are even trying to initialize the split index when the index is sparse, or when trying to write the split index extension for a sparse index.
Quoted 7 lines
Quoted 5 lines
Signed-off-by Johannes Schindelin <[email protected]>--- read-cache.c | 3 +++ split-index.c | 3 +++ 2 files changed, 6 insertions(+)Good. Will queue.
Thank you!
Dscho
Quoted 6 lines · Johannes Schindelin via GitGitGadget
We noticed split/sparse index-related issues while rebasing Microsoft's fork of Git. These fixes are necessary for that fork's test suite to pass, but they might not be super critical to get into upstream v2.35.0 (especially this close to -rc2). However, the team felt that the decision should be left to the Git maintainer whether to take these patches into v2.35.0 or not.
Thanks for digging into these and putting in some guardrails to prevent similar issues. I hit similar things with some of my changes and had to fight t1091 to get it to pass in CI under GIT_TEST_SPLIT_INDEX=1. I don't recall seeing the specific error you mention in patch 1, but maybe I just overlooked it. I ended up finding little workarounds such as disabling sparse-checkouts at the end of various tests before new ones I was adding, and being extra careful to not leave the sparse-index selected. I probably should have dug further, but didn't. Thanks for digging in.
I've read over the patch series; it looks good to me:
Quoted 19 lines
Johannes Schindelin (3):
sparse-index: sparse index is disallowed when split index is active
t1091: disable split index
split-index: it really is incompatible with the sparse indexread-cache.c | 3 ++ sparse-index.c | 2 +- split-index.c | 3 ++ t/t1091-sparse-checkout-builtin.sh | 54 ++++++++++++++---------------- 4 files changed, 33 insertions(+), 29 deletions(-)base-commit:
af4e5f569bc89f356eb34a9373d7f82aca6faa8a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1119%2Fdscho%2Fsparse-index-vs-split-index-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1119/dscho/sparse-index-vs-split-index-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1119
--
gitgitgadget
Hi Elijah,
Quoted 9 lines · Elijah Newren
Quoted 6 lines · Johannes Schindelin via GitGitGadget
We noticed split/sparse index-related issues while rebasing Microsoft's fork of Git. These fixes are necessary for that fork's test suite to pass, but they might not be super critical to get into upstream v2.35.0 (especially this close to -rc2). However, the team felt that the decision should be left to the Git maintainer whether to take these patches into v2.35.0 or not.
Quoted 9 lines
Thanks for digging into these and putting in some guardrails to prevent similar issues. I hit similar things with some of my changes and had to fight t1091 to get it to pass in CI under GIT_TEST_SPLIT_INDEX=1. I don't recall seeing the specific error you mention in patch 1, but maybe I just overlooked it. I ended up finding little workarounds such as disabling sparse-checkouts at the end of various tests before new ones I was adding, and being extra careful to not leave the sparse-index selected. I probably should have dug further, but didn't. Thanks for digging in.
Seeing how much time it took to properly diagnose and fix these issues, I do not fault you ;-)
Quoted 3 lines
I've read over the patch series; it looks good to me:
Reviewed-by Elijah Newren <[email protected]>
Thank you!
Dscho