Re: [PATCH] refs: fix format migration on Cygwin

From: Jeff King <peff@peff.net>
Date: Jul 23, 2024, 20:52 UTC
Message-ID: <20240723205232.GB6779@coredump.intra.peff.net>
In-Reply-To: <d031aef5552d894784650a8c6580925e877df794.1721731179.git.ps@pks.im>

On Tue, Jul 23, 2024 at 02:31:28PM +0200, Patrick Steinhardt wrote:

Show 15 quoted lines

> @@ -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;

I think this FREE_AND_NULL() is not technically part of the fix that the commit message describes. It is fixing an existing leak that happens when we overwrite repo->refs_private (whether with new_refs or with NULL).

That said, I don't know that it's worth going back to split it out now.

The rest of the patch looks good to me, and the commit message nicely describes the problem and solution.

-Peff