Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> On Wed, Mar 23 2022, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>>
>>> Use release_revisions() to various users of "struct rev_list" which
>>> need to have their "struct rev_info" zero-initialized before we can
>>> start using it. In all of these cases we might "goto cleanup" (or equivalent),
>>
>> I didn't look at the bisect code, but the bundle one looks iffy from
>> the point of view of API cleanliness. If we have not yet called
>> repo_init_revisions() on a revs, we should refrain from calling
>> release_revisions() on it in the first place, no?
>
> It could be avoided, but I'd really prefer not to for this series.
>
> repo_init_revisions() is a non-trivial function, and changing the
> various bits in this series that can easily have a "goto" pattern
> because we assume that { 0 }-init'd is safe to pass to
> release_revisions() would be a larger change...
>
> We assume that in a lot of other destructors throughout the codebase, I
> figured we could leave this for later.
>
> Is that OK with you?
Not really. If you introduce "#define REV_INFO_INIT { 0 }",
perhaps, though.
Without such a mechanism to clearly say "here is what we initialize
a rev_info", the first call to repo_init_revisions() looks like the
place that initializes a rev_info, and call to release_revisions()
on a rev_info that did not go through repo_init_revisions() looks
like a call to free() of a pointer that hasn't been assigned the
result from an allocation from the heap. That is where my "iffy
from the API cleanliness POV" comes from.
