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

From: Jeff King <peff@peff.net>
Date: Mar 1, 2016, 08:48 UTC
Message-ID: <20160301084830.GB4952@sigill.intra.peff.net>
In-Reply-To: <1456793586-22082-30-git-send-email-dturner@twopensource.com>

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

Show 15 quoted lines

> 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