On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote: > > 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! ;) Lucky guess. :) When Junio pointed out that we'd expect Windows to fail in that case, too, I thought for sure I was just wrong. So I'm glad it worked out. > 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. 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. 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. 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. -Peff
