Re: v2.46.0-rc0 test failures on cygwin

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:

quoted

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...

quoted

quoted

-	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