Re: [PATCH 3/3] diff.c: use diff_free_queue()

[Taylor Blau <me@ttaylorr.com>]

On Wed, Nov 02, 2022 at 11:01:42PM +0100, SZEDER Gábor wrote:
> Use diff_free_queue() instead of open-coding it.  This shortens the
> code and make it less repetitive.
>
> Note that the second hunk in diff_flush() is interesting, because the
> 'free_queue' label separates the loop freeing the queue's filepairs
> from free()-ing the queue's internal array.  This is somewhat
> suspicious, but it was not an issue before: there is only one place
> from where we jump to this label with a goto, and that is protected by
> an 'if (!q->nr && ...)' condition, i.e. we only skipped the loop
> freeing the filepairs when there were no filepairs in the queue to
> begin with.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  diff.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index ef94175163..03e6ffb5e4 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6337,13 +6337,9 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid
>  int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)
>  {
>  	struct diff_queue_struct *q = &diff_queued_diff;
> -	int i;
>  	int result = diff_get_patch_id(options, oid, diff_header_only);
>
> -	for (i = 0; i < q->nr; i++)
> -		diff_free_filepair(q->queue[i]);
> -
> -	free(q->queue);
> +	diff_free_queue(q);
>  	DIFF_QUEUE_CLEAR(q);

So, this all looks fine to me. But I did a quick grep around for
DIFF_QUEUE_CLEAR(), and this macro is used in quite a few places.
Mostly, as far as I can tell, to "empty" out the diff-queue by setting
its 'queue' pointer to NULL, and its 'nr' back to 0.

Should we be freeing the memory held by the queue there more
aggressively? I.e., should we make sure that there is a
diff_free_queue() call above each expansion of the DIFF_QUEUE_CLEAR()
macro?

Thanks,
Taylor