On 17/07/2024 07:42, Jeff King wrote: > On Tue, Jul 16, 2024 at 08:45:48PM +0100, Ramsay Jones wrote: > >> 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. Heh, a very good shot in the dark! ;) > 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. The patch given below fixes the test for me! (I have only run t1460-refs-migrate.sh, since the full test-suite takes 6 hours to run, but now all 30 tests pass!) I also don't know the code well enough to answer your question regarding the re-opening of the migrated ref-store, but it doesn't look like it would cause any problems (famous last words). Thanks! ATB, Ramsay Jones > --- > 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); > >
