Re: v2.46.0-rc0 test failures on cygwin
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jul 23, 2024, 10:30 UTC
- Message-ID
- <Zp-GQ0MQmuIyAear@tanuki>
- In-Reply-To
- <20240718005723.GA675057@coredump.intra.peff.net>
On Wed, Jul 17, 2024 at 08:57:23PM -0400, Jeff King wrote:
Show 13 quoted lines
> 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!
Show 21 quoted lines
> 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;Show 7 quoted lines
> 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