From: Karthik Nayak Date: Mon, 28 Sep 2026 09:18:52 GMT Subject: Re: [PATCH 6/7] repository: adapt `repo_clear()` to fully reset the repository Message-ID: In-Reply-To: <20260924-pks-create-repository-stateless-v1-6-11499557cf31@pks.im> Patrick Steinhardt writes: > The function `repo_clear()` can be used to clear a repository's state. > The way it's written though it's quite easy for it to accidentally leak > some state because we don't make sure to clear the whole structure. > > Refactor the function to set the whole repository to all-zeroes to avoid > any kind of leaking state. While at it, make it a bit more robust when > called on an already-blank repository. > > Signed-off-by: Patrick Steinhardt > --- > repository.c | 37 ++++++++++++++++++------------------- > repository.h | 2 +- > 2 files changed, 19 insertions(+), 20 deletions(-) > > diff --git a/repository.c b/repository.c > index b857e1c580..e67ff00550 100644 > --- a/repository.c > +++ b/repository.c > @@ -374,60 +374,57 @@ void repo_clear(struct repository *repo) > struct hashmap_iter iter; > struct strmap_entry *e; > > - FREE_AND_NULL(repo->gitdir); > - FREE_AND_NULL(repo->commondir); > - FREE_AND_NULL(repo->prefix); > - FREE_AND_NULL(repo->graft_file); > - FREE_AND_NULL(repo->index_file); > - FREE_AND_NULL(repo->worktree); > - FREE_AND_NULL(repo->submodule_prefix); > - FREE_AND_NULL(repo->ref_storage_payload); > + free(repo->gitdir); > + free(repo->commondir); > + free(repo->prefix); > + free(repo->graft_file); > + free(repo->index_file); > + free(repo->worktree); > + free(repo->submodule_prefix); > + free(repo->ref_storage_payload); > > odb_free(repo->objects); > - repo->objects = NULL; > > if (repo->parsed_objects) > parsed_object_pool_clear(repo->parsed_objects); > - FREE_AND_NULL(repo->parsed_objects); > + free(repo->parsed_objects); > > repo_settings_clear(repo); > repo_config_values_clear(&repo->config_values_private_); > > if (repo->config) { > git_configset_clear(repo->config); > - FREE_AND_NULL(repo->config); > + free(repo->config); > } > > - if (repo->submodule_cache) { > + if (repo->submodule_cache) > submodule_cache_free(repo->submodule_cache); > - repo->submodule_cache = NULL; > - } > > if (repo->index) { > discard_index(repo->index); > - FREE_AND_NULL(repo->index); > + free(repo->index); > } > > if (repo->hook_config_cache) { > hook_cache_clear(repo->hook_config_cache); > - FREE_AND_NULL(repo->hook_config_cache); > + free(repo->hook_config_cache); > } > strmap_clear(&repo->event_jobs, 0); /* values are uintptr_t, not heap ptrs */ > string_list_clear(&repo->disabled_events, 0); > > if (repo->promisor_remote_config) { > promisor_remote_clear(repo->promisor_remote_config); > - FREE_AND_NULL(repo->promisor_remote_config); > + free(repo->promisor_remote_config); > } > > if (repo->remote_state) { > remote_state_clear(repo->remote_state); > - FREE_AND_NULL(repo->remote_state); > + free(repo->remote_state); > } > > if (repo->refs_private) { > ref_store_release(repo->refs_private); > - FREE_AND_NULL(repo->refs_private); > + free(repo->refs_private); > } > > strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e) > @@ -439,6 +436,8 @@ void repo_clear(struct repository *repo) > strmap_clear(&repo->worktree_ref_stores, 1); > > repo_clear_path_cache(&repo->cached_paths); > + > + memset(repo, 0, sizeof(*repo)); The reason we swap `FREE_AND_NULL()` with `free()` is because we anyways set everything to 0. Okay. Or was this referring to the 'already blank' repository? Since FREE_AND_NULL() can already handle NULL values. > } > > int repo_read_index(struct repository *repo) > diff --git a/repository.h b/repository.h > index 11f5c2ed10..2a348012e8 100644 > --- a/repository.h > +++ b/repository.h > @@ -258,6 +258,7 @@ void repo_set_ref_storage_format(struct repository *repo, > void initialize_repository(struct repository *repo); > RESULT_MUST_BE_USED > int repo_init(struct repository *r, const char *gitdir, const char *worktree); > +void repo_clear(struct repository *repo); > > /* > * Initialize the repository 'subrepo' as the submodule at the given path. If > @@ -273,7 +274,6 @@ int repo_submodule_init(struct repository *subrepo, > struct repository *superproject, > const char *path, > const struct object_id *treeish_name); > -void repo_clear(struct repository *repo); > This is a purely cosmetic move to bring it closer to `repo_init()`, right? I think it makes sense. > /* > * Populates the repository's index from its index_file, an index struct will > > -- > 2.56.0.rc2.329.gd58861e689.dirty