From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 17 Feb 2026 07:24:10 GMT
Subject: Re: [PATCH v6 1/6] setup: don't modify repo in `create_reference_database()`
Message-ID: <aZQXmvZbVT1eRtSH@pks.im>
In-Reply-To: <20260214-kn-alternate-ref-dir-v6-1-86a82c77cf59@gmail.com>

On Sat, Feb 14, 2026 at 11:34:14PM +0100, Karthik Nayak wrote:
> The `create_reference_database()` function is used to create the
> reference database during initialization of a repository. The function
> calls `repo_set_ref_storage_format()` to set the repositories reference
> format. This is an unexpected side-effect of the function. More so
> because the function is only called in two locations:
> 
>   1. During git-init(1) where the value is propagated from the `struct
>      repository_format repo_fmt` value.
> 
>   2. During git-clone(1) where the value is propagated from the
>      `the_repository` value.
> 
> The former is valid, however the flow already calls
> `repo_set_ref_storage_format()`, so this effort is simply duplicated.
> The latter sets the existing value in `the_repository` back to itself.
> While this is okay for now, introduction of more fields in
> `repo_set_ref_storage_format()` would cause issues, especially
> dynamically allocated strings, where we would free/allocate the same
> string back into `the_repostiory`.
> 
> To avoid all this confusion, clean up the function to longer take in and

s/longer/no &/, I assume?

> diff --git a/builtin/clone.c b/builtin/clone.c
> index b40cee5968..cd43bb5aa2 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1442,7 +1442,7 @@ int cmd_clone(int argc,
>  	hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
>  	initialize_repository_version(hash_algo, the_repository->ref_storage_format, 1);
>  	repo_set_hash_algo(the_repository, hash_algo);
> -	create_reference_database(the_repository->ref_storage_format, NULL, 1);
> +	create_reference_database(NULL, 1);
>  
>  	/*
>  	 * Before fetching from the remote, download and install bundle

This is case (2), where we set the ref storage format to itself.

> diff --git a/setup.c b/setup.c
> index b723f8b339..1fc9ae3872 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -2701,8 +2699,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
>  				      &repo_fmt, init_shared_repository);
>  
>  	if (!(flags & INIT_DB_SKIP_REFDB))
> -		create_reference_database(repo_fmt.ref_storage_format,
> -					  initial_branch, flags & INIT_DB_QUIET);
> +		create_reference_database(initial_branch, flags & INIT_DB_QUIET);
>  	create_object_directory();
>  
>  	if (repo_settings_get_shared_repository(the_repository)) {

And this is the second case. We call `repository_format_configure()` a
few lines above, and that function calls `repo_set_ref_storage_format()`
itself.

Looks good to me, and a nice simplification. Thanks!

Patrick