Re: [PATCH v5 08/11] refs/files: lazy-load configuration to fix chicken-and-egg

[Justin Tobler <jltobler@gmail.com>]

On 26/06/22 10:28AM, Patrick Steinhardt wrote:
> When initializing the "files" reference backend we read the repository's
> config to parse "core.preferSymlinkRefs" and "core.logAllRefUpdates".
> This results in a chicken-and-egg problem though, because parsing the
> configuration may require us to have access to the reference store
> already when an "onbranch" condition exists.

Ok so both of these configuration options are currently parsed at ref
store initialization time. This is problematic because we need the ref
store to properly handle "onbranch" conditions in the config.

> Luckily, all the configuration that we honor only relates to writing
> references. Consequently, we don't strictly need that configuration to
> be readily available at initialization time, and we can easiliy defer
> parsing it to a later point in time.

That's nice. So we don't actually need this configuration during
initialization and can instead lazily load it when writing the first
references. Makes sense.

> Implement this fix and add tests that verify that we can indeed properly
> parse these config knobs via an "onbranch" condition.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/files-backend.c        | 37 ++++++++++++++++++++++++++-----------
>  t/t0600-reffiles-backend.sh | 21 +++++++++++++++++++++
>  2 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 79fb6735e1..d0f379dcd6 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -84,12 +84,14 @@ struct files_ref_store {
>  	unsigned int store_flags;
>  
>  	char *gitcommondir;
> -	enum log_refs_config log_all_ref_updates;
> -	int prefer_symlink_refs;
> -
>  	struct ref_cache *loose;
> -
>  	struct ref_store *packed_ref_store;
> +
> +	struct files_ref_store_write_options {
> +		enum log_refs_config log_all_ref_updates;
> +		int prefer_symlink_refs;
> +		bool initialized;
> +	} write_opts_lazy_loaded;

It might be nice to leave some sort of breadcrumb comment to future
readers to explain why we lazy load this configuration.

>  };
>  
>  static void clear_loose_ref_cache(struct files_ref_store *refs)
> @@ -121,17 +123,31 @@ static int files_ref_store_config(const char *var, const char *value,
>  				  const struct config_context *ctx UNUSED,
>  				  void *payload)
>  {
> -	struct files_ref_store *refs = payload;
> +	struct files_ref_store_write_options *opts = payload;
>  
>  	if (!strcmp(var, "core.prefersymlinkrefs")) {
> -		refs->prefer_symlink_refs = git_config_bool(var, value);
> +		opts->prefer_symlink_refs = git_config_bool(var, value);
>  	} else if (!strcmp(var, "core.logallrefupdates")) {
> -		refs->log_all_ref_updates = refs_parse_log_all_ref_updates_config(value);
> +		opts->log_all_ref_updates = refs_parse_log_all_ref_updates_config(value);
>  	}
>  
>  	return 0;
>  }
>  
> +static const struct files_ref_store_write_options *files_ref_store_write_options(struct files_ref_store *refs)
> +{
> +	struct files_ref_store_write_options *opts = &refs->write_opts_lazy_loaded;
> +
> +	if (opts->initialized)
> +		return opts;
> +
> +	opts->log_all_ref_updates = LOG_REFS_UNSET;
> +	repo_config(refs->base.repo, files_ref_store_config, opts);
> +
> +	opts->initialized = true;
> +	return opts;
> +}
> +
>  /*
>   * Create a new submodule ref cache and add it to the internal
>   * set of caches.
> @@ -156,9 +172,7 @@ static struct ref_store *files_ref_store_init(struct repository *repo,
>  	refs->packed_ref_store =
>  		packed_ref_store_init(repo, NULL, refs->gitcommondir, opts);
>  	refs->store_flags = opts->access_flags;
> -	refs->log_all_ref_updates = LOG_REFS_UNSET;
>  
> -	repo_config(repo, files_ref_store_config, refs);

Configs are no longer read eagerly during initialization.

The rest of this patch looks good to me.

-Justin