Re: [PATCH v2 3/8] setup: don't apply "GIT_REFERENCE_BACKEND" without a repository

[Justin Tobler <jltobler@gmail.com>]

On 26/06/15 03:56PM, Patrick Steinhardt wrote:
> When discovering a repository we eventually also apply the
> "GIT_REFERENCE_BACKEND" environment variable to the repository. There's
> two problems with that:
> 
>   - We do this unconditionally, which is rather pointless: we really
>     only have to configure the repository when we have found one.

I agree that configuring the repository reference format when there
isn't a repository to begin doesn't sound very useful.

>   - We have already applied the repository format at that point in time,
>     so we need to manually reapply it.
> 
> Move the logic around so that we only apply the environment variable
> when a repository was discovered. This also allows us to drop the
> explcit call to `repo_set_ref_storage_format()` because we now adjust
> the format before we apply it via `apply_repository_format()`.

Make sense.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> @@ -2023,6 +2022,8 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
>  	    startup_info->have_repository ||
>  	    /* GIT_DIR_EXPLICIT */
>  	    getenv(GIT_DIR_ENVIRONMENT)) {
> +		const char *ref_backend_uri;
> +
>  		if (!repo->gitdir) {
>  			const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
>  			if (!gitdir)
> @@ -2030,6 +2031,24 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
>  			setup_git_env_internal(repo, gitdir);
>  		}
>  
> +		/*
> +		 * The env variable should override the repository config
> +		 * for 'extensions.refStorage'.
> +		 */
> +		ref_backend_uri = getenv(GIT_REFERENCE_BACKEND_ENVIRONMENT);
> +		if (ref_backend_uri) {
> +			char *format;
> +
> +			free(repo_fmt.ref_storage_payload);
> +
> +			parse_reference_uri(ref_backend_uri, &format, &repo_fmt.ref_storage_payload);
> +			repo_fmt.ref_storage_format = ref_storage_format_by_name(format);
> +			if (repo_fmt.ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN)
> +				die(_("unknown ref storage format: '%s'"), format);
> +
> +			free(format);
> +		}
> +
>  		if (startup_info->have_repository) {
>  			struct strbuf err = STRBUF_INIT;

Hmmm, we only invoke `apply_repository_format()` if we indeed have a
repository (having just GIT_DIR_ENVIRONMENT set isn't enough). Should we
instead nest this logic right above `apply_repository_format()` in the
same block?

-Justin