Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> Does it? I just quickly scanned the output of
>>
>> git grep -e 'reftable_[mc]alloc' -e 'reftable_realloc' -A2 origin/master
>>
>> and I didn't see a single NULL check
>
> I think you're right, I retrieved that information from wetware. Looking
> again I think I was confusing it with the if (x) free(x) fixes in
> 34230514b83 (Merge branch 'hn/reftable-coverity-fixes', 2022-02-16).

True.  Initially I hoped that these RFC changes gives us a better
solution that comes from stepping back and rethinking the issues
around the original "why are we calling memset() with NULL?", and
if it were only "well, in _return_block() functions, we clear the
block before calling _free()---that shouldn't be necessary, if the
calling custom malloc-free pair cares, they can do the clearing, and
plain vanilla free() certainly doesn't, so let's stop calling
memset()", it certainly would have fallen into that category, but
everything these RFC patches do beyond that seems "oh, it does not
look important to me, so let's rip it out to simplify", without
knowing enough to say if that is sensible.

But ...

>> Isn't it there so the same code can be used by libgit2 and other
>> things that let the caller specify a custom allocator?
>
> I don't think so, but I may be wrong. I think it's a case of
> over-generalization where we'd be better off with something simpler,
> i.e. just using our normal allocation functions.

... many points that was raised on the reftable code in this thread
may deserve response to explain the rationale behind the current
code and design, as nobody can improve, without breaking the
intended way to be used, without knowing what it is.  Thanks for
marking the patches with RFC.  I think the "patch" is a bit too
dense a form to point out the issues in the existing code, but the
discussion that follows by quoting the points in the original code
and suggested changes is a good way to think about the code before
these RFC patches.

Ideally, it would have been much better if these points were raised
during the review before the code was accepted to the code base, but
it is better to have one now, rather than never ;-)

THanks.