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

From: Glen Choo <chooglen@google.com>
Date: Jun 7, 2022, 17:02 UTC
Message-ID: <kl6lv8tccspo.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-3.3-062fb3f454e-20220607T154520Z-avarab@gmail.com>

Thanks for sending out this series :) I feel a little guilty leaving behind this much junk. Coincidentally, I was already planning on revisiting my previous work to clean up debt/mistakes, so this was a good lesson in what to look out for.

The previous patches look good to me, and I have no comments on those. I have a style question on this patch (more for my own learning than a suggestion on this patch), but it also looks good to me.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> In this case however we do not use the "struct remote", so the
> FREE_AND_NULL() pattern added in fd3cb0501e1 (remote: move static
> variables into per-repository struct, 2021-11-17) can be replaced with
> free().

Did you mean *reuse* the "struct remote"?

Show 8 quoted lines

> 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.
>
> 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

Yes, this was specifically for consistency reasons. We'll have to read on to see whether or not this patch remains consistent with repo_clear()...

Show 15 quoted lines

>  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);
>  }

Now that remote_state_clear() free()-s the "struct remote_state", should we rename it to something like "remote_state_free()"? There's some precedent for this in the other repo_clear() 'destructors'...

Show 36 quoted lines

> 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);

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);
	}

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

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

	if (repo->promisor_remote_config) {
		promisor_remote_clear(repo->promisor_remote_config);
		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.