Re: [PATCH 3/3] remote API: don't buggily FREE_AND_NULL(), free() instead
From: Junio C Hamano <hidden>
Date: 2022-06-07 17:32:30
Ævar Arnfjörð Bjarmason [off-list ref] writes:
Change the buggy "remote_clear()" function to stop pretending to to be
"buggy" -> ""; "to to" -> "to". Over-using FREE_AND_NULL() is indeed irritating, but as long as we are not reusing the struct whose member are getting freed, there is no "bug" per-se, no?
able to zero out a "struct remote". Setting "url" and "pushurl" to NULL results in an invalid state unless the corresponding "url_nr" and "pushurl_nr" are also set to zero. In this case however we do not use the "struct remote", so the
"use" -> "reuse", with comma around "however" on both sides, probably.
FREE_AND_NULL() pattern added in fd3cb0501e1 (remote: move static variables into per-repository struct, 2021-11-17) can be replaced with free().
Yay.
The API was also odd in that remote_state_new() would xmalloc() for us, but the user had to free() it themselves, let's instead change the behavior to have the destructor free() what we malloc() in the constructer.
I am not sure if remote_clear() that does not free the remote goes well with remote_state_clear() that does free remote_state. As long as it is clear whose responsibility to release resources is, we can go either way, but helper functions with similar names having quite different resource maintainance semantics looks like a potential source of confusion to me.
quoted hunk
In this case this appears to have been done for consistency with repo_clear(), let's instead have repo_clear() handle the NULL-ing of its "remote_state", and not attempt to reset the structure in remote.c Signed-off-by: Ævar Arnfjörð Bjarmason <redacted> --- remote.c | 14 +++++++------- remote.h | 10 +++++++++- repository.c | 2 +- 3 files changed, 17 insertions(+), 9 deletions(-)diff --git a/remote.c b/remote.c index 0b243b090d9..c6ce04dacb7 100644 --- a/remote.c +++ b/remote.c@@ -147,15 +147,15 @@ static void remote_clear(struct remote *remote) for (i = 0; i < remote->url_nr; i++) free((char *)remote->url[i]); - FREE_AND_NULL(remote->url); + free(remote->url); for (i = 0; i < remote->pushurl_nr; i++) free((char *)remote->pushurl[i]); - FREE_AND_NULL(remote->pushurl); + free(remote->pushurl); free((char *)remote->receivepack); free((char *)remote->uploadpack); - FREE_AND_NULL(remote->http_proxy); - FREE_AND_NULL(remote->http_proxy_authmethod); + free(remote->http_proxy); + free(remote->http_proxy_authmethod); } static void add_merge(struct branch *branch, const char *name)@@ -2720,12 +2720,12 @@ void remote_state_clear(struct remote_state *remote_state) for (i = 0; i < remote_state->remotes_nr; i++) remote_clear(remote_state->remotes[i]); - FREE_AND_NULL(remote_state->remotes); - remote_state->remotes_alloc = 0; - remote_state->remotes_nr = 0; + free(remote_state->remotes); hashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent); hashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent); + + free(remote_state); } /*diff --git a/remote.h b/remote.h index dd4402436f1..d91b2b29373 100644 --- a/remote.h +++ b/remote.h@@ -54,9 +54,17 @@ struct remote_state { int initialized; }; -void remote_state_clear(struct remote_state *remote_state); +/** + * xmalloc() a "struct remote_state" and initialize it. The resulting + * data should be free'd with remote_state_clear(). + */ struct remote_state *remote_state_new(void); +/** + * free() the structure returned by remote_state_new(). + */ +void remote_state_clear(struct remote_state *remote_state); + struct remote { struct hashmap_entry ent;diff --git a/repository.c b/repository.c index 5d166b692c8..0a6df6937e4 100644 --- a/repository.c +++ b/repository.c@@ -292,7 +292,7 @@ void repo_clear(struct repository *repo) if (repo->remote_state) { remote_state_clear(repo->remote_state); - FREE_AND_NULL(repo->remote_state); + repo->remote_state = NULL; } repo_clear_path_cache(&repo->cached_paths);