[PATCH 0/2] status: stop holding index.lock while collecting

[PATCH 0/2] status: stop holding index.lock while collecting

[Mihai Cosma]

Background "git status" runs from editors and shell prompts hold
index.lock for their whole run, so a "git add" or "git commit" run at
the same time fails. Patch 2 makes status take the lock only to write,
and only when it has something to write.

Patch 1 fixes a bug that patch 2 relies on but that exists on its own:
under index.skipHash (on with feature.manyFiles and in Scalar repos),
the check that stops status from overwriting a concurrently updated
index passes for any index, so status can silently revert a "git add",
also on 2.56.0. It applies to maint and can go in alone. There's a
reproduction below its "---".

This overlaps with Jun Wu's "Wait for the index lock instead of
failing" series, and the two fit together: his makes writers wait for
the lock, this makes status hold it rarely and briefly. They only
conflict on the one lock call in cmd_status(), which his series renames
to repo_try_hold_locked_index(). I'm happy to rebase onto it. On top of
his v2, the slowest "git add" in the clean case below went from 1020 ms
(one failure at his 1s timeout) to 160 ms.

Numbers on master, from a gcc checkout (124,311 files) on Linux, timing
200 "git add" runs while "git status" runs in a loop. "Active" touches
a tracked file before each status, so every status run writes the
index:

                       master      this series
  clean   failed       104/200     0/200
          p95 (ms)     114         128
  active  failed       123/200     47/200
          p95 (ms)     117         111

Failed runs on master return fast, which is why its timings look good.
In the active case status still holds the lock while it writes, about
76 ms for gcc's 14 MB index, and that's the part a short wait like
Jun's covers.

The script, run from the top of the checkout as "bash measure.sh clean"
or "bash measure.sh active":

  touched=$(git ls-files | head -1); echo x >measure-file
  ( end=$((SECONDS+60)); while [ $SECONDS -lt $end ]; do
      [ "$1" = active ] && touch "$touched"
      git status --porcelain >/dev/null 2>&1
    done ) &
  sleep 1
  for i in $(seq 200); do
      s=$(date +%s%N); git add measure-file 2>/dev/null; rc=$?
      echo $(( ($(date +%s%N) - s) / 1000000 )) $rc; sleep 0.05
  done | sort -n | awk '{t[NR]=$1; f+=($2!=0)} END {
      print "median", t[int(NR/2)], "p95", t[int(NR*0.95)],
            "max", t[NR], "ms, failed", f "/" NR }'
  wait; git rm -q --cached measure-file; rm measure-file

The skipHash tests turn split index off, because index.skipHash and a
split index don't work together at all today: with both set, "git add"
stages nothing, which Peff noticed back in 2023 [1].

[1] https://lore.kernel.org/git/20230629083859.GA585934@coredump.intra.peff.net/

Used Claude Opus 5.5, and Gemini 4 Argon for review.

Mihai Cosma (2):
  read-cache: verify skipHash index by stat data
  status: lock the index only to write it

 builtin/commit.c  |  14 +++---
 read-cache-ll.h   |   1 +
 read-cache.c      |  59 +++++++++++++++++++++++--
 repository.h      |   7 +++
 t/t7508-status.sh | 108 ++++++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.c    |   1 +
 6 files changed, 178 insertions(+), 12 deletions(-)


base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926
-- 
2.56.0

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

[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

[PATCH 2/2] status: lock the index only to write it

[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

Re: [PATCH 0/2] status: stop holding index.lock while collecting

[Junio C Hamano]

Mihai Cosma <mcosma@gmail.com> writes:

> This overlaps with Jun Wu's "Wait for the index lock instead of
> failing" series, and the two fit together: his makes writers wait for
> the lock, this makes status hold it rarely and briefly. They only
> conflict on the one lock call in cmd_status(), which his series renames
> to repo_try_hold_locked_index(). I'm happy to rebase onto it. On top of
> his v2, the slowest "git add" in the clean case below went from 1020 ms
> (one failure at his 1s timeout) to 160 ms.

Perhaps you two can work together and send a single consolidated series?

Thanks.

Re: [PATCH 0/2] status: stop holding index.lock while collecting

[Jun Wu]

Hi Junio,

On Fri, 23 Oct 2026 11:33:12 -0700, Junio C Hamano wrote:
> Mihai Cosma <mcosma@gmail.com> writes:
>
> > This overlaps with Jun Wu's "Wait for the index lock instead of
> > failing" series, and the two fit together: his makes writers wait for
> > the lock, this makes status hold it rarely and briefly. They only
> > conflict on the one lock call in cmd_status(), which his series renames
> > to repo_try_hold_locked_index(). I'm happy to rebase onto it. On top of
> > his v2, the slowest "git add" in the clean case below went from 1020 ms
> > (one failure at his 1s timeout) to 160 ms.
>
> Perhaps you two can work together and send a single consolidated series?

I think the two series are relatively independent:
- Mine changed non-status commands.
- Mihai's mainly changed "status".

Neither series replaces any part of the other.

There are multiple subtle pre-existing race conditions involved:
- I fixed some read/lock wrong order races.
- Mihai fixed a update_index_if_able() race when skipHash=true.
  (This alone could go to maint.)

My preference is to keep them separate to bound the mental complexity
needed to understand related issues. The actual textual conflict is just
one function name.

-Jun