archive mirror
 help / color / mirror / Atom feed
From: "SZEDER Gábor" <>
Cc: "Junio C Hamano" <>,
	"Duy Nguyen" <>,
	"Thomas Gummerer" <>,
	"Ævar Arnfjörð Bjarmason" <>,
	"SZEDER Gábor" <>
Subject: [RFC PATCH 5/5] split-index: smudge and add racily clean cache entries to split index
Date: Thu,  6 Sep 2018 04:48:10 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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.

In the above example, however, in the second 'git update-index'
prepare_to_write_split_index() gathers all cache entries that should
be written to 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.  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

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 '', though,
because they actually check which entries are stored in the split
index; the previous patch made the necessary adjustments.  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 shows when the racy file was written,
which is not always in the failing test (but note that those lines are
in the 'after failure' fold, and your browser might unhelpfully fold
it up before you could take a good look).


There might be others, e.g. perhaps '' and
others using '', 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).

Signed-off-by: SZEDER Gábor <>
 cache.h                     |  2 ++
 read-cache.c                |  2 +-
 split-index.c               | 12 +++++++++++-
 t/ |  4 +---
 4 files changed, 15 insertions(+), 5 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 */
+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 84f067e10d..b4458f31f6 100644
--- a/split-index.c
+++ b/split-index.c
@@ -220,8 +220,18 @@ 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) {
+				/*
+				 * Racily clean cache entries must be
+				 * written to the split index, so the
+				 * subsequent do_write_index() can smudge
+				 * their stat data.
+				 */
+				if (!ce_uptodate(ce) &&
+				    is_racy_timestamp(istate, ce))
+					ce->ce_flags |= CE_UPDATE_IN_BASE;
+			}
 			if (ce->ce_namelen != base->ce_namelen ||
 			    strcmp(ce->name, base->name)) {
 				ce->index = 0;
diff --git a/t/ b/t/
index c92ba9ce5e..6780d08367 100755
--- a/t/
+++ b/t/
@@ -77,7 +77,7 @@ done
 for trial in 0 1 2 3 4
-	test_expect_failure "update the split index when the shared index contains a racily clean cache entry #$trial" '
+	test_expect_success "update the split index when the shared index contains a racily clean cache entry #$trial" '
 		test_when_finished "rm -f .git/index .git/sharedindex.*" &&
 		# The next three commands must be run within the same
@@ -100,8 +100,6 @@ do
 		# corresponding replacement cache entry with smudged
 		# stat data should be added to the new split index, so
 		# the file wont appear clean for subsequent git commands.
-		#
-		# Alas, such a smudged replacement entry is not added!
 		git update-index --add other-file &&

  parent reply	other threads:[~2018-09-06  2:48 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-06  2:48 [RFC PATCH 0/5] Fix the racy split index problem SZEDER Gábor
2018-09-06  2:48 ` [PATCH 1/5] t1700-split-index: drop unnecessary 'grep' SZEDER Gábor
2018-09-06 21:24   ` Junio C Hamano
2018-09-08 13:50   ` Duy Nguyen
2018-09-06  2:48 ` [PATCH 2/5] t0090: disable GIT_TEST_SPLIT_INDEX for the test checking split index SZEDER Gábor
2018-09-06  8:03   ` Ævar Arnfjörð Bjarmason
2018-09-06  2:48 ` [RFC PATCH 3/5] split index: add a test to demonstrate the racy split index problem SZEDER Gábor
2018-09-06  2:48 ` [RFC PATCH 4/5] t1700-split-index: date back files to avoid racy situations SZEDER Gábor
2018-09-06  8:02   ` Ævar Arnfjörð Bjarmason
2018-09-06  9:15     ` SZEDER Gábor
2018-09-06  9:20       ` Ævar Arnfjörð Bjarmason
2018-09-06  2:48 ` SZEDER Gábor [this message]
2018-09-06 10:26   ` [RFC PATCH 5/5] split-index: smudge and add racily clean cache entries to split index Ævar Arnfjörð Bjarmason
2018-09-06 12:26   ` Ævar Arnfjörð Bjarmason
2018-09-06 15:14     ` SZEDER Gábor
2018-09-06 15:26       ` Ævar Arnfjörð Bjarmason
2018-09-06 17:53         ` Ævar Arnfjörð Bjarmason
2018-09-07  3:49           ` SZEDER Gábor
2018-09-10 22:12           ` Paul-Sebastian Ungureanu
2018-09-08 16:45   ` Duy Nguyen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).