git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 5/5] split-index: smudge and add racily clean cache entries to split index

From
SZEDER Gábor <szeder.dev@gmail.com>
Date
Sep 27, 2018, 12:44 UTC
Message-ID
<20180927124434.30835-6-szeder.dev@gmail.com>
In-Reply-To
<20180927124434.30835-1-szeder.dev@gmail.com>

Ever since the split index feature was introduced [1], refreshing a split index is prone to a variant of the classic racy git problem.

Consider the following sequence of commands updating the split index when the shared index contains a racily clean cache entry, i.e. an entry whose cached stat data matches with the corresponding file in the worktree and the cached mtime matches that of the index:

  echo "cached content" >file
  git update-index --split-index --add file
  echo "dirty worktree" >file    # size stays the same!
  # ... wait ...
  git update-index --add other-file

Normally, when a non-split index is updated, then do_write_index() (the function responsible for writing all kinds of indexes, "regular", split, and shared) recognizes racily clean cache entries, and writes them with smudged stat data, i.e. with file size set to 0. When subsequent git commands read the index, they will notice that the smudged stat data doesn't match with the file in the worktree, and then go on to check the file's content and notice its dirtiness.

In the above example, however, in the second 'git update-index' prepare_to_write_split_index() decides which cache entries stored only in the shared index should be replaced in the new split index. Alas, this function never looks out for racily clean cache entries, and since the file's stat data in the worktree hasn't changed since the shared index was written, it won't be replaced in the new split index. Consequently, do_write_index() doesn't even get this racily clean cache entry, and can't smudge its stat data. Subsequent git commands will then see that the index has more recent mtime than the file and that the (not smudged) cached stat data still matches with the file in the worktree, and, ultimately, will erroneously consider the file clean.

Modify prepare_to_write_split_index() to recognize racily clean cache entries, and mark them to be added to the split index. Note that there are two places where it should check raciness: first those cache entries that are only stored in the shared index, and then those that have been copied by unpack_trees() from the shared index while it constructed a new index. This way do_write_index() will get these racily clean cache entries as well, and will then write them with smudged stat data to the new split index.

Note that after this change if the index is split when it contains a racily clean cache entry, then a smudged cache entry will be written both to the new shared and to the new split indexes. This doesn't affect regular git commands: as far as they are concerned this is just an entry in the split index replacing an outdated entry in the shared index. It did affect a few tests in 't1700-split-index.sh', though, because they actually check which entries are stored in the split index; a previous patch in this series has already made the necessary adjustments in 't1700'. And racily clean cache entries and index splitting are rare enough to not worry about the resulting duplicated smudged cache entries, and the additional complexity required to prevent them is not worth it.

Several tests failed occasionally when the test suite was run with 'GIT_TEST_SPLIT_INDEX=yes'. Here are those that I managed to trace back to this racy split index problem, starting with those failing more frequently, with a link to a failing Travis CI build job for each. The highlighted line [2] shows when the racy file was written, which is not always in the failing test but in a preceeding setup test.

  t3903-stash.sh:
    https://travis-ci.org/git/git/jobs/385542084#L5858
  t4024-diff-optimize-common.sh:
    https://travis-ci.org/git/git/jobs/386531969#L3174
  t4015-diff-whitespace.sh:
    https://travis-ci.org/git/git/jobs/360797600#L8215
  t2200-add-update.sh:
    https://travis-ci.org/git/git/jobs/382543426#L3051
  t0090-cache-tree.sh:
    https://travis-ci.org/git/git/jobs/416583010#L3679

There might be others, e.g. perhaps 't1000-read-tree-m-3way.sh' and others using 'lib-read-tree-m-3way.sh', but I couldn't confirm yet.

[1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge
    branch 'nd/split-index', 2014-07-16).
[2] Note that those highlighted lines are in the 'after failure' fold,
    and your browser might unhelpfully fold it up before you could
    take a good look.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 cache.h                     |  2 ++
 read-cache.c                |  2 +-
 split-index.c               | 42 ++++++++++++++++++++++++++++++++++++-
 t/t1701-racy-split-index.sh |  8 ++-----
 4 files changed, 46 insertions(+), 8 deletions(-)
