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
