On Wed, Jul 17, 2024 at 08:57:23PM -0400, Jeff King wrote: > On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote: > > 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 for testing. This is new in the upcoming release, but I think > it's localized to the "git ref migrate" command. So aside from the > annoyance of the test failure for you, it is not too urgent. I'm tempted > to put it off until Patrick has had a chance to weigh in, even if it > means missing the v2.46 cutoff. Thanks all for digging into this while I was out! > I'd also be OK with pursuing it in the meantime if folks feel > differently. Having slept on it, I think the answer to one of my > questions here... > > > > - 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); > > ...is that we must put _something_ useful into repo->refs_private, > because old_refs is an alias for it that we are freeing. I suspect that > "git ref migrate" does not really look at the repo any more after this > migration function returns, but it makes sense for it to leave things in > a consistent state. Yeah, `repo->refs_private` shouldn't ever be accessed after the migration has finished. Still, as you say, it feels like the correct thing to do to keep the `repo` in a consistent state, even though it's not necessary in our codebase right now. > So my biggest question is just whether there is any downside to doing > the release/init pair rather than trying to reuse the existing struct. There shouldn't be any downside, but it is wasted effort. The main ref store should always be accessed via `get_main_ref_store()`, and that function knows to lazily initialize the refdb as required. So instead, I think the preferable fix is to release the new ref store after we have populated it and set the main ref store of the repository to `NULL` instead of re-initializing it. So the following should be sufficient: 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; > I do think it probably causes a small memory leak. The "init" function > allocates the actual ref_store struct, but the "release" function > doesn't seem to free it. So we are probably leaking the store that > points to the temp directory. But that is also true of "old_refs", or of > "new_refs" if we hit an error. So I think the solution is probably for > init/release to be symmetric, and for the latter to clean up everything. > But again, I'd prefer to get input from Patrick there. Yeah, we'd have to free the new ref store struct, as well. I wouldn't make `release()` free the structure as that would be uncustomarily named. Patrick
Attachment:
signature.asc
Description: PGP signature