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@xxxxxxxxx> 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"?
> 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()...
> 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'...
> 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.
