On 23/07/2024 13:31, Patrick Steinhardt wrote:
> 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@xxxxxxxxxxxxxxxxxxxx>
> Helped-by: Jeff King <peff@xxxxxxxx>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
I applied this patch on top of v2.46.0-rc1, on both Linux and cygwin, and
ran the tests (just t1460-*.sh on cygwin, complete test-suite in Linux).
I can confirm all 30 tests pass on cygwin! :)
Thanks all.
ATB,
Ramsay Jones
> 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;
