Re: v2.46.0-rc0 test failures on cygwin

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 5111 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]