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);
+
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 &&
# "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
> 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.
