[0/3,Non-critical] : sparse index vs split index fixes

Message

Philippe Blain via GitGitGadget Jan. 19, 2022, 5:29 p.m. UTC

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

Comments

Elijah Newren Jan. 22, 2022, 5:44 a.m. UTC | #1

On Fri, Jan 21, 2022 at 11:53 AM Johannes Schindelin via GitGitGadget
<[e-mail]> wrote:
>
> 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:

Reviewed-by: Elijah Newren <[e-mail]>


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

Johannes Schindelin Jan. 22, 2022, 9:31 a.m. UTC | #2

Hi Elijah,

On Fri, 21 Jan 2022, Elijah Newren wrote:

> On Fri, Jan 21, 2022 at 11:53 AM Johannes Schindelin via GitGitGadget
> <[e-mail]> wrote:
> >
> > 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.

Seeing how much time it took to properly diagnose and fix these issues, I
do not fault you ;-)

> I've read over the patch series; it looks good to me:
>
> Reviewed-by: Elijah Newren <[e-mail]>

Thank you!
Dscho