Re: v2.46.0-rc0 test failures on cygwin

From: Jeff King <peff@peff.net>
Date: Jul 17, 2024, 06:42 UTC
Message-ID: <20240717064241.GF547635@coredump.intra.peff.net>
In-Reply-To: <aacb4067-a16b-4953-a72f-3c66efd3cf25@ramsayjones.plus.com>

On Tue, Jul 16, 2024 at 08:45:48PM +0100, Ramsay Jones wrote:

Show 9 quoted lines

>   error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied
>   migrated refs can be found at '.git/ref_migration.sr9pEF'
> [...]
> Now try to finish the migration by hand:
> 
>   $ mv .git/ref_migration.sr9pEF/reftable .git/reftable
> 
> Hmm, note no error; of course, the mv command may well do much more than
> the rename() library function, so they are not necessarily equivalent.

This is a shot in the dark, but: could the problem be an open file that cannot be moved? If I run a "ref migrate" on my Linux system in the debugger and stop at move_files(), checking /proc/<pid>/fd shows an open descriptor for .git/ref_migration.WnJ8TS/reftable/tables.list.

Does the patch below fix things for you? I'm not too familiar with the code, so this is what I cobbled together. The best response will be from Patrick, but I think he's offline for another week or so. In the meantime, this at least doesn't crash for me. ;) And I confirmed that the tables.list file is closed during the move_files() call.

-Peff

---

diff --git a/refs.c b/refs.c
index bb90a18875..06a0fc5099 100644
--- a/refs.c
+++ b/refs.c
@@ -2843,6 +2843,12 @@ int repo_migrate_ref_storage_format(struct repository *repo,
 		goto done;
 	}
 
+	/*
+	 * Close the new ref store to avoid holding on to any open files
+	 * which could interfere with moving things behind the scenes.
+	 */
+	ref_store_release(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,8 +2880,13 @@ 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);
+	/*
+	 * Re-open the now-migrated ref store. I'm not sure if this is strictly
+	 * needed or not. Perhaps it would also be a good time to check that
+	 * we correctly opened it, it's in the expected format, etc?
+	 */
+	new_refs = ref_store_init(repo, format, old_refs->gitdir,
+				  REF_STORE_ALL_CAPS);
 	repo->refs_private = new_refs;
 	ref_store_release(old_refs);