From: Karthik Nayak Date: Tue, 17 Feb 2026 09:15:40 GMT Subject: Re: [PATCH v6 1/6] setup: don't modify repo in `create_reference_database()` Message-ID: In-Reply-To: Patrick Steinhardt writes: > 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? > Yup. >> 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 Thanks!