Re: [PATCH v2] diff --no-index: unleak paths[] elements

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 7, 2022, 12:31 UTC
Message-ID: <220907.86mtbbl5ua.gmgdl@evledraar.gmail.com>
In-Reply-To: <36q9s463-rno9-9rs1-7qsq-5oqq23o30o0r@tzk.qr>

On Wed, Sep 07 2022, Johannes Schindelin wrote:

Show 14 quoted lines

> Hi Ævar,
>
> On Mon, 5 Sep 2022, Ævar Arnfjörð Bjarmason wrote:
>
>> On Mon, Sep 05 2022, Johannes Schindelin wrote:
>>
>> [...]
>> > +	string_list_clear(&to_release, 1);
>> [...]
>>
>>  * The free_util=1 in your code isn't needed/is a bug, we make no use of
>>    "util" here, so it should be free_util=0
>
> Calling it a bug is a bit strong[...]

Yes, FWIW I meant that in the sense of "the author probably didn't mean this" or "it was copy/pasted", but it has no effect currently, as we'll always have NULL "util" members.

> , and misses the reason why I did it: future-proofing.

I didn't think it was intentional, but obviously you know better about the intent.

"Future proofing" seems like a bad reason for that API use however. If you look at the various "util" users some of them want to free() it, and some don't. If you forget to free() the worst you'll have is a memory leak, but if you have some boilerplate free() there you'll probably get a segfault.

Without knowing what the future code looks like we don't know whether it would make any sense to free() that util use.

> In any case, René took up Junio's ask and provided a new iteration that
> side-steps this concern altogether, therefore the point is now moot.

Indeed, that patch LGTM.