* Re: [PATCH] cache-tree: do not die when the index is locked
2026-10-15 21:42 [PATCH] cache-tree: do not die when the index is locked Szymon Wilczek
@ 2026-10-16 5:56 ` Patrick Steinhardt
2026-10-16 9:16 ` [PATCH v2] " Szymon Wilczek
1 sibling, 0 replies; 3+ messages in thread
From: Patrick Steinhardt @ 2026-10-16 5:56 UTC (permalink / raw)
To: Szymon Wilczek; +Cc: git, Junio C Hamano, Phillip Wood
On Thu, Oct 15, 2026 at 11:42:42PM +0200, Szymon Wilczek wrote:
> `write_index_as_tree()` locks the index with `LOCK_DIE_ON_ERROR`, even
> though we only need the lock for the optional write-back of the updated
> cache-tree. As the comment next to `write_locked_index()` already notes,
> failing to write the index here is fine.
Giving a bit of context here would help the reader to figure out what
the function even does. Namely, it's two different things:
- It is responsible for turning the index into a tree object.
- And when that succeeds, it will also rewrite the index to populate
the cache tree extension.
The first part is what must succeed, but the second part is allowed to
fail without impacting the result.
> Since 356ee4659b (sequencer: try to commit without forking 'git commit',
> 2017-11-24), `try_to_commit()` calls `write_index_as_tree()` in-process
> right after the pick has updated the index and written
> `CHERRY_PICK_HEAD`. Before that commit the `die()` happened in a `git
> commit` child process, and the sequencer stopped cleanly with its state
> saved. Now, if a background `git status` holds `index.lock` at that
> moment, we die before writing `rebase-merge/message`, and a subsequent
> `git rebase --continue` refuses to run:
>
> error: you have staged changes in your working tree
This is the symptom that you're trying to fix and should come first in
your message. Commit messages typically describe the problem first
(this), then explain cause of and context around that problem (your
above information about `write_index_as_tree()`) and then finally how
you solve the problem (the next paragraph).
> diff --git a/cache-tree.c b/cache-tree.c
> index b8cbb5da22..1bea880912 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -809,11 +809,15 @@ struct tree *write_in_core_index_as_tree(struct repository *repo,
>
> int write_index_as_tree(struct object_id *oid, struct index_state *index_state, const char *index_path, int flags, const char *prefix)
> {
> - int entries, was_valid;
> + int entries, was_valid, fd;
> struct lock_file lock_file = LOCK_INIT;
> int ret;
>
> - hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
> + /*
> + * The lock only protects the cache-tree write-back below; if somebody
> + * else holds it, compute the tree without updating the index.
> + */
> + fd = hold_lock_file_for_update(&lock_file, index_path, 0);
An alternative would be to supply a locking timeout, but given that
writing the cache tree extension is entirely optional I agree that this
here is the better fix.
> @@ -828,7 +832,7 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state,
>
> ret = write_index_as_tree_internal(oid, index_state, was_valid, flags,
> prefix);
> - if (!ret && !was_valid) {
> + if (!ret && !was_valid && 0 <= fd) {
Nit: This may just be me, but this reverse notation always confuses me,
and I think writing this as `fd > 0` feels more natural to many. Anyway,
I don't feel strongly about this as this may very well be subjective.
> write_locked_index(index_state, &lock_file, COMMIT_LOCK);
> /* Not being able to write is fine -- we are only interested
> * in updating the cache-tree part, and if the next caller
And here we explain that it's fine if we fail writing the cache tree
extension. Makes sense.
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 0964718d7f..4b7547e108 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -286,4 +286,17 @@ test_expect_success 'cache-tree is used by write-tree when valid' '
> test_grep ! region_enter.*cache_tree.*update trace.output
> '
>
> +test_expect_success 'write-tree succeeds while the index is locked' '
> + test_when_finished "rm -f .git/index.lock" &&
> + test_commit locked &&
> + >uncommitted &&
> + git add uncommitted &&
> + >.git/index.lock &&
> + git write-tree >actual &&
> + test_path_is_file .git/index.lock &&
> + rm .git/index.lock &&
> + git write-tree >expect &&
> + test_cmp expect actual
> +'
The test looks obviously good to me. Nice that the issue is this easy to
reproduce.
Thanks!
Patrick
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH v2] cache-tree: do not die when the index is locked
2026-10-15 21:42 [PATCH] cache-tree: do not die when the index is locked Szymon Wilczek
2026-10-16 5:56 ` Patrick Steinhardt
@ 2026-10-16 9:16 ` Szymon Wilczek
1 sibling, 0 replies; 3+ messages in thread
From: Szymon Wilczek @ 2026-10-16 9:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Patrick Steinhardt, Szymon Wilczek
When a background process such as `git status` holds `index.lock` while
`git rebase` picks a commit, the rebase can die in the middle of the
pick and leave the sequencer in a state from which `git rebase
--continue` refuses to run:
error: you have staged changes in your working tree
This happens because `try_to_commit()` calls `write_index_as_tree()`
right after the pick has updated the index and written
`CHERRY_PICK_HEAD`, and `write_index_as_tree()` dies when it cannot
take the index lock. We then die before writing `rebase-merge/message`.
Since 356ee4659b (sequencer: try to commit without forking 'git commit',
2017-11-24), this call happens in-process. Before that commit the
`die()` happened in a `git commit` child process, and the sequencer
stopped cleanly with its state saved.
`write_index_as_tree()` does two different things:
- It turns the index into a tree object. This part must succeed.
- When that succeeds, it also rewrites the index to populate the
cache-tree extension. This part is allowed to fail without
impacting the result; as the comment next to
`write_locked_index()` already notes, failing to write the index
here is fine.
Yet the function locks the index with `LOCK_DIE_ON_ERROR`, even though
we only need the lock for the optional second part.
Drop `LOCK_DIE_ON_ERROR` so that we take the lock gently, just like
other callers that only refresh the index, and skip writing the index
when the lock is held by someone else. This also lets `git write-tree`
and `git am` proceed while another process holds the lock.
Signed-off-by: Szymon Wilczek <swilczek.lx@gmail.com>
---
Changes in v2:
- Reordered the commit message to describe the symptom first, then
its cause, and the fix last, and explained the two things that
`write_index_as_tree()` does (thanks, Patrick).
- Spelled the lock check as `fd >= 0` instead of `0 <= fd`.
Notes from v1:
I ran into this while rebasing a ~120-commit branch with Magit open,
which runs `git status` in the background. To recover from the
half-picked state, I had to commit the pick manually with
`git commit -C CHERRY_PICK_HEAD` before running `git rebase --continue`.
This can be reproduced within a few tries by rebasing a long branch
while running `git status --porcelain` in a loop, or triggered every
time in gdb:
break try_to_commit
run
shell touch .git/index.lock
continue
If the lock is taken slightly earlier, during `do_recursive_merge()`,
the pick still fails, but that path reports an error and reschedules the
pick, so `git rebase --continue` works there.
cache-tree.c | 10 +++++++---
t/t0090-cache-tree.sh | 13 +++++++++++++
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/cache-tree.c b/cache-tree.c
index b8cbb5da22..671d4165bf 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -809,11 +809,15 @@ struct tree *write_in_core_index_as_tree(struct repository *repo,
int write_index_as_tree(struct object_id *oid, struct index_state *index_state, const char *index_path, int flags, const char *prefix)
{
- int entries, was_valid;
+ int entries, was_valid, fd;
struct lock_file lock_file = LOCK_INIT;
int ret;
- hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
+ /*
+ * The lock only protects the cache-tree write-back below; if somebody
+ * else holds it, compute the tree without updating the index.
+ */
+ fd = hold_lock_file_for_update(&lock_file, index_path, 0);
entries = read_index_from(index_state, index_path,
repo_get_git_dir(index_state->repo));
@@ -828,7 +832,7 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state,
ret = write_index_as_tree_internal(oid, index_state, was_valid, flags,
prefix);
- if (!ret && !was_valid) {
+ if (!ret && !was_valid && fd >= 0) {
write_locked_index(index_state, &lock_file, COMMIT_LOCK);
/* Not being able to write is fine -- we are only interested
* in updating the cache-tree part, and if the next caller
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 0964718d7f..4b7547e108 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -286,4 +286,17 @@ test_expect_success 'cache-tree is used by write-tree when valid' '
test_grep ! region_enter.*cache_tree.*update trace.output
'
+test_expect_success 'write-tree succeeds while the index is locked' '
+ test_when_finished "rm -f .git/index.lock" &&
+ test_commit locked &&
+ >uncommitted &&
+ git add uncommitted &&
+ >.git/index.lock &&
+ git write-tree >actual &&
+ test_path_is_file .git/index.lock &&
+ rm .git/index.lock &&
+ git write-tree >expect &&
+ test_cmp expect actual
+'
+
test_done
Range-diff against v1:
1: d16173cf1c ! 1: cdfdd81c3f cache-tree: do not die when the index is locked
@@ Metadata
## Commit message ##
cache-tree: do not die when the index is locked
- `write_index_as_tree()` locks the index with `LOCK_DIE_ON_ERROR`, even
- though we only need the lock for the optional write-back of the updated
- cache-tree. As the comment next to `write_locked_index()` already notes,
- failing to write the index here is fine.
+ When a background process such as `git status` holds `index.lock` while
+ `git rebase` picks a commit, the rebase can die in the middle of the
+ pick and leave the sequencer in a state from which `git rebase
+ --continue` refuses to run:
- Since 356ee4659b (sequencer: try to commit without forking 'git commit',
- 2017-11-24), `try_to_commit()` calls `write_index_as_tree()` in-process
+ error: you have staged changes in your working tree
+
+ This happens because `try_to_commit()` calls `write_index_as_tree()`
right after the pick has updated the index and written
- `CHERRY_PICK_HEAD`. Before that commit the `die()` happened in a `git
- commit` child process, and the sequencer stopped cleanly with its state
- saved. Now, if a background `git status` holds `index.lock` at that
- moment, we die before writing `rebase-merge/message`, and a subsequent
- `git rebase --continue` refuses to run:
+ `CHERRY_PICK_HEAD`, and `write_index_as_tree()` dies when it cannot
+ take the index lock. We then die before writing `rebase-merge/message`.
+ Since 356ee4659b (sequencer: try to commit without forking 'git commit',
+ 2017-11-24), this call happens in-process. Before that commit the
+ `die()` happened in a `git commit` child process, and the sequencer
+ stopped cleanly with its state saved.
- error: you have staged changes in your working tree
+ `write_index_as_tree()` does two different things:
+
+ - It turns the index into a tree object. This part must succeed.
+
+ - When that succeeds, it also rewrites the index to populate the
+ cache-tree extension. This part is allowed to fail without
+ impacting the result; as the comment next to
+ `write_locked_index()` already notes, failing to write the index
+ here is fine.
+
+ Yet the function locks the index with `LOCK_DIE_ON_ERROR`, even though
+ we only need the lock for the optional second part.
Drop `LOCK_DIE_ON_ERROR` so that we take the lock gently, just like
other callers that only refresh the index, and skip writing the index
@@ cache-tree.c: int write_index_as_tree(struct object_id *oid, struct index_state
ret = write_index_as_tree_internal(oid, index_state, was_valid, flags,
prefix);
- if (!ret && !was_valid) {
-+ if (!ret && !was_valid && 0 <= fd) {
++ if (!ret && !was_valid && fd >= 0) {
write_locked_index(index_state, &lock_file, COMMIT_LOCK);
/* Not being able to write is fine -- we are only interested
* in updating the cache-tree part, and if the next caller
base-commit: f57c9a73884400380bddd172acc88b569834f909
--
2.56.0
^ permalink raw reply related [flat|nested] 3+ messages in thread