mirror of
https://github.com/git/git
synced 2024-08-25 02:35:46 +00:00
2 commits
Author | SHA1 | Message | Date | |
---|---|---|---|---|
![]() |
5581a019ba |
split-index: smudge and add racily clean cache entries to split index
Ever since the split index feature was introduced [1], refreshing a split index is prone to a variant of the classic racy git problem. Consider the following sequence of commands updating the split index when the shared index contains a racily clean cache entry, i.e. an entry whose cached stat data matches with the corresponding file in the worktree and the cached mtime matches that of the index: echo "cached content" >file git update-index --split-index --add file echo "dirty worktree" >file # size stays the same! # ... wait ... git update-index --add other-file Normally, when a non-split index is updated, then do_write_index() (the function responsible for writing all kinds of indexes, "regular", split, and shared) recognizes racily clean cache entries, and writes them with smudged stat data, i.e. with file size set to 0. When subsequent git commands read the index, they will notice that the smudged stat data doesn't match with the file in the worktree, and then go on to check the file's content and notice its dirtiness. In the above example, however, in the second 'git update-index' prepare_to_write_split_index() decides which cache entries stored only in the shared index should be replaced in the new split index. Alas, this function never looks out for racily clean cache entries, and since the file's stat data in the worktree hasn't changed since the shared index was written, it won't be replaced in the new split index. Consequently, do_write_index() doesn't even get this racily clean cache entry, and can't smudge its stat data. Subsequent git commands will then see that the index has more recent mtime than the file and that the (not smudged) cached stat data still matches with the file in the worktree, and, ultimately, will erroneously consider the file clean. Modify prepare_to_write_split_index() to recognize racily clean cache entries, and mark them to be added to the split index. Note that there are two places where it should check raciness: first those cache entries that are only stored in the shared index, and then those that have been copied by unpack_trees() from the shared index while it constructed a new index. This way do_write_index() will get these racily clean cache entries as well, and will then write them with smudged stat data to the new split index. This change makes all tests in 't1701-racy-split-index.sh' pass, so flip the two 'test_expect_failure' tests to success. Also add the '#' (as in nr. of trial) to those tests' description that were omitted when the tests expected failure. Note that after this change if the index is split when it contains a racily clean cache entry, then a smudged cache entry will be written both to the new shared and to the new split indexes. This doesn't affect regular git commands: as far as they are concerned this is just an entry in the split index replacing an outdated entry in the shared index. It did affect a few tests in 't1700-split-index.sh', though, because they actually check which entries are stored in the split index; a previous patch in this series has already made the necessary adjustments in 't1700'. And racily clean cache entries and index splitting are rare enough to not worry about the resulting duplicated smudged cache entries, and the additional complexity required to prevent them is not worth it. Several tests failed occasionally when the test suite was run with 'GIT_TEST_SPLIT_INDEX=yes'. Here are those that I managed to trace back to this racy split index problem, starting with those failing more frequently, with a link to a failing Travis CI build job for each. The highlighted line [2] shows when the racy file was written, which is not always in the failing test but in a preceeding setup test. t3903-stash.sh: https://travis-ci.org/git/git/jobs/385542084#L5858 t4024-diff-optimize-common.sh: https://travis-ci.org/git/git/jobs/386531969#L3174 t4015-diff-whitespace.sh: https://travis-ci.org/git/git/jobs/360797600#L8215 t2200-add-update.sh: https://travis-ci.org/git/git/jobs/382543426#L3051 t0090-cache-tree.sh: https://travis-ci.org/git/git/jobs/416583010#L3679 There might be others, e.g. perhaps 't1000-read-tree-m-3way.sh' and others using 'lib-read-tree-m-3way.sh', but I couldn't confirm yet. [1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge branch 'nd/split-index', 2014-07-16). [2] Note that those highlighted lines are in the 'after failure' fold, and your browser might unhelpfully fold it up before you could take a good look. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> |
||
![]() |
74e8addfaa |
split-index: add tests to demonstrate the racy split index problem
Ever since the split index feature was introduced [1], refreshing a split index is prone to a variant of the classic racy git problem. There are a couple of unrelated tests in the test suite that occasionally fail when run with 'GIT_TEST_SPLIT_INDEX=yes', but 't1700-split-index.sh', the only test script focusing solely on split index, has never noticed this issue, because it only cares about how the index is split under various circumstances and all the different ways to turn the split index feature on and off. Add a dedicated test script 't1701-racy-split-index.sh' to exercise the split index feature in racy situations as well; kind of a "t0010-racy-git.sh for split index" but with modern style (the tests do everything in &&-chained list of commands in 'test_expect_...' blocks, and use 'test_cmp' for more informative output on failure). The tests cover the following sequences of index splitting, updating, and racy file modifications, with the last two cases demonstrating the racy split index problem: 1. Split the index while adding a racily clean file: echo "cached content" >file git update-index --split-index --add file echo "dirty worktree" >file # size stays the same This case already works properly. Even though the cache entry's stat data matches with the modifid file in the worktree, subsequent git commands will notice that the (split) index and the file have the same mtime, and then will go on to check the file's content and notice its dirtiness. 2. Add a racily clean file to an already split index: git update-index --split-index echo "cached content" >file git update-index --add file echo "dirty worktree" >file This case already works properly. After the second 'git update-index' writes the newly added file's cache entry to the new split index, it basically works in the same way as case #1. 3. Split the index when it (i.e. the not yet splitted index) contains a racily clean cache entry, i.e. an entry whose cached stat data matches with the corresponding file in the worktree and the cached mtime matches that of the index: echo "cached content" >file git update-index --add file echo "dirty worktree" >file # ... wait ... git update-index --split-index --add other-file This case already works properly. The shared index is written by do_write_index(), i.e. the same function that is responsible for writing "regular" and split indexes as well. This function cleverly notices the racily clean cache entry, and writes the entry to the new shared index with smudged stat data, i.e. file size set to 0. When subsequent git commands read the index, they will notice that the smudged stat data doesn't match with the file in the worktree, and then go on to check the file's content and notice its dirtiness. 4. Update the split index when it contains a racily clean cache entry: git update-index --split-index echo "cached content" >file git update-index --add file echo "dirty worktree" >file # ... wait ... git update-index --add other-file This case already works properly. After the second 'git update-index' the newly added file's cache entry is only stored in the split index. If a cache entry is present in the split index (even if it is a replacement of an outdated entry in the shared index), then it will always be included in the new split index on subsequent split index updates (until the file is removed or a new shared index is written), independently from whether the entry is racily clean or not. When do_write_index() writes the new split index, it notices the racily clean cache entry, and smudges its stat date. Subsequent git commands reading the index will notice the smudged stat data and then go on to check the file's content and notice its dirtiness. 5. Update the split index when a racily clean cache entry is stored only in the shared index: echo "cached content" >file git update-index --split-index --add file echo "dirty worktree" >file # ... wait ... git update-index --add other-file This case fails due to the racy split index problem. In the second 'git update-index' prepare_to_write_split_index() decides, among other things, which cache entries stored only in the shared index should be replaced in the new split index. Alas, this function never looks out for racily clean cache entries, and since the file's stat data in the worktree hasn't changed since the shared index was written, the entry won't be replaced in the new split index. Consequently, do_write_index() doesn't even get this racily clean cache entry, and can't smudge its stat data. Subsequent git commands will then see that the index has more recent mtime than the file and that the (not smudged) cached stat data still matches with the file in the worktree, and, ultimately, will erroneously consider the file clean. 6. Update the split index after unpack_trees() copied a racily clean cache entry from the shared index: echo "cached content" >file git update-index --split-index --add file echo "dirty worktree" >file # ... wait ... git read-tree -m HEAD This case fails due to the racy split index problem. This basically fails for the same reason as case #5 above, but there is one important difference, which warrants the dedicated test. While that second 'git update-index' in case #5 updates index_state in place, in this case 'git read-tree -m' calls unpack_trees(), which throws out the entire index, and constructs a new one from the (potentially updated) copies of the original's cache entries. Consequently, when prepare_to_write_split_index() gets to work on this reconstructed index, it takes a different code path than in case #5 when deciding which cache entries in the shared index should be replaced. The result is the same, though: the racily clean cache entry goes unnoticed, it isn't added to the split index with smudged stat data, and subsequent git commands will then erroneously consider the file clean. Note that in the last two 'test_expect_failure' cases I omitted the '#' (as in nr. of trial) from the tests' description on purpose for now, as it breakes the TAP output [2]; it will be added at the end of the series, when those two tests will be flipped to 'test_expect_success'. [1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge branch 'nd/split-index', 2014-07-16). [2] In the TAP output a '#' should separate the test's description from the TODO directive emitted by 'test_expect_failure'. The additional '#' in "#$trial" interferes with this, the test harness won't recognize the TODO directive, and will report that those tests failed unexpectedly. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> |