[PATCH 1/2] repository: add core.indexLockTimeout

[Jun Wu <quark.zju@gmail.com>]

From: Jun Wu <quark@meta.com>

When an IDE runs `git status` periodically in the background, `status`
can take the index lock, and this prevents other commands like `add`,
`commit` from running.

Commit 27344d6a6c (git: add --no-optional-locks option, 2017-09-27)
avoids the conflict by letting background `git --no-optional-locks status`
skip the index lock, at the cost of losing refreshed stat info, so
the next `status` has to re-hash files.

Approach the conflict from the other side: let non-status commands wait
for the index lock. Mercurial and Sapling have a similar "waiting for wlock"
behavior, and Git already has `core.{config,filesRef}LockTimeout`.

Ideally all non-status commands can wait for the index lock. However,
some code paths (e.g. `commit`) load the index without locking, then lock and
write changes. It is already racy and the wait behavior would make it much
worse. As a defense, waiting is disabled if index was already read.

Default timeout is 1s, effective but not too long to avoid breaking potential
user expectation (e.g. "an index.lock error means I should check whether a
commit message editor is open somewhere"). Tests that are slowed down by the
default timeout now set the timeout to 0.

Signed-off-by: Jun Wu <quark@meta.com>
---
 Documentation/config/core.adoc |  6 ++++++
 repository.c                   | 17 +++++++++++++++-
 t/meson.build                  |  1 +
 t/t0031-lockfile-pid.sh        |  4 ++++
 t/t0032-index-lock-timeout.sh  | 37 ++++++++++++++++++++++++++++++++++
 t/t7400-submodule-basic.sh     |  1 +
 6 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100755 t/t0032-index-lock-timeout.sh

diff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc
index 0b697f53f1..d06990b7ea 100644
--- a/Documentation/config/core.adoc
+++ b/Documentation/config/core.adoc
@@ -604,6 +604,12 @@ core.configLockTimeout::
 	that is already on disk before the lock is taken, so it can be
 	set persistently like any other option.
 
+core.indexLockTimeout::
+	The length of time, in milliseconds, to retry when trying to
+	lock the index file for writing. Value 0 means not to retry at
+	all; -1 means to try indefinitely. Default is 1000 (i.e., retry
+	for 1 second). Not supported by all commands.
+
 core.pager::
 	Text viewer for use by Git commands (e.g., 'less').  The value
 	is meant to be interpreted by the shell.  The order of preference
diff --git a/repository.c b/repository.c
index b857e1c580..3eaea3143c 100644
--- a/repository.c
+++ b/repository.c
@@ -473,7 +473,22 @@ int repo_hold_locked_index(struct repository *repo,
 			   struct lock_file *lf,
 			   int flags)
 {
+	int timeout_ms;
+
 	if (!repo->index_file)
 		BUG("the repo hasn't been setup");
-	return repo_hold_lock_file_for_update(repo, lf, repo->index_file, flags);
+
+	if (repo->index->initialized) {
+		/*
+		 * Index was initialized (read) without lock. To minimize the
+		 * chance of lost update, do not wait.
+		 */
+		timeout_ms = 0;
+	} else {
+		timeout_ms = 1000;
+		repo_config_get_int(repo, "core.indexlocktimeout", &timeout_ms);
+	}
+
+	return repo_hold_lock_file_for_update_timeout(repo, lf, repo->index_file,
+						      flags, timeout_ms);
 }
diff --git a/t/meson.build b/t/meson.build
index f65eb04684..d4debf674e 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -102,6 +102,7 @@ integration_tests = [
   't0029-core-unsetenvvars.sh',
   't0030-stripspace.sh',
   't0031-lockfile-pid.sh',
+  't0032-index-lock-timeout.sh',
   't0033-safe-directory.sh',
   't0034-root-safe-directory.sh',
   't0035-safe-bare-repository.sh',
diff --git a/t/t0031-lockfile-pid.sh b/t/t0031-lockfile-pid.sh
index e9e2f04049..3fc8de02b6 100755
--- a/t/t0031-lockfile-pid.sh
+++ b/t/t0031-lockfile-pid.sh
@@ -8,6 +8,10 @@ The feature is opt-in via core.lockfilePid config setting (boolean).
 
 . ./test-lib.sh
 
+test_expect_success 'setup' '
+	git config --global core.indexLockTimeout 0
+'
+
 test_expect_success 'stale lock detected when PID is not running' '
 	git init repo &&
 	(
diff --git a/t/t0032-index-lock-timeout.sh b/t/t0032-index-lock-timeout.sh
new file mode 100755
index 0000000000..ca334c3eef
--- /dev/null
+++ b/t/t0032-index-lock-timeout.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='Test config core.indexLockTimeout'
+
+. ./test-lib.sh
+
+prepare_index_lock () {
+	test_when_finished "rm -f .git/index.lock" &&
+	touch .git/index.lock &&
+	{
+		( sleep 1 && rm -f .git/index.lock ) &
+	} &&
+	test_when_finished "wait"
+}
+
+test_must_fail_with_index_lock () {
+	test_must_fail "$@" 2>err &&
+	test_grep index.lock err
+}
+
+test_expect_success 'index locked, add fails with indexLockTimeout=0' '
+	prepare_index_lock &&
+	touch foo &&
+	test_must_fail_with_index_lock git -c core.indexLockTimeout=0 add foo
+'
+
+test_expect_success 'index locked, add waits with indexLockTimeout>0' '
+	prepare_index_lock &&
+	git -c core.indexLockTimeout=5000 add foo
+'
+
+test_expect_success 'index locked, commit fails even with indexLockTimeout>0 (read index before lock)' '
+	prepare_index_lock &&
+	test_must_fail_with_index_lock git -c core.indexLockTimeout=5000 commit -m init
+'
+
+test_done
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index eefdecb0bd..9a90b6a76d 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -276,6 +276,7 @@ test_expect_success 'submodule add to reconfigure existing submodule with --forc
 '
 
 test_expect_success 'submodule add relays add --dry-run stderr' '
+	test_config_global core.indexLockTimeout 0 &&
 	test_when_finished "rm -rf addtest/.git/index.lock" &&
 	(
 		cd addtest &&
-- 
2.56.0