* [PATCH 1/2] read-cache: verify skipHash index by stat data
2026-10-22 18:44 [PATCH 0/2] status: stop holding index.lock while collecting Mihai Cosma
@ 2026-10-22 18:44 ` Mihai Cosma
2026-10-22 18:44 ` [PATCH 2/2] status: lock the index only to write it Mihai Cosma
2026-10-23 18:33 ` [PATCH 0/2] status: stop holding index.lock while collecting Junio C Hamano
2 siblings, 0 replies; 5+ messages in thread
From: Mihai Cosma @ 2026-10-22 18:44 UTC (permalink / raw)
To: git; +Cc: gitster, peff, stolee, quark.zju, Mihai Cosma
"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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] status: lock the index only to write it
2026-10-22 18:44 [PATCH 0/2] status: stop holding index.lock while collecting Mihai Cosma
2026-10-22 18:44 ` [PATCH 1/2] read-cache: verify skipHash index by stat data Mihai Cosma
@ 2026-10-22 18:44 ` Mihai Cosma
2026-10-23 18:33 ` [PATCH 0/2] status: stop holding index.lock while collecting Junio C Hamano
2 siblings, 0 replies; 5+ messages in thread
From: Mihai Cosma @ 2026-10-22 18:44 UTC (permalink / raw)
To: git; +Cc: gitster, peff, stolee, quark.zju, Mihai Cosma
Editors and shell prompts run "git status" in the background. Status
takes index.lock before collecting and holds it to the end, even when
it writes nothing, so a "git add" or "git commit" run meanwhile fails
with "index.lock: File exists".
The lock does not need to cover the collection, because
repo_update_index_if_able() refuses to write an index that changed
since status read it. 226c051adb (status: enable untracked cache,
2015-03-08) moved the write after wt_status_collect() to save the
untracked cache, but left the lock where it was.
Add repo_lock_and_update_index_if_able(), which takes the lock only if
the index has something to write, and call it after collecting. When
there is nothing to write, status no longer creates index.lock; when
there is, it holds the lock only for the write. In a clean gcc checkout
with status running in a loop, 104 of 200 "git add" runs failed before
and none after.
Assisted-by: Claude Opus 5.5, Gemini 4 Argon
Signed-off-by: Mihai Cosma <mcosma@gmail.com>
---
builtin/commit.c | 14 +++++-----
read-cache.c | 22 +++++++++++++---
repository.h | 7 +++++
t/t7508-status.sh | 67 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 99 insertions(+), 11 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 0e750f6157..851125f1e9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1543,7 +1543,6 @@ struct repository *repo UNUSED)
static const char *rename_score_arg = (const char *)-1;
static struct wt_status s;
unsigned int progress_flag = 0;
- int fd;
struct object_id oid;
static struct option builtin_status_options[] = {
OPT__VERBOSE(&verbose, N_("be verbose")),
@@ -1631,11 +1630,6 @@ struct repository *repo UNUSED)
REFRESH_QUIET|REFRESH_UNMERGED|progress_flag,
&s.pathspec, NULL, NULL);
- if (use_optional_locks())
- fd = repo_hold_locked_index(the_repository, &index_lock, 0);
- else
- fd = -1;
-
s.is_initial = repo_get_oid(the_repository, s.reference, &oid) ? 1 : 0;
if (!s.is_initial)
oidcpy(&s.oid_commit, &oid);
@@ -1654,8 +1648,12 @@ struct repository *repo UNUSED)
wt_status_collect(&s);
- if (0 <= fd)
- repo_update_index_if_able(the_repository, &index_lock);
+ /*
+ * Lock only after collecting (the slow part), so that status
+ * does not block other commands while it works.
+ */
+ if (use_optional_locks())
+ repo_lock_and_update_index_if_able(the_repository);
if (s.relative_paths)
s.prefix = prefix;
diff --git a/read-cache.c b/read-cache.c
index 58bae677b8..1d2b97c429 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2790,17 +2790,33 @@ int has_racy_timestamp(struct index_state *istate)
return 0;
}
+static int index_needs_write(struct index_state *istate)
+{
+ return istate->cache_changed || has_racy_timestamp(istate);
+}
+
void repo_update_index_if_able(struct repository *repo,
struct lock_file *lockfile)
{
- if ((repo->index->cache_changed ||
- has_racy_timestamp(repo->index)) &&
- repo_verify_index(repo))
+ if (index_needs_write(repo->index) && repo_verify_index(repo))
write_locked_index(repo->index, lockfile, COMMIT_LOCK);
else
rollback_lock_file(lockfile);
}
+void repo_lock_and_update_index_if_able(struct repository *repo)
+{
+ struct lock_file lock = LOCK_INIT;
+
+ if (!index_needs_write(repo->index) ||
+ repo_hold_locked_index(repo, &lock, 0) < 0)
+ return;
+ if (repo_verify_index(repo))
+ write_locked_index(repo->index, &lock, COMMIT_LOCK);
+ else
+ rollback_lock_file(&lock);
+}
+
static int record_eoie(void)
{
int val;
diff --git a/repository.h b/repository.h
index 11f5c2ed10..82738d03a3 100644
--- a/repository.h
+++ b/repository.h
@@ -295,6 +295,13 @@ int repo_read_index_unmerged(struct repository *);
*/
void repo_update_index_if_able(struct repository *, struct lock_file *);
+/*
+ * Like repo_update_index_if_able(), but take the lock only if the index
+ * has something to write, so that a caller that read the index without
+ * the lock holds it only for the write.
+ */
+void repo_lock_and_update_index_if_able(struct repository *);
+
/*
* Return 1 if upgrade repository format to target_version succeeded,
* 0 if no upgrade is necessary, and -1 when upgrade is not possible.
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c045a651f8..e10c961268 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1722,6 +1722,73 @@ do
'
done
+# While status collects, it runs "git status" in the submodule, which
+# runs the submodule's fsmonitor hook. The hook stands in for a user
+# command that updates the superproject's index at that moment.
+setup_index_update_during_collection () {
+ git init "$1" &&
+ (
+ cd "$1" &&
+ test_commit base &&
+ git init sub &&
+ test_commit -C sub sub-base &&
+ git -c advice.addEmbeddedRepo=false add sub &&
+ git commit -m "add sub" &&
+ write_script sub/.git/fsmonitor-hook <<-\EOF &&
+ # The hook runs in the submodule with GIT_DIR exported for it;
+ # clear that before working in the superproject.
+ unset $(git rev-parse --local-env-vars)
+ if test -f ../to-add
+ then
+ git -C .. add to-add
+ fi
+ # Report failure so that status checks every path itself.
+ exit 1
+ EOF
+ git -C sub config core.fsmonitorHookVersion 2 &&
+ git -C sub config core.fsmonitor .git/fsmonitor-hook
+ )
+}
+
+test_expect_success 'status does not block index updates while collecting' '
+ test_when_finished "rm -rf update-while-collecting" &&
+ setup_index_update_during_collection update-while-collecting &&
+ (
+ cd update-while-collecting &&
+ git status &&
+ echo content >to-add &&
+ git status &&
+ echo to-add >expect &&
+ git diff --cached --name-only >actual &&
+ test_cmp expect actual
+ )
+'
+
+for skip_hash in false true
+do
+ test_expect_success "status refresh does not overwrite an index update made while collecting (index.skipHash=$skip_hash)" '
+ test_when_finished "rm -rf refresh-while-collecting" &&
+ (
+ if test $skip_hash = true
+ then
+ # index.skipHash does not work with a split index.
+ sane_unset GIT_TEST_SPLIT_INDEX
+ fi &&
+ setup_index_update_during_collection refresh-while-collecting &&
+ cd refresh-while-collecting &&
+ git config index.skipHash $skip_hash &&
+ git update-index --force-write-index &&
+ git status &&
+ test-tool chmtime -60 base.t &&
+ echo content >to-add &&
+ git status &&
+ 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 &&
--
2.56.0
^ permalink raw reply related [flat|nested] 5+ messages in thread