Re: [PATCH 5/5] commit: rewrite read_graft_line

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 16, 2017, 19:39 UTC
Message-ID: <xmqqmv6zjo9x.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAJfL8+RVNYh1ryZ5EkiWxiZRo3Loq-MjujD94zGBMEGykmSWeg@mail.gmail.com>

Patryk Obara <patryk.obara@gmail.com> writes:

Show 17 quoted lines

> Junio C Hamano <gitster@pobox.com> wrote:
>> I am not sure if this is a good approach.  Just like in 2/5 you can
>> use the MAX thing instead of 20, instead of having each graft entry
>> allocate a separate oid_array.oid[].
>
> Once MAX values were increased memory corruption was caused exactly by
> this line:
>
>> - graft = xmalloc(st_add(sizeof(*graft), st_mult(GIT_SHA1_RAWSZ, i)));
>
> I could've replaced it by:
>
>     graft = xmalloc(st_add(sizeof(*graft), st_mult(sizeof(struct
> object_id), i)));
>
> But it seemed to me like short-sighted solution (code might be broken if
> object_id will be modified again in future).

Why?

Assuming that "struct object_id" would have to be prepared to handle more than one types of hash functions at the same time, it would be sufficiently large to hold any type of hash, plus possibly another member that tells which kind.

The decision to choose between a flex array and a separately allocatable structure primarily should come from how the enclosing struct (i.e. the commti_graft structure) is meant to be used. It is not meant to be tweaked by adding more parents or removing parents once it is constructed, which argues for having a flex array to hold the known and fixed number of parents once it is constructed.