Re: [PATCH] cache-tree: do not die when the index is locked

[Patrick Steinhardt <ps@pks.im>]

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