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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 7, 2022, 18:09 UTC
Message-ID: <xmqqsfogwdjq.fsf@gitster.g>
In-Reply-To: <kl6lv8tccspo.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 12 quoted lines

> I suppose the question of whether or not to free() in the 'destructor'
> depends on whether we expect the struct to be reusable? I don't expect
> that "struct remote_state" needs to be reused, so free()-ing it is ok to
> me.
>
> The API is not _that_ odd though ;) As you noted, my initial use of
> FREE_AND_NULL() is for consistency reasons with the rest of
> repo_clear(), which looks like this:
>
> 	if (repo->config) {
> 		git_configset_clear(repo->config);
> 		FREE_AND_NULL(repo->config);

So git_configset_clear() does clear but does not free.

> 	}
>
> 	if (repo->submodule_cache) {
> 		submodule_cache_free(repo->submodule_cache);

submodule_cache_free() does (probably clear and) free.

Show 5 quoted lines

> 		repo->submodule_cache = NULL;
> 	}
>
> 	if (repo->index) {
> 		discard_index(repo->index);

discard_index() does not free.

Show 6 quoted lines

> 		if (repo->index != &the_index)
> 			FREE_AND_NULL(repo->index);
> 	}
>
> 	if (repo->promisor_remote_config) {
> 		promisor_remote_clear(repo->promisor_remote_config);

promisor_remote_clear() does not free.

Show 25 quoted lines

> 		FREE_AND_NULL(repo->promisor_remote_config);
> 	}
>
> 	if (repo->remote_state) {
> 		remote_state_clear(repo->remote_state);
>  -	FREE_AND_NULL(repo->remote_state);
>  +	repo->remote_state = NULL;
> 	}
>
> promisor_remote_clear(), discard_index(), and git_configset_clear()
> don't free() the struct, so it makes sense for them to use
> FREE_AND_NULL(). AFAICT, these structs are meant to be reused, so it
> makes sense that we "clear" it without freeing the struct pointer
> itself.
>
> On the other hand, submodule_cache_free() _does_ free() the struct, and
> so we just use "= NULL". I noticed that this uses the verb "free", and
> not "clear".
>
> So now that remote_state_clear() *does* free() the struct, it is
> perfectly fine to use "= NULL" here as well, though it uses the verb
> "clear".
>
> I'm not sure if we have a style around clear/free. Feel free to ignore
> if there isn't one.

It does bother me. Changing _clear() that did not free the container resource to free it, without changing the name to free or release or whatever, smells like leaving a source of confusion for future developers.