Re: [PATCH 3/3] remote API: don't buggily FREE_AND_NULL(), free() instead

[Junio C Hamano <gitster@pobox.com>]

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