Re: [PATCH] refs: fix format migration on Cygwin

> It was reported that t1460-refs-migrate.sh fails when using Cygwin with
> errors like the following:
> 
>     error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied
> 
> As some debugging surfaced, the root cause of this is that some files of
> the newly-initialized ref store are still open when the target format is
> the "reftable" format, and Cygwin refuses to rename open files.
> 
> Fix this issue by closing the new ref store before renaming its files
> into place. This is a slight change in behaviour compared to before,
> where we kept the new ref store open and then updated the repository's
> ref store to point to it.
> 
> While we could re-open the new ref store after we have moved files
> around, this is ultimately unnecessary. We know that the only user of
> `repo_migrate_ref_storage_format()` is the git-refs(1) command, and it
> won't access the ref store after it has been migrated anyway. So
> reinitializing the ref store would be a waste of time. Regardless of
> that it is still sensible to leave the repository in a consistent state.
> But instead of reinitializing the ref store, we can simply unset the
> repo's ref store altogether and let `get_main_ref_store()` lazily
> initialize the new ref store as required.
> 
> Reported-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

>  refs.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index bb90a18875..915aeb4d1d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2843,6 +2843,14 @@ int repo_migrate_ref_storage_format(struct repository *repo,
>  		goto done;
>  	}
>  
> +	/*
> +	 * Release the new ref store such that any potentially-open files will
> +	 * be closed. This is required for platforms like Cygwin, where
> +	 * renaming an open file results in EPERM.
> +	 */
> +	ref_store_release(new_refs);
> +	FREE_AND_NULL(new_refs);
> +
>  	/*
>  	 * Until now we were in the non-destructive phase, where we only
>  	 * populated the new ref store. From hereon though we are about
> @@ -2874,10 +2882,14 @@ int repo_migrate_ref_storage_format(struct repository *repo,
>  	 */
>  	initialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1);
>  
> -	free(new_refs->gitdir);
> -	new_refs->gitdir = xstrdup(old_refs->gitdir);
> -	repo->refs_private = new_refs;
> +	/*
> +	 * Unset the old ref store and release it. `get_main_ref_store()` will
> +	 * make sure to lazily re-initialize the repository's ref store with
> +	 * the new format.
> +	 */
>  	ref_store_release(old_refs);
> +	FREE_AND_NULL(old_refs);
> +	repo->refs_private = NULL;
>  
>  	ret = 0;
>  
> @@ -2888,8 +2900,10 @@ int repo_migrate_ref_storage_format(struct repository *repo,
>  			    new_gitdir.buf);
>  	}
>  
> -	if (ret && new_refs)
> +	if (new_refs) {
>  		ref_store_release(new_refs);
> +		free(new_refs);
> +	}
>  	ref_transaction_free(transaction);
>  	strbuf_release(&new_gitdir);
>  	return ret;