Re: [PATCH 4/6] apply: refactor code to drop `line_allocated`

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 17, 2024, 09:50 UTC
Message-ID: <ZulRAiILnz8WZSz4@pks.im>
In-Reply-To: <xmqqldzrmnqn.fsf@gitster.g>

On Mon, Sep 16, 2024 at 11:56:16AM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Refactor the function to use memmove(3P) instead, which allows us to get
> > rid of this double bookkeeping. We call this function at most once per
> > image anyway, so this shouldn't cause any performance regressions.
> 
> Don't we call remove_first_line() as long as leading is larger than
> trailing repeatedly?  Is "at most once" accurate?
> 
> As to the correctness, I think nobody takes the address of an
> element in the line[] array and expects the address to stay valid
> across a call to remove_first_line(), so this should be safe.

Oh, you're right. I did search for a loop surrounding `image_remove_first_line()`, but somehow I completely missed the obvious `for (;;)` loop around it. No idea how I was able to miss it.

I still very much doubt that this will cause performance issues in practice (even though it's only by gut feeling), but the statement is obviously incorrect. In case the assumption ever turns out to be wrong we can likely refactor the loop to only trim contents after we have found how many lines to remove, at which point we can remove them with a single call to `strbuf_remove()`.

Patrick