Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout

> From: Jörg Thalheim <joerg@thalheim.io>
>
> Concurrent config writers race for the ".lock" file, which is taken
> with open(O_EXCL) and no retry, so the losers fail right away with
> "could not lock config file".
>
> This shows up with parallel "git worktree add -b" against the same
> repository: each one writes a couple of branch.* keys and the losers
> fail at random. Worse, "git worktree add" doesn't propagate that
> failure to its exit code, so the tracking config is silently dropped.
> (The swallowed error is a separate bug.)
>
> Retry instead of giving up on the first EEXIST. The lock is only held
> while rewriting a small file, so the loser only has to wait out the
> other writers. Same approach as 4ff0f01cb7 (refs: retry acquiring
> reference locks for 100ms, 2017-08-21).
>
> On the semantics: the on-disk config is read only after the lock is
> taken, so writers touching different keys can't lose each other's
> change. Writers touching the same key still get last-writer-wins, but
> that is already the case today and would need a compare-and-swap config
> API to fix. The retry only turns hard failures into successes.
>
> Default to 1000ms, like core.packedRefsTimeout: same shape of problem,
> one shared file everyone serializes through. A larger timeout only
> costs anything when a stale lock is left behind by a crash, which is
> rare; a smaller one fails spuriously on slow filesystems (NTFS has
> been seen needing more than 100ms). Make it configurable as
> core.configLockTimeout. There is no chicken-and-egg problem: we read
> the config before we lock it.
>
> microsoft/git carries a similar patch (core.configWriteLockTimeoutMS,
> default off) for Scalar's tests. Defaulting to non-zero here because
> the worktree case fails silently.
>
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Jörg Thalheim <joerg@thalheim.io>
> ---
> Thanks for the review and for poking me, this had fallen off my radar.
>
> v1 -> v2:
>
> - added core.configLockTimeout. Johannes is right that there is no
>   chicken-and-egg problem (config is read before the lock), so no env
>   var needed.
> - default bumped to 1000ms; packed-refs is the closer precedent and it
>   keeps NTFS out of trouble.
> - commit message now covers the read-after-lock / last-writer-wins
>   semantics Patrick asked about.
> - added tests; existing stale-lock tests in t3200/t5505 now pass
>   -c core.configLockTimeout=0 so they still fail fast.
>
> I matched the core.filesRefLockTimeout naming rather than reusing
> microsoft/git's core.configWriteLockTimeoutMS, but can switch if the
> downstream compat matters more.