[PATCH v2 2/4] repository: add core.indexLockTimeout

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

From: Jun Wu <quark@meta.com>

Make "git add/rm/checkout" wait for the index lock, so they won't fail
immediately.

When an IDE runs "git status" every 5 seconds, and each "git status" holds the
index lock for 0.5s, a user has 10% chance to be unable to commit or checkout.
By waiting, the add and checkout commands can succeed.

Example commands that will wait:
- add, rm, mv, checkout / switch / restore, read-tree, apply --index /
  --cached, update-index, cherry-pick, revert, clone.

Example commands that are unchanged:
- repo_try_hold_locked_index() users (see previous commit):
  status, describe --dirty, part of diff / merge / rebase.
- repo_hold_locked_index() users, but racy (waiting makes race worse,
  see code comment for details): stash, part of merge, am,
  add -i / -p, checkout-index, sparse-checkout, rerere,
  commit (until the next commit), reset (until a later commit).

Default timeout is 1s. Mercurial uses 600s. In case of an unexpected
use-case with unexpected expectation, I picked a conservative middle
ground.

Some tests emulate the index lock being taken, the new behavior makes
those tests slower (but still passing). Those tests now set timeout to 0
to run as fast as before.

Alternatively, IDEs could use "git --no-optional-locks status" to avoid taking
the index lock. However, not all tool authors know about "--no-optional-locks",
and "--no-optional-locks" will lose the refreshed stat info, so later
(periodical) "git --no-optional-locks status" runs become slower over time
(assuming nothing refreshes the index).

Signed-off-by: Jun Wu <quark@meta.com>
---
 Documentation/config/core.adoc |  6 +++++
 repository.c                   | 26 +++++++++++++++++++-
 repository.h                   |  4 ++++
 t/meson.build                  |  1 +
 t/t0031-lockfile-pid.sh        |  4 ++++
 t/t0032-index-lock-timeout.sh  | 43 ++++++++++++++++++++++++++++++++++
 t/t7400-submodule-basic.sh     |  1 +
 7 files changed, 84 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 bdc74d291c..88f3529cc0 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 a74b1118cb..c7d098d166 100644
--- a/repository.c
+++ b/repository.c
@@ -473,9 +473,33 @@ 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 read before lock. Depending on the callsite
+		 * logic, the write might be racy:
+		 *
+		 * - read: repo_read_index()
+		 * - lock: repo_hold_locked_index() -> maybe sleep()
+		 * - write: repo_update_index_if_able() # okay
+		 *          write_locked_index() # racy, worse with sleep()
+		 *
+		 * Be conservative, avoid sleep() to not make the write race
+		 * worse. If the callsite wants the "waiting for index lock"
+		 * behavior, fix the read/lock order.
+		 */
+		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);
 }
 
 int repo_try_hold_locked_index(struct repository *repo,
diff --git a/repository.h b/repository.h
index 253a5f1d3f..fe4eedd7d3 100644
--- a/repository.h
+++ b/repository.h
@@ -284,6 +284,10 @@ void repo_clear(struct repository *repo);
  * populated then the number of entries will simply be returned.
  */
 int repo_read_index(struct repository *repo);
+
+/*
+ * Obtain the index lock. This can block (retry with backoff).
+ */
 int repo_hold_locked_index(struct repository *repo,
 			   struct lock_file *lf,
 			   int flags);
diff --git a/t/meson.build b/t/meson.build
index 11b3752f00..3aff2a9f50 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -108,6 +108,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..cde6f1892c
--- /dev/null
+++ b/t/t0032-index-lock-timeout.sh
@@ -0,0 +1,43 @@
+#!/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_expect_success 'index locked, checkout waits with indexLockTimeout>0' '
+	git commit -m init &&
+	prepare_index_lock &&
+	git -c core.indexLockTimeout=5000 checkout HEAD
+'
+
+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