[PATCH 0/2] Add a way to wait for index lock

[PATCH 0/2] Add a way to wait for index lock

[Jun Wu]

From: Jun Wu <quark@meta.com>

An IDE that runs `git status` periodically in the background has 2
choices. Both are suboptimal:
- with `--no-optional-locks`: `status` becomes slower over time.
- without `--no-optional-locks`: conflicts with other commands like
  `add`, `commit`, often run by a user from the IDE terminal.

Patch 1 adds index lock timeout so `add` can wait instead of fail.
Patch 2 adjusts the read/lock order so `commit` can also wait.

To minimize write race, if the index is read before locking, the
wait will be skipped, as if timeout is 0. Depending on the read/lock
order, some commands (e.g. add, rm, mv, checkout, restore, read-tree,
apply, update-index) respect the timeout. Some (e.g. reset, stash,
merge, rebase, cherry-pick, am) do not. I started with the simple and
common commands. If there is interest in other commands I can take a
look too.

The default timeout is 1s (reasoning in patch 1). I'm happy to change it
to 0 if people prefer no behavior change.

Related context:
- (2017-9-22) https://lore.kernel.org/git/20170923033425.6ilykxl5pi7unlj6@sigill.intra.peff.net/
  proposed index lock timeout.
- (2017-9-27) Commit 27344d6a6c (git: add --no-optional-locks option, 2017-09-27)
  introduces `--no-optional-locks` for the same issue.
- (2019-5-3) https://lore.kernel.org/git/CACsJy8BcBk6Fsfy7o1aBTQAPeH-w+rBwLZrzw4jJ-OM_jhkxMQ@mail.gmail.com/
  mentioned `update_if_able` and "gap" between read and lock, mitigated
  in patch 1 with the `index->initialized` check.

Jun Wu (2):
  repository: add core.indexLockTimeout
  commit: lock index before reading

 Documentation/config/core.adoc |  6 ++++++
 builtin/commit.c               |  9 ++-------
 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 +
 7 files changed, 67 insertions(+), 8 deletions(-)
 create mode 100755 t/t0032-index-lock-timeout.sh


base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
-- 
2.56.0

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

[Jun Wu]

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

[PATCH 2/2] commit: lock index before reading

[Jun Wu]

From: Jun Wu <quark@meta.com>

Take the index lock before reading the index, in one place, instead
of separately in each `if` branch.

`commit` now respects the `core.indexLockTimeout` config.

History suggests no particular reason for reading before locking:
- f5bbc3225c (Port git commit to C., 2007-11-08): lock before reading
- 2888605c64 (builtin-commit: fix partial-commit support, 2007-11-18):
  seems to accidentally move reading to before locking

Signed-off-by: Jun Wu <quark@meta.com>
---
 builtin/commit.c              | 9 ++-------
 t/t0032-index-lock-timeout.sh | 4 ++--
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 840b6b4083..08dd53ad4e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -389,13 +389,13 @@ static const char *prepare_index(const char **argv, const char *prefix,
 	    (!amend || (fixup_message && strcmp(fixup_prefix, "amend"))))))
 		die(_("No paths with --include/--only does not make sense."));
 
+	repo_hold_locked_index(the_repository, &index_lock, LOCK_DIE_ON_ERROR);
+
 	if (repo_read_index_preload(the_repository, &pathspec, 0) < 0)
 		die(_("index file corrupt"));
 
 	if (interactive) {
 		char *old_index_env = NULL, *old_repo_index_file;
-		repo_hold_locked_index(the_repository, &index_lock,
-				       LOCK_DIE_ON_ERROR);
 
 		refresh_cache_or_die(refresh_flags);
 
@@ -453,8 +453,6 @@ static const char *prepare_index(const char **argv, const char *prefix,
 	 */
 	if (all || (also && pathspec.nr)) {
 		char *ps_matched = xcalloc(pathspec.nr, 1);
-		repo_hold_locked_index(the_repository, &index_lock,
-				       LOCK_DIE_ON_ERROR);
 		add_files_to_cache(the_repository, also ? prefix : NULL,
 				   &pathspec, ps_matched, 0, 0, 0 );
 		if (!all && report_path_error(ps_matched, &pathspec))
@@ -480,8 +478,6 @@ static const char *prepare_index(const char **argv, const char *prefix,
 	 * We still need to refresh the index here.
 	 */
 	if (!only && !pathspec.nr) {
-		repo_hold_locked_index(the_repository, &index_lock,
-				       LOCK_DIE_ON_ERROR);
 		refresh_cache_or_die(refresh_flags);
 		if (the_repository->index->cache_changed
 		    || !cache_tree_fully_valid(the_repository->index))
@@ -531,7 +527,6 @@ static const char *prepare_index(const char **argv, const char *prefix,
 	if (repo_read_index(the_repository) < 0)
 		die(_("cannot read the index"));
 
-	repo_hold_locked_index(the_repository, &index_lock, LOCK_DIE_ON_ERROR);
 	add_remove_files(&partial);
 	refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
 	cache_tree_update(the_repository->index, WRITE_TREE_SILENT);
diff --git a/t/t0032-index-lock-timeout.sh b/t/t0032-index-lock-timeout.sh
index ca334c3eef..0c36a7cdfe 100755
--- a/t/t0032-index-lock-timeout.sh
+++ b/t/t0032-index-lock-timeout.sh
@@ -29,9 +29,9 @@ test_expect_success 'index locked, add waits with indexLockTimeout>0' '
 	git -c core.indexLockTimeout=5000 add foo
 '
 
-test_expect_success 'index locked, commit fails even with indexLockTimeout>0 (read index before lock)' '
+test_expect_success 'index locked, commit waits with indexLockTimeout>0' '
 	prepare_index_lock &&
-	test_must_fail_with_index_lock git -c core.indexLockTimeout=5000 commit -m init
+	git -c core.indexLockTimeout=5000 commit -m init
 '
 
 test_done
-- 
2.56.0