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

[Jun Wu <quark.zju@gmail.com>]

On Tue, 13 Oct 2026 15:05:52 -0700, Junio C Hamano wrote:
> > 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.

Yes. I'm aware of end-user behavior change. The test changes aren't strictly
necessary - without the changes tests still pass, although run a bit slower.

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

It seems the trade-off is between command success rate and command duration.

I suspect that end-users want commands to work (not fail) first, performance
comes next. In my experience, high command failure rate escalates to oncall
incidents, occasional slightly slower commands often get unnoticed unless we
look at the metrics explicitly.

Mercurial has been using 10-minute timeout by default. I picked 1 second as a
middle ground, limit the "slowness". I'm open to setting it to 10 minutes
(matches hg), or 0 (preserves the old behavior), or another value.

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

I think the lock infrastructure might want to force dropping outdated in-memory
index once lock gets acquired. The users of the lock APIs need to handle this
new cache invalidation situation. Locks in hg work this way.

How each command treats the "cache gets dropped by locking" can vary by case.
For example,
- status: read -> refresh -> lock(timeout=0) -> invalidated ? skip : write
- add -i: read -> lock(timeout>0) -> invalidated ? re-apply "add" : write

>  (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?

There are commands (e.g. rm --dry-run) that lock without writing, but these
might be for convenience, not consistency. This change will make them wait.

Since a single read of the index is already consistent (it is replaced
atomically), I assume by "view" you also mean the working tree files.
Then, there is a question about the intended scope of the index lock:
  a. only covers the index
  b. covers both the index and working copy files

The answer probably does not matter that much, since commands that write
working copy files likely also want to update stat, therefore take the index
lock. The only exception seems to be "clean", since index has no stat for
untracked files. Today, "clean" does not lock. So this change doesn't 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?

## Does initialized=1 mean reading from disk already?

Not always.

initialized=1 is set by either 'do_read_index' or 'unpack_trees'.
'do_read_index' is the regular path. 'unpack_trees' sets result->initialized=1
so it depends on the input of 'unpack_trees'.  Some callers (e.g. read-tree)
call unpack_trees when repo->index is empty, not initialized. So in this case,
initialized=1 does not mean "read from disk".

## Cases we created index from scratch, then call repo_hold_locked_index

I didn't find any.

There are commands that create index from scratch, intend to write: clone and
read-tree. But they all take the index lock (when initialized=0) before calling
'unpack_trees' (sets initialized=1).