From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 18 May 2026 08:07:08 GMT
Subject: Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout
Message-ID: <agrIrGwSMFlKTx9x@pks.im>
In-Reply-To: <xmqqzf1xbl4i.fsf@gitster.g>

On Mon, May 18, 2026 at 09:46:05AM +0900, Junio C Hamano wrote:
> Joerg Thalheim <joerg@thalheim.io> writes:
> 
> > +/*
> > + * How long to retry acquiring config.lock when another process holds
> > + * it. Default matches core.packedRefsTimeout; override via
> > + * core.configLockTimeout.
> > + */
> > +static long config_lock_timeout_ms(struct repository *r)
> > +{
> > +	static int configured;
> > +	static int timeout_ms = 1000;
> > +
> > +	if (!configured) {
> > +		repo_config_get_int(r, "core.configlocktimeout", &timeout_ms);
> > +		configured = 1;
> > +	}
> > +
> > +	return timeout_ms;
> > +}
> 
> The above design means whichever repository happens to be passed for
> the first time as "r" to this call will fix the return value from
> the function for the rest of the system, meaning that the lock timeout
> is a per-process property and the repository parameter passed to the
> function does not matter all that much.
> 
> It may make sense to admit that this is not a per-repository
> property (due to the use of local caching), have the function take
> no parameter and use the_repository to the config_get call.  That
> would make the intention more clear.
> 
> Of course the other end of the spectrum is to get rid of the
> "configured" caching here, and ask the config system to make a
> hashtable look-up every time the function is called.  That will keep
> the lock timeout per-repository, which is closer to what the current
> function signature suggests.
> 
> I dunno.  My gut feeling is that there aren't valid reasons why you
> would want to specifically set different timeout values per
> repository, so the simplicity of using the_repository (i.e. the
> primary repository instance this process deals with) sounds like a
> better way to go.

There probably is no reason to have different values per repo. But to
me the question is whether there even are any use cases where we have to
lock the config file so often in quick succession that the caching
mechanism even matters. My gut feeling says no, also because parsing the
value from the configuration is going to be drowned out by actually
writing the lockfile and renaming it into place.

So I'd rather lean towards dropping the cache and keeping the repository
parameter.

Patrick