On Mon, Jun 15, 2020 at 11:50:20PM +0200, SZEDER Gábor wrote:
> On Mon, Jun 15, 2020 at 05:27:15PM +0200, SZEDER Gábor wrote:
> > - Should we even allow 'splitIndex.sharedIndexExpire=now'?
> >
> > I believe, though haven't confirmed, that it can cause trouble
> > even without using an alternate index. Consider the following
> > sequence of events:
> >
> > - Git process A reads '.git/index', finds the 'link' extension,
> > and reads the SHA1 recorded there that determines the filename
> > of its shared index.
> >
> > - The scheduler steps in, and puts process A to sleep.
> >
> > - Git process B updates the index, decides that it's time to
> > write a new shared index, does so, and then because of
> > 'splitIndex.sharedIndexExpire=now' it removes all other shared
> > index files.
> >
> > - The scheduler wakes process A, which now tries to open the
> > shared index file it just learned about, but fails because
> > that file has just been removed by process B.
>
> Confirmed.
>
> To help reproduce the issue, this diff adds a strategically-placed
> controllable delay between reading '.git/index' and reading its
> shared/base index:
>
> diff --git a/read-cache.c b/read-cache.c
> index b888c5df44..5a66e9bf4b 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2319,6 +2319,9 @@ int read_index_from(struct index_state *istate, const char *path,
> else
> split_index->base = xcalloc(1, sizeof(*split_index->base));
>
> + if (git_env_bool("GIT_TEST_WAIT", 0))
> + sleep(3);
We don't even need this sleep() call ...
> base_oid_hex = oid_to_hex(&split_index->base_oid);
> base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
> trace2_region_enter_printf("index", "shared/do_read_index",
>
>
> Then this test creates the above described sequence of events:
>
> test_expect_failure 'splitIndex.sharedIndexExpire=now can be harmful' '
> >file1 &&
> >file2 &&
> git update-index --split-index --add file1 &&
>
> {
> sleep 1 &&
... and this 'sleep 1' command.
> # "process B"
> git -c splitIndex.sharedIndexExpire=now \
> update-index --split-index --add file2 &
> } &&
>
> # "process A"
> GIT_TEST_WAIT=1 git diff --cached --name-only
> '
>
> ... and fails reliably with:
>
> [...]
> + GIT_TEST_WAIT=1 git diff --cached --name-only
> [ ... trace from background commands removed ...]
> fatal: .git/sharedindex.818f65852e7402f236aeaadd32efdbb62291aa75: index file open failed: No such file or directory
Flipping the test to 'test_expect_success' and running it with
'--stress' almost always fails within 2 seconds.
> > This is similar to the issue we have with 'git gc --prune=now',
> > except that 'git gc's documentation explicitly warns about the
> > risks of using '--prune=now', while the description of
> > 'splitIndex.sharedIndexExpire' doesn't have any such warning.
> >
> > I think that 'splitIndex.sharedIndexExpire=now' should be allowed,
> > for those who hopefully know what they are doing, just as we allow
> > 'git gc --prune=now', but the documentation should clearly warn
> > against its potential pitfalls.
