Re: [PATCH 1/7] refs/packed: de-globalize handling of "core.packedRefsTimeout"

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

Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 499cb55dfa..5c49c06493 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -162,6 +162,13 @@ struct packed_ref_store {
>  	 * `packed_ref_store`) must not be freed.
>  	 */
>  	struct tempfile *tempfile;
> +
> +	/*
> +	 * Timeout when taking the "packed-refs.lock" file. configurable via
> +	 * "core.packedRefsTimeout".
> +	 */
> +	bool timeout_configured;
> +	int timeout_value;
>  };
>  
>  /*
> @@ -1233,12 +1240,10 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
>  	struct packed_ref_store *refs =
>  		packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
>  				"packed_refs_lock");
> -	static int timeout_configured = 0;
> -	static int timeout_value = 1000;
>  
> -	if (!timeout_configured) {
> -		repo_config_get_int(the_repository, "core.packedrefstimeout", &timeout_value);
> -		timeout_configured = 1;

In the original code, when core.packedrefstimeout is not configured,
our call to repo_config_get_int() does not touch timeout_value.  As
a result, we get the static 1000 and flip the "configured" flag to
prevent this _value from further getting updated.

> +	if (!refs->timeout_configured) {
> +		repo_config_get_int(ref_store->repo, "core.packedrefstimeout", &refs->timeout_value);
> +		refs->timeout_configured = true;

But what happens in the new code when core.packedrefstimeout is not
configured?  It is up to whoever initialised refs->timeout_value.

If I am not mistaken, packed_ref_store_init() does xcalloc(), lets
base_ref_store_init() initialise some members, initialises a few
members itself (such as .store_flags and .path), and leaves other
members, including .timeout_configured and .timeout_value,
NUL-filled.  .timeout_configured starting as false is perfectly
fine, but shouldn't we initialise .timeout_value to 1000 as before?

Thanks.

> @@ -1249,7 +1254,7 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
>  	if (hold_lock_file_for_update_timeout(
>  			    &refs->lock,
>  			    refs->path,
> -			    flags, timeout_value) < 0) {
> +			    flags, refs->timeout_value) < 0) {
>  		unable_to_lock_message(refs->path, errno, err);
>  		return -1;
>  	}