[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

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

[D. Ben Knoble]

Hi Jun,

On Tue, Oct 13, 2026 at 5:07 PM Jun Wu <quark.zju@gmail.com> wrote:
>
> 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.

Interesting. That must be the slow down you mentioned in your cover
letter? I'm trying to make sense of this. I think you're saying that
not having the index lock means status would "lose" refreshed info:
we're not throwing away the *existing* stat info, just the ones we
recomputed for this "git --no-optional-locks status" invocation,
right?

I suppose I don't see how that causes things to slow down continually,
but I bet I'm missing something :)

> 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.

It feels weird to me to fix mostly-invisible behavior from external
tools with a config option, though. I think of config *mostly* as
user-facing (whether that user is a client of the Git CLI or
configuring a server repo), and I don't see how we could teach users
to look for this or know what a reasonable setting should be for
themselves :/ Further, we already have a lot of config options! (I'm
guilty of adding one recently, heh.) It can definitely be overwhelming
for users to think about all the options, and I suspect many just
don't as a result.

I applaud the effort for sensible defaults here, which I think will help.

Anyway, maybe it's just me: if others are happy with this approach,
great. I think I would prefer to see us try to address the underlying
performance problems, though, if we can. (Maybe we can have some way
for git-status to not throw away all its work, or some other good way
to communicate to IDEs that want to poll?)

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

[Junio C Hamano]

Jun Wu <quark.zju@gmail.com> writes:

> 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.

It is unfair to expect that the readers would guess what you left
unsaid here, which probably is "we will introduce the third choice,
which does such and such things, and it improves the situation in
such and such ways because ...", before listing ...

> 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.

... these.

> 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.

Is the above supposed to be the description of the proposed solution?

There are multiple parties (i.e., `git status` that needed to
discard the result with "--no-optional-locks" and other end-user
actions that do need to safely update the index) involved, and it is
unclear whose actions many of verbs in your writing refer to in the
above.  E.g., "if the index is read before locking, the wait will be
skipped" for whom?  "git status" that wants to opportunistically
write out the result of refreshing?  End-user actions like "git add"
that wanted to register new contents to the index?  Both?

> 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

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

[Junio C Hamano]

Jun Wu <quark.zju@gmail.com> writes:

> 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.

Need to change the existing tests is a sign that end-user workflows
that are emulated by them will be impacted, and we'd need to be
careful.

I am not sure if the change is making the right trade-off between
background tasks and end-user action that expects immediate
response.

> @@ -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);
>  }

A few comments and/or questions..

 (1) In the unhappy case where the index was read already and then
     we try to take the lock, what should happen when we notice that
     the index is locked and better yet if we notice that the index
     has been updated before we write the in-core index we wanted to
     write out?  The question is what "should" happen, not "what
     happens with the current code".

 (2) Are there cases where we take lock only to make sure nobody
     will update the index without any intention to write out the
     contents of our in-core index?  IOW, we are holding the lock
     only to have a consistent world view while performing a
     read-only operation?  How would this change affect it?

 (3) When 'repo->index->initialized' is true, does it always mean we
     have read the on-disk index already, or are there cases where
     we created the in-core index from scratch and called this
     function with the intention to write over it?  For such a use
     case, what, if any, are the implications of this change?