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

From: Patrick Steinhardt <ps@pks.im>
Date: May 11, 2026, 10:01 UTC
Message-ID: <agGo9Prt8Hs2gbic@pks.im>
In-Reply-To: <91335804a092b09757331cac72092a3835020b3a@thalheim.io>

On Mon, May 11, 2026 at 09:06:00AM +0000, Jörg Thalheim wrote:

Show 42 quoted lines

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

Thanks!

Patrick