Re: [PATCH 6/7] repository: adapt `repo_clear()` to fully reset the repository
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Sep 28, 2026, 09:18 UTC
- Message-ID
- <CAOLa=ZQ_+Ofya1q01fpZjd_wDn=tk8YxbQWNxhFHya47hFRp-Q@mail.gmail.com>
- In-Reply-To
- <20260924-pks-create-repository-stateless-v1-6-11499557cf31@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 101 quoted lines
> 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 <ps@pks.im>
> ---
> 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.
Show 21 quoted lines
> } > > 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.
Show 5 quoted lines
> /* > * Populates the repository's index from its index_file, an index struct will > > -- > 2.56.0.rc2.329.gd58861e689.dirty