[PATCH 1/2] read-cache: verify skipHash index by stat data

[Mihai Cosma <mcosma@gmail.com>]

"git status" and other commands read and refresh the index without the
lock, then write it only if repo_update_index_if_able() finds the
file's trailing checksum unchanged, so they cannot undo an update made
by another process in between.

Since ee1f0c242e (read-cache: add index.skipHash config option,
2023-01-06), the trailer can be all zeros, and feature.manyFiles and
Scalar turn that on. The check then passes for any index on disk, and
a "git add" that lands between status reading the index and taking the
lock is silently reverted.

When the recorded checksum is null, check instead that the stat data of
the index file is unchanged since it was read or written, apart from
the ctime, which renaming the lockfile into place changes. Ignore
core.checkStat and core.useNanosec here: a false match loses an
update, while a false mismatch only skips an optional write. On Windows,
which reports no inode numbers, this rests on the size and the mtime,
so two same-size rewrites within one 100ns tick could still be missed.

unpack_trees() builds a new index from the one it was given and copies
its checksum, so copy the stat data along with it.

Assisted-by: Claude Opus 5.5, Gemini 4 Argon
Signed-off-by: Mihai Cosma <mcosma@gmail.com>
---
To see the bug without building anything, this loses a "git add" with
v2.56.0 (it prints nothing) and keeps it with this patch (it prints
"b"). The fsmonitor hook stands in for another process that
updates the index while status is reading it:

  git init -q repro && cd repro &&
  git config index.skipHash true &&
  echo a >a && git add a && git commit -qm a &&
  # this hook runs while status reads the index: stage b behind its back
  printf '%s\n' '#!/bin/sh' \
    'test -f b && ! test -f .git/done && >.git/done && git add b' \
    'exit 1' >.git/hook &&
  chmod +x .git/hook &&
  git config core.fsmonitor .git/hook &&
  git config core.fsmonitorHookVersion 2 &&
  touch -d '1 hour ago' a && echo b >b &&
  git status >/dev/null &&
  git diff --cached --name-only

 read-cache-ll.h   |  1 +
 read-cache.c      | 37 ++++++++++++++++++++++++++++++++++++-
 t/t7508-status.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 unpack-trees.c    |  1 +
 4 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/read-cache-ll.h b/read-cache-ll.h
index 066dd8bc3b..a45aff02ea 100644
--- a/read-cache-ll.h
+++ b/read-cache-ll.h
@@ -181,6 +181,7 @@ struct index_state {
 	struct hashmap name_hash;
 	struct hashmap dir_hash;
 	struct object_id oid;
+	struct stat_data file_stat;
 	struct untracked_cache *untracked;
 	char *fsmonitor_last_update;
 	struct ewah_bitmap *fsmonitor_dirty;
diff --git a/read-cache.c b/read-cache.c
index c4cf08a3a3..58bae677b8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2309,6 +2309,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 
 	istate->timestamp.sec = st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
+	fill_stat_data(&istate->file_stat, &st);
 
 	/* if we created a thread, join it otherwise load the extensions on the primary thread */
 	if (extension_offset) {
@@ -2699,9 +2700,35 @@ static int ce_write_entry(struct hashfile *f, struct cache_entry *ce,
 	return 0;
 }
 
+/*
+ * Whether the index file is still the one whose stat data was recorded
+ * when the index was read or written. This stands in for the checksum
+ * of an index written without one, so unlike match_stat_data() it
+ * always compares the inode and the mtime to the nanosecond, whatever
+ * core.checkStat and core.useNanosec say: a false "unchanged" would
+ * discard an update made by another process, while a false "changed"
+ * only skips an opportunistic write. The ctime is left out because
+ * renaming the written lockfile into place changes it.
+ */
+static int index_file_unchanged(const struct stat_data *sd, struct stat *st)
+{
+	struct stat_data now;
+
+	fill_stat_data(&now, st);
+	return sd->sd_mtime.sec == now.sd_mtime.sec &&
+	       sd->sd_mtime.nsec == now.sd_mtime.nsec &&
+	       sd->sd_dev == now.sd_dev &&
+	       sd->sd_ino == now.sd_ino &&
+	       sd->sd_uid == now.sd_uid &&
+	       sd->sd_gid == now.sd_gid &&
+	       sd->sd_size == now.sd_size;
+}
+
 /*
  * This function verifies if index_state has the correct sha1 of the
- * index file.  Don't die if we have any other failure, just return 0.
+ * index file, or, when the index was written without a checksum
+ * (index.skipHash), that the file's stat data has not changed. Don't
+ * die if we have any other failure, just return 0.
  */
 static int verify_index_from(const struct index_state *istate, const char *path)
 {
@@ -2720,6 +2747,13 @@ static int verify_index_from(const struct index_state *istate, const char *path)
 	if (fstat(fd, &st))
 		goto out;
 
+	if (is_null_oid(&istate->oid)) {
+		if (!index_file_unchanged(&istate->file_stat, &st))
+			goto out;
+		close(fd);
+		return 1;
+	}
+
 	if (st.st_size < sizeof(struct cache_header) + the_hash_algo->rawsz)
 		goto out;
 
@@ -3098,6 +3132,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	}
 	istate->timestamp.sec = (unsigned int)st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
+	fill_stat_data(&istate->file_stat, &st);
 	trace_performance_since(start, "write index, changed mask = %x", istate->cache_changed);
 
 	trace2_data_intmax("index", istate->repo, "write/version",
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 0fd7c79911..c045a651f8 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1681,6 +1681,47 @@ test_expect_success '--no-optional-locks prevents index update' '
 	! test_is_magic_mtime .git/index
 '
 
+# The fsmonitor hook runs while status reads the index, before status
+# takes the lock, and stands in for a user command that updates the
+# index at that moment.
+for skip_hash in false true
+do
+	test_expect_success "status keeps an index update made after it read the index (index.skipHash=$skip_hash)" '
+		test_when_finished "rm -rf update-after-read" &&
+		git init update-after-read &&
+		(
+			cd update-after-read &&
+			if test $skip_hash = true
+			then
+				# index.skipHash does not work with a split index.
+				sane_unset GIT_TEST_SPLIT_INDEX
+			fi &&
+			git config index.skipHash $skip_hash &&
+			test_commit base &&
+			write_script .git/fsmonitor-hook <<-\EOF &&
+			# Act once; the "git add" below runs this hook again.
+			if test -f to-add && ! test -f .git/hook-ran
+			then
+				>.git/hook-ran &&
+				git add to-add
+			fi
+			# Report failure so that status checks every path itself.
+			exit 1
+			EOF
+			git config core.fsmonitorHookVersion 2 &&
+			git config core.fsmonitor .git/fsmonitor-hook &&
+			git status &&
+			test-tool chmtime -60 base.t &&
+			echo content >to-add &&
+			git status &&
+			test_path_is_file .git/hook-ran &&
+			echo to-add >expect &&
+			git diff --cached --name-only >actual &&
+			test_cmp expect actual
+		)
+	'
+done
+
 test_expect_success 'racy timestamps will be fixed for clean worktree' '
 	echo content >racy-dirty &&
 	echo content >racy-racy &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 1802809ad3..d6a859420f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1959,6 +1959,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 			init_split_index(&o->internal.result);
 	}
 	oidcpy(&o->internal.result.oid, &o->src_index->oid);
+	o->internal.result.file_stat = o->src_index->file_stat;
 	o->internal.merge_size = len;
 	mark_all_ce_unused(o->src_index);
 
-- 
2.56.0