Re: [PATCH v7 29/33] setup: configure ref storage on setup

[Jeff King <peff@peff.net>]

On Mon, Feb 29, 2016 at 07:53:02PM -0500, David Turner wrote:

> diff --git a/setup.c b/setup.c
> index bd3a2cf..e2e1220 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -457,6 +457,10 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
>  		ret = -1;
>  	}
>  
> +	register_ref_storage_backends();
> +	if (set_ref_storage_backend(ref_storage_backend))
> +		die(_("Unknown ref storage backend %s"), ref_storage_backend);
> +
>  	strbuf_release(&sb);
>  	return ret;
>  }

Much nicer than the one it replaces, I think.

This whole block should probably go inside

  if (ret == 0) {
     ...
  }

If we are doing setup_git_repository_gently() and we do _not_ find a
valid repository, we would not want to enable the ref storage.

I also wondered what happens to ref_storage_backend in a case where we
call check_repository_format_gently() multiple times. I think we don't
actually call it at all for submodules, so we at least we know we are
always dealing with the main repository.

But what if we call check_repository_format_gently(), find an
extensions.refstorage config key, set the global, but then _don't_
actually accept the repo (e.g., because its version is too high). Then
we keep looking and find another repo, which does not have
extensions.refstorage set. But we use the stale value from the first
directory.

I admit this is a pretty unlikely scenario. But I think it does point to
a mis-design in the way we read extensions in the config callback. They
should not go into globals until we're sure we're accepting this config
as the actual repository.  So the existing "preciousobjects" extension
has this problem, too.

-Peff