Re: [PATCH 1/3] line-log: free diff queue when processing non-merge commits

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 7, 2022, 15:29 UTC
Message-ID: <221107.86r0yevkt5.gmgdl@evledraar.gmail.com>
In-Reply-To: <20221107151115.GA1951@szeder.dev>

On Mon, Nov 07 2022, SZEDER Gábor wrote:

Show 17 quoted lines

> On Wed, Nov 02, 2022 at 08:20:21PM -0400, Taylor Blau wrote:
>> > +void diff_free_queue(struct diff_queue_struct *q)
>> > +{
>> > +	for (int i = 0; i < q->nr; i++)
>> > +		diff_free_filepair(q->queue[i]);
>> > +	free(q->queue);
>> > +}
>> 
>> Though I wonder, should diff_free_queue() be a noop when q is NULL? The
>> caller in process_ranges_ordinary_commit() doesn't care, of course,
>> since q is always non-NULL there.
>> 
>> But if we're making it part of the diff API, we should probably err on
>> the side of flexibility.
>
> On one hand, strbuf_reset(), string_list_clear(), or strvec_clear()
> would all segfault on a NULL strbuf, string_list, or strvec pointer.

But the reason we do that is because those APIs will always ensure that the struct is never in an inconsistent state, as opposed to the destructor you're adding here.

I.e. if you were to work with the queue after this diff_free_queue() call in process_ranges_ordinary_commit() you'd segfault, not so with those other APIs.

Show 6 quoted lines

> On the other hand, given the usage patterns of the diff API, and that
> it mostly only works on the dreaded global 'diff_queued_diff'
> instance, I don't think there is any flexibility to be gained with
> this; indeed it is already more flexible than many diff API functions
> as it works on the diff queue given as parameter instead of that
> global instance.

I pointed how this could be nicer if you made it work like those other APIs in https://lore.kernel.org/git/221103.864jvg2yit.gmgdl@evledraar.gmail.com/; I.e. we could do away with DIFF_QUEUE_CLEAR() after calling this "free()".

But in lieu of such a larger change, just adding a call to "DIFF_QUEUE_CLEAR()" in this new free() function seems like it could make thing safer at very little cost.

We're also far from consistent about this, but I wish it worked like that and were called:

	diff_queue_struct_{release,clear}()

I.e. the usual naming is:

	<struct name>_{release,clear}()

In cases where we don't free() the pointer itself, but assume that we're working on a struct on the stack, whereas *_free() functions will free the malloc'd pointer itself, as well as anything it contains.