git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: git@vger.kernel.org, "Thomas Gummerer" <t.gummerer@gmail.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: Re: [PATCH 5/6] tests: disable GIT_TEST_SPLIT_INDEX for sparse index tests
Date: Tue, 17 Aug 2021 23:32:01 +0200	[thread overview]
Message-ID: <20210817213201.GD2257957@szeder.dev> (raw)
In-Reply-To: <6f900a58-19b4-753c-18f5-852eaebc1527@gmail.com>

On Tue, Aug 17, 2021 at 02:26:23PM -0400, Derrick Stolee wrote:
> On 8/17/2021 1:49 PM, SZEDER Gábor wrote:
> > The sparse index and split index features are said to be currently
> > incompatible [1], and consequently GIT_TEST_SPLIT_INDEX=1 might
> > interfere with the test cases exercising the sparse index feature.
> > Therefore GIT_TEST_SPLIT_INDEX is already explicitly disabled for the
> > whole of 't1092-sparse-checkout-compatibility.sh'.  There are,
> > however, two other test cases exercising sparse index, namely
> > 'sparse-index enabled and disabled' in
> > 't1091-sparse-checkout-builtin.sh' and 'status succeeds with sparse
> > index' in 't7519-status-fsmonitor.sh', and these two could fail with
> > GIT_TEST_SPLIT_INDEX=1 as well [2].
> > 
> > Unset GIT_TEST_SPLIT_INDEX and disable the split index in these two
> > test cases to avoid such interference.
> > 
> > Note that this is the minimal change to merely avoid failures when
> > these test cases are run with GIT_TEST_SPLIT_INDEX=1.  Interestingly,
> > though, without these changes the 'git sparse-checkout init --cone
> > --sparse-index' commands still succeed even with split index, and set
> > all the necessary configuration variables and create the initial
> > '$GIT_DIR/info/sparse-checkout' file, but the test failures are caused
> > by later sanity checks finding that the index is not in fact a sparse
> > index.  This indicates that 'git sparse-checkout init --sparse-index'
> > lacks some error checking and its tests lack coverage related to split
> > index, but fixing those issues (let alone making sparse index
> > comparible with split index) is beyond the scope of this patch series.
> 
> s/comparible/compatible.
> 
> I agree that making these two things compatible is not something to
> solve today. I'm not sure they should _ever_ be solved because of
> the complexity involved (what if the base index is not sparse but
> the tip wants to be, or vice-versa?,

I think that this's not an issue, because a shared index file is not
for forever, but it's expected that a new shared index is written
every once in a while anyway, see splitIndex.maxPercentChange.  So
whenever sparse index gets enabled/disabled we could just write a new
shared index accordingly.  Maybe even when the sparse patterns change,
if necessary.

> or if a directory must be expanded because of a conflict?).

I'm not quite up-to-date in sparse index terminology, so I'm not sure
that this means...  but as mentioned above we can just write a new
shared index when deemed necessary.

> They use very different approaches
> to solve a similar problem: how to deal with large index files.
> 
> * The split index reduces index _write_ time by only editing a diff
>   of the base index.
> 
> * The sparse index reduces index _read and write_ time by writing a
>   smaller index, but only if the user is using cone mode sparse-
>   checkout.

Yeah, I think that in general there is more to be gained with sparse
index than with split index, though the split index might further
reduce the write time of a sparse index, because the amount of data
written is proportional with the nr of changed files instead of the nr
of all files in the sparse index.  I'm not sure that it's worth it,
either.

My remarks about compatibility primarily stem from your remarks about
compatibility in response to my sparse vs. split test failure report
in:

  https://public-inbox.org/git/48e9c3d6-407a-1843-2d91-22112410e3f8@gmail.com/

  "the sparse-index is (currently) incompatible with the split-index"

I assumed that that "(currently)" implies that eventually the two will
be made compatible.

Anyway, I'm fine with leaving them incompatible, but 'git
sparse-checkout' should still learn about split index, so it won't
pretend to succeed after it couldn't do what it was asked to when
there is a split index.  Maybe vice-versa as well.

