Re: [PATCH] config: retry acquiring config.lock for 100ms

> On Mon, May 11, 2026 at 09:06:00AM +0000, Jörg Thalheim wrote:
> > May 11, 2026 at 4:32 AM, "Junio C Hamano" <gitster@pobox.com
> > mailto:gitster@pobox.com?to=%22Junio%20C%20Hamano%22%20%3Cgitster%40pobox.com%3E
> > > wrote:
> > > Patrick Steinhardt <ps@pks.im> writes:
> > > > > This bites in practice when running `git worktree add -b` concurrently
> > > > >  against the same repository. Each invocation makes several writes to
> > > > >  ".git/config" to set up branch tracking, and tooling that creates
> > > > >  worktrees in parallel sees intermittent failures. Worse, `git worktree
> > > > >  add` does not propagate the failed config write to its exit code: the
> > > > >  worktree is created and the command exits 0, but tracking
> > > > >  configuration is silently dropped.
> > > > > 
> > > >  This very much sounds like a bug that is worth fixing independently.
> > > > 
> > > > > 
> > > > > The lock is held only for the duration of rewriting a small file, so
> > > > >  retrying for 100 ms papers over any realistic contention while still
> > > > >  failing fast if a stale lock has been left behind by a crashed
> > > > >  process. This mirrors what we already do for individual reference
> > > > >  locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,
> > > > >  2017-08-21)).
> > > > > 
> > > >  Famous last words :) Experience tells me that any timeout value that
> > > >  isn't excessive will eventually be hit in some production system. Which
> > > >  raises the question whether we want to make the timeout configurable,
> > > >  similar to "core.filesRefLockTimeout" and "core.packedRefsTimeout".
> > > >  ...
> > > >  Honestly though, I'm not really sure what to make with this. We could
> > > >  of course also add some validation that the configuration we want to set
> > > >  hasn't been modified meanwhile. But that would now lead to a situation
> > > >  where we have to update every single caller in our tree to make use of
> > > >  the new mechanism, which would be a bunch of work.
> > > > 
> > > >  And adding the timeout doesn't really change the status quo, either. We
> > > >  already have the case that we'll happily overwrite changes made by
> > > >  concurrent processes. The only thing that changes is that we make it
> > > >  more likely for concurrent changes to succeed.
> > > > 
> > > We haven't heard any response to these points raised in the message
> > > I am responding to. Should I still keep the patch in my tree,
> > > hoping that a responses may come some day? I am tempted to discard
> > > the topic as it has been quite a while since we last looked at it.
> > 
> > I am not really sure what you want me to do here.
> 
> In general, the idea here is to engage in a discussion that can
> ultimately lead to one of two outcomes:
> 
>   - The discussion surfaces an area the author hasn't thought about, so
>     the patch is adapted accordingly.
> 
>   - The discussion shows that the author already did think about the
>     issue, but hasn't documented the assumptions. In this case, it
>     should be the commit message that gets adapted.

> > I don't see how git can have this value configurable, given it's about
> > reading the configuration itself. Is the user supposed via command
> > line?
> 
> This is a fair point indeed. But if it's not possible to change via the
> configuration itself, then the next-best thing might be to introduce an
> environment variable that allows configuring it.

> The other aspect that wasn't discussed in the commit message is how
> concurrent writes are handled, both when they are non-conflicting
> (updating different keys) and when they are conflicting (updating the
> same key). After spending some more time in the code I think it's
> ultimately nothing we have to worry about too much, as we only start
> reading the configuration after we've locked it.

> So in the semantically non-conflicting case there isn't really much of a
> race, because things already work as expected. But in the semantically
> conflicting case it's a bit different, as the latter writer will
> overwrite the result of the former one. In theory it would be possible
> to detect such conflicts by:
> 
>   - Reading the configuration file.
> 
>   - Taking the lock.
> 
>   - Rereading the configuration to check for conflicts.
> 
> But even that is racy as the first writer might have succeeded before we
> read the configuration the first time. So I'm not sure whether we can do
> anything about that in the first place, as the race basically exists in
> the outer loop controlled by the caller.
> 
> So there probably isn't much we can do about that, and unless I missed
> something I think your timeout is sensible. But ideally, such nuances
> would be discussed as part of the commit message so that reviewers and
> future readers are made aware of them.