Re: [PATCH 1/3] ref-filter: free memory from used_atom

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

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Release memory from used_atom variable.

That much readers would know from a quick look of the patch text.

Without knowing what you are aiming at, it is impossible to judge if
the patch is a good change.

Seeing FREE_AND_NULL(array->items) in the same function makes me
think that the designer of ref_array_clear() function would want
this sequence of events to work correctly in an ideal future:

 * Do a ref-filter operation by calling filter_refs(), receiving the
   result into an array..

 * Do another ref-filter by calling filter_refs(), using different
   criteria, receiving the result into a different array.

 * Iterate over the resulting refs in the first array, and call
   format_ref_array_item().

 * ref_array_clear() the first array, as the caller is done with it.

 * Iterate over the resulting refs in the second array, and call
   format_ref_array_item().

 * ref_array_clear() the second array, as the caller is done with
   it.

However, I think it would make it impossible for the second call to
work correctly if this code freed used_atom without clearing, and
not re-initializing the used_atom_cnt etc.

If on the other hand, the only thing you are interested in is to
just discard pieces of memory we no longer use, and you are not
interested in helping to move us closer to the world in which we can
call filter_refs() twice, then the change this patch makes is
sufficient.

And the place to answer the "what are you aiming at?" question is in
the proposed commit log message.

In an ideal future, I _think_ the file-scope static variables in
ref-filter.c like used_atom and used_atom_cnt should become fields
of a new structure (say "struct ref_filter"), with initializer and
uninitializer ref_filter_new() and ref_filter_destroy().  When that
happens, I think FREE_AND_NULL(used_atom) + used_atom_cnt=0 should
become part of ref_filter_destroy(), not part of ref_array_clear().

But we are not there yet, and a clean-up patch like this does not
have to be a step towards that goal.

> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
>  ref-filter.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index e1bcb4ca8a197..1b71d08a43a84 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1996,6 +1996,9 @@ void ref_array_clear(struct ref_array *array)
>  {
>  	int i;
>  
> +	for (i = 0; i < used_atom_cnt; i++)
> +		free((char *)used_atom[i].name);
> +	free(used_atom);
>  	for (i = 0; i < array->nr; i++)
>  		free_array_item(array->items[i]);
>  	FREE_AND_NULL(array->items);
>
> --
> https://github.com/git/git/pull/538