Re: [PATCH v2 7/8] refs: fix recursing `get_main_ref_store()` with "onbranch" config

[Justin Tobler <jltobler@gmail.com>]

On 26/06/15 03:56PM, Patrick Steinhardt wrote:
[snip]
> diff --git a/refs.c b/refs.c
> index d3caa9a633..e69b9b8ac8 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2351,15 +2351,31 @@ void ref_store_release(struct ref_store *ref_store)
>  
>  struct ref_store *get_main_ref_store(struct repository *r)
>  {
> +	enum ref_storage_format format;
> +
>  	if (r->refs_private)
>  		return r->refs_private;
>  
>  	if (!r->gitdir)
>  		BUG("attempting to get main_ref_store outside of repository");
>  
> -	r->refs_private = ref_store_init(r, r->ref_storage_format,
> -					 r->gitdir, REF_STORE_ALL_CAPS);
> +	/*
> +	 * When constructing the reference backend we'll end up reading the Git
> +	 * configuration. This means we'll also try to evaluate "onbranch"
> +	 * conditions.
> +	 *
> +	 * We cannot read branches when constructing the refdb, so it is not
> +	 * possible to evaluate those conditions in the first place. To gate
> +	 * their evaluation we check whether or not the reference storage
> +	 * format has been configured -- we thus have to temporarily set it to
> +	 * UNKNOWN here so that we don't end up recursing.
> +	 */
> +	format = r->ref_storage_format;
> +	r->ref_storage_format = REF_STORAGE_FORMAT_UNKNOWN;

Is this really the best signal to indicate that a repository ref store
has not been initialized? Temporarily setting the storage format to
REF_STORAGE_FORMAT_UNKNOWN feels rather awkward and suggests to me that
`include_by_branch()` probably shouldn't be using it to begin with if
its not reliable.

-Justin