* [PATCH 1/2] repository: add core.indexLockTimeout
2026-10-13 17:23 [PATCH 0/2] Add a way to wait for index lock Jun Wu
@ 2026-10-13 17:23 ` Jun Wu
2026-10-13 21:40 ` D. Ben Knoble
2026-10-13 22:05 ` Junio C Hamano
2026-10-13 17:23 ` [PATCH 2/2] commit: lock index before reading Jun Wu
` (2 subsequent siblings)
3 siblings, 2 replies; 10+ messages in thread
From: Jun Wu @ 2026-10-13 17:23 UTC (permalink / raw)
To: git; +Cc: gitster, peff, ps, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] repository: add core.indexLockTimeout
2026-10-13 17:23 ` [PATCH 1/2] repository: add core.indexLockTimeout Jun Wu
@ 2026-10-13 21:40 ` D. Ben Knoble
2026-10-14 2:38 ` Jun Wu
2026-10-13 22:05 ` Junio C Hamano
1 sibling, 1 reply; 10+ messages in thread
From: D. Ben Knoble @ 2026-10-13 21:40 UTC (permalink / raw)
To: Jun Wu; +Cc: git, gitster, peff, ps, Jun Wu
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?)
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] repository: add core.indexLockTimeout
2026-10-13 21:40 ` D. Ben Knoble
@ 2026-10-14 2:38 ` Jun Wu
2026-10-14 16:29 ` D. Ben Knoble
0 siblings, 1 reply; 10+ messages in thread
From: Jun Wu @ 2026-10-14 2:38 UTC (permalink / raw)
To: D. Ben Knoble; +Cc: git, gitster, peff, ps, Jun Wu
On Tue, 13 Oct 2026 17:40:01 -0400, D. Ben Knoble wrote:
> Hi Jun,
Hi Ben,
Thanks for taking a look. This area is a bit tricky, so let me try to
explain more clearly.
> 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?
Yes. A file's stat info might be updated without content change (e.g.
`touch`-ed) so the existing stat info can become outdated over time, and status
cannot use the stat fast path to confirm a file is not changed, so status falls
back to slow content check. Note: if the user runs a command like `add` that
writes the index, the performance can recover, but based on our experience,
users might not run such recovery commands for a long time, so they suffer from
slow status.
> I suppose I don't see how that causes things to slow down continually,
> but I bet I'm missing something :)
To give some (hopefully useful) examples,
1. https://github.com/facebook/sapling/issues/929#issuecomment-2628723586
A user says "I use CLI exclusively so I would be really interested if
--no-optional-locks was applied only to automation". The user was running
Sapling in a `.git` repo. Sapling (back then) passed `--no-optional-locks`
to every `git status` call. The user found `status` too slow to use, and
hope that non-automation commands would not use `--no-optional-locks`.
In other words, always using `--no-optional-locks` caused slowness.
2. https://github.com/facebook/sapling/commit/f11d555a3a237254e1788f9e9da40b37cd6e452f
This is Sapling, but the concept (stat fast path) matches Git. Before this
change, similar to Git, Sapling's status takes the wlock (akin to
index.lock) in a best-effort way, and skip updating if it cannot get the
lock.
In production, we observed cases where `status` cannot get the lock for a
long time, and the user's `status` remains slow (IIRC tens of seconds to
minutes) without clear way to recover. This change forces a recovery path:
if there are too many (>200) files with outdated `stat` info, then wait for
the lock and update stats.
> 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 agree that users shouldn't need to know about extra options to get reasonable
behavior. In this case, I think the default (1s) is good, so users already
don't need to learn about it. The before/after matrix looks like:
| Background status | Before | After |
| | slow | cannot | slow | cannot |
| | status | commit | status | commit |
--------------------------------------------------------------
| --no-optional-locks | yes | no | yes | no |
| (regular) status | no | sometimes | no | rarely |
The bottom-right "no, rarely" + no need for `--no-optional-locks` seems to be
the ideal trade-off to me.
I think the same preference (good out-of-box) applies to `--no-optional-locks`,
too. I suspect that not a lot of tool authors know about `--no-optional-locks`.
https://github.com/facebook/sapling/commit/39380e1871d7c5f6f91ebb67c4efb7588eb9f65f
documented our version of "tool author doesn't know" - tool didn't set the
"is automation" env var, akin to forgetting `--no-optional-locks`. The repo in
question had a spiked ~40% `git checkout` failure rate.
> 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?)
To recap, the user-facing problem is "commit/checkout" didn't work when
`status` was running, or `--no-optional-locks status` can be too slow.
By "address the underlying performance problems", do you mean:
1. Improve `status` performance so it takes index lock for a shorter duration.
It helps, but as long as `status` takes the lock, there is a chance it
conflicts with other commands. This series allows tools to drop
`--no-optional-locks`, which indirectly helps `status` performance.
2. Improve `--no-optional-locks status` performance so it's fast.
This sounds like "lock-free writes to the index", which might require
a complex file format change.
3. Switch IDEs from polling `git status` to fsmonitor-style `status` notification.
This helps. But similar to 1, reducing `status` frequency alone does not
address the root issue.
For what it's worth, Mercurial and Sapling have waited for the wlock by
default for a long time, with default timeout set to 10 minutes. In our
experience that has worked well. The most common "waiting for lock" report we
see is an editor left open somewhere, which got rarer after we started printing
the process tree of the lock holder.
Thanks, Jun
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] repository: add core.indexLockTimeout
2026-10-14 2:38 ` Jun Wu
@ 2026-10-14 16:29 ` D. Ben Knoble
0 siblings, 0 replies; 10+ messages in thread
From: D. Ben Knoble @ 2026-10-14 16:29 UTC (permalink / raw)
To: Jun Wu; +Cc: git, gitster, peff, ps, Jun Wu
On Tue, Oct 13, 2026 at 10:38 PM Jun Wu <quark.zju@gmail.com> wrote:
>
> On Tue, 13 Oct 2026 17:40:01 -0400, D. Ben Knoble wrote:
> > Hi Jun,
>
> Hi Ben,
>
> Thanks for taking a look. This area is a bit tricky, so let me try to
> explain more clearly.
>
> > 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?
>
> Yes. A file's stat info might be updated without content change (e.g.
> `touch`-ed) so the existing stat info can become outdated over time, and status
> cannot use the stat fast path to confirm a file is not changed, so status falls
> back to slow content check. Note: if the user runs a command like `add` that
> writes the index, the performance can recover, but based on our experience,
> users might not run such recovery commands for a long time, so they suffer from
> slow status.
Makes sense.
> > I suppose I don't see how that causes things to slow down continually,
> > but I bet I'm missing something :)
>
> To give some (hopefully useful) examples,
Sorry, I wonder if I wasn't clear: I read the cover letter as saying
that performance worsens over time, and I'm not sure how that's
implied. Maybe the idea is that more and more files need to be
content-hashed, though?
> > 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?)
It does sound like others (Peff) think this is a fine approach, so
I'll bow out now :)
> By "address the underlying performance problems", do you mean:
>
> 1. Improve `status` performance so it takes index lock for a shorter duration.
> It helps, but as long as `status` takes the lock, there is a chance it
> conflicts with other commands. This series allows tools to drop
> `--no-optional-locks`, which indirectly helps `status` performance.
> 2. Improve `--no-optional-locks status` performance so it's fast.
> This sounds like "lock-free writes to the index", which might require
> a complex file format change.
> 3. Switch IDEs from polling `git status` to fsmonitor-style `status` notification.
> This helps. But similar to 1, reducing `status` frequency alone does not
> address the root issue.
These all sound like great ideas! I actually didn't have a concrete
suggestion, other than trying to address the root cause rather than
(IMO) band-aiding the surface.
Looking a bit more at how my editor integration works (tpope's
fugitive for Vim), it seems to *not* use --no-optional-locks and
(presumably) avoids the issues you're seeing by limiting background
refreshes to specific events [1] and doing some kind of caching, but I
haven't really worked through it all and I'm not sure how easy it
would be to extract generic ideas for different editors :) It's also
possible I just haven't ever run into the situation where
--no-optional-locks would have helped, so might be a moot point.
[1]: https://github.com/tpope/vim-fugitive/tree/3b753cf8c6a4dcde6edee8827d464ba9b8c4a6f0
--
D. Ben Knoble
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] repository: add core.indexLockTimeout
2026-10-13 17:23 ` [PATCH 1/2] repository: add core.indexLockTimeout Jun Wu
2026-10-13 21:40 ` D. Ben Knoble
@ 2026-10-13 22:05 ` Junio C Hamano
1 sibling, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2026-10-13 22:05 UTC (permalink / raw)
To: Jun Wu; +Cc: git, peff, ps, Jun Wu
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] commit: lock index before reading
2026-10-13 17:23 [PATCH 0/2] Add a way to wait for index lock Jun Wu
2026-10-13 17:23 ` [PATCH 1/2] repository: add core.indexLockTimeout Jun Wu
@ 2026-10-13 17:23 ` Jun Wu
2026-10-14 10:04 ` Jeff King
2026-10-13 22:05 ` [PATCH 0/2] Add a way to wait for index lock Junio C Hamano
2026-10-14 10:14 ` Jeff King
3 siblings, 1 reply; 10+ messages in thread
From: Jun Wu @ 2026-10-13 17:23 UTC (permalink / raw)
To: git; +Cc: gitster, peff, ps, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] commit: lock index before reading
2026-10-13 17:23 ` [PATCH 2/2] commit: lock index before reading Jun Wu
@ 2026-10-14 10:04 ` Jeff King
0 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2026-10-14 10:04 UTC (permalink / raw)
To: Jun Wu; +Cc: git, gitster, ps, Jun Wu
On Tue, Oct 13, 2026 at 10:23:15AM -0700, Jun Wu wrote:
> 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
Hmm. We may do a lot more work under lock than we used to, running
stat() on the whole repo.
But I think there's a deeper issue here. We're reading the index, _then_
taking the lock, but not necessarily discarding what we read earlier. So
it feels like two processes could race here, with git-commit discarding
some update by another process (like git-add).
If we instrument git-commit like this:
diff --git a/builtin/commit.c b/builtin/commit.c
index 840b6b4083..2a78cb473a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -392,6 +392,8 @@ static const char *prepare_index(const char **argv, const char *prefix,
if (repo_read_index_preload(the_repository, &pathspec, 0) < 0)
die(_("index file corrupt"));
+ sleep(5);
+
if (interactive) {
char *old_index_env = NULL, *old_repo_index_file;
repo_hold_locked_index(the_repository, &index_lock,
that widens the race window. And then something like this:
echo new content >old-file
git commit -am foo &
sleep 1 ;# commit is sleeping in race window
git add new-file
will clobber the index change from git-add.
And that is true with today's code, not using any timeouts. Adding a
timeout doesn't really change anything, though if you _do_ see lock
contention it is a good sign that you are about to hit the race and
clobber something. ;) So it is widening the window. But the correctness
problem is already there.
I do think your patch fixes the problem. But we probably want to discuss
that in the commit message as a motivation (and likewise, discuss the
fact that we may be holding the lock for longer).
There's probably some kind of hybrid world where we optimistically read
without the lock, then take the lock, and then check (cheaply-ish) that
the index didn't change since what we read. But I don't think we do that
in this code path.
-Peff
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Add a way to wait for index lock
2026-10-13 17:23 [PATCH 0/2] Add a way to wait for index lock Jun Wu
2026-10-13 17:23 ` [PATCH 1/2] repository: add core.indexLockTimeout Jun Wu
2026-10-13 17:23 ` [PATCH 2/2] commit: lock index before reading Jun Wu
@ 2026-10-13 22:05 ` Junio C Hamano
2026-10-14 10:14 ` Jeff King
3 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2026-10-13 22:05 UTC (permalink / raw)
To: Jun Wu; +Cc: git, peff, ps, Jun Wu
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Add a way to wait for index lock
2026-10-13 17:23 [PATCH 0/2] Add a way to wait for index lock Jun Wu
` (2 preceding siblings ...)
2026-10-13 22:05 ` [PATCH 0/2] Add a way to wait for index lock Junio C Hamano
@ 2026-10-14 10:14 ` Jeff King
3 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2026-10-14 10:14 UTC (permalink / raw)
To: Jun Wu; +Cc: git, gitster, ps, Jun Wu
On Tue, Oct 13, 2026 at 10:23:13AM -0700, Jun Wu wrote:
> 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.
The idea of --no-optional-locks is that the user eventually does actually
run some command in the CLI that updates the index (or even via the IDE
if it's not git-status). But yeah, that isn't necessarily guaranteed.
It's possible that an IDE caller could be smarter about _sometimes_
running a normal git-status, if it's been a while. But then of course it
has to worry about what "a while" means. ;)
So I think looking at lock timeouts and retries on the other side is a
reasonable approach.
> 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.
I think any problem exacerbated by the timeout was already a possible
race even without it. But it would be increasing the race window (which
is fine for racing benign commands like "git status", but probably bad
for racing "git add"; see my response to patch 2).
I think the git-commit race, at least, has probably been there for 20+
years. It's just that nobody has really seen it in practice because
making commits tends to be a serial-ish process (as opposed to things
like receiving pushes, where big sites hit every kind of race). But it
does seem reasonable to close those races, at which point the timeout
behavior seems like a no-brainer.
-Peff
^ permalink raw reply [flat|nested] 10+ messages in thread