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

From: Patryk Obara <patryk.obara@gmail.com>
Date: Aug 16, 2017, 13:07 UTC
Message-ID: <CAJfL8+RVNYh1ryZ5EkiWxiZRo3Loq-MjujD94zGBMEGykmSWeg@mail.gmail.com>
In-Reply-To: <xmqqo9rgr8eh.fsf@gitster.mtv.corp.google.com>

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).

> Is this because you expect more than one _kind_ of hashes are used
> at the same time?

Yes - I think more than one kind of hashes will be used at the same time (meaning in single git binary, not necessarily in a single process), at the very least to allow read access to repositories in "old" format(s), with sha1-based grafts. Using MAX for parsing here would prevent this, requiring either modifying grafts code again in future or forcing right now some unwelcome design decisions down the line.

My goal here was to make this code compatible with whatever hash function transition plan will be chosen.

-- 
| ← Ceci n'est pas une pipe
Patryk Obara