Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 26, 2024, 17:45 UTC
Message-ID: <xmqqmsn71ttw.fsf@gitster.g>
In-Reply-To: <xmqqsewz1ua5.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 10 quoted lines

> Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:
>
>> To me, this looks much better.  child_process_clear's name already
>> suggests that is sort of like a destructor, so it makes sense to
>> re-initialize everything here.  I even wonder why it was not that way to
>> begin with.  I suppose no callers are assuming that it only clears args
>> and env though?
>
> I guess that validating that supposition is a prerequisite to
> declare the change as "much better" and "makes sense".

Having said that, a lot more plausible reason is that most callers are expected to use a single struct just once without reusing it, so the cost of re-initialization is wasted cycles for them if it were part of child_process_clear() which is primarily about releasing resources (instead of leaking them).

It can be argued that it is a sensible design decision to make those minority callers that do reuse the same struct to pay the cost of reinitialization themselves, instead of the majority callers that use it once and then release resources held in there.