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

[Junio C Hamano <gitster@pobox.com>]

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.

Thanks.