{"thread":{"id":"49437","subject":"[PATCH v2 0/5] Fix the racy split index problem","startedAt":"2018-09-27T12:44:47Z","lastAt":"2018-10-12T03:20:39Z","messageCount":42,"participants":["SZEDER Gábor","Ævar Arnfjörð Bjarmason","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"359042","messageId":"20180927124434.30835-1-szeder.dev@gmail.com","threadId":"49437","inReplyTo":null,"subject":"[PATCH v2 0/5] Fix the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-27T12:44:29Z","receivedAt":"2018-09-27T12:44:47Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This is the second attempt to fix the racy split index problem, which\ncauses occasional failures in several random test scripts when run\nwith 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches\n1 and 5 (corresponding to v1's 3 and 5).\n\nChanges since v1:\n\n - Most importanly, patch 5 adds a second is_racy_timestamp()\n condition to prepare_to_write_split_index() to deal with those\n racily clean cache entries that were copied from the shared index\n by unpack_trees() while it created a new index.\n This should fix the remainig failures in 't3903-stash.sh' with\n split index enabled.\n\n Furthermore, together with patch 4 they add a bunch of comments to\n the most (alas, not all) conditions in\n prepare_to_write_split_index()'s first loop, trying to explain in\n plain English which cache entries are handled by each of those\n conditions. I might have gone a bit overboard with the comments,\n but I think they would have helped me a lot while working on these\n fixes...\n\n - Patch 1 adds one more failing test to demonstrate the raciness\n after unpack_trees() created a new index.\n\n Updated some of the comments in those tests, and I also reordered\n some tests, because I found that it's more logical to explain them\n in this order. (However, this made 'range-diff' think that it's a\n total rewrite, which is not really true... but even though I could\n convince it otherwise by adjusting the --creation-factor, the\n results were utterly unreadable.)\n\n - Patch 4 is new and... interesting? I'd like to hear Duy's\n thoughts on this one.\n\n - Patch 3 is new and... I hesitate to call it a bugfix, because,\n luckily, there is no real and visible bug, but it's a fix\n nonetheless.\n\n - Commit message updates here and there.\n\n - Addressed Ævar's nits in patch 2.\n\n - The first two patches from v1 have already been merged to\n 'master' (cff90bdc5c (Merge branch 'sg/split-index-test',\n 2018-09-24)).\n\n\nSZEDER Gábor (5):\n split-index: add tests to demonstrate the racy split index problem\n t1700-split-index: date back files to avoid racy situations\n split-index: count the number of deleted entries\n split-index: don't compare stat data of entries already marked for\n split index\n split-index: smudge and add racily clean cache entries to split index\n\n cache.h | 2 +\n read-cache.c | 2 +-\n split-index.c | 121 +++++++++++++++++---\n t/t1700-split-index.sh | 49 +++++----\n t/t1701-racy-split-index.sh | 214 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 348 insertions(+), 40 deletions(-)\n create mode 100755 t/t1701-racy-split-index.sh\n\nRange-diff:\n1: 71d973e4c0 < -: ---------- t1700-split-index: drop unnecessary 'grep'\n2: 5d46f58a07 < -: ---------- t0090: disable GIT_TEST_SPLIT_INDEX for the test checking split index\n3: e60fa2dd9e < -: ---------- split index: add a test to demonstrate the racy split index problem\n-: ---------- > 1: a5b46af0ff split-index: add tests to demonstrate the racy split index problem\n4: 799c9c7739 ! 2: ed26c9703e t1700-split-index: date back files to avoid racy situations\n @@ -43,7 +43,7 @@\n \n +# Create a file named as $1 with content read from stdin.\n +# Set the file's mtime to a few seconds in the past to avoid racy situations.\n -+create_file () {\n ++create_non_racy_file () {\n +\tcat >\"$1\" &&\n +\ttest-tool chmtime =-5 \"$1\"\n +}\n @@ -56,7 +56,7 @@\n \n test_expect_success 'add one file' '\n -\t: >one &&\n -+\tcreate_file one &&\n ++\tcreate_non_racy_file one &&\n \tgit update-index --add one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n @@ -65,7 +65,7 @@\n \n test_expect_success 'modify original file, base index untouched' '\n -\techo modified >one &&\n -+\techo modified |create_file one &&\n ++\techo modified | create_non_racy_file one &&\n \tgit update-index one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n @@ -74,7 +74,7 @@\n \n test_expect_success 'add another file, which stays index' '\n -\t: >two &&\n -+\tcreate_file two &&\n ++\tcreate_non_racy_file two &&\n \tgit update-index --add two &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n @@ -83,7 +83,7 @@\n \n test_expect_success 'add original file back' '\n -\t: >one &&\n -+\tcreate_file one &&\n ++\tcreate_non_racy_file one &&\n \tgit update-index --add one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n @@ -92,7 +92,7 @@\n \n test_expect_success 'add new file' '\n -\t: >two &&\n -+\tcreate_file two &&\n ++\tcreate_non_racy_file two &&\n \tgit update-index --add two &&\n \tgit ls-files --stage >actual &&\n \tcat >expect <<-EOF &&\n @@ -101,7 +101,7 @@\n test_expect_success 'set core.splitIndex config variable to true' '\n \tgit config core.splitIndex true &&\n -\t: >three &&\n -+\tcreate_file three &&\n ++\tcreate_non_racy_file three &&\n \tgit update-index --add three &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n @@ -113,7 +113,7 @@\n +test_expect_success 'set core.splitIndex config variable back to true' '\n \tgit config core.splitIndex true &&\n -\t: >three &&\n -+\tcreate_file three &&\n ++\tcreate_non_racy_file three &&\n \tgit update-index --add three &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n @@ -122,7 +122,7 @@\n \tEOF\n \ttest_cmp expect actual &&\n -\t: >four &&\n -+\tcreate_file four &&\n ++\tcreate_non_racy_file four &&\n \tgit update-index --add four &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n \tcat >expect <<-EOF &&\n @@ -131,7 +131,7 @@\n test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '\n \tgit config --unset splitIndex.maxPercentChange &&\n -\t: >five &&\n -+\tcreate_file five &&\n ++\tcreate_non_racy_file five &&\n \tgit update-index --add five &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n @@ -140,7 +140,7 @@\n \tEOF\n \ttest_cmp expect actual &&\n -\t: >six &&\n -+\tcreate_file six &&\n ++\tcreate_non_racy_file six &&\n \tgit update-index --add six &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n \tcat >expect <<-EOF &&\n @@ -149,7 +149,7 @@\n test_expect_success 'check splitIndex.maxPercentChange set to 0' '\n \tgit config splitIndex.maxPercentChange 0 &&\n -\t: >seven &&\n -+\tcreate_file seven &&\n ++\tcreate_non_racy_file seven &&\n \tgit update-index --add seven &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n @@ -158,7 +158,7 @@\n \tEOF\n \ttest_cmp expect actual &&\n -\t: >eight &&\n -+\tcreate_file eight &&\n ++\tcreate_non_racy_file eight &&\n \tgit update-index --add eight &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n @@ -167,19 +167,19 @@\n \n test_expect_success 'shared index files expire after 2 weeks by default' '\n -\t: >ten &&\n -+\tcreate_file ten &&\n ++\tcreate_non_racy_file ten &&\n \tgit update-index --add ten &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_under_2_weeks_ago=$((5-14*86400)) &&\n \ttest-tool chmtime =$just_under_2_weeks_ago .git/sharedindex.* &&\n -\t: >eleven &&\n -+\tcreate_file eleven &&\n ++\tcreate_non_racy_file eleven &&\n \tgit update-index --add eleven &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_over_2_weeks_ago=$((-1-14*86400)) &&\n \ttest-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&\n -\t: >twelve &&\n -+\tcreate_file twelve &&\n ++\tcreate_non_racy_file twelve &&\n \tgit update-index --add twelve &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n @@ -188,13 +188,13 @@\n \tgit config splitIndex.sharedIndexExpire \"16.days.ago\" &&\n \ttest-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&\n -\t: >thirteen &&\n -+\tcreate_file thirteen &&\n ++\tcreate_non_racy_file thirteen &&\n \tgit update-index --add thirteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_over_16_days_ago=$((-1-16*86400)) &&\n \ttest-tool chmtime =$just_over_16_days_ago .git/sharedindex.* &&\n -\t: >fourteen &&\n -+\tcreate_file fourteen &&\n ++\tcreate_non_racy_file fourteen &&\n \tgit update-index --add fourteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n @@ -203,14 +203,14 @@\n \tjust_10_years_ago=$((-365*10*86400)) &&\n \ttest-tool chmtime =$just_10_years_ago .git/sharedindex.* &&\n -\t: >fifteen &&\n -+\tcreate_file fifteen &&\n ++\tcreate_non_racy_file fifteen &&\n \tgit update-index --add fifteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tgit config splitIndex.sharedIndexExpire now &&\n \tjust_1_second_ago=-1 &&\n \ttest-tool chmtime =$just_1_second_ago .git/sharedindex.* &&\n -\t: >sixteen &&\n -+\tcreate_file sixteen &&\n ++\tcreate_non_racy_file sixteen &&\n \tgit update-index --add sixteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n @@ -219,7 +219,7 @@\n \t\tgit config core.sharedrepository \"$mode\" &&\n \t\tgit config core.splitIndex true &&\n -\t\t: >one &&\n -+\t\tcreate_file one &&\n ++\t\tcreate_non_racy_file one &&\n \t\tgit update-index --add one &&\n \t\techo \"$modebits\" >expect &&\n \t\ttest_modebits .git/index >actual &&\n-: ---------- > 3: 899e5090d3 split-index: count the number of deleted entries\n-: ---------- > 4: 02646f1a1b split-index: don't compare stat data of entries already marked for split index\n5: 38b5f0fe72 ! 5: 290ac26055 split-index: smudge and add racily clean cache entries to split index\n @@ -22,25 +22,29 @@\n them with smudged stat data, i.e. with file size set to 0. When\n subsequent git commands read the index, they will notice that the\n smudged stat data doesn't match with the file in the worktree, and\n - then go on to check the file's content.\n + then go on to check the file's content and notice its dirtiness.\n \n In the above example, however, in the second 'git update-index'\n - prepare_to_write_split_index() gathers all cache entries that should\n - be written to the new split index. Alas, this function never looks\n - out for racily clean cache entries, and since the file's stat data in\n - the worktree hasn't changed since the shared index was written, it\n - won't be replaced in the new split index. Consequently,\n - do_write_index() doesn't even get this racily clean cache entry, and\n - can't smudge its stat data. Subsequent git commands will then see\n - that the index has more recent mtime than the file and that the (not\n - smudged) cached stat data still matches with the file in the worktree,\n - and, ultimately, will erroneously consider the file clean.\n + prepare_to_write_split_index() decides which cache entries stored only\n + in the shared index should be replaced in the new split index. Alas,\n + this function never looks out for racily clean cache entries, and\n + since the file's stat data in the worktree hasn't changed since the\n + shared index was written, it won't be replaced in the new split index.\n + Consequently, do_write_index() doesn't even get this racily clean\n + cache entry, and can't smudge its stat data. Subsequent git commands\n + will then see that the index has more recent mtime than the file and\n + that the (not smudged) cached stat data still matches with the file in\n + the worktree, and, ultimately, will erroneously consider the file\n + clean.\n \n Modify prepare_to_write_split_index() to recognize racily clean cache\n - entries, and mark them to be added to the split index. This way\n - do_write_index() will get these racily clean cache entries as well,\n - and will then write them with smudged stat data to the new split\n - index.\n + entries, and mark them to be added to the split index. Note that\n + there are two places where it should check raciness: first those cache\n + entries that are only stored in the shared index, and then those that\n + have been copied by unpack_trees() from the shared index while it\n + constructed a new index. This way do_write_index() will get these\n + racily clean cache entries as well, and will then write them with\n + smudged stat data to the new split index.\n \n Note that after this change if the index is split when it contains a\n racily clean cache entry, then a smudged cache entry will be written\n @@ -49,19 +53,19 @@\n an entry in the split index replacing an outdated entry in the shared\n index. It did affect a few tests in 't1700-split-index.sh', though,\n because they actually check which entries are stored in the split\n - index; the previous patch made the necessary adjustments. And racily\n - clean cache entries and index splitting are rare enough to not worry\n - about the resulting duplicated smudged cache entries, and the\n - additional complexity required to prevent them is not worth it.\n + index; a previous patch in this series has already made the necessary\n + adjustments in 't1700'. And racily clean cache entries and index\n + splitting are rare enough to not worry about the resulting duplicated\n + smudged cache entries, and the additional complexity required to\n + prevent them is not worth it.\n \n Several tests failed occasionally when the test suite was run with\n 'GIT_TEST_SPLIT_INDEX=yes'. Here are those that I managed to trace\n back to this racy split index problem, starting with those failing\n more frequently, with a link to a failing Travis CI build job for\n - each. The highlighted line shows when the racy file was written,\n - which is not always in the failing test (but note that those lines are\n - in the 'after failure' fold, and your browser might unhelpfully fold\n - it up before you could take a good look).\n + each. The highlighted line [2] shows when the racy file was written,\n + which is not always in the failing test but in a preceeding setup\n + test.\n \n t3903-stash.sh:\n https://travis-ci.org/git/git/jobs/385542084#L5858\n @@ -75,7 +79,7 @@\n t2200-add-update.sh:\n https://travis-ci.org/git/git/jobs/382543426#L3051\n \n - t0090-cache-tree:\n + t0090-cache-tree.sh:\n https://travis-ci.org/git/git/jobs/416583010#L3679\n \n There might be others, e.g. perhaps 't1000-read-tree-m-3way.sh' and\n @@ -84,6 +88,10 @@\n [1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge\n branch 'nd/split-index', 2014-07-16).\n \n + [2] Note that those highlighted lines are in the 'after failure' fold,\n + and your browser might unhelpfully fold it up before you could\n + take a good look.\n +\n Signed-off-by: SZEDER Gábor \n \n diff --git a/cache.h b/cache.h\n @@ -121,39 +129,94 @@\n \t\t\tbase = si->base->cache[ce->index - 1];\n -\t\t\tif (ce == base)\n +\t\t\tif (ce == base) {\n -+\t\t\t\t/*\n -+\t\t\t\t * Racily clean cache entries must be\n -+\t\t\t\t * written to the split index, so the\n -+\t\t\t\t * subsequent do_write_index() can smudge\n -+\t\t\t\t * their stat data.\n -+\t\t\t\t */\n -+\t\t\t\tif (!ce_uptodate(ce) &&\n -+\t\t\t\t is_racy_timestamp(istate, ce))\n ++\t\t\t\t/* The entry is present in the shared index. */\n ++\t\t\t\tif (ce->ce_flags & CE_UPDATE_IN_BASE) {\n ++\t\t\t\t\t/*\n ++\t\t\t\t\t * Already marked for inclusion in\n ++\t\t\t\t\t * the split index, either because\n ++\t\t\t\t\t * the corresponding file was\n ++\t\t\t\t\t * modified and the cached stat data\n ++\t\t\t\t\t * was refreshed, or because there\n ++\t\t\t\t\t * is already a replacement entry in\n ++\t\t\t\t\t * the split index.\n ++\t\t\t\t\t * Nothing more to do here.\n ++\t\t\t\t\t */\n ++\t\t\t\t} else if (!ce_uptodate(ce) &&\n ++\t\t\t\t\t is_racy_timestamp(istate, ce)) {\n ++\t\t\t\t\t/*\n ++\t\t\t\t\t * A racily clean cache entry stored\n ++\t\t\t\t\t * only in the shared index: it must\n ++\t\t\t\t\t * be added to the split index, so\n ++\t\t\t\t\t * the subsequent do_write_index()\n ++\t\t\t\t\t * can smudge its stat data.\n ++\t\t\t\t\t */\n +\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n ++\t\t\t\t} else {\n ++\t\t\t\t\t/*\n ++\t\t\t\t\t * The entry is only present in the\n ++\t\t\t\t\t * shared index and it was not\n ++\t\t\t\t\t * refreshed.\n ++\t\t\t\t\t * Just leave it there.\n ++\t\t\t\t\t */\n ++\t\t\t\t}\n \t\t\t\tcontinue;\n +\t\t\t}\n \t\t\tif (ce->ce_namelen != base->ce_namelen ||\n \t\t\t strcmp(ce->name, base->name)) {\n \t\t\t\tce->index = 0;\n +@@\n + \t\t\t\t * the split index.\n + \t\t\t\t * Nothing to do.\n + \t\t\t\t */\n ++\t\t\t} else if (!ce_uptodate(ce) &&\n ++\t\t\t\t is_racy_timestamp(istate, ce)) {\n ++\t\t\t\t/*\n ++\t\t\t\t * A copy of a racily clean cache entry from\n ++\t\t\t\t * the shared index. It must be added to\n ++\t\t\t\t * the split index, so the subsequent\n ++\t\t\t\t * do_write_index() can smudge its stat data.\n ++\t\t\t\t */\n ++\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n + \t\t\t} else {\n + \t\t\t\t/*\n + \t\t\t\t * Thoroughly compare the cached data to see\n \n diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\n --- a/t/t1701-racy-split-index.sh\n +++ b/t/t1701-racy-split-index.sh\n @@\n \n - for trial in 0 1 2 3 4\n + for trial in $trials\n do\n --\ttest_expect_failure \"update the split index when the shared index contains a racily clean cache entry #$trial\" '\n -+\ttest_expect_success \"update the split index when the shared index contains a racily clean cache entry #$trial\" '\n - \t\ttest_when_finished \"rm -f .git/index .git/sharedindex.*\" &&\n +-\ttest_expect_failure \"update the split index when a racily clean cache entry is stored only in the shared index $trial\" '\n ++\ttest_expect_success \"update the split index when a racily clean cache entry is stored only in the shared index #$trial\" '\n + \t\trm -f .git/index .git/sharedindex.* &&\n \n \t\t# The next three commands must be run within the same\n @@\n - \t\t# corresponding replacement cache entry with smudged\n - \t\t# stat data should be added to the new split index, so\n - \t\t# the file wont appear clean for subsequent git commands.\n + \t\t# entry of racy-file is only stored in the shared index.\n + \t\t# A corresponding replacement cache entry with smudged\n + \t\t# stat data should be added to the new split index.\n -\t\t#\n -\t\t# Alas, such a smudged replacement entry is not added!\n \t\tgit update-index --add other-file &&\n \n - \t\tcheck_cached_diff\n + \t\t# Subsequent git commands should notice the smudged\n +@@\n + \n + for trial in $trials\n + do\n +-\ttest_expect_failure \"update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial\" '\n ++\ttest_expect_success \"update the split index after unpack trees() copied a racily clean cache entry from the shared index #$trial\" '\n + \t\trm -f .git/index .git/sharedindex.* &&\n + \n + \t\t# The next three commands must be run within the same\n +@@\n + \t\t# index. A corresponding replacement cache entry\n + \t\t# with smudged stat data should be added to the new\n + \t\t# split index.\n +-\t\t#\n +-\t\t# Alas, such a smudged replacement entry is not added!\n + \t\tgit read-tree -m HEAD &&\n + \n + \t\t# Subsequent git commands should notice the smudged\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359043","messageId":"20180927124434.30835-2-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180927124434.30835-1-szeder.dev@gmail.com","subject":"[PATCH v2 1/5] split-index: add tests to demonstrate the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-27T12:44:30Z","receivedAt":"2018-09-27T12:44:55Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Ever since the split index feature was introduced [1], refreshing a\nsplit index is prone to a variant of the classic racy git problem.\nThere are a couple of unrelated tests in the test suite that\noccasionally fail when run with 'GIT_TEST_SPLIT_INDEX=yes', but\n't1700-split-index.sh', the only test script focusing solely on split\nindex, has never noticed this issue, because it only cares about how\nthe index is split under various circumstances and all the different\nways to turn the split index feature on and off.\n\nAdd a dedicated test script 't1701-racy-split-index.sh' to exercise\nthe split index feature in racy situations as well; kind of a\n\"t0010-racy-git.sh for split index\" but with modern style (the tests\ndo everything in &&-chained list of commands in 'test_expect_...'\nblocks, and use 'test_cmp' for more informative output on failure).\n\nThe tests cover the following sequences of index splitting, updating,\nand racy file modifications, with the last two cases demonstrating the\nracy split index problem:\n\n 1. Split the index while adding a racily clean file:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file # size stays the same\n\n This case already works properly. Even though the cache entry's\n stat data matches with the modifid file in the worktree,\n subsequent git commands will notice that the (split) index and\n the file have the same mtime, and then will go on to check the\n file's content and notice its dirtiness.\n\n 2. Add a racily clean file to an already split index:\n\n git update-index --split-index\n echo \"cached content\" >file\n git update-index --add file\n echo \"dirty worktree\" >file\n\n This case already works properly. After the second 'git\n update-index' writes the newly added file's cache entry to the\n new split index, it basically works in the same way as case #1.\n\n 3. Split the index when it (i.e. the not yet splitted index)\n contains a racily clean cache entry, i.e. an entry whose cached\n stat data matches with the corresponding file in the worktree and\n the cached mtime matches that of the index:\n\n echo \"cached content\" >file\n git update-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git update-index --split-index --add other-file\n\n This case already works properly. The shared index is written by\n do_write_index(), i.e. the same function that is responsible for\n writing \"regular\" and split indexes as well. This function\n cleverly notices the racily clean cache entry, and writes the\n entry to the new shared index with smudged stat data, i.e. file\n size set to 0. When subsequent git commands read the index, they\n will notice that the smudged stat data doesn't match with the\n file in the worktree, and then go on to check the file's content\n and notice its dirtiness.\n\n 4. Update the split index when it contains a racily clean cache\n entry:\n\n git update-index --split-index\n echo \"cached content\" >file\n git update-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git update-index --add other-file\n\n This case already works properly. After the second 'git\n update-index' the newly added file's cache entry is only stored\n in the split index. If a cache entry is present in the split\n index (even if it is a replacement of an outdated entry in the\n shared index), then it will always be included in the new split\n index on subsequent split index updates (until the file is\n removed or a new shared index is written), independently from\n whether the entry is racily clean or not. When do_write_index()\n writes the new split index, it notices the racily clean cache\n entry, and smudges its stat date. Subsequent git commands\n reading the index will notice the smudged stat data and then go\n on to check the file's content and notice its dirtiness.\n\n 5. Update the split index when a racily clean cache entry is stored\n only in the shared index:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git update-index --add other-file\n\n This case fails due to the racy split index problem. In the\n second 'git update-index' prepare_to_write_split_index() decides,\n among other things, which cache entries stored only in the shared\n index should be replaced in the new split index. Alas, this\n function never looks out for racily clean cache entries, and\n since the file's stat data in the worktree hasn't changed since\n the shared index was written, the entry won't be replaced in the\n new split index. Consequently, do_write_index() doesn't even get\n this racily clean cache entry, and can't smudge its stat data.\n Subsequent git commands will then see that the index has more\n recent mtime than the file and that the (not smudged) cached stat\n data still matches with the file in the worktree, and,\n ultimately, will erroneously consider the file clean.\n\n 6. Update the split index after unpack_trees() copied a racily clean\n cache entry from the shared index:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git read-tree -m HEAD\n\n This case fails due to the racy split index problem. This\n basically fails for the same reason as case #5 above, but there\n is one important difference, which warrants the dedicated test.\n While that second 'git update-index' in case #5 updates\n index_state in place, in this case 'git read-tree -m' calls\n unpack_trees(), which throws out the entire index, and constructs\n a new one from the (potentially updated) copies of the original's\n cache entries. Consequently, when prepare_to_write_split_index()\n gets to work on this reconstructed index, it takes a different\n code path than in case #5 when deciding which cache entries in\n the shared index should be replaced. The result is the same,\n though: the racily clean cache entry goes unnoticed, it isn't\n added to the split index with smudged stat data, and subsequent\n git commands will then erroneously consider the file clean.\n\nNote that in the last two 'test_expect_failure' cases I omitted the\n'#' (as in nr. of trial) from the tests' name on purpose for now, as\nit confuses 'prove' into thinking that those tests failed\nunexpectedly.\n\n[1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge\n branch 'nd/split-index', 2014-07-16).\n\nSigned-off-by: SZEDER Gábor \n---\n t/t1701-racy-split-index.sh | 218 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 218 insertions(+)\n create mode 100755 t/t1701-racy-split-index.sh\n\ndiff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\nnew file mode 100755\nindex 0000000000..ebde418d7e\n--- /dev/null\n+++ b/t/t1701-racy-split-index.sh\n@@ -0,0 +1,218 @@\n+#!/bin/sh\n+\n+# This test can give false success if your machine is sufficiently\n+# slow or all trials happened to happen on second boundaries.\n+\n+test_description='racy split index'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\t# Only split the index when the test explicitly says so.\n+\tsane_unset GIT_TEST_SPLIT_INDEX GIT_FSMONITOR_TEST &&\n+\tgit config splitIndex.maxPercentChange 100 &&\n+\n+\techo \"cached content\" >racy-file &&\n+\tgit add racy-file &&\n+\tgit commit -m initial &&\n+\n+\techo something >other-file &&\n+\t# No raciness with this file.\n+\ttest-tool chmtime =-20 other-file &&\n+\n+\techo \"+cached content\" >expect\n+'\n+\n+check_cached_diff () {\n+\tgit diff-index --patch --cached $EMPTY_TREE racy-file >diff &&\n+\ttail -1 diff >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+trials=\"0 1 2 3 4\"\n+for trial in $trials\n+do\n+\ttest_expect_success \"split the index while adding a racily clean file #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second (so both writes to racy-file result in the same\n+\t\t# mtime) to create the interesting racy situation.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update and split the index. The cache entry of\n+\t\t# racy-file will be stored only in the shared index.\n+\t\tgit update-index --split-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Subsequent git commands should notice that racy-file\n+\t\t# and the split index have the same mtime, and check\n+\t\t# the content of the file to see if it is actually\n+\t\t# clean.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_success \"add a racily clean file to an already split index #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\tgit update-index --split-index &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update the split index. The cache entry of racy-file\n+\t\t# will be stored only in the split index.\n+\t\tgit update-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Subsequent git commands should notice that racy-file\n+\t\t# and the split index have the same mtime, and check\n+\t\t# the content of the file to see if it is actually\n+\t\t# clean.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_success \"split the index when the index contains a racily clean cache entry #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\tgit update-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update and split the index when the index contains\n+\t\t# the racily clean cache entry of racy-file.\n+\t\t# A corresponding replacement cache entry with smudged\n+\t\t# stat data should be added to the new split index.\n+\t\tgit update-index --split-index --add other-file &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data in the replacement cache entry and that it\n+\t\t# doesnt match with the file the worktree.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_success \"update the split index when it contains a new racily clean cache entry #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\tgit update-index --split-index &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update the split index. The cache entry of racy-file\n+\t\t# will be stored only in the split index.\n+\t\tgit update-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update the split index when the racily clean cache\n+\t\t# entry of racy-file is only stored in the split index.\n+\t\t# An updated cache entry with smudged stat data should\n+\t\t# be added to the new split index.\n+\t\tgit update-index --add other-file &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_failure \"update the split index when a racily clean cache entry is stored only in the shared index $trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update and split the index. The cache entry of\n+\t\t# racy-file will be stored only in the shared index.\n+\t\tgit update-index --split-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update the split index when the racily clean cache\n+\t\t# entry of racy-file is only stored in the shared index.\n+\t\t# A corresponding replacement cache entry with smudged\n+\t\t# stat data should be added to the new split index.\n+\t\t#\n+\t\t# Alas, such a smudged replacement entry is not added!\n+\t\tgit update-index --add other-file &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_failure \"update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update and split the index. The cache entry of\n+\t\t# racy-file will be stored only in the shared index.\n+\t\tgit update-index --split-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update the split index after unpack_trees() copied the\n+\t\t# racily clean cache entry of racy-file from the shared\n+\t\t# index. A corresponding replacement cache entry\n+\t\t# with smudged stat data should be added to the new\n+\t\t# split index.\n+\t\t#\n+\t\t# Alas, such a smudged replacement entry is not added!\n+\t\tgit read-tree -m HEAD &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+test_done\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359044","messageId":"20180927124434.30835-3-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180927124434.30835-1-szeder.dev@gmail.com","subject":"[PATCH v2 2/5] t1700-split-index: date back files to avoid racy situations","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-27T12:44:31Z","receivedAt":"2018-09-27T12:45:02Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'t1700-split-index.sh' checks that the index was split correctly under\nvarious circumstances and that all the different ways to turn the\nsplit index feature on and off work correctly. To do so, most of its\ntests use 'test-tool dump-split-index' to see which files have their\ncache entries in the split index. All these tests assume that all\ncache entries are written to the shared index (called \"base\"\nthroughout these tests) when a new shared index is created. This is\nan implementation detail: most git commands (basically all except 'git\nupdate-index') don't care or know at all about split index or whether\na cache entry is stored in the split or shared index.\n\nAs demonstrated in the previous patch, refreshing a split index is\nprone to a variant of the classic racy git issue. The next patch will\nfix this issue, but while doing so it will also slightly change this\nbehaviour: only cache entries with mtime in the past will be written\nonly to the newly created shared index, but racily clean cache entries\nwill be written to the new split index (with smudged stat data).\n\nWhile this upcoming change won't at all affect any git commands, it\nwill violate the above mentioned assumption of 't1700's tests. Since\nthese tests create or modify files and create or refresh the split\nindex in rapid succession, there are plenty of racily clean cache\nentries to be dealt with, which will then be written to the new split\nindexes, and, ultimately, will cause several tests in 't1700' to fail.\n\nLet's prepare 't1700-split-index.sh' for this upcoming change and\nmodify its tests to avoid racily clean files by backdating the mtime\nof any file modifications (and since a lot of tests create or modify\nfiles, encapsulate it into a helper function).\n\nSigned-off-by: SZEDER Gábor \n---\n t/t1700-split-index.sh | 49 ++++++++++++++++++++++++------------------\n 1 file changed, 28 insertions(+), 21 deletions(-)\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex be22398a85..c8acbc50d0 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -8,6 +8,13 @@ test_description='split index mode tests'\n sane_unset GIT_TEST_SPLIT_INDEX\n sane_unset GIT_FSMONITOR_TEST\n \n+# Create a file named as $1 with content read from stdin.\n+# Set the file's mtime to a few seconds in the past to avoid racy situations.\n+create_non_racy_file () {\n+\tcat >\"$1\" &&\n+\ttest-tool chmtime =-5 \"$1\"\n+}\n+\n test_expect_success 'enable split index' '\n \tgit config splitIndex.maxPercentChange 100 &&\n \tgit update-index --split-index &&\n@@ -31,7 +38,7 @@ test_expect_success 'enable split index' '\n '\n \n test_expect_success 'add one file' '\n-\t: >one &&\n+\tcreate_non_racy_file one &&\n \tgit update-index --add one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -83,7 +90,7 @@ test_expect_success 'enable split index again, \"one\" now belongs to base index\"'\n '\n \n test_expect_success 'modify original file, base index untouched' '\n-\techo modified >one &&\n+\techo modified | create_non_racy_file one &&\n \tgit update-index one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -102,7 +109,7 @@ test_expect_success 'modify original file, base index untouched' '\n '\n \n test_expect_success 'add another file, which stays index' '\n-\t: >two &&\n+\tcreate_non_racy_file two &&\n \tgit update-index --add two &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -155,7 +162,7 @@ test_expect_success 'remove file in base index' '\n '\n \n test_expect_success 'add original file back' '\n-\t: >one &&\n+\tcreate_non_racy_file one &&\n \tgit update-index --add one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -174,7 +181,7 @@ test_expect_success 'add original file back' '\n '\n \n test_expect_success 'add new file' '\n-\t: >two &&\n+\tcreate_non_racy_file two &&\n \tgit update-index --add two &&\n \tgit ls-files --stage >actual &&\n \tcat >expect <<-EOF &&\n@@ -218,7 +225,7 @@ test_expect_success 'rev-parse --shared-index-path' '\n \n test_expect_success 'set core.splitIndex config variable to true' '\n \tgit config core.splitIndex true &&\n-\t: >three &&\n+\tcreate_non_racy_file three &&\n \tgit update-index --add three &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -253,9 +260,9 @@ test_expect_success 'set core.splitIndex config variable to false' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'set core.splitIndex config variable to true' '\n+test_expect_success 'set core.splitIndex config variable back to true' '\n \tgit config core.splitIndex true &&\n-\t: >three &&\n+\tcreate_non_racy_file three &&\n \tgit update-index --add three &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -265,7 +272,7 @@ test_expect_success 'set core.splitIndex config variable to true' '\n \tdeletions:\n \tEOF\n \ttest_cmp expect actual &&\n-\t: >four &&\n+\tcreate_non_racy_file four &&\n \tgit update-index --add four &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n \tcat >expect <<-EOF &&\n@@ -279,7 +286,7 @@ test_expect_success 'set core.splitIndex config variable to true' '\n \n test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '\n \tgit config --unset splitIndex.maxPercentChange &&\n-\t: >five &&\n+\tcreate_non_racy_file five &&\n \tgit update-index --add five &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -289,7 +296,7 @@ test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '\n \tdeletions:\n \tEOF\n \ttest_cmp expect actual &&\n-\t: >six &&\n+\tcreate_non_racy_file six &&\n \tgit update-index --add six &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n \tcat >expect <<-EOF &&\n@@ -303,7 +310,7 @@ test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '\n \n test_expect_success 'check splitIndex.maxPercentChange set to 0' '\n \tgit config splitIndex.maxPercentChange 0 &&\n-\t: >seven &&\n+\tcreate_non_racy_file seven &&\n \tgit update-index --add seven &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -313,7 +320,7 @@ test_expect_success 'check splitIndex.maxPercentChange set to 0' '\n \tdeletions:\n \tEOF\n \ttest_cmp expect actual &&\n-\t: >eight &&\n+\tcreate_non_racy_file eight &&\n \tgit update-index --add eight &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -326,17 +333,17 @@ test_expect_success 'check splitIndex.maxPercentChange set to 0' '\n '\n \n test_expect_success 'shared index files expire after 2 weeks by default' '\n-\t: >ten &&\n+\tcreate_non_racy_file ten &&\n \tgit update-index --add ten &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_under_2_weeks_ago=$((5-14*86400)) &&\n \ttest-tool chmtime =$just_under_2_weeks_ago .git/sharedindex.* &&\n-\t: >eleven &&\n+\tcreate_non_racy_file eleven &&\n \tgit update-index --add eleven &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_over_2_weeks_ago=$((-1-14*86400)) &&\n \ttest-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&\n-\t: >twelve &&\n+\tcreate_non_racy_file twelve &&\n \tgit update-index --add twelve &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n@@ -344,12 +351,12 @@ test_expect_success 'shared index files expire after 2 weeks by default' '\n test_expect_success 'check splitIndex.sharedIndexExpire set to 16 days' '\n \tgit config splitIndex.sharedIndexExpire \"16.days.ago\" &&\n \ttest-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&\n-\t: >thirteen &&\n+\tcreate_non_racy_file thirteen &&\n \tgit update-index --add thirteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_over_16_days_ago=$((-1-16*86400)) &&\n \ttest-tool chmtime =$just_over_16_days_ago .git/sharedindex.* &&\n-\t: >fourteen &&\n+\tcreate_non_racy_file fourteen &&\n \tgit update-index --add fourteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n@@ -358,13 +365,13 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"\n \tgit config splitIndex.sharedIndexExpire never &&\n \tjust_10_years_ago=$((-365*10*86400)) &&\n \ttest-tool chmtime =$just_10_years_ago .git/sharedindex.* &&\n-\t: >fifteen &&\n+\tcreate_non_racy_file fifteen &&\n \tgit update-index --add fifteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tgit config splitIndex.sharedIndexExpire now &&\n \tjust_1_second_ago=-1 &&\n \ttest-tool chmtime =$just_1_second_ago .git/sharedindex.* &&\n-\t: >sixteen &&\n+\tcreate_non_racy_file sixteen &&\n \tgit update-index --add sixteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n@@ -379,7 +386,7 @@ do\n \t\t# Create one new shared index file\n \t\tgit config core.sharedrepository \"$mode\" &&\n \t\tgit config core.splitIndex true &&\n-\t\t: >one &&\n+\t\tcreate_non_racy_file one &&\n \t\tgit update-index --add one &&\n \t\techo \"$modebits\" >expect &&\n \t\ttest_modebits .git/index >actual &&\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359045","messageId":"20180927124434.30835-4-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180927124434.30835-1-szeder.dev@gmail.com","subject":"[PATCH v2 3/5] split-index: count the number of deleted entries","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-27T12:44:32Z","receivedAt":"2018-09-27T12:45:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'struct split_index' contains the field 'nr_deletions', whose name\nwith the 'nr_' prefix suggests that it contains the number of deleted\ncache entries. However, barring its initialization to 0, this field\nis only ever set to 1, indicating that there is at least one deleted\nentry, but not the number of deleted entries. Luckily, this doesn't\ncause any issues (other than confusing the reader, that is), because\nthe only place reading this field uses it in the same sense, i.e.: 'if\n(si->nr_deletions)'.\n\nTo avoid confusion, we could either rename this field to something\nlike 'has_deletions' to make its name match its role, or make it a\ncounter of deleted cache entries to match its name.\n\nLet's make it a counter, to keep it in sync with the related field\n'nr_replacements', which does contain the number of replaced cache\nentries. This will also give developers debugging the split index\ncode easy access to the number of deleted cache entries.\n\nSigned-off-by: SZEDER Gábor \n---\n split-index.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/split-index.c b/split-index.c\nindex 84f067e10d..548272ec33 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -111,7 +111,7 @@ static void mark_entry_for_delete(size_t pos, void *data)\n \t\tdie(\"position for delete %d exceeds base index size %d\",\n \t\t (int)pos, istate->cache_nr);\n \tistate->cache[pos]->ce_flags |= CE_REMOVE;\n-\tistate->split_index->nr_deletions = 1;\n+\tistate->split_index->nr_deletions++;\n }\n \n static void replace_entry(size_t pos, void *data)\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359046","messageId":"20180927124434.30835-5-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180927124434.30835-1-szeder.dev@gmail.com","subject":"[PATCH v2 4/5] split-index: don't compare stat data of entries already marked for split index","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-27T12:44:33Z","receivedAt":"2018-09-27T12:45:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When unpack_trees() constructs a new index, it copies cache entries\nfrom the original index [1]. prepare_to_write_split_index() has to\ndeal with this, and it has a dedicated code path for copied entries\nthat are present in the shared index, where it compares the cached\ndata in the corresponding copied and original entries. If the cached\ndata matches, then they are considered the same; if it differs, then\nthe copied entry will be marked for inclusion as a replacement entry\nin the just about to be written split index by setting the\nCE_UPDATE_IN_BASE flag.\n\nHowever, a cache entry already has its CE_UPDATE_IN_BASE flag set upon\nreading the split index, if the entry already has a replacement entry\nthere, or upon refreshing the cached stat data, if the corresponding\nfile was modified. The state of this flag is then preserved when\nunpack_trees() copies a cache entry from the shared index.\n\nSo modify prepare_to_write_split_index() to check the copied cache\nentries' CE_UPDATE_IN_BASE flag first, and skip the thorough\ncomparison of cached data if the flag is already set.\n\nNote that comparing the cached data in copied and original entries in\nthe shared index might actually be entirely unnecessary. In theory\nall code paths refreshing the cached stat data of an entry in the\nshared index should set the CE_UPDATE_IN_BASE flag in that entry, and\nunpack_trees() should preserve this flag when copying cache entries.\nThis means that the cached data is only ever changed if the\nCE_UPDATE_IN_BASE flag is set as well. Our test suite seems to\nconfirm this: instrumenting the conditions in question and running the\ntest suite repeatedly with 'GIT_TEST_SPLIT_INDEX=yes' showed that the\ncached data in a copied entry differs from the data in the shared\nentry only if its CE_UPDATE_IN_BASE flag is indeed set.\n\nIn practice, however, our test suite doesn't have 100% coverage,\nGIT_TEST_SPLIT_INDEX is inherently random, and I certainly can't claim\nto possess complete understanding of what goes on in unpack_trees()...\nTherefore I kept the comparison of the cached data when\nCE_UPDATE_IN_BASE is not set, just in case that an unnoticed or future\ncode path were to accidentally miss setting this flag upon refreshing\nthe cached stat data or unpack_trees() were to drop this flag while\ncopying a cache entry.\n\n[1] Note that when unpack_trees() constructs the new index and decides\n that a cache entry should now refer to different content than what\n was recorded in the original index (e.g. 'git read-tree -m\n HEAD^'), then that can't really be considered a copy of the\n original, but rather the creation of a new entry. Notably and\n pertinent to the split index feature, such a new entry doesn't\n have a reference to the original's shared index entry anymore,\n i.e. its 'index' field is set to 0. Consequently, such an entry\n is treated by prepare_to_write_split_index() as an entry not\n present in the shared index and it will be added to the new split\n index, while the original entry will be marked as deleted, and\n neither the above discussion nor the changes in this patch apply\n to them.\n\nSigned-off-by: SZEDER Gábor \n---\n split-index.c | 79 ++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 62 insertions(+), 17 deletions(-)\n\ndiff --git a/split-index.c b/split-index.c\nindex 548272ec33..7d8799f6b7 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -204,19 +204,34 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t * that are not marked with either CE_MATCHED or\n \t\t * CE_UPDATE_IN_BASE. If istate->cache[i] is a\n \t\t * duplicate, deduplicate it.\n \t\t */\n \t\tfor (i = 0; i < istate->cache_nr; i++) {\n \t\t\tstruct cache_entry *base;\n-\t\t\t/* namelen is checked separately */\n-\t\t\tconst unsigned int ondisk_flags =\n-\t\t\t\tCE_STAGEMASK | CE_VALID | CE_EXTENDED_FLAGS;\n-\t\t\tunsigned int ce_flags, base_flags, ret;\n \t\t\tce = istate->cache[i];\n-\t\t\tif (!ce->index)\n+\t\t\tif (!ce->index) {\n+\t\t\t\t/*\n+\t\t\t\t * During simple update index operations this\n+\t\t\t\t * is a cache entry that is not present in\n+\t\t\t\t * the shared index. It will be added to the\n+\t\t\t\t * split index.\n+\t\t\t\t *\n+\t\t\t\t * However, it might also represent a file\n+\t\t\t\t * that already has a cache entry in the\n+\t\t\t\t * shared index, but a new index has just\n+\t\t\t\t * been constructed by unpack_trees(), and\n+\t\t\t\t * this entry now refers to different content\n+\t\t\t\t * than what was recorded in the original\n+\t\t\t\t * index, e.g. during 'read-tree -m HEAD^' or\n+\t\t\t\t * 'checkout HEAD^'. In this case the\n+\t\t\t\t * original entry in the shared index will be\n+\t\t\t\t * marked as deleted, and this entry will be\n+\t\t\t\t * added to the split index.\n+\t\t\t\t */\n \t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (ce->index > si->base->cache_nr) {\n \t\t\t\tce->index = 0;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tce->ce_flags |= CE_MATCHED; /* or \"shared\" */\n \t\t\tbase = si->base->cache[ce->index - 1];\n@@ -224,24 +239,54 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t\tcontinue;\n \t\t\tif (ce->ce_namelen != base->ce_namelen ||\n \t\t\t strcmp(ce->name, base->name)) {\n \t\t\t\tce->index = 0;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tce_flags = ce->ce_flags;\n-\t\t\tbase_flags = base->ce_flags;\n-\t\t\t/* only on-disk flags matter */\n-\t\t\tce->ce_flags &= ondisk_flags;\n-\t\t\tbase->ce_flags &= ondisk_flags;\n-\t\t\tret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,\n-\t\t\t\t offsetof(struct cache_entry, name) -\n-\t\t\t\t offsetof(struct cache_entry, ce_stat_data));\n-\t\t\tce->ce_flags = ce_flags;\n-\t\t\tbase->ce_flags = base_flags;\n-\t\t\tif (ret)\n-\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\t\t\t/*\n+\t\t\t * This is the copy of a cache entry that is present\n+\t\t\t * in the shared index, created by unpack_trees()\n+\t\t\t * while it constructed a new index.\n+\t\t\t */\n+\t\t\tif (ce->ce_flags & CE_UPDATE_IN_BASE) {\n+\t\t\t\t/*\n+\t\t\t\t * Already marked for inclusion in the split\n+\t\t\t\t * index, either because the corresponding\n+\t\t\t\t * file was modified and the cached stat data\n+\t\t\t\t * was refreshed, or because the original\n+\t\t\t\t * entry already had a replacement entry in\n+\t\t\t\t * the split index.\n+\t\t\t\t * Nothing to do.\n+\t\t\t\t */\n+\t\t\t} else {\n+\t\t\t\t/*\n+\t\t\t\t * Thoroughly compare the cached data to see\n+\t\t\t\t * whether it should be marked for inclusion\n+\t\t\t\t * in the split index.\n+\t\t\t\t *\n+\t\t\t\t * This comparison might be unnecessary, as\n+\t\t\t\t * code paths modifying the cached data do\n+\t\t\t\t * set CE_UPDATE_IN_BASE as well.\n+\t\t\t\t */\n+\t\t\t\tconst unsigned int ondisk_flags =\n+\t\t\t\t\tCE_STAGEMASK | CE_VALID |\n+\t\t\t\t\tCE_EXTENDED_FLAGS;\n+\t\t\t\tunsigned int ce_flags, base_flags, ret;\n+\t\t\t\tce_flags = ce->ce_flags;\n+\t\t\t\tbase_flags = base->ce_flags;\n+\t\t\t\t/* only on-disk flags matter */\n+\t\t\t\tce->ce_flags &= ondisk_flags;\n+\t\t\t\tbase->ce_flags &= ondisk_flags;\n+\t\t\t\tret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,\n+\t\t\t\t\t offsetof(struct cache_entry, name) -\n+\t\t\t\t\t offsetof(struct cache_entry, ce_stat_data));\n+\t\t\t\tce->ce_flags = ce_flags;\n+\t\t\t\tbase->ce_flags = base_flags;\n+\t\t\t\tif (ret)\n+\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\t\t\t}\n \t\t\tdiscard_cache_entry(base);\n \t\t\tsi->base->cache[ce->index - 1] = ce;\n \t\t}\n \t\tfor (i = 0; i < si->base->cache_nr; i++) {\n \t\t\tce = si->base->cache[i];\n \t\t\tif ((ce->ce_flags & CE_REMOVE) ||\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359047","messageId":"20180927124434.30835-6-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180927124434.30835-1-szeder.dev@gmail.com","subject":"[PATCH v2 5/5] split-index: smudge and add racily clean cache entries to split index","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-27T12:44:34Z","receivedAt":"2018-09-27T12:45:23Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Ever since the split index feature was introduced [1], refreshing a\nsplit index is prone to a variant of the classic racy git problem.\n\nConsider the following sequence of commands updating the split index\nwhen the shared index contains a racily clean cache entry, i.e. an\nentry whose cached stat data matches with the corresponding file in\nthe worktree and the cached mtime matches that of the index:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file # size stays the same!\n # ... wait ...\n git update-index --add other-file\n\nNormally, when a non-split index is updated, then do_write_index()\n(the function responsible for writing all kinds of indexes, \"regular\",\nsplit, and shared) recognizes racily clean cache entries, and writes\nthem with smudged stat data, i.e. with file size set to 0. When\nsubsequent git commands read the index, they will notice that the\nsmudged stat data doesn't match with the file in the worktree, and\nthen go on to check the file's content and notice its dirtiness.\n\nIn the above example, however, in the second 'git update-index'\nprepare_to_write_split_index() decides which cache entries stored only\nin the shared index should be replaced in the new split index. Alas,\nthis function never looks out for racily clean cache entries, and\nsince the file's stat data in the worktree hasn't changed since the\nshared index was written, it won't be replaced in the new split index.\nConsequently, do_write_index() doesn't even get this racily clean\ncache entry, and can't smudge its stat data. Subsequent git commands\nwill then see that the index has more recent mtime than the file and\nthat the (not smudged) cached stat data still matches with the file in\nthe worktree, and, ultimately, will erroneously consider the file\nclean.\n\nModify prepare_to_write_split_index() to recognize racily clean cache\nentries, and mark them to be added to the split index. Note that\nthere are two places where it should check raciness: first those cache\nentries that are only stored in the shared index, and then those that\nhave been copied by unpack_trees() from the shared index while it\nconstructed a new index. This way do_write_index() will get these\nracily clean cache entries as well, and will then write them with\nsmudged stat data to the new split index.\n\nNote that after this change if the index is split when it contains a\nracily clean cache entry, then a smudged cache entry will be written\nboth to the new shared and to the new split indexes. This doesn't\naffect regular git commands: as far as they are concerned this is just\nan entry in the split index replacing an outdated entry in the shared\nindex. It did affect a few tests in 't1700-split-index.sh', though,\nbecause they actually check which entries are stored in the split\nindex; a previous patch in this series has already made the necessary\nadjustments in 't1700'. And racily clean cache entries and index\nsplitting are rare enough to not worry about the resulting duplicated\nsmudged cache entries, and the additional complexity required to\nprevent them is not worth it.\n\nSeveral tests failed occasionally when the test suite was run with\n'GIT_TEST_SPLIT_INDEX=yes'. Here are those that I managed to trace\nback to this racy split index problem, starting with those failing\nmore frequently, with a link to a failing Travis CI build job for\neach. The highlighted line [2] shows when the racy file was written,\nwhich is not always in the failing test but in a preceeding setup\ntest.\n\n t3903-stash.sh:\n https://travis-ci.org/git/git/jobs/385542084#L5858\n\n t4024-diff-optimize-common.sh:\n https://travis-ci.org/git/git/jobs/386531969#L3174\n\n t4015-diff-whitespace.sh:\n https://travis-ci.org/git/git/jobs/360797600#L8215\n\n t2200-add-update.sh:\n https://travis-ci.org/git/git/jobs/382543426#L3051\n\n t0090-cache-tree.sh:\n https://travis-ci.org/git/git/jobs/416583010#L3679\n\nThere might be others, e.g. perhaps 't1000-read-tree-m-3way.sh' and\nothers using 'lib-read-tree-m-3way.sh', but I couldn't confirm yet.\n\n[1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge\n branch 'nd/split-index', 2014-07-16).\n\n[2] Note that those highlighted lines are in the 'after failure' fold,\n and your browser might unhelpfully fold it up before you could\n take a good look.\n\nSigned-off-by: SZEDER Gábor \n---\n cache.h | 2 ++\n read-cache.c | 2 +-\n split-index.c | 42 ++++++++++++++++++++++++++++++++++++-\n t/t1701-racy-split-index.sh | 8 ++-----\n 4 files changed, 46 insertions(+), 8 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4d014541ab..3f419b6c79 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -781,6 +781,8 @@ extern void *read_blob_data_from_index(const struct index_state *, const char *,\n #define CE_MATCH_REFRESH\t\t0x10\n /* don't refresh_fsmonitor state or do stat comparison even if CE_FSMONITOR_VALID is true */\n #define CE_MATCH_IGNORE_FSMONITOR 0X20\n+extern int is_racy_timestamp(const struct index_state *istate,\n+\t\t\t const struct cache_entry *ce);\n extern int ie_match_stat(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n extern int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 7b1354d759..8f644f68b4 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -337,7 +337,7 @@ static int is_racy_stat(const struct index_state *istate,\n \t\t);\n }\n \n-static int is_racy_timestamp(const struct index_state *istate,\n+int is_racy_timestamp(const struct index_state *istate,\n \t\t\t const struct cache_entry *ce)\n {\n \treturn (!S_ISGITLINK(ce->ce_mode) &&\ndiff --git a/split-index.c b/split-index.c\nindex 7d8799f6b7..d989b27286 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -235,8 +235,39 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t}\n \t\t\tce->ce_flags |= CE_MATCHED; /* or \"shared\" */\n \t\t\tbase = si->base->cache[ce->index - 1];\n-\t\t\tif (ce == base)\n+\t\t\tif (ce == base) {\n+\t\t\t\t/* The entry is present in the shared index. */\n+\t\t\t\tif (ce->ce_flags & CE_UPDATE_IN_BASE) {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * Already marked for inclusion in\n+\t\t\t\t\t * the split index, either because\n+\t\t\t\t\t * the corresponding file was\n+\t\t\t\t\t * modified and the cached stat data\n+\t\t\t\t\t * was refreshed, or because there\n+\t\t\t\t\t * is already a replacement entry in\n+\t\t\t\t\t * the split index.\n+\t\t\t\t\t * Nothing more to do here.\n+\t\t\t\t\t */\n+\t\t\t\t} else if (!ce_uptodate(ce) &&\n+\t\t\t\t\t is_racy_timestamp(istate, ce)) {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * A racily clean cache entry stored\n+\t\t\t\t\t * only in the shared index: it must\n+\t\t\t\t\t * be added to the split index, so\n+\t\t\t\t\t * the subsequent do_write_index()\n+\t\t\t\t\t * can smudge its stat data.\n+\t\t\t\t\t */\n+\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\t\t\t\t} else {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * The entry is only present in the\n+\t\t\t\t\t * shared index and it was not\n+\t\t\t\t\t * refreshed.\n+\t\t\t\t\t * Just leave it there.\n+\t\t\t\t\t */\n+\t\t\t\t}\n \t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (ce->ce_namelen != base->ce_namelen ||\n \t\t\t strcmp(ce->name, base->name)) {\n \t\t\t\tce->index = 0;\n@@ -257,6 +288,15 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t\t * the split index.\n \t\t\t\t * Nothing to do.\n \t\t\t\t */\n+\t\t\t} else if (!ce_uptodate(ce) &&\n+\t\t\t\t is_racy_timestamp(istate, ce)) {\n+\t\t\t\t/*\n+\t\t\t\t * A copy of a racily clean cache entry from\n+\t\t\t\t * the shared index. It must be added to\n+\t\t\t\t * the split index, so the subsequent\n+\t\t\t\t * do_write_index() can smudge its stat data.\n+\t\t\t\t */\n+\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n \t\t\t} else {\n \t\t\t\t/*\n \t\t\t\t * Thoroughly compare the cached data to see\ndiff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\nindex ebde418d7e..7f16f5f7a3 100755\n--- a/t/t1701-racy-split-index.sh\n+++ b/t/t1701-racy-split-index.sh\n@@ -148,7 +148,7 @@ done\n \n for trial in $trials\n do\n-\ttest_expect_failure \"update the split index when a racily clean cache entry is stored only in the shared index $trial\" '\n+\ttest_expect_success \"update the split index when a racily clean cache entry is stored only in the shared index #$trial\" '\n \t\trm -f .git/index .git/sharedindex.* &&\n \n \t\t# The next three commands must be run within the same\n@@ -170,8 +170,6 @@ do\n \t\t# entry of racy-file is only stored in the shared index.\n \t\t# A corresponding replacement cache entry with smudged\n \t\t# stat data should be added to the new split index.\n-\t\t#\n-\t\t# Alas, such a smudged replacement entry is not added!\n \t\tgit update-index --add other-file &&\n \n \t\t# Subsequent git commands should notice the smudged\n@@ -182,7 +180,7 @@ done\n \n for trial in $trials\n do\n-\ttest_expect_failure \"update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial\" '\n+\ttest_expect_success \"update the split index after unpack trees() copied a racily clean cache entry from the shared index #$trial\" '\n \t\trm -f .git/index .git/sharedindex.* &&\n \n \t\t# The next three commands must be run within the same\n@@ -205,8 +203,6 @@ do\n \t\t# index. A corresponding replacement cache entry\n \t\t# with smudged stat data should be added to the new\n \t\t# split index.\n-\t\t#\n-\t\t# Alas, such a smudged replacement entry is not added!\n \t\tgit read-tree -m HEAD &&\n \n \t\t# Subsequent git commands should notice the smudged\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359048","messageId":"20180927134324.GI27036@localhost","threadId":"49437","inReplyTo":"20180927124434.30835-5-szeder.dev@gmail.com","subject":"Re: [PATCH v2 4/5] split-index: don't compare stat data of entries already marked for split index","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-27T13:43:24Z","receivedAt":"2018-09-27T13:43:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Sep 27, 2018 at 02:44:33PM +0200, SZEDER Gábor wrote:\n> split-index.c | 79 ++++++++++++++++++++++++++++++++++++++++-----------\n> 1 file changed, 62 insertions(+), 17 deletions(-)\n\nI generated this patch with more context lines than usual, so the two\nconditions that I didn't add any comments to in this or in the next\npatch are fully visible.\n\n> diff --git a/split-index.c b/split-index.c\n> index 548272ec33..7d8799f6b7 100644\n> --- a/split-index.c\n> +++ b/split-index.c\n> @@ -204,19 +204,34 @@ void prepare_to_write_split_index(struct index_state *istate)\n> \t\t * that are not marked with either CE_MATCHED or\n> \t\t * CE_UPDATE_IN_BASE. If istate->cache[i] is a\n> \t\t * duplicate, deduplicate it.\n> \t\t */\n> \t\tfor (i = 0; i < istate->cache_nr; i++) {\n> \t\t\tstruct cache_entry *base;\n> -\t\t\t/* namelen is checked separately */\n> -\t\t\tconst unsigned int ondisk_flags =\n> -\t\t\t\tCE_STAGEMASK | CE_VALID | CE_EXTENDED_FLAGS;\n> -\t\t\tunsigned int ce_flags, base_flags, ret;\n> \t\t\tce = istate->cache[i];\n> -\t\t\tif (!ce->index)\n> +\t\t\tif (!ce->index) {\n> +\t\t\t\t/*\n> +\t\t\t\t * During simple update index operations this\n> +\t\t\t\t * is a cache entry that is not present in\n> +\t\t\t\t * the shared index. It will be added to the\n> +\t\t\t\t * split index.\n> +\t\t\t\t *\n> +\t\t\t\t * However, it might also represent a file\n> +\t\t\t\t * that already has a cache entry in the\n> +\t\t\t\t * shared index, but a new index has just\n> +\t\t\t\t * been constructed by unpack_trees(), and\n> +\t\t\t\t * this entry now refers to different content\n> +\t\t\t\t * than what was recorded in the original\n> +\t\t\t\t * index, e.g. during 'read-tree -m HEAD^' or\n> +\t\t\t\t * 'checkout HEAD^'. In this case the\n> +\t\t\t\t * original entry in the shared index will be\n> +\t\t\t\t * marked as deleted, and this entry will be\n> +\t\t\t\t * added to the split index.\n> +\t\t\t\t */\n> \t\t\t\tcontinue;\n> +\t\t\t}\n> \t\t\tif (ce->index > si->base->cache_nr) {\n> \t\t\t\tce->index = 0;\n> \t\t\t\tcontinue;\n> \t\t\t}\n\nThis condition in the context above checks whether a cache entry\nrefers to a non-existing entry in the shared index.\n\nI don't understand the role of this condition, for two reasons:\n\n - Under what circumstances can this condition be ever fulfilled?\n\n I instrumented it and run the test suite repeatedly with\n 'GIT_TEST_SPLIT_INDEX=yes', but it has never been fulfilled. I\n also tried to come up with all kinds of elaborate scenarios to\n trigger it, but no joy, and code inspection didn't bring anything\n either.\n\n - There are similar conditions in 'split-index.c' in the functions\n mark_entry_for_delete() and replace_entry(); here is the one from\n the latter, but they only differ in the error message:\n\n if (pos >= istate->cache_nr)\n die(\"position for replacement %d exceeds base index size %d\",\n (int)pos, istate->cache_nr);\n\n (Note that this 'istate->cache_nr' here equals\n to 'si->base->cache_nr'; see their caller merge_base_index().)\n\n The die() clearly indicates that fulfilling this condition is a\n Bad Thing. These two functions are invoked to create a unified\n view of the just read split and shared indexes, so the fulfillment\n of this condition could indicate a corrupt index file, and\n die()ing right away seems to be justified.\n\n Then why doesn't the condition in prepare_to_write_split_index()\n die() as well?! After all if it were fulfilled, then it would\n indicate a corruption in the current index_state, and writing a\n new split index from corrupt data doesn't seem like a particularly\n good idea.\n\n\n> \t\t\tce->ce_flags |= CE_MATCHED; /* or \"shared\" */\n> \t\t\tbase = si->base->cache[ce->index - 1];\n> @@ -224,24 +239,54 @@ void prepare_to_write_split_index(struct index_state *istate)\n> \t\t\t\tcontinue;\n> \t\t\tif (ce->ce_namelen != base->ce_namelen ||\n> \t\t\t strcmp(ce->name, base->name)) {\n> \t\t\t\tce->index = 0;\n> \t\t\t\tcontinue;\n> \t\t\t}\n\nI don't understand the role of this condition either, and just like\nthe one discussed above, the test suite with\n'GIT_TEST_SPLIT_INDEX=yes' seems to never fulfill it.\n\n> -\t\t\tce_flags = ce->ce_flags;\n> -\t\t\tbase_flags = base->ce_flags;\n> -\t\t\t/* only on-disk flags matter */\n> -\t\t\tce->ce_flags &= ondisk_flags;\n> -\t\t\tbase->ce_flags &= ondisk_flags;\n> -\t\t\tret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,\n> -\t\t\t\t offsetof(struct cache_entry, name) -\n> -\t\t\t\t offsetof(struct cache_entry, ce_stat_data));\n> -\t\t\tce->ce_flags = ce_flags;\n> -\t\t\tbase->ce_flags = base_flags;\n> -\t\t\tif (ret)\n> -\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n> +\t\t\t/*\n> +\t\t\t * This is the copy of a cache entry that is present\n> +\t\t\t * in the shared index, created by unpack_trees()\n> +\t\t\t * while it constructed a new index.\n> +\t\t\t */\n> +\t\t\tif (ce->ce_flags & CE_UPDATE_IN_BASE) {\n> +\t\t\t\t/*\n> +\t\t\t\t * Already marked for inclusion in the split\n> +\t\t\t\t * index, either because the corresponding\n> +\t\t\t\t * file was modified and the cached stat data\n> +\t\t\t\t * was refreshed, or because the original\n> +\t\t\t\t * entry already had a replacement entry in\n> +\t\t\t\t * the split index.\n> +\t\t\t\t * Nothing to do.\n> +\t\t\t\t */\n> +\t\t\t} else {\n> +\t\t\t\t/*\n> +\t\t\t\t * Thoroughly compare the cached data to see\n> +\t\t\t\t * whether it should be marked for inclusion\n> +\t\t\t\t * in the split index.\n> +\t\t\t\t *\n> +\t\t\t\t * This comparison might be unnecessary, as\n> +\t\t\t\t * code paths modifying the cached data do\n> +\t\t\t\t * set CE_UPDATE_IN_BASE as well.\n> +\t\t\t\t */\n> +\t\t\t\tconst unsigned int ondisk_flags =\n> +\t\t\t\t\tCE_STAGEMASK | CE_VALID |\n> +\t\t\t\t\tCE_EXTENDED_FLAGS;\n> +\t\t\t\tunsigned int ce_flags, base_flags, ret;\n> +\t\t\t\tce_flags = ce->ce_flags;\n> +\t\t\t\tbase_flags = base->ce_flags;\n> +\t\t\t\t/* only on-disk flags matter */\n> +\t\t\t\tce->ce_flags &= ondisk_flags;\n> +\t\t\t\tbase->ce_flags &= ondisk_flags;\n> +\t\t\t\tret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,\n> +\t\t\t\t\t offsetof(struct cache_entry, name) -\n> +\t\t\t\t\t offsetof(struct cache_entry, ce_stat_data));\n> +\t\t\t\tce->ce_flags = ce_flags;\n> +\t\t\t\tbase->ce_flags = base_flags;\n> +\t\t\t\tif (ret)\n> +\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n> +\t\t\t}\n> \t\t\tdiscard_cache_entry(base);\n> \t\t\tsi->base->cache[ce->index - 1] = ce;\n> \t\t}\n> \t\tfor (i = 0; i < si->base->cache_nr; i++) {\n> \t\t\tce = si->base->cache[i];\n> \t\t\tif ((ce->ce_flags & CE_REMOVE) ||\n> -- \n> 2.19.0.361.gafc87ffe72\n> \n"},{"id":"359049","messageId":"871s9fjbcb.fsf@evledraar.gmail.com","threadId":"49437","inReplyTo":"20180927124434.30835-1-szeder.dev@gmail.com","subject":"Re: [PATCH v2 0/5] Fix the racy split index problem","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-27T13:53:24Z","receivedAt":"2018-09-27T13:53:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 27 2018, SZEDER Gábor wrote:\n\n> This is the second attempt to fix the racy split index problem, which\n> causes occasional failures in several random test scripts when run\n> with 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches\n> 1 and 5 (corresponding to v1's 3 and 5).\n\nThanks. I'm running the same sorts of tests I noted in\nhttps://public-inbox.org/git/87va7ireuu.fsf@evledraar.gmail.com/ on\nthis. The fix Jeff had that you noted in\nhttps://public-inbox.org/git/20180906151439.GA8016@localhost/ is now in\n\"master\".\n\nI take it your\nhttps://github.com/szeder/git/commits/racy-split-index-fix is the same\nas this submission? Anyway, I'm testing that cherry-picked on top of the\nlatest master.\n\nUnfortunate that we couldn't get the isolated test you made in\nhttps://public-inbox.org/git/20180907034942.GA10370@localhost/ but I\ndon't see how it could be added without some very liberal\ngetenv(\"GIT_TEST_blahblah\"), so it's probably best to not add it,\nparticularly with the C rewrite of git-stash in-flight.\n\nI'll report back when I have enough test data to say how these patches\naffect the intermittent test failures under GIT_TEST_SPLIT_INDEX=yes.\n"},{"id":"359050","messageId":"20180927142336.GJ27036@localhost","threadId":"49437","inReplyTo":"871s9fjbcb.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 0/5] Fix the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-27T14:23:36Z","receivedAt":"2018-09-27T14:23:42Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Sep 27, 2018 at 03:53:24PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Sep 27 2018, SZEDER Gábor wrote:\n> \n> > This is the second attempt to fix the racy split index problem, which\n> > causes occasional failures in several random test scripts when run\n> > with 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches\n> > 1 and 5 (corresponding to v1's 3 and 5).\n> \n> Thanks. I'm running the same sorts of tests I noted in\n> https://public-inbox.org/git/87va7ireuu.fsf@evledraar.gmail.com/ on\n> this. The fix Jeff had that you noted in\n> https://public-inbox.org/git/20180906151439.GA8016@localhost/ is now in\n> \"master\".\n> \n> I take it your\n> https://github.com/szeder/git/commits/racy-split-index-fix is the same\n> as this submission?\n\nYes.\n\n> Anyway, I'm testing that cherry-picked on top of the\n> latest master.\n> \n> Unfortunate that we couldn't get the isolated test you made in\n> https://public-inbox.org/git/20180907034942.GA10370@localhost/\n\nNah, that's not an isolated test case, that's only a somewhat\nnarrowed-down, but rather reliable reproduction recipe while I still\nhad no idea what was going on :)\n\nThe _real_ isolated test is the last test in t1701, that's what it\neventually boiled down to.\n\n> but I\n> don't see how it could be added without some very liberal\n> getenv(\"GIT_TEST_blahblah\"), so it's probably best to not add it,\n> particularly with the C rewrite of git-stash in-flight.\n> \n> I'll report back when I have enough test data to say how these patches\n> affect the intermittent test failures under GIT_TEST_SPLIT_INDEX=yes.\n"},{"id":"359057","messageId":"87zhw3hsiv.fsf@evledraar.gmail.com","threadId":"49437","inReplyTo":"20180927142336.GJ27036@localhost","subject":"Re: [PATCH v2 0/5] Fix the racy split index problem","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-27T15:25:12Z","receivedAt":"2018-09-27T15:25:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 27 2018, SZEDER Gábor wrote:\n\n> On Thu, Sep 27, 2018 at 03:53:24PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Thu, Sep 27 2018, SZEDER Gábor wrote:\n>>\n>> > This is the second attempt to fix the racy split index problem, which\n>> > causes occasional failures in several random test scripts when run\n>> > with 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches\n>> > 1 and 5 (corresponding to v1's 3 and 5).\n>>\n>> Thanks. I'm running the same sorts of tests I noted in\n>> https://public-inbox.org/git/87va7ireuu.fsf@evledraar.gmail.com/ on\n>> this. The fix Jeff had that you noted in\n>> https://public-inbox.org/git/20180906151439.GA8016@localhost/ is now in\n>> \"master\".\n>>\n>> I take it your\n>> https://github.com/szeder/git/commits/racy-split-index-fix is the same\n>> as this submission?\n>\n> Yes.\n>\n>> Anyway, I'm testing that cherry-picked on top of the\n>> latest master.\n>>\n>> Unfortunate that we couldn't get the isolated test you made in\n>> https://public-inbox.org/git/20180907034942.GA10370@localhost/\n>\n> Nah, that's not an isolated test case, that's only a somewhat\n> narrowed-down, but rather reliable reproduction recipe while I still\n> had no idea what was going on :)\n>\n> The _real_ isolated test is the last test in t1701, that's what it\n> eventually boiled down to.\n\nThanks. I had ~400 runs of the tests I ran before and they were all\nOK. Now trying also with t1701 (which I hadn't noticed was a new\ntest...).\n\n\n>> but I\n>> don't see how it could be added without some very liberal\n>> getenv(\"GIT_TEST_blahblah\"), so it's probably best to not add it,\n>> particularly with the C rewrite of git-stash in-flight.\n>>\n>> I'll report back when I have enough test data to say how these patches\n>> affect the intermittent test failures under GIT_TEST_SPLIT_INDEX=yes.\n"},{"id":"359167","messageId":"20180928004843.GP27036@localhost","threadId":"49437","inReplyTo":"20180927124434.30835-2-szeder.dev@gmail.com","subject":"Re: [PATCH v2 1/5] split-index: add tests to demonstrate the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-28T00:48:43Z","receivedAt":"2018-09-28T00:48:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Junio,\n\nOn Thu, Sep 27, 2018 at 02:44:30PM +0200, SZEDER Gábor wrote:\n> diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\n> new file mode 100755\n> index 0000000000..ebde418d7e\n> --- /dev/null\n> +++ b/t/t1701-racy-split-index.sh\n> @@ -0,0 +1,218 @@\n> +#!/bin/sh\n> +\n> +# This test can give false success if your machine is sufficiently\n> +# slow or all trials happened to happen on second boundaries.\n> +\n> +test_description='racy split index'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\t# Only split the index when the test explicitly says so.\n> +\tsane_unset GIT_TEST_SPLIT_INDEX GIT_FSMONITOR_TEST &&\n\nPlease note that this patch adds another use of the environment\nvariable GIT_FSMONITOR_TEST, while the topic 'bp/rename-test-env-var'\nis about to rename that variable to GIT_TEST_FSMONITOR.\n\n"},{"id":"359177","messageId":"20180928024022.GQ27036@localhost","threadId":"49437","inReplyTo":"20180928004843.GP27036@localhost","subject":"Re: [PATCH v2 1/5] split-index: add tests to demonstrate the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-28T02:40:22Z","receivedAt":"2018-09-28T02:40:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 28, 2018 at 02:48:43AM +0200, SZEDER Gábor wrote:\n> Junio,\n> \n> On Thu, Sep 27, 2018 at 02:44:30PM +0200, SZEDER Gábor wrote:\n> > diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\n> > new file mode 100755\n> > index 0000000000..ebde418d7e\n> > --- /dev/null\n> > +++ b/t/t1701-racy-split-index.sh\n> > @@ -0,0 +1,218 @@\n> > +#!/bin/sh\n> > +\n> > +# This test can give false success if your machine is sufficiently\n> > +# slow or all trials happened to happen on second boundaries.\n> > +\n> > +test_description='racy split index'\n> > +\n> > +. ./test-lib.sh\n> > +\n> > +test_expect_success 'setup' '\n> > +\t# Only split the index when the test explicitly says so.\n> > +\tsane_unset GIT_TEST_SPLIT_INDEX GIT_FSMONITOR_TEST &&\n> \n> Please note that this patch adds another use of the environment\n> variable GIT_FSMONITOR_TEST, while the topic 'bp/rename-test-env-var'\n> is about to rename that variable to GIT_TEST_FSMONITOR.\n\nHang on for a sec. I unset GIT_FSMONITOR_TEST in this test, because I\nsaw that 't1700-split-index.sh' unsets it, and I just followed suit.\nBut come to think of it, t1700 has to unset it, because some of its\ntests check the index's SHA-1 checksum, and the FSMN extension would\ninterfere with that, of course. However, that's not an issue for\nt1701, because none of its tests care about the index's checksum, so\nunsetting GIT_FSMONITOR_TEST is actually unnecessary here... unless\nit could have other side effects that I'm not aware of.\n\n"},{"id":"359190","messageId":"87k1n6hzwu.fsf@evledraar.gmail.com","threadId":"49437","inReplyTo":"87zhw3hsiv.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 0/5] Fix the racy split index problem","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-28T06:57:53Z","receivedAt":"2018-09-28T06:57:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 27 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Thu, Sep 27 2018, SZEDER Gábor wrote:\n>\n>> On Thu, Sep 27, 2018 at 03:53:24PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>>\n>>> On Thu, Sep 27 2018, SZEDER Gábor wrote:\n>>>\n>>> > This is the second attempt to fix the racy split index problem, which\n>>> > causes occasional failures in several random test scripts when run\n>>> > with 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches\n>>> > 1 and 5 (corresponding to v1's 3 and 5).\n>>>\n>>> Thanks. I'm running the same sorts of tests I noted in\n>>> https://public-inbox.org/git/87va7ireuu.fsf@evledraar.gmail.com/ on\n>>> this. The fix Jeff had that you noted in\n>>> https://public-inbox.org/git/20180906151439.GA8016@localhost/ is now in\n>>> \"master\".\n>>>\n>>> I take it your\n>>> https://github.com/szeder/git/commits/racy-split-index-fix is the same\n>>> as this submission?\n>>\n>> Yes.\n>>\n>>> Anyway, I'm testing that cherry-picked on top of the\n>>> latest master.\n>>>\n>>> Unfortunate that we couldn't get the isolated test you made in\n>>> https://public-inbox.org/git/20180907034942.GA10370@localhost/\n>>\n>> Nah, that's not an isolated test case, that's only a somewhat\n>> narrowed-down, but rather reliable reproduction recipe while I still\n>> had no idea what was going on :)\n>>\n>> The _real_ isolated test is the last test in t1701, that's what it\n>> eventually boiled down to.\n>\n> Thanks. I had ~400 runs of the tests I ran before and they were all\n> OK. Now trying also with t1701 (which I hadn't noticed was a new\n> test...).\n\nRan that overnight with the same conditions as before. 2683 OK runs and\n0 failures (and counting). So it seems like the combination of the two\nfixed the split index bugs.\n\n>>> but I\n>>> don't see how it could be added without some very liberal\n>>> getenv(\"GIT_TEST_blahblah\"), so it's probably best to not add it,\n>>> particularly with the C rewrite of git-stash in-flight.\n>>>\n>>> I'll report back when I have enough test data to say how these patches\n>>> affect the intermittent test failures under GIT_TEST_SPLIT_INDEX=yes.\n"},{"id":"359196","messageId":"20180928101700.GB23446@localhost","threadId":"49437","inReplyTo":"87k1n6hzwu.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 0/5] Fix the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-28T10:17:00Z","receivedAt":"2018-09-28T10:17:07Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 28, 2018 at 08:57:53AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> > Thanks. I had ~400 runs of the tests I ran before and they were all\n> > OK. Now trying also with t1701 (which I hadn't noticed was a new\n> > test...).\n> \n> Ran that overnight with the same conditions as before. 2683 OK runs and\n> 0 failures (and counting). So it seems like the combination of the two\n> fixed the split index bugs.\n\nYeah, I thought they would. If you look at the first loop of\nprepare_to_write_split_index() classifying which cache entries should\nbe included in the new split index, you'll see only two code paths\nthat could leave out an entry from the split index, i.e. where an\nentry could be left with a non-zero 'ce->index' and without its\nCE_UPDATE_IN_BASE flag set. Now, with the fix in patch 5/5 both of\nthose code paths have the is_race_timestamp() check.\n\n"},{"id":"359209","messageId":"20180928162459.17138-1-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180927124434.30835-1-szeder.dev@gmail.com","subject":"[PATCH v3 0/6] Fix the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-28T16:24:53Z","receivedAt":"2018-09-28T16:25:22Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Third round of fixing occasional test failures when run with\n'GIT_TEST_SPLIT_INDEX=yes', with only two rather minor changes since the\nprevious version; just look at the trivial interdiff below.\n\nSZEDER Gábor (6):\n t1700-split-index: document why FSMONITOR is disabled in this test\n script\n split-index: add tests to demonstrate the racy split index problem\n t1700-split-index: date back files to avoid racy situations\n split-index: count the number of deleted entries\n split-index: don't compare stat data of entries already marked for\n split index\n split-index: smudge and add racily clean cache entries to split index\n\n cache.h | 2 +\n read-cache.c | 2 +-\n split-index.c | 121 +++++++++++++++++---\n t/t1700-split-index.sh | 52 +++++----\n t/t1701-racy-split-index.sh | 214 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 351 insertions(+), 40 deletions(-)\n create mode 100755 t/t1701-racy-split-index.sh\n\nInterdiff:\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex c8acbc50d0..f053bf83eb 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -6,6 +6,9 @@ test_description='split index mode tests'\n \n # We need total control of index splitting here\n sane_unset GIT_TEST_SPLIT_INDEX\n+# A couple of tests expect the index to have a specific checksum,\n+# but the presence of the optional FSMN extension would interfere\n+# with those checks, so disable it in this test script.\n sane_unset GIT_FSMONITOR_TEST\n \n # Create a file named as $1 with content read from stdin.\ndiff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\nindex 7f16f5f7a3..5dc221ef38 100755\n--- a/t/t1701-racy-split-index.sh\n+++ b/t/t1701-racy-split-index.sh\n@@ -9,7 +9,7 @@ test_description='racy split index'\n \n test_expect_success 'setup' '\n \t# Only split the index when the test explicitly says so.\n-\tsane_unset GIT_TEST_SPLIT_INDEX GIT_FSMONITOR_TEST &&\n+\tsane_unset GIT_TEST_SPLIT_INDEX &&\n \tgit config splitIndex.maxPercentChange 100 &&\n \n \techo \"cached content\" >racy-file &&\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359210","messageId":"20180928162459.17138-2-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180928162459.17138-1-szeder.dev@gmail.com","subject":"[PATCH v3 1/6] t1700-split-index: document why FSMONITOR is disabled in this test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-28T16:24:54Z","receivedAt":"2018-09-28T16:25:23Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Signed-off-by: SZEDER Gábor \n---\n t/t1700-split-index.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex be22398a85..822257ff7d 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -6,6 +6,9 @@ test_description='split index mode tests'\n \n # We need total control of index splitting here\n sane_unset GIT_TEST_SPLIT_INDEX\n+# A couple of tests expect the index to have a specific checksum,\n+# but the presence of the optional FSMN extension would interfere\n+# with those checks, so disable it in this test script.\n sane_unset GIT_FSMONITOR_TEST\n \n test_expect_success 'enable split index' '\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359211","messageId":"20180928162459.17138-3-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180928162459.17138-1-szeder.dev@gmail.com","subject":"[PATCH v3 2/6] split-index: add tests to demonstrate the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-28T16:24:55Z","receivedAt":"2018-09-28T16:25:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Ever since the split index feature was introduced [1], refreshing a\nsplit index is prone to a variant of the classic racy git problem.\nThere are a couple of unrelated tests in the test suite that\noccasionally fail when run with 'GIT_TEST_SPLIT_INDEX=yes', but\n't1700-split-index.sh', the only test script focusing solely on split\nindex, has never noticed this issue, because it only cares about how\nthe index is split under various circumstances and all the different\nways to turn the split index feature on and off.\n\nAdd a dedicated test script 't1701-racy-split-index.sh' to exercise\nthe split index feature in racy situations as well; kind of a\n\"t0010-racy-git.sh for split index\" but with modern style (the tests\ndo everything in &&-chained list of commands in 'test_expect_...'\nblocks, and use 'test_cmp' for more informative output on failure).\n\nThe tests cover the following sequences of index splitting, updating,\nand racy file modifications, with the last two cases demonstrating the\nracy split index problem:\n\n 1. Split the index while adding a racily clean file:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file # size stays the same\n\n This case already works properly. Even though the cache entry's\n stat data matches with the modifid file in the worktree,\n subsequent git commands will notice that the (split) index and\n the file have the same mtime, and then will go on to check the\n file's content and notice its dirtiness.\n\n 2. Add a racily clean file to an already split index:\n\n git update-index --split-index\n echo \"cached content\" >file\n git update-index --add file\n echo \"dirty worktree\" >file\n\n This case already works properly. After the second 'git\n update-index' writes the newly added file's cache entry to the\n new split index, it basically works in the same way as case #1.\n\n 3. Split the index when it (i.e. the not yet splitted index)\n contains a racily clean cache entry, i.e. an entry whose cached\n stat data matches with the corresponding file in the worktree and\n the cached mtime matches that of the index:\n\n echo \"cached content\" >file\n git update-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git update-index --split-index --add other-file\n\n This case already works properly. The shared index is written by\n do_write_index(), i.e. the same function that is responsible for\n writing \"regular\" and split indexes as well. This function\n cleverly notices the racily clean cache entry, and writes the\n entry to the new shared index with smudged stat data, i.e. file\n size set to 0. When subsequent git commands read the index, they\n will notice that the smudged stat data doesn't match with the\n file in the worktree, and then go on to check the file's content\n and notice its dirtiness.\n\n 4. Update the split index when it contains a racily clean cache\n entry:\n\n git update-index --split-index\n echo \"cached content\" >file\n git update-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git update-index --add other-file\n\n This case already works properly. After the second 'git\n update-index' the newly added file's cache entry is only stored\n in the split index. If a cache entry is present in the split\n index (even if it is a replacement of an outdated entry in the\n shared index), then it will always be included in the new split\n index on subsequent split index updates (until the file is\n removed or a new shared index is written), independently from\n whether the entry is racily clean or not. When do_write_index()\n writes the new split index, it notices the racily clean cache\n entry, and smudges its stat date. Subsequent git commands\n reading the index will notice the smudged stat data and then go\n on to check the file's content and notice its dirtiness.\n\n 5. Update the split index when a racily clean cache entry is stored\n only in the shared index:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git update-index --add other-file\n\n This case fails due to the racy split index problem. In the\n second 'git update-index' prepare_to_write_split_index() decides,\n among other things, which cache entries stored only in the shared\n index should be replaced in the new split index. Alas, this\n function never looks out for racily clean cache entries, and\n since the file's stat data in the worktree hasn't changed since\n the shared index was written, the entry won't be replaced in the\n new split index. Consequently, do_write_index() doesn't even get\n this racily clean cache entry, and can't smudge its stat data.\n Subsequent git commands will then see that the index has more\n recent mtime than the file and that the (not smudged) cached stat\n data still matches with the file in the worktree, and,\n ultimately, will erroneously consider the file clean.\n\n 6. Update the split index after unpack_trees() copied a racily clean\n cache entry from the shared index:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git read-tree -m HEAD\n\n This case fails due to the racy split index problem. This\n basically fails for the same reason as case #5 above, but there\n is one important difference, which warrants the dedicated test.\n While that second 'git update-index' in case #5 updates\n index_state in place, in this case 'git read-tree -m' calls\n unpack_trees(), which throws out the entire index, and constructs\n a new one from the (potentially updated) copies of the original's\n cache entries. Consequently, when prepare_to_write_split_index()\n gets to work on this reconstructed index, it takes a different\n code path than in case #5 when deciding which cache entries in\n the shared index should be replaced. The result is the same,\n though: the racily clean cache entry goes unnoticed, it isn't\n added to the split index with smudged stat data, and subsequent\n git commands will then erroneously consider the file clean.\n\nNote that in the last two 'test_expect_failure' cases I omitted the\n'#' (as in nr. of trial) from the tests' name on purpose for now, as\nit confuses 'prove' into thinking that those tests failed\nunexpectedly.\n\n[1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge\n branch 'nd/split-index', 2014-07-16).\n\nSigned-off-by: SZEDER Gábor \n---\n t/t1701-racy-split-index.sh | 218 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 218 insertions(+)\n create mode 100755 t/t1701-racy-split-index.sh\n\ndiff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\nnew file mode 100755\nindex 0000000000..fbb77046da\n--- /dev/null\n+++ b/t/t1701-racy-split-index.sh\n@@ -0,0 +1,218 @@\n+#!/bin/sh\n+\n+# This test can give false success if your machine is sufficiently\n+# slow or all trials happened to happen on second boundaries.\n+\n+test_description='racy split index'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\t# Only split the index when the test explicitly says so.\n+\tsane_unset GIT_TEST_SPLIT_INDEX &&\n+\tgit config splitIndex.maxPercentChange 100 &&\n+\n+\techo \"cached content\" >racy-file &&\n+\tgit add racy-file &&\n+\tgit commit -m initial &&\n+\n+\techo something >other-file &&\n+\t# No raciness with this file.\n+\ttest-tool chmtime =-20 other-file &&\n+\n+\techo \"+cached content\" >expect\n+'\n+\n+check_cached_diff () {\n+\tgit diff-index --patch --cached $EMPTY_TREE racy-file >diff &&\n+\ttail -1 diff >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+trials=\"0 1 2 3 4\"\n+for trial in $trials\n+do\n+\ttest_expect_success \"split the index while adding a racily clean file #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second (so both writes to racy-file result in the same\n+\t\t# mtime) to create the interesting racy situation.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update and split the index. The cache entry of\n+\t\t# racy-file will be stored only in the shared index.\n+\t\tgit update-index --split-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Subsequent git commands should notice that racy-file\n+\t\t# and the split index have the same mtime, and check\n+\t\t# the content of the file to see if it is actually\n+\t\t# clean.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_success \"add a racily clean file to an already split index #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\tgit update-index --split-index &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update the split index. The cache entry of racy-file\n+\t\t# will be stored only in the split index.\n+\t\tgit update-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Subsequent git commands should notice that racy-file\n+\t\t# and the split index have the same mtime, and check\n+\t\t# the content of the file to see if it is actually\n+\t\t# clean.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_success \"split the index when the index contains a racily clean cache entry #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\tgit update-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update and split the index when the index contains\n+\t\t# the racily clean cache entry of racy-file.\n+\t\t# A corresponding replacement cache entry with smudged\n+\t\t# stat data should be added to the new split index.\n+\t\tgit update-index --split-index --add other-file &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data in the replacement cache entry and that it\n+\t\t# doesnt match with the file the worktree.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_success \"update the split index when it contains a new racily clean cache entry #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\tgit update-index --split-index &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update the split index. The cache entry of racy-file\n+\t\t# will be stored only in the split index.\n+\t\tgit update-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update the split index when the racily clean cache\n+\t\t# entry of racy-file is only stored in the split index.\n+\t\t# An updated cache entry with smudged stat data should\n+\t\t# be added to the new split index.\n+\t\tgit update-index --add other-file &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_failure \"update the split index when a racily clean cache entry is stored only in the shared index $trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update and split the index. The cache entry of\n+\t\t# racy-file will be stored only in the shared index.\n+\t\tgit update-index --split-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update the split index when the racily clean cache\n+\t\t# entry of racy-file is only stored in the shared index.\n+\t\t# A corresponding replacement cache entry with smudged\n+\t\t# stat data should be added to the new split index.\n+\t\t#\n+\t\t# Alas, such a smudged replacement entry is not added!\n+\t\tgit update-index --add other-file &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_failure \"update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update and split the index. The cache entry of\n+\t\t# racy-file will be stored only in the shared index.\n+\t\tgit update-index --split-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update the split index after unpack_trees() copied the\n+\t\t# racily clean cache entry of racy-file from the shared\n+\t\t# index. A corresponding replacement cache entry\n+\t\t# with smudged stat data should be added to the new\n+\t\t# split index.\n+\t\t#\n+\t\t# Alas, such a smudged replacement entry is not added!\n+\t\tgit read-tree -m HEAD &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+test_done\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359212","messageId":"20180928162459.17138-4-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180928162459.17138-1-szeder.dev@gmail.com","subject":"[PATCH v3 3/6] t1700-split-index: date back files to avoid racy situations","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-28T16:24:56Z","receivedAt":"2018-09-28T16:25:27Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'t1700-split-index.sh' checks that the index was split correctly under\nvarious circumstances and that all the different ways to turn the\nsplit index feature on and off work correctly. To do so, most of its\ntests use 'test-tool dump-split-index' to see which files have their\ncache entries in the split index. All these tests assume that all\ncache entries are written to the shared index (called \"base\"\nthroughout these tests) when a new shared index is created. This is\nan implementation detail: most git commands (basically all except 'git\nupdate-index') don't care or know at all about split index or whether\na cache entry is stored in the split or shared index.\n\nAs demonstrated in the previous patch, refreshing a split index is\nprone to a variant of the classic racy git issue. The next patch will\nfix this issue, but while doing so it will also slightly change this\nbehaviour: only cache entries with mtime in the past will be written\nonly to the newly created shared index, but racily clean cache entries\nwill be written to the new split index (with smudged stat data).\n\nWhile this upcoming change won't at all affect any git commands, it\nwill violate the above mentioned assumption of 't1700's tests. Since\nthese tests create or modify files and create or refresh the split\nindex in rapid succession, there are plenty of racily clean cache\nentries to be dealt with, which will then be written to the new split\nindexes, and, ultimately, will cause several tests in 't1700' to fail.\n\nLet's prepare 't1700-split-index.sh' for this upcoming change and\nmodify its tests to avoid racily clean files by backdating the mtime\nof any file modifications (and since a lot of tests create or modify\nfiles, encapsulate it into a helper function).\n\nSigned-off-by: SZEDER Gábor \n---\n t/t1700-split-index.sh | 49 ++++++++++++++++++++++++------------------\n 1 file changed, 28 insertions(+), 21 deletions(-)\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 822257ff7d..f053bf83eb 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -11,6 +11,13 @@ sane_unset GIT_TEST_SPLIT_INDEX\n # with those checks, so disable it in this test script.\n sane_unset GIT_FSMONITOR_TEST\n \n+# Create a file named as $1 with content read from stdin.\n+# Set the file's mtime to a few seconds in the past to avoid racy situations.\n+create_non_racy_file () {\n+\tcat >\"$1\" &&\n+\ttest-tool chmtime =-5 \"$1\"\n+}\n+\n test_expect_success 'enable split index' '\n \tgit config splitIndex.maxPercentChange 100 &&\n \tgit update-index --split-index &&\n@@ -34,7 +41,7 @@ test_expect_success 'enable split index' '\n '\n \n test_expect_success 'add one file' '\n-\t: >one &&\n+\tcreate_non_racy_file one &&\n \tgit update-index --add one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -86,7 +93,7 @@ test_expect_success 'enable split index again, \"one\" now belongs to base index\"'\n '\n \n test_expect_success 'modify original file, base index untouched' '\n-\techo modified >one &&\n+\techo modified | create_non_racy_file one &&\n \tgit update-index one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -105,7 +112,7 @@ test_expect_success 'modify original file, base index untouched' '\n '\n \n test_expect_success 'add another file, which stays index' '\n-\t: >two &&\n+\tcreate_non_racy_file two &&\n \tgit update-index --add two &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -158,7 +165,7 @@ test_expect_success 'remove file in base index' '\n '\n \n test_expect_success 'add original file back' '\n-\t: >one &&\n+\tcreate_non_racy_file one &&\n \tgit update-index --add one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -177,7 +184,7 @@ test_expect_success 'add original file back' '\n '\n \n test_expect_success 'add new file' '\n-\t: >two &&\n+\tcreate_non_racy_file two &&\n \tgit update-index --add two &&\n \tgit ls-files --stage >actual &&\n \tcat >expect <<-EOF &&\n@@ -221,7 +228,7 @@ test_expect_success 'rev-parse --shared-index-path' '\n \n test_expect_success 'set core.splitIndex config variable to true' '\n \tgit config core.splitIndex true &&\n-\t: >three &&\n+\tcreate_non_racy_file three &&\n \tgit update-index --add three &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -256,9 +263,9 @@ test_expect_success 'set core.splitIndex config variable to false' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'set core.splitIndex config variable to true' '\n+test_expect_success 'set core.splitIndex config variable back to true' '\n \tgit config core.splitIndex true &&\n-\t: >three &&\n+\tcreate_non_racy_file three &&\n \tgit update-index --add three &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -268,7 +275,7 @@ test_expect_success 'set core.splitIndex config variable to true' '\n \tdeletions:\n \tEOF\n \ttest_cmp expect actual &&\n-\t: >four &&\n+\tcreate_non_racy_file four &&\n \tgit update-index --add four &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n \tcat >expect <<-EOF &&\n@@ -282,7 +289,7 @@ test_expect_success 'set core.splitIndex config variable to true' '\n \n test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '\n \tgit config --unset splitIndex.maxPercentChange &&\n-\t: >five &&\n+\tcreate_non_racy_file five &&\n \tgit update-index --add five &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -292,7 +299,7 @@ test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '\n \tdeletions:\n \tEOF\n \ttest_cmp expect actual &&\n-\t: >six &&\n+\tcreate_non_racy_file six &&\n \tgit update-index --add six &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n \tcat >expect <<-EOF &&\n@@ -306,7 +313,7 @@ test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '\n \n test_expect_success 'check splitIndex.maxPercentChange set to 0' '\n \tgit config splitIndex.maxPercentChange 0 &&\n-\t: >seven &&\n+\tcreate_non_racy_file seven &&\n \tgit update-index --add seven &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -316,7 +323,7 @@ test_expect_success 'check splitIndex.maxPercentChange set to 0' '\n \tdeletions:\n \tEOF\n \ttest_cmp expect actual &&\n-\t: >eight &&\n+\tcreate_non_racy_file eight &&\n \tgit update-index --add eight &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -329,17 +336,17 @@ test_expect_success 'check splitIndex.maxPercentChange set to 0' '\n '\n \n test_expect_success 'shared index files expire after 2 weeks by default' '\n-\t: >ten &&\n+\tcreate_non_racy_file ten &&\n \tgit update-index --add ten &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_under_2_weeks_ago=$((5-14*86400)) &&\n \ttest-tool chmtime =$just_under_2_weeks_ago .git/sharedindex.* &&\n-\t: >eleven &&\n+\tcreate_non_racy_file eleven &&\n \tgit update-index --add eleven &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_over_2_weeks_ago=$((-1-14*86400)) &&\n \ttest-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&\n-\t: >twelve &&\n+\tcreate_non_racy_file twelve &&\n \tgit update-index --add twelve &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n@@ -347,12 +354,12 @@ test_expect_success 'shared index files expire after 2 weeks by default' '\n test_expect_success 'check splitIndex.sharedIndexExpire set to 16 days' '\n \tgit config splitIndex.sharedIndexExpire \"16.days.ago\" &&\n \ttest-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&\n-\t: >thirteen &&\n+\tcreate_non_racy_file thirteen &&\n \tgit update-index --add thirteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_over_16_days_ago=$((-1-16*86400)) &&\n \ttest-tool chmtime =$just_over_16_days_ago .git/sharedindex.* &&\n-\t: >fourteen &&\n+\tcreate_non_racy_file fourteen &&\n \tgit update-index --add fourteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n@@ -361,13 +368,13 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"\n \tgit config splitIndex.sharedIndexExpire never &&\n \tjust_10_years_ago=$((-365*10*86400)) &&\n \ttest-tool chmtime =$just_10_years_ago .git/sharedindex.* &&\n-\t: >fifteen &&\n+\tcreate_non_racy_file fifteen &&\n \tgit update-index --add fifteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tgit config splitIndex.sharedIndexExpire now &&\n \tjust_1_second_ago=-1 &&\n \ttest-tool chmtime =$just_1_second_ago .git/sharedindex.* &&\n-\t: >sixteen &&\n+\tcreate_non_racy_file sixteen &&\n \tgit update-index --add sixteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n@@ -382,7 +389,7 @@ do\n \t\t# Create one new shared index file\n \t\tgit config core.sharedrepository \"$mode\" &&\n \t\tgit config core.splitIndex true &&\n-\t\t: >one &&\n+\t\tcreate_non_racy_file one &&\n \t\tgit update-index --add one &&\n \t\techo \"$modebits\" >expect &&\n \t\ttest_modebits .git/index >actual &&\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359213","messageId":"20180928162459.17138-5-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180928162459.17138-1-szeder.dev@gmail.com","subject":"[PATCH v3 4/6] split-index: count the number of deleted entries","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-28T16:24:57Z","receivedAt":"2018-09-28T16:25:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'struct split_index' contains the field 'nr_deletions', whose name\nwith the 'nr_' prefix suggests that it contains the number of deleted\ncache entries. However, barring its initialization to 0, this field\nis only ever set to 1, indicating that there is at least one deleted\nentry, but not the number of deleted entries. Luckily, this doesn't\ncause any issues (other than confusing the reader, that is), because\nthe only place reading this field uses it in the same sense, i.e.: 'if\n(si->nr_deletions)'.\n\nTo avoid confusion, we could either rename this field to something\nlike 'has_deletions' to make its name match its role, or make it a\ncounter of deleted cache entries to match its name.\n\nLet's make it a counter, to keep it in sync with the related field\n'nr_replacements', which does contain the number of replaced cache\nentries. This will also give developers debugging the split index\ncode easy access to the number of deleted cache entries.\n\nSigned-off-by: SZEDER Gábor \n---\n split-index.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/split-index.c b/split-index.c\nindex 84f067e10d..548272ec33 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -111,7 +111,7 @@ static void mark_entry_for_delete(size_t pos, void *data)\n \t\tdie(\"position for delete %d exceeds base index size %d\",\n \t\t (int)pos, istate->cache_nr);\n \tistate->cache[pos]->ce_flags |= CE_REMOVE;\n-\tistate->split_index->nr_deletions = 1;\n+\tistate->split_index->nr_deletions++;\n }\n \n static void replace_entry(size_t pos, void *data)\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359214","messageId":"20180928162459.17138-6-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180928162459.17138-1-szeder.dev@gmail.com","subject":"[PATCH v3 5/6] split-index: don't compare stat data of entries already marked for split index","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-28T16:24:58Z","receivedAt":"2018-09-28T16:25:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When unpack_trees() constructs a new index, it copies cache entries\nfrom the original index [1]. prepare_to_write_split_index() has to\ndeal with this, and it has a dedicated code path for copied entries\nthat are present in the shared index, where it compares the cached\ndata in the corresponding copied and original entries. If the cached\ndata matches, then they are considered the same; if it differs, then\nthe copied entry will be marked for inclusion as a replacement entry\nin the just about to be written split index by setting the\nCE_UPDATE_IN_BASE flag.\n\nHowever, a cache entry already has its CE_UPDATE_IN_BASE flag set upon\nreading the split index, if the entry already has a replacement entry\nthere, or upon refreshing the cached stat data, if the corresponding\nfile was modified. The state of this flag is then preserved when\nunpack_trees() copies a cache entry from the shared index.\n\nSo modify prepare_to_write_split_index() to check the copied cache\nentries' CE_UPDATE_IN_BASE flag first, and skip the thorough\ncomparison of cached data if the flag is already set.\n\nNote that comparing the cached data in copied and original entries in\nthe shared index might actually be entirely unnecessary. In theory\nall code paths refreshing the cached stat data of an entry in the\nshared index should set the CE_UPDATE_IN_BASE flag in that entry, and\nunpack_trees() should preserve this flag when copying cache entries.\nThis means that the cached data is only ever changed if the\nCE_UPDATE_IN_BASE flag is set as well. Our test suite seems to\nconfirm this: instrumenting the conditions in question and running the\ntest suite repeatedly with 'GIT_TEST_SPLIT_INDEX=yes' showed that the\ncached data in a copied entry differs from the data in the shared\nentry only if its CE_UPDATE_IN_BASE flag is indeed set.\n\nIn practice, however, our test suite doesn't have 100% coverage,\nGIT_TEST_SPLIT_INDEX is inherently random, and I certainly can't claim\nto possess complete understanding of what goes on in unpack_trees()...\nTherefore I kept the comparison of the cached data when\nCE_UPDATE_IN_BASE is not set, just in case that an unnoticed or future\ncode path were to accidentally miss setting this flag upon refreshing\nthe cached stat data or unpack_trees() were to drop this flag while\ncopying a cache entry.\n\n[1] Note that when unpack_trees() constructs the new index and decides\n that a cache entry should now refer to different content than what\n was recorded in the original index (e.g. 'git read-tree -m\n HEAD^'), then that can't really be considered a copy of the\n original, but rather the creation of a new entry. Notably and\n pertinent to the split index feature, such a new entry doesn't\n have a reference to the original's shared index entry anymore,\n i.e. its 'index' field is set to 0. Consequently, such an entry\n is treated by prepare_to_write_split_index() as an entry not\n present in the shared index and it will be added to the new split\n index, while the original entry will be marked as deleted, and\n neither the above discussion nor the changes in this patch apply\n to them.\n\nSigned-off-by: SZEDER Gábor \n---\n split-index.c | 79 ++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 62 insertions(+), 17 deletions(-)\n\ndiff --git a/split-index.c b/split-index.c\nindex 548272ec33..7d8799f6b7 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -207,13 +207,28 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t */\n \t\tfor (i = 0; i < istate->cache_nr; i++) {\n \t\t\tstruct cache_entry *base;\n-\t\t\t/* namelen is checked separately */\n-\t\t\tconst unsigned int ondisk_flags =\n-\t\t\t\tCE_STAGEMASK | CE_VALID | CE_EXTENDED_FLAGS;\n-\t\t\tunsigned int ce_flags, base_flags, ret;\n \t\t\tce = istate->cache[i];\n-\t\t\tif (!ce->index)\n+\t\t\tif (!ce->index) {\n+\t\t\t\t/*\n+\t\t\t\t * During simple update index operations this\n+\t\t\t\t * is a cache entry that is not present in\n+\t\t\t\t * the shared index. It will be added to the\n+\t\t\t\t * split index.\n+\t\t\t\t *\n+\t\t\t\t * However, it might also represent a file\n+\t\t\t\t * that already has a cache entry in the\n+\t\t\t\t * shared index, but a new index has just\n+\t\t\t\t * been constructed by unpack_trees(), and\n+\t\t\t\t * this entry now refers to different content\n+\t\t\t\t * than what was recorded in the original\n+\t\t\t\t * index, e.g. during 'read-tree -m HEAD^' or\n+\t\t\t\t * 'checkout HEAD^'. In this case the\n+\t\t\t\t * original entry in the shared index will be\n+\t\t\t\t * marked as deleted, and this entry will be\n+\t\t\t\t * added to the split index.\n+\t\t\t\t */\n \t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (ce->index > si->base->cache_nr) {\n \t\t\t\tce->index = 0;\n \t\t\t\tcontinue;\n@@ -227,18 +242,48 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t\tce->index = 0;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tce_flags = ce->ce_flags;\n-\t\t\tbase_flags = base->ce_flags;\n-\t\t\t/* only on-disk flags matter */\n-\t\t\tce->ce_flags &= ondisk_flags;\n-\t\t\tbase->ce_flags &= ondisk_flags;\n-\t\t\tret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,\n-\t\t\t\t offsetof(struct cache_entry, name) -\n-\t\t\t\t offsetof(struct cache_entry, ce_stat_data));\n-\t\t\tce->ce_flags = ce_flags;\n-\t\t\tbase->ce_flags = base_flags;\n-\t\t\tif (ret)\n-\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\t\t\t/*\n+\t\t\t * This is the copy of a cache entry that is present\n+\t\t\t * in the shared index, created by unpack_trees()\n+\t\t\t * while it constructed a new index.\n+\t\t\t */\n+\t\t\tif (ce->ce_flags & CE_UPDATE_IN_BASE) {\n+\t\t\t\t/*\n+\t\t\t\t * Already marked for inclusion in the split\n+\t\t\t\t * index, either because the corresponding\n+\t\t\t\t * file was modified and the cached stat data\n+\t\t\t\t * was refreshed, or because the original\n+\t\t\t\t * entry already had a replacement entry in\n+\t\t\t\t * the split index.\n+\t\t\t\t * Nothing to do.\n+\t\t\t\t */\n+\t\t\t} else {\n+\t\t\t\t/*\n+\t\t\t\t * Thoroughly compare the cached data to see\n+\t\t\t\t * whether it should be marked for inclusion\n+\t\t\t\t * in the split index.\n+\t\t\t\t *\n+\t\t\t\t * This comparison might be unnecessary, as\n+\t\t\t\t * code paths modifying the cached data do\n+\t\t\t\t * set CE_UPDATE_IN_BASE as well.\n+\t\t\t\t */\n+\t\t\t\tconst unsigned int ondisk_flags =\n+\t\t\t\t\tCE_STAGEMASK | CE_VALID |\n+\t\t\t\t\tCE_EXTENDED_FLAGS;\n+\t\t\t\tunsigned int ce_flags, base_flags, ret;\n+\t\t\t\tce_flags = ce->ce_flags;\n+\t\t\t\tbase_flags = base->ce_flags;\n+\t\t\t\t/* only on-disk flags matter */\n+\t\t\t\tce->ce_flags &= ondisk_flags;\n+\t\t\t\tbase->ce_flags &= ondisk_flags;\n+\t\t\t\tret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,\n+\t\t\t\t\t offsetof(struct cache_entry, name) -\n+\t\t\t\t\t offsetof(struct cache_entry, ce_stat_data));\n+\t\t\t\tce->ce_flags = ce_flags;\n+\t\t\t\tbase->ce_flags = base_flags;\n+\t\t\t\tif (ret)\n+\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\t\t\t}\n \t\t\tdiscard_cache_entry(base);\n \t\t\tsi->base->cache[ce->index - 1] = ce;\n \t\t}\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359215","messageId":"20180928162459.17138-7-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180928162459.17138-1-szeder.dev@gmail.com","subject":"[PATCH v3 6/6] split-index: smudge and add racily clean cache entries to split index","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-28T16:24:59Z","receivedAt":"2018-09-28T16:25:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Ever since the split index feature was introduced [1], refreshing a\nsplit index is prone to a variant of the classic racy git problem.\n\nConsider the following sequence of commands updating the split index\nwhen the shared index contains a racily clean cache entry, i.e. an\nentry whose cached stat data matches with the corresponding file in\nthe worktree and the cached mtime matches that of the index:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file # size stays the same!\n # ... wait ...\n git update-index --add other-file\n\nNormally, when a non-split index is updated, then do_write_index()\n(the function responsible for writing all kinds of indexes, \"regular\",\nsplit, and shared) recognizes racily clean cache entries, and writes\nthem with smudged stat data, i.e. with file size set to 0. When\nsubsequent git commands read the index, they will notice that the\nsmudged stat data doesn't match with the file in the worktree, and\nthen go on to check the file's content and notice its dirtiness.\n\nIn the above example, however, in the second 'git update-index'\nprepare_to_write_split_index() decides which cache entries stored only\nin the shared index should be replaced in the new split index. Alas,\nthis function never looks out for racily clean cache entries, and\nsince the file's stat data in the worktree hasn't changed since the\nshared index was written, it won't be replaced in the new split index.\nConsequently, do_write_index() doesn't even get this racily clean\ncache entry, and can't smudge its stat data. Subsequent git commands\nwill then see that the index has more recent mtime than the file and\nthat the (not smudged) cached stat data still matches with the file in\nthe worktree, and, ultimately, will erroneously consider the file\nclean.\n\nModify prepare_to_write_split_index() to recognize racily clean cache\nentries, and mark them to be added to the split index. Note that\nthere are two places where it should check raciness: first those cache\nentries that are only stored in the shared index, and then those that\nhave been copied by unpack_trees() from the shared index while it\nconstructed a new index. This way do_write_index() will get these\nracily clean cache entries as well, and will then write them with\nsmudged stat data to the new split index.\n\nNote that after this change if the index is split when it contains a\nracily clean cache entry, then a smudged cache entry will be written\nboth to the new shared and to the new split indexes. This doesn't\naffect regular git commands: as far as they are concerned this is just\nan entry in the split index replacing an outdated entry in the shared\nindex. It did affect a few tests in 't1700-split-index.sh', though,\nbecause they actually check which entries are stored in the split\nindex; a previous patch in this series has already made the necessary\nadjustments in 't1700'. And racily clean cache entries and index\nsplitting are rare enough to not worry about the resulting duplicated\nsmudged cache entries, and the additional complexity required to\nprevent them is not worth it.\n\nSeveral tests failed occasionally when the test suite was run with\n'GIT_TEST_SPLIT_INDEX=yes'. Here are those that I managed to trace\nback to this racy split index problem, starting with those failing\nmore frequently, with a link to a failing Travis CI build job for\neach. The highlighted line [2] shows when the racy file was written,\nwhich is not always in the failing test but in a preceeding setup\ntest.\n\n t3903-stash.sh:\n https://travis-ci.org/git/git/jobs/385542084#L5858\n\n t4024-diff-optimize-common.sh:\n https://travis-ci.org/git/git/jobs/386531969#L3174\n\n t4015-diff-whitespace.sh:\n https://travis-ci.org/git/git/jobs/360797600#L8215\n\n t2200-add-update.sh:\n https://travis-ci.org/git/git/jobs/382543426#L3051\n\n t0090-cache-tree.sh:\n https://travis-ci.org/git/git/jobs/416583010#L3679\n\nThere might be others, e.g. perhaps 't1000-read-tree-m-3way.sh' and\nothers using 'lib-read-tree-m-3way.sh', but I couldn't confirm yet.\n\n[1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge\n branch 'nd/split-index', 2014-07-16).\n\n[2] Note that those highlighted lines are in the 'after failure' fold,\n and your browser might unhelpfully fold it up before you could\n take a good look.\n\nSigned-off-by: SZEDER Gábor \n---\n cache.h | 2 ++\n read-cache.c | 2 +-\n split-index.c | 42 ++++++++++++++++++++++++++++++++++++-\n t/t1701-racy-split-index.sh | 8 ++-----\n 4 files changed, 46 insertions(+), 8 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4d014541ab..3f419b6c79 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -781,6 +781,8 @@ extern void *read_blob_data_from_index(const struct index_state *, const char *,\n #define CE_MATCH_REFRESH\t\t0x10\n /* don't refresh_fsmonitor state or do stat comparison even if CE_FSMONITOR_VALID is true */\n #define CE_MATCH_IGNORE_FSMONITOR 0X20\n+extern int is_racy_timestamp(const struct index_state *istate,\n+\t\t\t const struct cache_entry *ce);\n extern int ie_match_stat(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n extern int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 7b1354d759..8f644f68b4 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -337,7 +337,7 @@ static int is_racy_stat(const struct index_state *istate,\n \t\t);\n }\n \n-static int is_racy_timestamp(const struct index_state *istate,\n+int is_racy_timestamp(const struct index_state *istate,\n \t\t\t const struct cache_entry *ce)\n {\n \treturn (!S_ISGITLINK(ce->ce_mode) &&\ndiff --git a/split-index.c b/split-index.c\nindex 7d8799f6b7..d989b27286 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -235,8 +235,39 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t}\n \t\t\tce->ce_flags |= CE_MATCHED; /* or \"shared\" */\n \t\t\tbase = si->base->cache[ce->index - 1];\n-\t\t\tif (ce == base)\n+\t\t\tif (ce == base) {\n+\t\t\t\t/* The entry is present in the shared index. */\n+\t\t\t\tif (ce->ce_flags & CE_UPDATE_IN_BASE) {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * Already marked for inclusion in\n+\t\t\t\t\t * the split index, either because\n+\t\t\t\t\t * the corresponding file was\n+\t\t\t\t\t * modified and the cached stat data\n+\t\t\t\t\t * was refreshed, or because there\n+\t\t\t\t\t * is already a replacement entry in\n+\t\t\t\t\t * the split index.\n+\t\t\t\t\t * Nothing more to do here.\n+\t\t\t\t\t */\n+\t\t\t\t} else if (!ce_uptodate(ce) &&\n+\t\t\t\t\t is_racy_timestamp(istate, ce)) {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * A racily clean cache entry stored\n+\t\t\t\t\t * only in the shared index: it must\n+\t\t\t\t\t * be added to the split index, so\n+\t\t\t\t\t * the subsequent do_write_index()\n+\t\t\t\t\t * can smudge its stat data.\n+\t\t\t\t\t */\n+\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\t\t\t\t} else {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * The entry is only present in the\n+\t\t\t\t\t * shared index and it was not\n+\t\t\t\t\t * refreshed.\n+\t\t\t\t\t * Just leave it there.\n+\t\t\t\t\t */\n+\t\t\t\t}\n \t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (ce->ce_namelen != base->ce_namelen ||\n \t\t\t strcmp(ce->name, base->name)) {\n \t\t\t\tce->index = 0;\n@@ -257,6 +288,15 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t\t * the split index.\n \t\t\t\t * Nothing to do.\n \t\t\t\t */\n+\t\t\t} else if (!ce_uptodate(ce) &&\n+\t\t\t\t is_racy_timestamp(istate, ce)) {\n+\t\t\t\t/*\n+\t\t\t\t * A copy of a racily clean cache entry from\n+\t\t\t\t * the shared index. It must be added to\n+\t\t\t\t * the split index, so the subsequent\n+\t\t\t\t * do_write_index() can smudge its stat data.\n+\t\t\t\t */\n+\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n \t\t\t} else {\n \t\t\t\t/*\n \t\t\t\t * Thoroughly compare the cached data to see\ndiff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\nindex fbb77046da..5dc221ef38 100755\n--- a/t/t1701-racy-split-index.sh\n+++ b/t/t1701-racy-split-index.sh\n@@ -148,7 +148,7 @@ done\n \n for trial in $trials\n do\n-\ttest_expect_failure \"update the split index when a racily clean cache entry is stored only in the shared index $trial\" '\n+\ttest_expect_success \"update the split index when a racily clean cache entry is stored only in the shared index #$trial\" '\n \t\trm -f .git/index .git/sharedindex.* &&\n \n \t\t# The next three commands must be run within the same\n@@ -170,8 +170,6 @@ do\n \t\t# entry of racy-file is only stored in the shared index.\n \t\t# A corresponding replacement cache entry with smudged\n \t\t# stat data should be added to the new split index.\n-\t\t#\n-\t\t# Alas, such a smudged replacement entry is not added!\n \t\tgit update-index --add other-file &&\n \n \t\t# Subsequent git commands should notice the smudged\n@@ -182,7 +180,7 @@ done\n \n for trial in $trials\n do\n-\ttest_expect_failure \"update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial\" '\n+\ttest_expect_success \"update the split index after unpack trees() copied a racily clean cache entry from the shared index #$trial\" '\n \t\trm -f .git/index .git/sharedindex.* &&\n \n \t\t# The next three commands must be run within the same\n@@ -205,8 +203,6 @@ do\n \t\t# index. A corresponding replacement cache entry\n \t\t# with smudged stat data should be added to the new\n \t\t# split index.\n-\t\t#\n-\t\t# Alas, such a smudged replacement entry is not added!\n \t\tgit read-tree -m HEAD &&\n \n \t\t# Subsequent git commands should notice the smudged\n-- \n2.19.0.361.gafc87ffe72\n\n"},{"id":"359223","messageId":"xmqqo9chcyxi.fsf@gitster-ct.c.googlers.com","threadId":"49437","inReplyTo":"20180928004843.GP27036@localhost","subject":"Re: [PATCH v2 1/5] split-index: add tests to demonstrate the racy split index problem","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-28T17:30:17Z","receivedAt":"2018-09-28T17:30:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Junio,\n>\n> On Thu, Sep 27, 2018 at 02:44:30PM +0200, SZEDER Gábor wrote:\n>> diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\n>> new file mode 100755\n>> index 0000000000..ebde418d7e\n>> --- /dev/null\n>> +++ b/t/t1701-racy-split-index.sh\n>> @@ -0,0 +1,218 @@\n>> +#!/bin/sh\n>> +\n>> +# This test can give false success if your machine is sufficiently\n>> +# slow or all trials happened to happen on second boundaries.\n>> +\n>> +test_description='racy split index'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +test_expect_success 'setup' '\n>> +\t# Only split the index when the test explicitly says so.\n>> +\tsane_unset GIT_TEST_SPLIT_INDEX GIT_FSMONITOR_TEST &&\n>\n> Please note that this patch adds another use of the environment\n> variable GIT_FSMONITOR_TEST, while the topic 'bp/rename-test-env-var'\n> is about to rename that variable to GIT_TEST_FSMONITOR.\n\nI think the most sensible thing to do during transition is to set\n(or unset) both consistently (and leave a note in t/test-lib.sh near\ncheck_var_migration that t1701 has done so and the dup should be\ncorrected when transition is over---but that is optional).\n\n\n"},{"id":"359266","messageId":"20180929052136.GA20349@duynguyen.home","threadId":"49437","inReplyTo":"20180928162459.17138-7-szeder.dev@gmail.com","subject":"Re: [PATCH v3 6/6] split-index: smudge and add racily clean cache entries to split index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-29T05:21:36Z","receivedAt":"2018-09-29T05:21:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Sep 28, 2018 at 06:24:59PM +0200, SZEDER Gábor wrote:\n> diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\n> index fbb77046da..5dc221ef38 100755\n> --- a/t/t1701-racy-split-index.sh\n> +++ b/t/t1701-racy-split-index.sh\n> @@ -148,7 +148,7 @@ done\n> \n> for trial in $trials\n> do\n> -\ttest_expect_failure \"update the split index when a racily clean cache entry is stored only in the shared index $trial\" '\n> +\ttest_expect_success \"update the split index when a racily clean cache entry is stored only in the shared index #$trial\" '\n\nThe the new '#' before '$trial' intended?\n\n> \t\trm -f .git/index .git/sharedindex.* &&\n> \n> \t\t# The next three commands must be run within the same\n> @@ -170,8 +170,6 @@ do\n> \t\t# entry of racy-file is only stored in the shared index.\n> \t\t# A corresponding replacement cache entry with smudged\n> \t\t# stat data should be added to the new split index.\n> -\t\t#\n> -\t\t# Alas, such a smudged replacement entry is not added!\n> \t\tgit update-index --add other-file &&\n> \n> \t\t# Subsequent git commands should notice the smudged\n> @@ -182,7 +180,7 @@ done\n> \n> for trial in $trials\n> do\n> -\ttest_expect_failure \"update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial\" '\n> +\ttest_expect_success \"update the split index after unpack trees() copied a racily clean cache entry from the shared index #$trial\" '\n> \t\trm -f .git/index .git/sharedindex.* &&\n> \n> \t\t# The next three commands must be run within the same\n--\nDuy\n"},{"id":"359267","messageId":"20180929053608.GB20349@duynguyen.home","threadId":"49437","inReplyTo":"20180928162459.17138-6-szeder.dev@gmail.com","subject":"Re: [PATCH v3 5/6] split-index: don't compare stat data of entries already marked for split index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-29T05:36:08Z","receivedAt":"2018-09-29T05:36:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Sep 28, 2018 at 06:24:58PM +0200, SZEDER Gábor wrote:\n> When unpack_trees() constructs a new index, it copies cache entries\n> from the original index [1]. prepare_to_write_split_index() has to\n> deal with this, and it has a dedicated code path for copied entries\n> that are present in the shared index, where it compares the cached\n> data in the corresponding copied and original entries. If the cached\n> data matches, then they are considered the same; if it differs, then\n> the copied entry will be marked for inclusion as a replacement entry\n> in the just about to be written split index by setting the\n> CE_UPDATE_IN_BASE flag.\n> \n> However, a cache entry already has its CE_UPDATE_IN_BASE flag set upon\n> reading the split index, if the entry already has a replacement entry\n> there, or upon refreshing the cached stat data, if the corresponding\n> file was modified. The state of this flag is then preserved when\n> unpack_trees() copies a cache entry from the shared index.\n> \n> So modify prepare_to_write_split_index() to check the copied cache\n> entries' CE_UPDATE_IN_BASE flag first, and skip the thorough\n> comparison of cached data if the flag is already set.\n\nOK so this is an optimization, not a bug fix. Right?\n\n> Note that comparing the cached data in copied and original entries in\n\ns/cached data/cached stat data/ ? I was confused for a bit.\n\n> the shared index might actually be entirely unnecessary. In theory\n> all code paths refreshing the cached stat data of an entry in the\n> shared index should set the CE_UPDATE_IN_BASE flag in that entry, and\n> unpack_trees() should preserve this flag when copying cache entries.\n> This means that the cached data is only ever changed if the\n> CE_UPDATE_IN_BASE flag is set as well. Our test suite seems to\n> confirm this: instrumenting the conditions in question and running the\n> test suite repeatedly with 'GIT_TEST_SPLIT_INDEX=yes' showed that the\n> cached data in a copied entry differs from the data in the shared\n> entry only if its CE_UPDATE_IN_BASE flag is indeed set.\n\nYes I was probably just being paranoid (or sticking to simpler\nchecks). I was told that split index is computation expensive and not\ndoing unnecesary/expensive checks may help. But let's leave it for\nlater.\n\n> +\t\t\t} else {\n> +\t\t\t\t/*\n> +\t\t\t\t * Thoroughly compare the cached data to see\n> +\t\t\t\t * whether it should be marked for inclusion\n> +\t\t\t\t * in the split index.\n> +\t\t\t\t *\n> +\t\t\t\t * This comparison might be unnecessary, as\n> +\t\t\t\t * code paths modifying the cached data do\n> +\t\t\t\t * set CE_UPDATE_IN_BASE as well.\n> +\t\t\t\t */\n> +\t\t\t\tconst unsigned int ondisk_flags =\n> +\t\t\t\t\tCE_STAGEMASK | CE_VALID |\n> +\t\t\t\t\tCE_EXTENDED_FLAGS;\n> +\t\t\t\tunsigned int ce_flags, base_flags, ret;\n> +\t\t\t\tce_flags = ce->ce_flags;\n> +\t\t\t\tbase_flags = base->ce_flags;\n> +\t\t\t\t/* only on-disk flags matter */\n> +\t\t\t\tce->ce_flags &= ondisk_flags;\n> +\t\t\t\tbase->ce_flags &= ondisk_flags;\n> +\t\t\t\tret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,\n> +\t\t\t\t\t offsetof(struct cache_entry, name) -\n> +\t\t\t\t\t offsetof(struct cache_entry, ce_stat_data));\n> +\t\t\t\tce->ce_flags = ce_flags;\n> +\t\t\t\tbase->ce_flags = base_flags;\n\nMaybe make this block a separate function (compare_ce_content or\nsomething). The amount of indentation is getting too high.\n\n> +\t\t\t\tif (ret)\n> +\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n> +\t\t\t}\n> \t\t\tdiscard_cache_entry(base);\n> \t\t\tsi->base->cache[ce->index - 1] = ce;\n> \t\t}\n> -- \n> 2.19.0.361.gafc87ffe72\n> \n"},{"id":"359279","messageId":"20180929075726.GE23446@localhost","threadId":"49437","inReplyTo":"20180929052136.GA20349@duynguyen.home","subject":"Re: [PATCH v3 6/6] split-index: smudge and add racily clean cache entries to split index","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-29T07:57:26Z","receivedAt":"2018-09-29T07:57:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Sep 29, 2018 at 07:21:36AM +0200, Duy Nguyen wrote:\n> On Fri, Sep 28, 2018 at 06:24:59PM +0200, SZEDER Gábor wrote:\n> > diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\n> > index fbb77046da..5dc221ef38 100755\n> > --- a/t/t1701-racy-split-index.sh\n> > +++ b/t/t1701-racy-split-index.sh\n> > @@ -148,7 +148,7 @@ done\n> > \n> > for trial in $trials\n> > do\n> > -\ttest_expect_failure \"update the split index when a racily clean cache entry is stored only in the shared index $trial\" '\n> > +\ttest_expect_success \"update the split index when a racily clean cache entry is stored only in the shared index #$trial\" '\n> \n> The the new '#' before '$trial' intended?\n\nYes, they only cause troubles for 'prove' (or in the TAP output in\ngeneral) in 'test_expect_failure', all the previous tests already have\n'#$trial', just like the related tests in 't0010-racy-git.sh'. And\nmany more tests have '#' in their names:\n\n $ git grep 'test_expect_success.*#' master |wc -l\n 121\n\n\n> \n> > \t\trm -f .git/index .git/sharedindex.* &&\n> > \n> > \t\t# The next three commands must be run within the same\n> > @@ -170,8 +170,6 @@ do\n> > \t\t# entry of racy-file is only stored in the shared index.\n> > \t\t# A corresponding replacement cache entry with smudged\n> > \t\t# stat data should be added to the new split index.\n> > -\t\t#\n> > -\t\t# Alas, such a smudged replacement entry is not added!\n> > \t\tgit update-index --add other-file &&\n> > \n> > \t\t# Subsequent git commands should notice the smudged\n> > @@ -182,7 +180,7 @@ done\n> > \n> > for trial in $trials\n> > do\n> > -\ttest_expect_failure \"update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial\" '\n> > +\ttest_expect_success \"update the split index after unpack trees() copied a racily clean cache entry from the shared index #$trial\" '\n> > \t\trm -f .git/index .git/sharedindex.* &&\n> > \n> > \t\t# The next three commands must be run within the same\n> --\n> Duy\n"},{"id":"359283","messageId":"20180929091429.GF23446@localhost","threadId":"49437","inReplyTo":"20180929053608.GB20349@duynguyen.home","subject":"Re: [PATCH v3 5/6] split-index: don't compare stat data of entries already marked for split index","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-29T09:14:29Z","receivedAt":"2018-09-29T09:14:36Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Sep 29, 2018 at 07:36:08AM +0200, Duy Nguyen wrote:\n> On Fri, Sep 28, 2018 at 06:24:58PM +0200, SZEDER Gábor wrote:\n> > When unpack_trees() constructs a new index, it copies cache entries\n> > from the original index [1]. prepare_to_write_split_index() has to\n> > deal with this, and it has a dedicated code path for copied entries\n> > that are present in the shared index, where it compares the cached\n> > data in the corresponding copied and original entries. If the cached\n> > data matches, then they are considered the same; if it differs, then\n> > the copied entry will be marked for inclusion as a replacement entry\n> > in the just about to be written split index by setting the\n> > CE_UPDATE_IN_BASE flag.\n> > \n> > However, a cache entry already has its CE_UPDATE_IN_BASE flag set upon\n> > reading the split index, if the entry already has a replacement entry\n> > there, or upon refreshing the cached stat data, if the corresponding\n> > file was modified. The state of this flag is then preserved when\n> > unpack_trees() copies a cache entry from the shared index.\n> > \n> > So modify prepare_to_write_split_index() to check the copied cache\n> > entries' CE_UPDATE_IN_BASE flag first, and skip the thorough\n> > comparison of cached data if the flag is already set.\n> \n> OK so this is an optimization, not a bug fix. Right?\n\nWell, a microoptimization at most: with all what's going on in\nunpack_trees() I seriously doubt that it's effect is measurable.\n\n> > Note that comparing the cached data in copied and original entries in\n> \n> s/cached data/cached stat data/ ? I was confused for a bit.\n\nNo, it's indeed cached data, but now that you mention it, the subject\nline does need a s/stat //.\n\nThe comparison is done with this call:\n\n ret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,\n offsetof(struct cache_entry, name) -\n offsetof(struct cache_entry, ce_stat_data));\n\ni.e. it starts at the stat data and ends just before the cache entry's\nname, and 'struct cache_entry' has several other fields between these\ntwo, including e.g. the cached oid:\n\n struct cache_entry {\n struct hashmap_entry ent;\n struct stat_data ce_stat_data;\n unsigned int ce_mode;\n unsigned int ce_flags;\n unsigned int mem_pool_allocated;\n unsigned int ce_namelen;\n unsigned int index; /* for link extension */\n struct object_id oid;\n char name[FLEX_ARRAY]; /* more */\n };\n\nHowever, to me it's mostly about clarity of the code, and about\ndocumenting that CE_UPDATE_IN_BASE might have already been set at that\npoint and why, so the next dev diving in to debug the split index\ndoesn't have to figure this out himself.\n\n> > the shared index might actually be entirely unnecessary. In theory\n> > all code paths refreshing the cached stat data of an entry in the\n> > shared index should set the CE_UPDATE_IN_BASE flag in that entry, and\n> > unpack_trees() should preserve this flag when copying cache entries.\n> > This means that the cached data is only ever changed if the\n> > CE_UPDATE_IN_BASE flag is set as well. Our test suite seems to\n> > confirm this: instrumenting the conditions in question and running the\n> > test suite repeatedly with 'GIT_TEST_SPLIT_INDEX=yes' showed that the\n> > cached data in a copied entry differs from the data in the shared\n> > entry only if its CE_UPDATE_IN_BASE flag is indeed set.\n> \n> Yes I was probably just being paranoid (or sticking to simpler\n> checks). I was told that split index is computation expensive and not\n> doing unnecesary/expensive checks may help. But let's leave it for\n> later.\n> \n> > +\t\t\t} else {\n> > +\t\t\t\t/*\n> > +\t\t\t\t * Thoroughly compare the cached data to see\n> > +\t\t\t\t * whether it should be marked for inclusion\n> > +\t\t\t\t * in the split index.\n> > +\t\t\t\t *\n> > +\t\t\t\t * This comparison might be unnecessary, as\n> > +\t\t\t\t * code paths modifying the cached data do\n> > +\t\t\t\t * set CE_UPDATE_IN_BASE as well.\n> > +\t\t\t\t */\n> > +\t\t\t\tconst unsigned int ondisk_flags =\n> > +\t\t\t\t\tCE_STAGEMASK | CE_VALID |\n> > +\t\t\t\t\tCE_EXTENDED_FLAGS;\n> > +\t\t\t\tunsigned int ce_flags, base_flags, ret;\n> > +\t\t\t\tce_flags = ce->ce_flags;\n> > +\t\t\t\tbase_flags = base->ce_flags;\n> > +\t\t\t\t/* only on-disk flags matter */\n> > +\t\t\t\tce->ce_flags &= ondisk_flags;\n> > +\t\t\t\tbase->ce_flags &= ondisk_flags;\n> > +\t\t\t\tret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,\n> > +\t\t\t\t\t offsetof(struct cache_entry, name) -\n> > +\t\t\t\t\t offsetof(struct cache_entry, ce_stat_data));\n> > +\t\t\t\tce->ce_flags = ce_flags;\n> > +\t\t\t\tbase->ce_flags = base_flags;\n> \n> Maybe make this block a separate function (compare_ce_content or\n> something). The amount of indentation is getting too high.\n\nAh, I was secretly hoping for something along the lines of \"your\nanalysis is correct, we can safely drop this comparison\" :)\n\nBtw, I thought about extracing this whole loop into a separate\nfunction like mark_updated_entries_for_split_index() or something, but\nthere are other things going on in this loop as well, e.g. marking\nwith CE_MATCHED and deduplicating copied entries, not to mention the\nconditions that set 'ce->index = 0', which I think should die() or\nBUG() or are unnecessary, see my followup email to this patch in v4:\n\n https://public-inbox.org/git/20180927134324.GI27036@localhost/\n\n> > +\t\t\t\tif (ret)\n> > +\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n> > +\t\t\t}\n> > \t\t\tdiscard_cache_entry(base);\n> > \t\t\tsi->base->cache[ce->index - 1] = ce;\n> > \t\t}\n> > -- \n> > 2.19.0.361.gafc87ffe72\n> > \n"},{"id":"359284","messageId":"20180929100737.GG23446@localhost","threadId":"49437","inReplyTo":"20180929091429.GF23446@localhost","subject":"Re: [PATCH v3 5/6] split-index: don't compare stat data of entries already marked for split index","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-29T10:07:37Z","receivedAt":"2018-09-29T10:07:43Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Sep 29, 2018 at 11:14:29AM +0200, SZEDER Gábor wrote:\n> > > Note that comparing the cached data in copied and original entries in\n> > \n> > s/cached data/cached stat data/ ? I was confused for a bit.\n> \n> No, it's indeed cached data, but now that you mention it, the subject\n> line does need a s/stat //.\n\nOr rather s/stat/cached/\n\n"},{"id":"359342","messageId":"20180930144731.GJ23446@localhost","threadId":"49437","inReplyTo":"20180928162459.17138-1-szeder.dev@gmail.com","subject":"Re: [PATCH v3 0/6] Fix the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-30T14:47:31Z","receivedAt":"2018-09-30T14:49:20Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Junio,\n\n\nOn Fri, Sep 28, 2018 at 06:24:53PM +0200, SZEDER Gábor wrote:\n> Interdiff:\n> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> index c8acbc50d0..f053bf83eb 100755\n> --- a/t/t1700-split-index.sh\n> +++ b/t/t1700-split-index.sh\n> @@ -6,6 +6,9 @@ test_description='split index mode tests'\n> \n> # We need total control of index splitting here\n> sane_unset GIT_TEST_SPLIT_INDEX\n\nThe conflict resolution around here in 3f725b07d3 (Merge branch\n'sg/split-index-racefix' into jch, 2018-09-29) accidentally removed\nthe above line, which makes the test fail when run with\n'GIT_TEST_SPLIT_INDEX=yes', e.g.:\n\n https://travis-ci.org/git/git/jobs/435077629#L3389\n\n> +# A couple of tests expect the index to have a specific checksum,\n> +# but the presence of the optional FSMN extension would interfere\n> +# with those checks, so disable it in this test script.\n> sane_unset GIT_FSMONITOR_TEST\n> \n> # Create a file named as $1 with content read from stdin.\n"},{"id":"359653","messageId":"xmqqbm883omo.fsf@gitster-ct.c.googlers.com","threadId":"49437","inReplyTo":"20180930144731.GJ23446@localhost","subject":"Re: [PATCH v3 0/6] Fix the racy split index problem","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-05T06:15:43Z","receivedAt":"2018-10-05T06:15:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Fri, Sep 28, 2018 at 06:24:53PM +0200, SZEDER Gábor wrote:\n>> Interdiff:\n>> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n>> index c8acbc50d0..f053bf83eb 100755\n>> --- a/t/t1700-split-index.sh\n>> +++ b/t/t1700-split-index.sh\n>> @@ -6,6 +6,9 @@ test_description='split index mode tests'\n>> \n>> # We need total control of index splitting here\n>> sane_unset GIT_TEST_SPLIT_INDEX\n>\n> The conflict resolution around here in 3f725b07d3 (Merge branch\n> 'sg/split-index-racefix' into jch, 2018-09-29) accidentally removed\n> the above line, ...\n\nYeah, I see it in \"git show --cc\". Will fix the rerere database\nentry to prevent the mismerge from recurring..\n\nSorry, and thanks for helping me correcting the mismerge.\n\n"},{"id":"359828","messageId":"878t38a3pg.fsf@evledraar.gmail.com","threadId":"49437","inReplyTo":"87k1n6hzwu.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 0/5] Fix the racy split index problem","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-08T14:54:51Z","receivedAt":"2018-10-08T14:54:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 28 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Thu, Sep 27 2018, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Thu, Sep 27 2018, SZEDER Gábor wrote:\n>>\n>>> On Thu, Sep 27, 2018 at 03:53:24PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>>>\n>>>> On Thu, Sep 27 2018, SZEDER Gábor wrote:\n>>>>\n>>>> > This is the second attempt to fix the racy split index problem, which\n>>>> > causes occasional failures in several random test scripts when run\n>>>> > with 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches\n>>>> > 1 and 5 (corresponding to v1's 3 and 5).\n>>>>\n>>>> Thanks. I'm running the same sorts of tests I noted in\n>>>> https://public-inbox.org/git/87va7ireuu.fsf@evledraar.gmail.com/ on\n>>>> this. The fix Jeff had that you noted in\n>>>> https://public-inbox.org/git/20180906151439.GA8016@localhost/ is now in\n>>>> \"master\".\n>>>>\n>>>> I take it your\n>>>> https://github.com/szeder/git/commits/racy-split-index-fix is the same\n>>>> as this submission?\n>>>\n>>> Yes.\n>>>\n>>>> Anyway, I'm testing that cherry-picked on top of the\n>>>> latest master.\n>>>>\n>>>> Unfortunate that we couldn't get the isolated test you made in\n>>>> https://public-inbox.org/git/20180907034942.GA10370@localhost/\n>>>\n>>> Nah, that's not an isolated test case, that's only a somewhat\n>>> narrowed-down, but rather reliable reproduction recipe while I still\n>>> had no idea what was going on :)\n>>>\n>>> The _real_ isolated test is the last test in t1701, that's what it\n>>> eventually boiled down to.\n>>\n>> Thanks. I had ~400 runs of the tests I ran before and they were all\n>> OK. Now trying also with t1701 (which I hadn't noticed was a new\n>> test...).\n>\n> Ran that overnight with the same conditions as before. 2683 OK runs and\n> 0 failures (and counting). So it seems like the combination of the two\n> fixed the split index bugs.\n\nI forgot I ad this running, and got up to 45482 OKs and 0 FAILs before\nfinally Ctrl+C-ing it now :)\n\n>>>> but I\n>>>> don't see how it could be added without some very liberal\n>>>> getenv(\"GIT_TEST_blahblah\"), so it's probably best to not add it,\n>>>> particularly with the C rewrite of git-stash in-flight.\n>>>>\n>>>> I'll report back when I have enough test data to say how these patches\n>>>> affect the intermittent test failures under GIT_TEST_SPLIT_INDEX=yes.\n"},{"id":"359835","messageId":"20181008154126.GY23446@localhost","threadId":"49437","inReplyTo":"878t38a3pg.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 0/5] Fix the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-08T15:41:26Z","receivedAt":"2018-10-08T15:41:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Oct 08, 2018 at 04:54:51PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> >> Thanks. I had ~400 runs of the tests I ran before and they were all\n> >> OK. Now trying also with t1701 (which I hadn't noticed was a new\n> >> test...).\n> >\n> > Ran that overnight with the same conditions as before. 2683 OK runs and\n> > 0 failures (and counting). So it seems like the combination of the two\n> > fixed the split index bugs.\n> \n> I forgot I ad this running, and got up to 45482 OKs and 0 FAILs before\n> finally Ctrl+C-ing it now :)\n\nYay! \\o/\n\nThanks for testing, and I feel sorry for your poor machine...\n\nI will send an updated version to address the (minor) points raised in\n[1]... well, most likely not today, but hopefully soon-ish.\n\n1 - https://public-inbox.org/git/20180929091429.GF23446@localhost/\n\n"},{"id":"360156","messageId":"20181011094309.18626-1-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20180928162459.17138-1-szeder.dev@gmail.com","subject":"[PATCH v4 0/6] Fix the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-11T09:43:03Z","receivedAt":"2018-10-11T09:43:19Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Fourth and hopefully final round of fixing occasional test failures when\nrun with 'GIT_TEST_SPLIT_INDEX=yes'. The only code change is the\nextraction of a helper function to compare two cache entries' content,\nand then a couple of minor log message clarifications. The range-diff\nbelow is rather clear on that.\n\nI will send a 7/6 follow-up patch shortly as well.\n\nSZEDER Gábor (6):\n t1700-split-index: document why FSMONITOR is disabled in this test\n script\n split-index: add tests to demonstrate the racy split index problem\n t1700-split-index: date back files to avoid racy situations\n split-index: count the number of deleted entries\n split-index: don't compare cached data of entries already marked for\n split index\n split-index: smudge and add racily clean cache entries to split index\n\n cache.h | 2 +\n read-cache.c | 2 +-\n split-index.c | 131 +++++++++++++++++++---\n t/t1700-split-index.sh | 52 +++++----\n t/t1701-racy-split-index.sh | 214 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 361 insertions(+), 40 deletions(-)\n create mode 100755 t/t1701-racy-split-index.sh\n\nRange-diff:\n1: ba2b1bdf16 = 1: ba2b1bdf16 t1700-split-index: document why FSMONITOR is disabled in this test script\n2: bf1b038f10 ! 2: c7cb9d9115 split-index: add tests to demonstrate the racy split index problem\n @@ -136,13 +136,20 @@\n git commands will then erroneously consider the file clean.\n \n Note that in the last two 'test_expect_failure' cases I omitted the\n - '#' (as in nr. of trial) from the tests' name on purpose for now, as\n - it confuses 'prove' into thinking that those tests failed\n - unexpectedly.\n + '#' (as in nr. of trial) from the tests' description on purpose for\n + now, as it breakes the TAP output [2]; it will be added at the end of\n + the series, when those two tests will be flipped to\n + 'test_expect_success'.\n \n [1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge\n branch 'nd/split-index', 2014-07-16).\n \n + [2] In the TAP output a '#' should separate the test's description\n + from the TODO directive emitted by 'test_expect_failure'. The\n + additional '#' in \"#$trial\" interferes with this, the test harness\n + won't recognize the TODO directive, and will report that those\n + tests failed unexpectedly.\n +\n Signed-off-by: SZEDER Gábor \n \n diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\n3: e7f7fb6e2d = 3: ce803d8064 t1700-split-index: date back files to avoid racy situations\n4: 6dc0b01ad3 = 4: 1d12d718d1 split-index: count the number of deleted entries\n5: 9c420f9c66 ! 5: 0dd448c707 split-index: don't compare stat data of entries already marked for split index\n @@ -1,6 +1,6 @@\n Author: SZEDER Gábor \n \n - split-index: don't compare stat data of entries already marked for split index\n + split-index: don't compare cached data of entries already marked for split index\n \n When unpack_trees() constructs a new index, it copies cache entries\n from the original index [1]. prepare_to_write_split_index() has to\n @@ -20,7 +20,9 @@\n \n So modify prepare_to_write_split_index() to check the copied cache\n entries' CE_UPDATE_IN_BASE flag first, and skip the thorough\n - comparison of cached data if the flag is already set.\n + comparison of cached data if the flag is already set. Those couple of\n + lines comparing the cached data would then have too many levels of\n + indentation, so extract them into a helper function.\n \n Note that comparing the cached data in copied and original entries in\n the shared index might actually be entirely unnecessary. In theory\n @@ -62,6 +64,37 @@\n diff --git a/split-index.c b/split-index.c\n --- a/split-index.c\n +++ b/split-index.c\n +@@\n + \tsi->saved_cache_nr = 0;\n + }\n + \n ++/*\n ++ * Compare most of the fields in two cache entries, i.e. all except the\n ++ * hashmap_entry and the name.\n ++ */\n ++static int compare_ce_content(struct cache_entry *a, struct cache_entry *b)\n ++{\n ++\tconst unsigned int ondisk_flags = CE_STAGEMASK | CE_VALID |\n ++\t\t\t\t\t CE_EXTENDED_FLAGS;\n ++\tunsigned int ce_flags = a->ce_flags;\n ++\tunsigned int base_flags = b->ce_flags;\n ++\tint ret;\n ++\n ++\t/* only on-disk flags matter */\n ++\ta->ce_flags &= ondisk_flags;\n ++\tb->ce_flags &= ondisk_flags;\n ++\tret = memcmp(&a->ce_stat_data, &b->ce_stat_data,\n ++\t\t offsetof(struct cache_entry, name) -\n ++\t\t offsetof(struct cache_entry, ce_stat_data));\n ++\ta->ce_flags = ce_flags;\n ++\tb->ce_flags = base_flags;\n ++\n ++\treturn ret;\n ++}\n ++\n + void prepare_to_write_split_index(struct index_state *istate)\n + {\n + \tstruct split_index *si = init_split_index(istate);\n @@\n \t\t */\n \t\tfor (i = 0; i < istate->cache_nr; i++) {\n @@ -137,21 +170,7 @@\n +\t\t\t\t * code paths modifying the cached data do\n +\t\t\t\t * set CE_UPDATE_IN_BASE as well.\n +\t\t\t\t */\n -+\t\t\t\tconst unsigned int ondisk_flags =\n -+\t\t\t\t\tCE_STAGEMASK | CE_VALID |\n -+\t\t\t\t\tCE_EXTENDED_FLAGS;\n -+\t\t\t\tunsigned int ce_flags, base_flags, ret;\n -+\t\t\t\tce_flags = ce->ce_flags;\n -+\t\t\t\tbase_flags = base->ce_flags;\n -+\t\t\t\t/* only on-disk flags matter */\n -+\t\t\t\tce->ce_flags &= ondisk_flags;\n -+\t\t\t\tbase->ce_flags &= ondisk_flags;\n -+\t\t\t\tret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,\n -+\t\t\t\t\t offsetof(struct cache_entry, name) -\n -+\t\t\t\t\t offsetof(struct cache_entry, ce_stat_data));\n -+\t\t\t\tce->ce_flags = ce_flags;\n -+\t\t\t\tbase->ce_flags = base_flags;\n -+\t\t\t\tif (ret)\n ++\t\t\t\tif (compare_ce_content(ce, base))\n +\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n +\t\t\t}\n \t\t\tdiscard_cache_entry(base);\n6: 52c755f210 ! 6: 384b440345 split-index: smudge and add racily clean cache entries to split index\n @@ -46,6 +46,11 @@\n racily clean cache entries as well, and will then write them with\n smudged stat data to the new split index.\n \n + This change makes all tests in 't1701-racy-split-index.sh' pass, so\n + flip the two 'test_expect_failure' tests to success. Also add the '#'\n + (as in nr. of trial) to those tests' description that were omitted\n + when the tests expected failure.\n +\n Note that after this change if the index is split when it contains a\n racily clean cache entry, then a smudged cache entry will be written\n both to the new shared and to the new split indexes. This doesn't\n-- \n2.19.1.465.gaff195083f\n\n"},{"id":"360157","messageId":"20181011094309.18626-2-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20181011094309.18626-1-szeder.dev@gmail.com","subject":"[PATCH v4 1/6] t1700-split-index: document why FSMONITOR is disabled in this test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-11T09:43:04Z","receivedAt":"2018-10-11T09:43:19Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Signed-off-by: SZEDER Gábor \n---\n t/t1700-split-index.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex be22398a85..822257ff7d 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -6,6 +6,9 @@ test_description='split index mode tests'\n \n # We need total control of index splitting here\n sane_unset GIT_TEST_SPLIT_INDEX\n+# A couple of tests expect the index to have a specific checksum,\n+# but the presence of the optional FSMN extension would interfere\n+# with those checks, so disable it in this test script.\n sane_unset GIT_FSMONITOR_TEST\n \n test_expect_success 'enable split index' '\n-- \n2.19.1.465.gaff195083f\n\n"},{"id":"360158","messageId":"20181011094309.18626-4-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20181011094309.18626-1-szeder.dev@gmail.com","subject":"[PATCH v4 3/6] t1700-split-index: date back files to avoid racy situations","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-11T09:43:06Z","receivedAt":"2018-10-11T09:43:22Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'t1700-split-index.sh' checks that the index was split correctly under\nvarious circumstances and that all the different ways to turn the\nsplit index feature on and off work correctly. To do so, most of its\ntests use 'test-tool dump-split-index' to see which files have their\ncache entries in the split index. All these tests assume that all\ncache entries are written to the shared index (called \"base\"\nthroughout these tests) when a new shared index is created. This is\nan implementation detail: most git commands (basically all except 'git\nupdate-index') don't care or know at all about split index or whether\na cache entry is stored in the split or shared index.\n\nAs demonstrated in the previous patch, refreshing a split index is\nprone to a variant of the classic racy git issue. The next patch will\nfix this issue, but while doing so it will also slightly change this\nbehaviour: only cache entries with mtime in the past will be written\nonly to the newly created shared index, but racily clean cache entries\nwill be written to the new split index (with smudged stat data).\n\nWhile this upcoming change won't at all affect any git commands, it\nwill violate the above mentioned assumption of 't1700's tests. Since\nthese tests create or modify files and create or refresh the split\nindex in rapid succession, there are plenty of racily clean cache\nentries to be dealt with, which will then be written to the new split\nindexes, and, ultimately, will cause several tests in 't1700' to fail.\n\nLet's prepare 't1700-split-index.sh' for this upcoming change and\nmodify its tests to avoid racily clean files by backdating the mtime\nof any file modifications (and since a lot of tests create or modify\nfiles, encapsulate it into a helper function).\n\nSigned-off-by: SZEDER Gábor \n---\n t/t1700-split-index.sh | 49 ++++++++++++++++++++++++------------------\n 1 file changed, 28 insertions(+), 21 deletions(-)\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 822257ff7d..f053bf83eb 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -11,6 +11,13 @@ sane_unset GIT_TEST_SPLIT_INDEX\n # with those checks, so disable it in this test script.\n sane_unset GIT_FSMONITOR_TEST\n \n+# Create a file named as $1 with content read from stdin.\n+# Set the file's mtime to a few seconds in the past to avoid racy situations.\n+create_non_racy_file () {\n+\tcat >\"$1\" &&\n+\ttest-tool chmtime =-5 \"$1\"\n+}\n+\n test_expect_success 'enable split index' '\n \tgit config splitIndex.maxPercentChange 100 &&\n \tgit update-index --split-index &&\n@@ -34,7 +41,7 @@ test_expect_success 'enable split index' '\n '\n \n test_expect_success 'add one file' '\n-\t: >one &&\n+\tcreate_non_racy_file one &&\n \tgit update-index --add one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -86,7 +93,7 @@ test_expect_success 'enable split index again, \"one\" now belongs to base index\"'\n '\n \n test_expect_success 'modify original file, base index untouched' '\n-\techo modified >one &&\n+\techo modified | create_non_racy_file one &&\n \tgit update-index one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -105,7 +112,7 @@ test_expect_success 'modify original file, base index untouched' '\n '\n \n test_expect_success 'add another file, which stays index' '\n-\t: >two &&\n+\tcreate_non_racy_file two &&\n \tgit update-index --add two &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -158,7 +165,7 @@ test_expect_success 'remove file in base index' '\n '\n \n test_expect_success 'add original file back' '\n-\t: >one &&\n+\tcreate_non_racy_file one &&\n \tgit update-index --add one &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -177,7 +184,7 @@ test_expect_success 'add original file back' '\n '\n \n test_expect_success 'add new file' '\n-\t: >two &&\n+\tcreate_non_racy_file two &&\n \tgit update-index --add two &&\n \tgit ls-files --stage >actual &&\n \tcat >expect <<-EOF &&\n@@ -221,7 +228,7 @@ test_expect_success 'rev-parse --shared-index-path' '\n \n test_expect_success 'set core.splitIndex config variable to true' '\n \tgit config core.splitIndex true &&\n-\t: >three &&\n+\tcreate_non_racy_file three &&\n \tgit update-index --add three &&\n \tgit ls-files --stage >ls-files.actual &&\n \tcat >ls-files.expect <<-EOF &&\n@@ -256,9 +263,9 @@ test_expect_success 'set core.splitIndex config variable to false' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'set core.splitIndex config variable to true' '\n+test_expect_success 'set core.splitIndex config variable back to true' '\n \tgit config core.splitIndex true &&\n-\t: >three &&\n+\tcreate_non_racy_file three &&\n \tgit update-index --add three &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -268,7 +275,7 @@ test_expect_success 'set core.splitIndex config variable to true' '\n \tdeletions:\n \tEOF\n \ttest_cmp expect actual &&\n-\t: >four &&\n+\tcreate_non_racy_file four &&\n \tgit update-index --add four &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n \tcat >expect <<-EOF &&\n@@ -282,7 +289,7 @@ test_expect_success 'set core.splitIndex config variable to true' '\n \n test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '\n \tgit config --unset splitIndex.maxPercentChange &&\n-\t: >five &&\n+\tcreate_non_racy_file five &&\n \tgit update-index --add five &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -292,7 +299,7 @@ test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '\n \tdeletions:\n \tEOF\n \ttest_cmp expect actual &&\n-\t: >six &&\n+\tcreate_non_racy_file six &&\n \tgit update-index --add six &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n \tcat >expect <<-EOF &&\n@@ -306,7 +313,7 @@ test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '\n \n test_expect_success 'check splitIndex.maxPercentChange set to 0' '\n \tgit config splitIndex.maxPercentChange 0 &&\n-\t: >seven &&\n+\tcreate_non_racy_file seven &&\n \tgit update-index --add seven &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -316,7 +323,7 @@ test_expect_success 'check splitIndex.maxPercentChange set to 0' '\n \tdeletions:\n \tEOF\n \ttest_cmp expect actual &&\n-\t: >eight &&\n+\tcreate_non_racy_file eight &&\n \tgit update-index --add eight &&\n \tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n \ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n@@ -329,17 +336,17 @@ test_expect_success 'check splitIndex.maxPercentChange set to 0' '\n '\n \n test_expect_success 'shared index files expire after 2 weeks by default' '\n-\t: >ten &&\n+\tcreate_non_racy_file ten &&\n \tgit update-index --add ten &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_under_2_weeks_ago=$((5-14*86400)) &&\n \ttest-tool chmtime =$just_under_2_weeks_ago .git/sharedindex.* &&\n-\t: >eleven &&\n+\tcreate_non_racy_file eleven &&\n \tgit update-index --add eleven &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_over_2_weeks_ago=$((-1-14*86400)) &&\n \ttest-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&\n-\t: >twelve &&\n+\tcreate_non_racy_file twelve &&\n \tgit update-index --add twelve &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n@@ -347,12 +354,12 @@ test_expect_success 'shared index files expire after 2 weeks by default' '\n test_expect_success 'check splitIndex.sharedIndexExpire set to 16 days' '\n \tgit config splitIndex.sharedIndexExpire \"16.days.ago\" &&\n \ttest-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&\n-\t: >thirteen &&\n+\tcreate_non_racy_file thirteen &&\n \tgit update-index --add thirteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tjust_over_16_days_ago=$((-1-16*86400)) &&\n \ttest-tool chmtime =$just_over_16_days_ago .git/sharedindex.* &&\n-\t: >fourteen &&\n+\tcreate_non_racy_file fourteen &&\n \tgit update-index --add fourteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n@@ -361,13 +368,13 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"\n \tgit config splitIndex.sharedIndexExpire never &&\n \tjust_10_years_ago=$((-365*10*86400)) &&\n \ttest-tool chmtime =$just_10_years_ago .git/sharedindex.* &&\n-\t: >fifteen &&\n+\tcreate_non_racy_file fifteen &&\n \tgit update-index --add fifteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n \tgit config splitIndex.sharedIndexExpire now &&\n \tjust_1_second_ago=-1 &&\n \ttest-tool chmtime =$just_1_second_ago .git/sharedindex.* &&\n-\t: >sixteen &&\n+\tcreate_non_racy_file sixteen &&\n \tgit update-index --add sixteen &&\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n@@ -382,7 +389,7 @@ do\n \t\t# Create one new shared index file\n \t\tgit config core.sharedrepository \"$mode\" &&\n \t\tgit config core.splitIndex true &&\n-\t\t: >one &&\n+\t\tcreate_non_racy_file one &&\n \t\tgit update-index --add one &&\n \t\techo \"$modebits\" >expect &&\n \t\ttest_modebits .git/index >actual &&\n-- \n2.19.1.465.gaff195083f\n\n"},{"id":"360159","messageId":"20181011094309.18626-3-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20181011094309.18626-1-szeder.dev@gmail.com","subject":"[PATCH v4 2/6] split-index: add tests to demonstrate the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-11T09:43:05Z","receivedAt":"2018-10-11T09:43:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Ever since the split index feature was introduced [1], refreshing a\nsplit index is prone to a variant of the classic racy git problem.\nThere are a couple of unrelated tests in the test suite that\noccasionally fail when run with 'GIT_TEST_SPLIT_INDEX=yes', but\n't1700-split-index.sh', the only test script focusing solely on split\nindex, has never noticed this issue, because it only cares about how\nthe index is split under various circumstances and all the different\nways to turn the split index feature on and off.\n\nAdd a dedicated test script 't1701-racy-split-index.sh' to exercise\nthe split index feature in racy situations as well; kind of a\n\"t0010-racy-git.sh for split index\" but with modern style (the tests\ndo everything in &&-chained list of commands in 'test_expect_...'\nblocks, and use 'test_cmp' for more informative output on failure).\n\nThe tests cover the following sequences of index splitting, updating,\nand racy file modifications, with the last two cases demonstrating the\nracy split index problem:\n\n 1. Split the index while adding a racily clean file:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file # size stays the same\n\n This case already works properly. Even though the cache entry's\n stat data matches with the modifid file in the worktree,\n subsequent git commands will notice that the (split) index and\n the file have the same mtime, and then will go on to check the\n file's content and notice its dirtiness.\n\n 2. Add a racily clean file to an already split index:\n\n git update-index --split-index\n echo \"cached content\" >file\n git update-index --add file\n echo \"dirty worktree\" >file\n\n This case already works properly. After the second 'git\n update-index' writes the newly added file's cache entry to the\n new split index, it basically works in the same way as case #1.\n\n 3. Split the index when it (i.e. the not yet splitted index)\n contains a racily clean cache entry, i.e. an entry whose cached\n stat data matches with the corresponding file in the worktree and\n the cached mtime matches that of the index:\n\n echo \"cached content\" >file\n git update-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git update-index --split-index --add other-file\n\n This case already works properly. The shared index is written by\n do_write_index(), i.e. the same function that is responsible for\n writing \"regular\" and split indexes as well. This function\n cleverly notices the racily clean cache entry, and writes the\n entry to the new shared index with smudged stat data, i.e. file\n size set to 0. When subsequent git commands read the index, they\n will notice that the smudged stat data doesn't match with the\n file in the worktree, and then go on to check the file's content\n and notice its dirtiness.\n\n 4. Update the split index when it contains a racily clean cache\n entry:\n\n git update-index --split-index\n echo \"cached content\" >file\n git update-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git update-index --add other-file\n\n This case already works properly. After the second 'git\n update-index' the newly added file's cache entry is only stored\n in the split index. If a cache entry is present in the split\n index (even if it is a replacement of an outdated entry in the\n shared index), then it will always be included in the new split\n index on subsequent split index updates (until the file is\n removed or a new shared index is written), independently from\n whether the entry is racily clean or not. When do_write_index()\n writes the new split index, it notices the racily clean cache\n entry, and smudges its stat date. Subsequent git commands\n reading the index will notice the smudged stat data and then go\n on to check the file's content and notice its dirtiness.\n\n 5. Update the split index when a racily clean cache entry is stored\n only in the shared index:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git update-index --add other-file\n\n This case fails due to the racy split index problem. In the\n second 'git update-index' prepare_to_write_split_index() decides,\n among other things, which cache entries stored only in the shared\n index should be replaced in the new split index. Alas, this\n function never looks out for racily clean cache entries, and\n since the file's stat data in the worktree hasn't changed since\n the shared index was written, the entry won't be replaced in the\n new split index. Consequently, do_write_index() doesn't even get\n this racily clean cache entry, and can't smudge its stat data.\n Subsequent git commands will then see that the index has more\n recent mtime than the file and that the (not smudged) cached stat\n data still matches with the file in the worktree, and,\n ultimately, will erroneously consider the file clean.\n\n 6. Update the split index after unpack_trees() copied a racily clean\n cache entry from the shared index:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file\n # ... wait ...\n git read-tree -m HEAD\n\n This case fails due to the racy split index problem. This\n basically fails for the same reason as case #5 above, but there\n is one important difference, which warrants the dedicated test.\n While that second 'git update-index' in case #5 updates\n index_state in place, in this case 'git read-tree -m' calls\n unpack_trees(), which throws out the entire index, and constructs\n a new one from the (potentially updated) copies of the original's\n cache entries. Consequently, when prepare_to_write_split_index()\n gets to work on this reconstructed index, it takes a different\n code path than in case #5 when deciding which cache entries in\n the shared index should be replaced. The result is the same,\n though: the racily clean cache entry goes unnoticed, it isn't\n added to the split index with smudged stat data, and subsequent\n git commands will then erroneously consider the file clean.\n\nNote that in the last two 'test_expect_failure' cases I omitted the\n'#' (as in nr. of trial) from the tests' description on purpose for\nnow, as it breakes the TAP output [2]; it will be added at the end of\nthe series, when those two tests will be flipped to\n'test_expect_success'.\n\n[1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge\n branch 'nd/split-index', 2014-07-16).\n\n[2] In the TAP output a '#' should separate the test's description\n from the TODO directive emitted by 'test_expect_failure'. The\n additional '#' in \"#$trial\" interferes with this, the test harness\n won't recognize the TODO directive, and will report that those\n tests failed unexpectedly.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t1701-racy-split-index.sh | 218 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 218 insertions(+)\n create mode 100755 t/t1701-racy-split-index.sh\n\ndiff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\nnew file mode 100755\nindex 0000000000..fbb77046da\n--- /dev/null\n+++ b/t/t1701-racy-split-index.sh\n@@ -0,0 +1,218 @@\n+#!/bin/sh\n+\n+# This test can give false success if your machine is sufficiently\n+# slow or all trials happened to happen on second boundaries.\n+\n+test_description='racy split index'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\t# Only split the index when the test explicitly says so.\n+\tsane_unset GIT_TEST_SPLIT_INDEX &&\n+\tgit config splitIndex.maxPercentChange 100 &&\n+\n+\techo \"cached content\" >racy-file &&\n+\tgit add racy-file &&\n+\tgit commit -m initial &&\n+\n+\techo something >other-file &&\n+\t# No raciness with this file.\n+\ttest-tool chmtime =-20 other-file &&\n+\n+\techo \"+cached content\" >expect\n+'\n+\n+check_cached_diff () {\n+\tgit diff-index --patch --cached $EMPTY_TREE racy-file >diff &&\n+\ttail -1 diff >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+trials=\"0 1 2 3 4\"\n+for trial in $trials\n+do\n+\ttest_expect_success \"split the index while adding a racily clean file #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second (so both writes to racy-file result in the same\n+\t\t# mtime) to create the interesting racy situation.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update and split the index. The cache entry of\n+\t\t# racy-file will be stored only in the shared index.\n+\t\tgit update-index --split-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Subsequent git commands should notice that racy-file\n+\t\t# and the split index have the same mtime, and check\n+\t\t# the content of the file to see if it is actually\n+\t\t# clean.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_success \"add a racily clean file to an already split index #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\tgit update-index --split-index &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update the split index. The cache entry of racy-file\n+\t\t# will be stored only in the split index.\n+\t\tgit update-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Subsequent git commands should notice that racy-file\n+\t\t# and the split index have the same mtime, and check\n+\t\t# the content of the file to see if it is actually\n+\t\t# clean.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_success \"split the index when the index contains a racily clean cache entry #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\tgit update-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update and split the index when the index contains\n+\t\t# the racily clean cache entry of racy-file.\n+\t\t# A corresponding replacement cache entry with smudged\n+\t\t# stat data should be added to the new split index.\n+\t\tgit update-index --split-index --add other-file &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data in the replacement cache entry and that it\n+\t\t# doesnt match with the file the worktree.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_success \"update the split index when it contains a new racily clean cache entry #$trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\tgit update-index --split-index &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update the split index. The cache entry of racy-file\n+\t\t# will be stored only in the split index.\n+\t\tgit update-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update the split index when the racily clean cache\n+\t\t# entry of racy-file is only stored in the split index.\n+\t\t# An updated cache entry with smudged stat data should\n+\t\t# be added to the new split index.\n+\t\tgit update-index --add other-file &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_failure \"update the split index when a racily clean cache entry is stored only in the shared index $trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update and split the index. The cache entry of\n+\t\t# racy-file will be stored only in the shared index.\n+\t\tgit update-index --split-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update the split index when the racily clean cache\n+\t\t# entry of racy-file is only stored in the shared index.\n+\t\t# A corresponding replacement cache entry with smudged\n+\t\t# stat data should be added to the new split index.\n+\t\t#\n+\t\t# Alas, such a smudged replacement entry is not added!\n+\t\tgit update-index --add other-file &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+for trial in $trials\n+do\n+\ttest_expect_failure \"update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial\" '\n+\t\trm -f .git/index .git/sharedindex.* &&\n+\n+\t\t# The next three commands must be run within the same\n+\t\t# second.\n+\t\techo \"cached content\" >racy-file &&\n+\n+\t\t# Update and split the index. The cache entry of\n+\t\t# racy-file will be stored only in the shared index.\n+\t\tgit update-index --split-index --add racy-file &&\n+\n+\t\t# File size must stay the same.\n+\t\techo \"dirty worktree\" >racy-file &&\n+\n+\t\t# Now wait a bit to ensure that the split index written\n+\t\t# below will get a more recent mtime than racy-file.\n+\t\tsleep 1 &&\n+\n+\t\t# Update the split index after unpack_trees() copied the\n+\t\t# racily clean cache entry of racy-file from the shared\n+\t\t# index. A corresponding replacement cache entry\n+\t\t# with smudged stat data should be added to the new\n+\t\t# split index.\n+\t\t#\n+\t\t# Alas, such a smudged replacement entry is not added!\n+\t\tgit read-tree -m HEAD &&\n+\n+\t\t# Subsequent git commands should notice the smudged\n+\t\t# stat data.\n+\t\tcheck_cached_diff\n+\t'\n+done\n+\n+test_done\n-- \n2.19.1.465.gaff195083f\n\n"},{"id":"360160","messageId":"20181011094309.18626-5-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20181011094309.18626-1-szeder.dev@gmail.com","subject":"[PATCH v4 4/6] split-index: count the number of deleted entries","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-11T09:43:07Z","receivedAt":"2018-10-11T09:43:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'struct split_index' contains the field 'nr_deletions', whose name\nwith the 'nr_' prefix suggests that it contains the number of deleted\ncache entries. However, barring its initialization to 0, this field\nis only ever set to 1, indicating that there is at least one deleted\nentry, but not the number of deleted entries. Luckily, this doesn't\ncause any issues (other than confusing the reader, that is), because\nthe only place reading this field uses it in the same sense, i.e.: 'if\n(si->nr_deletions)'.\n\nTo avoid confusion, we could either rename this field to something\nlike 'has_deletions' to make its name match its role, or make it a\ncounter of deleted cache entries to match its name.\n\nLet's make it a counter, to keep it in sync with the related field\n'nr_replacements', which does contain the number of replaced cache\nentries. This will also give developers debugging the split index\ncode easy access to the number of deleted cache entries.\n\nSigned-off-by: SZEDER Gábor \n---\n split-index.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/split-index.c b/split-index.c\nindex 84f067e10d..548272ec33 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -111,7 +111,7 @@ static void mark_entry_for_delete(size_t pos, void *data)\n \t\tdie(\"position for delete %d exceeds base index size %d\",\n \t\t (int)pos, istate->cache_nr);\n \tistate->cache[pos]->ce_flags |= CE_REMOVE;\n-\tistate->split_index->nr_deletions = 1;\n+\tistate->split_index->nr_deletions++;\n }\n \n static void replace_entry(size_t pos, void *data)\n-- \n2.19.1.465.gaff195083f\n\n"},{"id":"360161","messageId":"20181011094309.18626-6-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20181011094309.18626-1-szeder.dev@gmail.com","subject":"[PATCH v4 5/6] split-index: don't compare cached data of entries already marked for split index","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-11T09:43:08Z","receivedAt":"2018-10-11T09:43:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When unpack_trees() constructs a new index, it copies cache entries\nfrom the original index [1]. prepare_to_write_split_index() has to\ndeal with this, and it has a dedicated code path for copied entries\nthat are present in the shared index, where it compares the cached\ndata in the corresponding copied and original entries. If the cached\ndata matches, then they are considered the same; if it differs, then\nthe copied entry will be marked for inclusion as a replacement entry\nin the just about to be written split index by setting the\nCE_UPDATE_IN_BASE flag.\n\nHowever, a cache entry already has its CE_UPDATE_IN_BASE flag set upon\nreading the split index, if the entry already has a replacement entry\nthere, or upon refreshing the cached stat data, if the corresponding\nfile was modified. The state of this flag is then preserved when\nunpack_trees() copies a cache entry from the shared index.\n\nSo modify prepare_to_write_split_index() to check the copied cache\nentries' CE_UPDATE_IN_BASE flag first, and skip the thorough\ncomparison of cached data if the flag is already set. Those couple of\nlines comparing the cached data would then have too many levels of\nindentation, so extract them into a helper function.\n\nNote that comparing the cached data in copied and original entries in\nthe shared index might actually be entirely unnecessary. In theory\nall code paths refreshing the cached stat data of an entry in the\nshared index should set the CE_UPDATE_IN_BASE flag in that entry, and\nunpack_trees() should preserve this flag when copying cache entries.\nThis means that the cached data is only ever changed if the\nCE_UPDATE_IN_BASE flag is set as well. Our test suite seems to\nconfirm this: instrumenting the conditions in question and running the\ntest suite repeatedly with 'GIT_TEST_SPLIT_INDEX=yes' showed that the\ncached data in a copied entry differs from the data in the shared\nentry only if its CE_UPDATE_IN_BASE flag is indeed set.\n\nIn practice, however, our test suite doesn't have 100% coverage,\nGIT_TEST_SPLIT_INDEX is inherently random, and I certainly can't claim\nto possess complete understanding of what goes on in unpack_trees()...\nTherefore I kept the comparison of the cached data when\nCE_UPDATE_IN_BASE is not set, just in case that an unnoticed or future\ncode path were to accidentally miss setting this flag upon refreshing\nthe cached stat data or unpack_trees() were to drop this flag while\ncopying a cache entry.\n\n[1] Note that when unpack_trees() constructs the new index and decides\n that a cache entry should now refer to different content than what\n was recorded in the original index (e.g. 'git read-tree -m\n HEAD^'), then that can't really be considered a copy of the\n original, but rather the creation of a new entry. Notably and\n pertinent to the split index feature, such a new entry doesn't\n have a reference to the original's shared index entry anymore,\n i.e. its 'index' field is set to 0. Consequently, such an entry\n is treated by prepare_to_write_split_index() as an entry not\n present in the shared index and it will be added to the new split\n index, while the original entry will be marked as deleted, and\n neither the above discussion nor the changes in this patch apply\n to them.\n\nSigned-off-by: SZEDER Gábor \n---\n split-index.c | 89 +++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 72 insertions(+), 17 deletions(-)\n\ndiff --git a/split-index.c b/split-index.c\nindex 548272ec33..187b910f5b 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -188,6 +188,30 @@ void merge_base_index(struct index_state *istate)\n \tsi->saved_cache_nr = 0;\n }\n \n+/*\n+ * Compare most of the fields in two cache entries, i.e. all except the\n+ * hashmap_entry and the name.\n+ */\n+static int compare_ce_content(struct cache_entry *a, struct cache_entry *b)\n+{\n+\tconst unsigned int ondisk_flags = CE_STAGEMASK | CE_VALID |\n+\t\t\t\t\t CE_EXTENDED_FLAGS;\n+\tunsigned int ce_flags = a->ce_flags;\n+\tunsigned int base_flags = b->ce_flags;\n+\tint ret;\n+\n+\t/* only on-disk flags matter */\n+\ta->ce_flags &= ondisk_flags;\n+\tb->ce_flags &= ondisk_flags;\n+\tret = memcmp(&a->ce_stat_data, &b->ce_stat_data,\n+\t\t offsetof(struct cache_entry, name) -\n+\t\t offsetof(struct cache_entry, ce_stat_data));\n+\ta->ce_flags = ce_flags;\n+\tb->ce_flags = base_flags;\n+\n+\treturn ret;\n+}\n+\n void prepare_to_write_split_index(struct index_state *istate)\n {\n \tstruct split_index *si = init_split_index(istate);\n@@ -207,13 +231,28 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t */\n \t\tfor (i = 0; i < istate->cache_nr; i++) {\n \t\t\tstruct cache_entry *base;\n-\t\t\t/* namelen is checked separately */\n-\t\t\tconst unsigned int ondisk_flags =\n-\t\t\t\tCE_STAGEMASK | CE_VALID | CE_EXTENDED_FLAGS;\n-\t\t\tunsigned int ce_flags, base_flags, ret;\n \t\t\tce = istate->cache[i];\n-\t\t\tif (!ce->index)\n+\t\t\tif (!ce->index) {\n+\t\t\t\t/*\n+\t\t\t\t * During simple update index operations this\n+\t\t\t\t * is a cache entry that is not present in\n+\t\t\t\t * the shared index. It will be added to the\n+\t\t\t\t * split index.\n+\t\t\t\t *\n+\t\t\t\t * However, it might also represent a file\n+\t\t\t\t * that already has a cache entry in the\n+\t\t\t\t * shared index, but a new index has just\n+\t\t\t\t * been constructed by unpack_trees(), and\n+\t\t\t\t * this entry now refers to different content\n+\t\t\t\t * than what was recorded in the original\n+\t\t\t\t * index, e.g. during 'read-tree -m HEAD^' or\n+\t\t\t\t * 'checkout HEAD^'. In this case the\n+\t\t\t\t * original entry in the shared index will be\n+\t\t\t\t * marked as deleted, and this entry will be\n+\t\t\t\t * added to the split index.\n+\t\t\t\t */\n \t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (ce->index > si->base->cache_nr) {\n \t\t\t\tce->index = 0;\n \t\t\t\tcontinue;\n@@ -227,18 +266,34 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t\tce->index = 0;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tce_flags = ce->ce_flags;\n-\t\t\tbase_flags = base->ce_flags;\n-\t\t\t/* only on-disk flags matter */\n-\t\t\tce->ce_flags &= ondisk_flags;\n-\t\t\tbase->ce_flags &= ondisk_flags;\n-\t\t\tret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,\n-\t\t\t\t offsetof(struct cache_entry, name) -\n-\t\t\t\t offsetof(struct cache_entry, ce_stat_data));\n-\t\t\tce->ce_flags = ce_flags;\n-\t\t\tbase->ce_flags = base_flags;\n-\t\t\tif (ret)\n-\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\t\t\t/*\n+\t\t\t * This is the copy of a cache entry that is present\n+\t\t\t * in the shared index, created by unpack_trees()\n+\t\t\t * while it constructed a new index.\n+\t\t\t */\n+\t\t\tif (ce->ce_flags & CE_UPDATE_IN_BASE) {\n+\t\t\t\t/*\n+\t\t\t\t * Already marked for inclusion in the split\n+\t\t\t\t * index, either because the corresponding\n+\t\t\t\t * file was modified and the cached stat data\n+\t\t\t\t * was refreshed, or because the original\n+\t\t\t\t * entry already had a replacement entry in\n+\t\t\t\t * the split index.\n+\t\t\t\t * Nothing to do.\n+\t\t\t\t */\n+\t\t\t} else {\n+\t\t\t\t/*\n+\t\t\t\t * Thoroughly compare the cached data to see\n+\t\t\t\t * whether it should be marked for inclusion\n+\t\t\t\t * in the split index.\n+\t\t\t\t *\n+\t\t\t\t * This comparison might be unnecessary, as\n+\t\t\t\t * code paths modifying the cached data do\n+\t\t\t\t * set CE_UPDATE_IN_BASE as well.\n+\t\t\t\t */\n+\t\t\t\tif (compare_ce_content(ce, base))\n+\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\t\t\t}\n \t\t\tdiscard_cache_entry(base);\n \t\t\tsi->base->cache[ce->index - 1] = ce;\n \t\t}\n-- \n2.19.1.465.gaff195083f\n\n"},{"id":"360162","messageId":"20181011094309.18626-7-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20181011094309.18626-1-szeder.dev@gmail.com","subject":"[PATCH v4 6/6] split-index: smudge and add racily clean cache entries to split index","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-11T09:43:09Z","receivedAt":"2018-10-11T09:43:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Ever since the split index feature was introduced [1], refreshing a\nsplit index is prone to a variant of the classic racy git problem.\n\nConsider the following sequence of commands updating the split index\nwhen the shared index contains a racily clean cache entry, i.e. an\nentry whose cached stat data matches with the corresponding file in\nthe worktree and the cached mtime matches that of the index:\n\n echo \"cached content\" >file\n git update-index --split-index --add file\n echo \"dirty worktree\" >file # size stays the same!\n # ... wait ...\n git update-index --add other-file\n\nNormally, when a non-split index is updated, then do_write_index()\n(the function responsible for writing all kinds of indexes, \"regular\",\nsplit, and shared) recognizes racily clean cache entries, and writes\nthem with smudged stat data, i.e. with file size set to 0. When\nsubsequent git commands read the index, they will notice that the\nsmudged stat data doesn't match with the file in the worktree, and\nthen go on to check the file's content and notice its dirtiness.\n\nIn the above example, however, in the second 'git update-index'\nprepare_to_write_split_index() decides which cache entries stored only\nin the shared index should be replaced in the new split index. Alas,\nthis function never looks out for racily clean cache entries, and\nsince the file's stat data in the worktree hasn't changed since the\nshared index was written, it won't be replaced in the new split index.\nConsequently, do_write_index() doesn't even get this racily clean\ncache entry, and can't smudge its stat data. Subsequent git commands\nwill then see that the index has more recent mtime than the file and\nthat the (not smudged) cached stat data still matches with the file in\nthe worktree, and, ultimately, will erroneously consider the file\nclean.\n\nModify prepare_to_write_split_index() to recognize racily clean cache\nentries, and mark them to be added to the split index. Note that\nthere are two places where it should check raciness: first those cache\nentries that are only stored in the shared index, and then those that\nhave been copied by unpack_trees() from the shared index while it\nconstructed a new index. This way do_write_index() will get these\nracily clean cache entries as well, and will then write them with\nsmudged stat data to the new split index.\n\nThis change makes all tests in 't1701-racy-split-index.sh' pass, so\nflip the two 'test_expect_failure' tests to success. Also add the '#'\n(as in nr. of trial) to those tests' description that were omitted\nwhen the tests expected failure.\n\nNote that after this change if the index is split when it contains a\nracily clean cache entry, then a smudged cache entry will be written\nboth to the new shared and to the new split indexes. This doesn't\naffect regular git commands: as far as they are concerned this is just\nan entry in the split index replacing an outdated entry in the shared\nindex. It did affect a few tests in 't1700-split-index.sh', though,\nbecause they actually check which entries are stored in the split\nindex; a previous patch in this series has already made the necessary\nadjustments in 't1700'. And racily clean cache entries and index\nsplitting are rare enough to not worry about the resulting duplicated\nsmudged cache entries, and the additional complexity required to\nprevent them is not worth it.\n\nSeveral tests failed occasionally when the test suite was run with\n'GIT_TEST_SPLIT_INDEX=yes'. Here are those that I managed to trace\nback to this racy split index problem, starting with those failing\nmore frequently, with a link to a failing Travis CI build job for\neach. The highlighted line [2] shows when the racy file was written,\nwhich is not always in the failing test but in a preceeding setup\ntest.\n\n t3903-stash.sh:\n https://travis-ci.org/git/git/jobs/385542084#L5858\n\n t4024-diff-optimize-common.sh:\n https://travis-ci.org/git/git/jobs/386531969#L3174\n\n t4015-diff-whitespace.sh:\n https://travis-ci.org/git/git/jobs/360797600#L8215\n\n t2200-add-update.sh:\n https://travis-ci.org/git/git/jobs/382543426#L3051\n\n t0090-cache-tree.sh:\n https://travis-ci.org/git/git/jobs/416583010#L3679\n\nThere might be others, e.g. perhaps 't1000-read-tree-m-3way.sh' and\nothers using 'lib-read-tree-m-3way.sh', but I couldn't confirm yet.\n\n[1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge\n branch 'nd/split-index', 2014-07-16).\n\n[2] Note that those highlighted lines are in the 'after failure' fold,\n and your browser might unhelpfully fold it up before you could\n take a good look.\n\nSigned-off-by: SZEDER Gábor \n---\n cache.h | 2 ++\n read-cache.c | 2 +-\n split-index.c | 42 ++++++++++++++++++++++++++++++++++++-\n t/t1701-racy-split-index.sh | 8 ++-----\n 4 files changed, 46 insertions(+), 8 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4d014541ab..3f419b6c79 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -781,6 +781,8 @@ extern void *read_blob_data_from_index(const struct index_state *, const char *,\n #define CE_MATCH_REFRESH\t\t0x10\n /* don't refresh_fsmonitor state or do stat comparison even if CE_FSMONITOR_VALID is true */\n #define CE_MATCH_IGNORE_FSMONITOR 0X20\n+extern int is_racy_timestamp(const struct index_state *istate,\n+\t\t\t const struct cache_entry *ce);\n extern int ie_match_stat(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n extern int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 7b1354d759..8f644f68b4 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -337,7 +337,7 @@ static int is_racy_stat(const struct index_state *istate,\n \t\t);\n }\n \n-static int is_racy_timestamp(const struct index_state *istate,\n+int is_racy_timestamp(const struct index_state *istate,\n \t\t\t const struct cache_entry *ce)\n {\n \treturn (!S_ISGITLINK(ce->ce_mode) &&\ndiff --git a/split-index.c b/split-index.c\nindex 187b910f5b..875f538802 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -259,8 +259,39 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t}\n \t\t\tce->ce_flags |= CE_MATCHED; /* or \"shared\" */\n \t\t\tbase = si->base->cache[ce->index - 1];\n-\t\t\tif (ce == base)\n+\t\t\tif (ce == base) {\n+\t\t\t\t/* The entry is present in the shared index. */\n+\t\t\t\tif (ce->ce_flags & CE_UPDATE_IN_BASE) {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * Already marked for inclusion in\n+\t\t\t\t\t * the split index, either because\n+\t\t\t\t\t * the corresponding file was\n+\t\t\t\t\t * modified and the cached stat data\n+\t\t\t\t\t * was refreshed, or because there\n+\t\t\t\t\t * is already a replacement entry in\n+\t\t\t\t\t * the split index.\n+\t\t\t\t\t * Nothing more to do here.\n+\t\t\t\t\t */\n+\t\t\t\t} else if (!ce_uptodate(ce) &&\n+\t\t\t\t\t is_racy_timestamp(istate, ce)) {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * A racily clean cache entry stored\n+\t\t\t\t\t * only in the shared index: it must\n+\t\t\t\t\t * be added to the split index, so\n+\t\t\t\t\t * the subsequent do_write_index()\n+\t\t\t\t\t * can smudge its stat data.\n+\t\t\t\t\t */\n+\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\t\t\t\t} else {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * The entry is only present in the\n+\t\t\t\t\t * shared index and it was not\n+\t\t\t\t\t * refreshed.\n+\t\t\t\t\t * Just leave it there.\n+\t\t\t\t\t */\n+\t\t\t\t}\n \t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (ce->ce_namelen != base->ce_namelen ||\n \t\t\t strcmp(ce->name, base->name)) {\n \t\t\t\tce->index = 0;\n@@ -281,6 +312,15 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t\t * the split index.\n \t\t\t\t * Nothing to do.\n \t\t\t\t */\n+\t\t\t} else if (!ce_uptodate(ce) &&\n+\t\t\t\t is_racy_timestamp(istate, ce)) {\n+\t\t\t\t/*\n+\t\t\t\t * A copy of a racily clean cache entry from\n+\t\t\t\t * the shared index. It must be added to\n+\t\t\t\t * the split index, so the subsequent\n+\t\t\t\t * do_write_index() can smudge its stat data.\n+\t\t\t\t */\n+\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n \t\t\t} else {\n \t\t\t\t/*\n \t\t\t\t * Thoroughly compare the cached data to see\ndiff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh\nindex fbb77046da..5dc221ef38 100755\n--- a/t/t1701-racy-split-index.sh\n+++ b/t/t1701-racy-split-index.sh\n@@ -148,7 +148,7 @@ done\n \n for trial in $trials\n do\n-\ttest_expect_failure \"update the split index when a racily clean cache entry is stored only in the shared index $trial\" '\n+\ttest_expect_success \"update the split index when a racily clean cache entry is stored only in the shared index #$trial\" '\n \t\trm -f .git/index .git/sharedindex.* &&\n \n \t\t# The next three commands must be run within the same\n@@ -170,8 +170,6 @@ do\n \t\t# entry of racy-file is only stored in the shared index.\n \t\t# A corresponding replacement cache entry with smudged\n \t\t# stat data should be added to the new split index.\n-\t\t#\n-\t\t# Alas, such a smudged replacement entry is not added!\n \t\tgit update-index --add other-file &&\n \n \t\t# Subsequent git commands should notice the smudged\n@@ -182,7 +180,7 @@ done\n \n for trial in $trials\n do\n-\ttest_expect_failure \"update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial\" '\n+\ttest_expect_success \"update the split index after unpack trees() copied a racily clean cache entry from the shared index #$trial\" '\n \t\trm -f .git/index .git/sharedindex.* &&\n \n \t\t# The next three commands must be run within the same\n@@ -205,8 +203,6 @@ do\n \t\t# index. A corresponding replacement cache entry\n \t\t# with smudged stat data should be added to the new\n \t\t# split index.\n-\t\t#\n-\t\t# Alas, such a smudged replacement entry is not added!\n \t\tgit read-tree -m HEAD &&\n \n \t\t# Subsequent git commands should notice the smudged\n-- \n2.19.1.465.gaff195083f\n\n"},{"id":"360164","messageId":"20181011095357.19061-1-szeder.dev@gmail.com","threadId":"49437","inReplyTo":"20181011094309.18626-1-szeder.dev@gmail.com","subject":"[PATCH 7/6] split-index: BUG() when cache entry refers to non-existing shared entry","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-11T09:53:57Z","receivedAt":"2018-10-11T09:54:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When the split index feature is in use, then a cache entry is:\n\n - either only present in the split index, in which case its 'index'\n field must be 0,\n\n - or it should refer to an existing entry in the shared index, i.e.\n the 'index' field can't be greater than the size of the shared\n index.\n\nIf a cache entry were to refer to a non-existing entry in the shared\nindex, then that's a sign of something being wrong in the index state,\neither as a result of a bug in dealing with the split/shared index\nentries, or perhaps a (potentially unrelated) memory corruption issue.\n\nprepare_to_write_split_index() already has a condition to catch cache\nentries with such bogus 'index' field, but instead of calling BUG() it\njust sets cache entry's 'index = 0', and the entry will then be\nwritten to the new split index.\n\nDon't write a new index file from bogus index state, and call BUG()\nupon encountering an cache entry referring to a non-existing shared\nindex entry.\n\nRunning the test suite repeatedly with 'GIT_TEST_SPLIT_INDEX=yes'\ndoesn't trigger this condition.\n\nSigned-off-by: SZEDER Gábor \n---\n\nA follow-up to:\n\n https://public-inbox.org/git/20180927134324.GI27036@localhost/\n\n\n split-index.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/split-index.c b/split-index.c\nindex 875f538802..5820412dc5 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -254,8 +254,8 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (ce->index > si->base->cache_nr) {\n-\t\t\t\tce->index = 0;\n-\t\t\t\tcontinue;\n+\t\t\t\tBUG(\"ce refers to a shared ce at %d, which is beyond the shared index size %d\",\n+\t\t\t\t ce->index, si->base->cache_nr);\n \t\t\t}\n \t\t\tce->ce_flags |= CE_MATCHED; /* or \"shared\" */\n \t\t\tbase = si->base->cache[ce->index - 1];\n-- \n2.19.1.465.gaff195083f\n\n"},{"id":"360169","messageId":"87d0sgepmo.fsf@evledraar.gmail.com","threadId":"49437","inReplyTo":"20181011094309.18626-1-szeder.dev@gmail.com","subject":"Re: [PATCH v4 0/6] Fix the racy split index problem","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-11T10:36:47Z","receivedAt":"2018-10-11T10:36:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 11 2018, SZEDER Gábor wrote:\n\n> Fourth and hopefully final round of fixing occasional test failures when\n> run with 'GIT_TEST_SPLIT_INDEX=yes'. The only code change is the\n> extraction of a helper function to compare two cache entries' content,\n> and then a couple of minor log message clarifications. The range-diff\n> below is rather clear on that.\n\nLooks good. I'm not going to run the stress test I did on v5 on this\nsince the changes are just moving existing code into a fuction, unless\nyou'd like me to or think there's a reason to that is.\n"},{"id":"360178","messageId":"20181011113841.GB19800@szeder.dev","threadId":"49437","inReplyTo":"87d0sgepmo.fsf@evledraar.gmail.com","subject":"Re: [PATCH v4 0/6] Fix the racy split index problem","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-11T11:38:41Z","receivedAt":"2018-10-11T11:38:47Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Oct 11, 2018 at 12:36:47PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Oct 11 2018, SZEDER Gábor wrote:\n> \n> > Fourth and hopefully final round of fixing occasional test failures when\n> > run with 'GIT_TEST_SPLIT_INDEX=yes'. The only code change is the\n> > extraction of a helper function to compare two cache entries' content,\n> > and then a couple of minor log message clarifications. The range-diff\n> > below is rather clear on that.\n> \n> Looks good. I'm not going to run the stress test I did on v5 on this\n> since the changes are just moving existing code into a fuction, unless\n> you'd like me to or think there's a reason to that is.\n\nFWIW, I intend to carry this patch below and use it in tests both\nlocally and on Travis CI. I could never trigger any of those three\nconditions by repeated test runs with 'GIT_TEST_SPLIT_INDEX=yes', or\nby deliberately constructing tricky scenarios where they might be\ntriggered.\n\nPerhaps with enough time I'll get lucky eventually :)\n\nIf it's not too much trouble, then perhaps you could pick it up as\nwell? While testing previous versions of this series it turned out\nthat your setup has much more \"luck\" in finding problematic timings\nthan mine.\n\n\ndiff --git a/split-index.c b/split-index.c\nindex 5820412dc5..4af535e236 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -254,8 +254,8 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (ce->index > si->base->cache_nr) {\n-\t\t\t\tBUG(\"ce refers to a shared ce at %d, which is beyond the shared index size %d\",\n-\t\t\t\t ce->index, si->base->cache_nr);\n+\t\t\t\tBUG(\"ce of '%s' refers to a shared ce at %d, which is beyond the shared index size %d\",\n+\t\t\t\t ce->name, ce->index, si->base->cache_nr);\n \t\t\t}\n \t\t\tce->ce_flags |= CE_MATCHED; /* or \"shared\" */\n \t\t\tbase = si->base->cache[ce->index - 1];\n@@ -293,10 +293,9 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (ce->ce_namelen != base->ce_namelen ||\n-\t\t\t strcmp(ce->name, base->name)) {\n-\t\t\t\tce->index = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n+\t\t\t strcmp(ce->name, base->name))\n+\t\t\t\tBUG(\"ce of '%s' refers to the shared ce of a different file '%s'\",\n+\t\t\t\t ce->name, base->name);\n \t\t\t/*\n \t\t\t * This is the copy of a cache entry that is present\n \t\t\t * in the shared index, created by unpack_trees()\n@@ -332,7 +331,8 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\t\t * set CE_UPDATE_IN_BASE as well.\n \t\t\t\t */\n \t\t\t\tif (compare_ce_content(ce, base))\n-\t\t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\t\t\t\t\tBUG(\"ce of '%s' differs from its shared ce, but the CE_UPDATE_IN_BASE flag was not set\",\n+\t\t\t\t\t ce->name);\n \t\t\t}\n \t\t\tdiscard_cache_entry(base);\n \t\t\tsi->base->cache[ce->index - 1] = ce;\n"},{"id":"360268","messageId":"xmqqpnwfdf5o.fsf@gitster-ct.c.googlers.com","threadId":"49437","inReplyTo":"87d0sgepmo.fsf@evledraar.gmail.com","subject":"Re: [PATCH v4 0/6] Fix the racy split index problem","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-12T03:20:35Z","receivedAt":"2018-10-12T03:20:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Oct 11 2018, SZEDER Gábor wrote:\n>\n>> Fourth and hopefully final round of fixing occasional test failures when\n>> run with 'GIT_TEST_SPLIT_INDEX=yes'. The only code change is the\n>> extraction of a helper function to compare two cache entries' content,\n>> and then a couple of minor log message clarifications. The range-diff\n>> below is rather clear on that.\n>\n> Looks good. I'm not going to run the stress test I did on v5 on this\n> since the changes are just moving existing code into a fuction, unless\n> you'd like me to or think there's a reason to that is.\n\nThanks, both. Let's merge this round to 'next'; it would be OK to\ndo any further tweaks incrementally on top.\n\n\n"}]}