diff --git a/cache.h b/cache.h
index 4d014541ab..3f419b6c79 100644
--- a/cache.h
+++ b/cache.h
@@ -781,6 +781,8 @@ extern void *read_blob_data_from_index(const struct index_state *, const char *,
 #define CE_MATCH_REFRESH		0x10
 /* don't refresh_fsmonitor state or do stat comparison even if CE_FSMONITOR_VALID is true */
 #define CE_MATCH_IGNORE_FSMONITOR 0X20
+extern int is_racy_timestamp(const struct index_state *istate,
+			     const struct cache_entry *ce);
 extern int ie_match_stat(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
 extern int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
 
diff --git a/read-cache.c b/read-cache.c
index 7b1354d759..8f644f68b4 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -337,7 +337,7 @@ static int is_racy_stat(const struct index_state *istate,
 		);
 }
 
-static int is_racy_timestamp(const struct index_state *istate,
+int is_racy_timestamp(const struct index_state *istate,
 			     const struct cache_entry *ce)
 {
 	return (!S_ISGITLINK(ce->ce_mode) &&
diff --git a/split-index.c b/split-index.c
index 7d8799f6b7..d989b27286 100644
--- a/split-index.c
+++ b/split-index.c
@@ -235,8 +235,39 @@ void prepare_to_write_split_index(struct index_state *istate)
 			}
 			ce->ce_flags |= CE_MATCHED; /* or "shared" */
 			base = si->base->cache[ce->index - 1];
-			if (ce == base)
+			if (ce == base) {
+				/* The entry is present in the shared index. */
+				if (ce->ce_flags & CE_UPDATE_IN_BASE) {
+					/*
+					 * Already marked for inclusion in
+					 * the split index, either because
+					 * the corresponding file was
+					 * modified and the cached stat data
+					 * was refreshed, or because there
+					 * is already a replacement entry in
+					 * the split index.
+					 * Nothing more to do here.
+					 */
+				} else if (!ce_uptodate(ce) &&
+					   is_racy_timestamp(istate, ce)) {
+					/*
+					 * A racily clean cache entry stored
+					 * only in the shared index: it must
+					 * be added to the split index, so
+					 * the subsequent do_write_index()
+					 * can smudge its stat data.
+					 */
+					ce->ce_flags |= CE_UPDATE_IN_BASE;
+				} else {
+					/*
+					 * The entry is only present in the
+					 * shared index and it was not
+					 * refreshed.
+					 * Just leave it there.
+					 */
+				}
 				continue;
+			}
 			if (ce->ce_namelen != base->ce_namelen ||
 			    strcmp(ce->name, base->name)) {
 				ce->index = 0;
@@ -257,6 +288,15 @@ void prepare_to_write_split_index(struct index_state *istate)
 				 * the split index.
 				 * Nothing to do.
 				 */
+			} else if (!ce_uptodate(ce) &&
+				   is_racy_timestamp(istate, ce)) {
+				/*
+				 * A copy of a racily clean cache entry from
+				 * the shared index.  It must be added to
+				 * the split index, so the subsequent
+				 * do_write_index() can smudge its stat data.
+				 */
+				ce->ce_flags |= CE_UPDATE_IN_BASE;
 			} else {
 				/*
 				 * Thoroughly compare the cached data to see
diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh
index ebde418d7e..7f16f5f7a3 100755
--- a/t/t1701-racy-split-index.sh
+++ b/t/t1701-racy-split-index.sh
@@ -148,7 +148,7 @@ done
 
 for trial in $trials
 do
-	test_expect_failure "update the split index when a racily clean cache entry is stored only in the shared index $trial" '
+	test_expect_success "update the split index when a racily clean cache entry is stored only in the shared index #$trial" '
 		rm -f .git/index .git/sharedindex.* &&
 
 		# The next three commands must be run within the same
@@ -170,8 +170,6 @@ do
 		# entry of racy-file is only stored in the shared index.
 		# A corresponding replacement cache entry with smudged
 		# stat data should be added to the new split index.
-		#
-		# Alas, such a smudged replacement entry is not added!
 		git update-index --add other-file &&
 
 		# Subsequent git commands should notice the smudged
@@ -182,7 +180,7 @@ done
 
 for trial in $trials
 do
-	test_expect_failure "update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial" '
+	test_expect_success "update the split index after unpack trees() copied a racily clean cache entry from the shared index #$trial" '
 		rm -f .git/index .git/sharedindex.* &&
 
 		# The next three commands must be run within the same
@@ -205,8 +203,6 @@ do
 		# index.  A corresponding replacement cache entry
 		# with smudged stat data should be added to the new
 		# split index.
-		#
-		# Alas, such a smudged replacement entry is not added!
 		git read-tree -m HEAD &&
 
 		# Subsequent git commands should notice the smudged

-- 
2.19.0.361.gafc87ffe72
Previous: SZEDER GáborNext: Ævar Arnfjörð Bjarmason
Message 10 of 42 in “Fix the racy split index problem”
  1. 0/5 Fix the racy split index problemSZEDER Gábor, Sep 27, 2018
  2. 1/5 split-index: add tests to demonstrate the racy split index problemSZEDER Gábor, Sep 27, 2018
  3. SZEDER GáborSep 28, 2018
  4. SZEDER GáborSep 28, 2018
  5. Junio C HamanoSep 28, 2018
  6. 2/5 t1700-split-index: date back files to avoid racy situationsSZEDER Gábor, Sep 27, 2018
  7. 3/5 split-index: count the number of deleted entriesSZEDER Gábor, Sep 27, 2018
  8. 4/5 split-index: don't compare stat data of entries already marked for split indexSZEDER Gábor, Sep 27, 2018
  9. SZEDER GáborSep 27, 2018
  10. 5/5 split-index: smudge and add racily clean cache entries to split indexSZEDER Gábor, Sep 27, 2018
  11. Ævar Arnfjörð BjarmasonSep 27, 2018
  12. SZEDER GáborSep 27, 2018
  13. Ævar Arnfjörð BjarmasonSep 27, 2018
  14. Ævar Arnfjörð BjarmasonSep 28, 2018
  15. SZEDER GáborSep 28, 2018
  16. Ævar Arnfjörð BjarmasonOct 8, 2018
  17. SZEDER GáborOct 8, 2018
  18. 0/6 Fix the racy split index problemSZEDER Gábor, Sep 28, 2018
  19. 1/6 t1700-split-index: document why FSMONITOR is disabled in this test scriptSZEDER Gábor, Sep 28, 2018
  20. 2/6 split-index: add tests to demonstrate the racy split index problemSZEDER Gábor, Sep 28, 2018
  21. 3/6 t1700-split-index: date back files to avoid racy situationsSZEDER Gábor, Sep 28, 2018
  22. 4/6 split-index: count the number of deleted entriesSZEDER Gábor, Sep 28, 2018
  23. 5/6 split-index: don't compare stat data of entries already marked for split indexSZEDER Gábor, Sep 28, 2018
  24. Duy NguyenSep 29, 2018
  25. SZEDER GáborSep 29, 2018
  26. SZEDER GáborSep 29, 2018
  27. 6/6 split-index: smudge and add racily clean cache entries to split indexSZEDER Gábor, Sep 28, 2018
  28. Duy NguyenSep 29, 2018
  29. SZEDER GáborSep 29, 2018
  30. SZEDER GáborSep 30, 2018
  31. Junio C HamanoOct 5, 2018
  32. 0/6 Fix the racy split index problemSZEDER Gábor, Oct 11, 2018
  33. 1/6 t1700-split-index: document why FSMONITOR is disabled in this test scriptSZEDER Gábor, Oct 11, 2018
  34. 3/6 t1700-split-index: date back files to avoid racy situationsSZEDER Gábor, Oct 11, 2018
  35. 2/6 split-index: add tests to demonstrate the racy split index problemSZEDER Gábor, Oct 11, 2018
  36. 4/6 split-index: count the number of deleted entriesSZEDER Gábor, Oct 11, 2018
  37. 5/6 split-index: don't compare cached data of entries already marked for split indexSZEDER Gábor, Oct 11, 2018
  38. 6/6 split-index: smudge and add racily clean cache entries to split indexSZEDER Gábor, Oct 11, 2018
  39. 7/6 split-index: BUG() when cache entry refers to non-existing shared entrySZEDER Gábor, Oct 11, 2018
  40. Ævar Arnfjörð BjarmasonOct 11, 2018
  41. SZEDER GáborOct 11, 2018
  42. Junio C HamanoOct 12, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.