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

[Mihai Cosma <mcosma@gmail.com>]

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