Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
> 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 <avarab@xxxxxxxxx>
> ---
> 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);
