Re: v2.46.0-rc0 test failures on cygwin

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Jul 17, 2024, 18:05 UTC
Message-ID: <3e6abe6c-2c15-47f9-89e8-3e8710802562@ramsayjones.plus.com>
In-Reply-To: <20240717064241.GF547635@coredump.intra.peff.net>

On 17/07/2024 07:42, Jeff King wrote:

Show 16 quoted lines

> On Tue, Jul 16, 2024 at 08:45:48PM +0100, Ramsay Jones wrote:
> 
>>   error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied
>>   migrated refs can be found at '.git/ref_migration.sr9pEF'
>> [...]
>> Now try to finish the migration by hand:
>>
>>   $ mv .git/ref_migration.sr9pEF/reftable .git/reftable
>>
>> Hmm, note no error; of course, the mv command may well do much more than
>> the rename() library function, so they are not necessarily equivalent.
> 
> 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! ;)

Show 5 quoted lines

> Does the patch below fix things for you? I'm not too familiar with the
> code, so this is what I cobbled together.  The best response will be
> from Patrick, but I think he's offline for another week or so. In the
> meantime, this at least doesn't crash for me. ;) And I confirmed that
> the tables.list file is closed during the move_files() call.

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!

ATB, Ramsay Jones

Show 35 quoted lines

> ---
> diff --git a/refs.c b/refs.c
> index bb90a18875..06a0fc5099 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2843,6 +2843,12 @@ int repo_migrate_ref_storage_format(struct repository *repo,
>  		goto done;
>  	}
>  
> +	/*
> +	 * Close the new ref store to avoid holding on to any open files
> +	 * which could interfere with moving things behind the scenes.
> +	 */
> +	ref_store_release(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,8 +2880,13 @@ 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);
> +	/*
> +	 * 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);
>  
>