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

Glen Choo [off-list ref] writes:

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.

		repo->submodule_cache = NULL;
	}

	if (repo->index) {
		discard_index(repo->index);

discard_index() does not free.

		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.

		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.