> >  test_expect_success 'sparse-index enabled and disabled' '
> > -	git -C repo sparse-checkout init --cone --sparse-index &&
> > -	test_cmp_config -C repo true index.sparse &&
> > -	test-tool -C repo read-cache --table >cache &&
> > -	grep " tree " cache &&
> > -
> > -	git -C repo sparse-checkout disable &&
> > -	test-tool -C repo read-cache --table >cache &&
> > -	! grep " tree " cache &&
> > -	git -C repo config --list >config &&
> > -	! grep index.sparse config
> > +	(
> > +		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 &&
> > +		test-tool -C repo read-cache --table >cache &&
> > +		grep " tree " cache &&
> > +
> > +		git -C repo sparse-checkout disable &&
> > +		test-tool -C repo read-cache --table >cache &&
> > +		! grep " tree " cache &&
> > +		git -C repo config --list >config &&
> > +		! grep index.sparse config
> > +	)
> >  '
> 
> This test is safe for now.
> 
> >  test_expect_success 'status succeeds with sparse index' '
> 
> This test is being edited in ds/sparse-index-ignored-files. v3
> of the relevant patch was just sent today [1].
> 
> [1] https://lore.kernel.org/git/e66106f7a99d94145eec983ea5e72b7cf8a8a479.1629206603.git.gitgitgadget@gmail.com/
> 
> You might want to rebase on top of that topic. The edits to
> the test are likely stable now.

Oh, no :)  The test 'cone mode clears ignored subdirectories' added in
that patch series (as in 'seen', so not the newes version) fails with
the working GIT_TEST_SPLIT_INDEX=1 as well, and I don't immediately
see why, as it doesn't seem to use sparse index.


  reply	other threads:[~2021-08-17 21:32 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-17 17:49 [PATCH 0/6] Fix GIT_TEST_SPLIT_INDEX SZEDER Gábor
2021-08-17 17:49 ` [PATCH 1/6] t1600-index: remove unnecessary redirection SZEDER Gábor
2021-08-17 18:12   ` Derrick Stolee
2021-08-17 18:39     ` SZEDER Gábor
2021-08-17 18:48       ` Derrick Stolee
2021-08-17 17:49 ` [PATCH 2/6] t1600-index: don't run git commands upstream of a pipe SZEDER Gábor
2021-08-17 17:49 ` [PATCH 3/6] t1600-index: disable GIT_TEST_SPLIT_INDEX SZEDER Gábor
2021-08-17 17:49 ` [PATCH 4/6] read-cache: look for shared index files next to the index, too SZEDER Gábor
2021-08-17 17:49 ` [PATCH 5/6] tests: disable GIT_TEST_SPLIT_INDEX for sparse index tests SZEDER Gábor
2021-08-17 18:26   ` Derrick Stolee
2021-08-17 21:32     ` SZEDER Gábor [this message]
2021-08-18 18:51       ` Derrick Stolee
2021-08-17 17:49 ` [PATCH 6/6] read-cache: fix GIT_TEST_SPLIT_INDEX SZEDER Gábor
2021-08-17 18:29   ` Derrick Stolee
2021-08-26 20:59 ` [PATCH v2 0/6] Fix GIT_TEST_SPLIT_INDEX SZEDER Gábor
2021-08-26 20:59   ` [PATCH v2 1/6] t1600-index: remove unnecessary redirection SZEDER Gábor
2021-08-26 21:00   ` [PATCH v2 2/6] t1600-index: don't run git commands upstream of a pipe SZEDER Gábor
2021-08-26 21:00   ` [PATCH v2 3/6] t1600-index: disable GIT_TEST_SPLIT_INDEX SZEDER Gábor
2021-08-26 21:00   ` [PATCH v2 4/6] read-cache: look for shared index files next to the index, too SZEDER Gábor
2021-08-26 21:00   ` [PATCH v2 5/6] tests: disable GIT_TEST_SPLIT_INDEX for sparse index tests SZEDER Gábor
2021-08-26 21:00   ` [PATCH v2 6/6] read-cache: fix GIT_TEST_SPLIT_INDEX SZEDER Gábor
2021-08-31 14:47   ` [PATCH v2 0/6] Fix GIT_TEST_SPLIT_INDEX Derrick Stolee
2021-08-31 17:38     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210817213201.GD2257957@szeder.dev \
    --to=szeder.dev@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --cc=stolee@gmail.com \
    --cc=t.gummerer@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).