Re: [PATCH v4 4/4] commit: rewrite read_graft_line

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 18, 2017, 19:47 UTC
Message-ID: <xmqqefs8bqvf.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAJfL8+T3vqnmFJmx19H-v8yGiY4Se78SM+ax_q07_PF4VHDv3Q@mail.gmail.com>

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

> Actually, I don't think I needed to remove free(graft) line, but I don't
> know if freeing NULL is considered ok in git code. Let me know if I
> should bring it back, please.

Either free(graft) or assert(!graft) is fine, but we should have one of them there. I'll add assert(!graft) there while queuing, at least for now.

In the current code, when the control reaches the bad_graft_data label, 'graft' must be NULL, or there is a bug in our code. Because we are parsing exactly the same input using the same helper routines in both passes, we should see failure during the first pass before 'graft' points to an allocated piece of memory. So it may be a good idea to have assert(!graft) there than free(graft); the latter would sweep a potential bug under the carpet.

If this were a part of the system whose design is still fluid (it is not), it is not implausible that we would later want to add new test that jumps to the bad_graft_data label. For example, after the "runs exactly twice" loop, we may add a new test that iterates over the graft->parents[] to ensure that there is no duplicate and jumps to bad_graft_data when we find one.

If we add assert(!graft) there today, and if such an enhancement forgets to replace it with free(graft), the assert() will catch the mistake. If we have neither, it makes it more likely that such an enhancement leaves a possible memory leak in its error codepath.

Thanks.