Re: [PATCH v2 05/27] revision.[ch]: split freeing of revs->commit into a function
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 24, 2022, 17:01 UTC
- Message-ID
- <220324.86h77nmgha.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqqa6dgm0l8.fsf@gitster.g>
On Wed, Mar 23 2022, Junio C Hamano wrote:
Show 35 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> +static void release_revisions_commit_list(struct rev_info *revs)
>> +{
>> + struct commit_list *commits = revs->commits;
>> +
>> + if (!commits)
>> + return;
>> + free_commit_list(commits);
>> + revs->commits = NULL;
>> +}
>
> It makes sense to have this as a separate helper, but the original
> implementation this was lifted from is much easier to follow than
> this version with an unnecessary rewrite, I would think.
>
>> @@ -4080,10 +4090,7 @@ static void create_boundary_commit_list(struct rev_info *revs)
>> * boundary commits anyway. (This is what the code has always
>> * done.)
>> */
>> - if (revs->commits) {
>> - free_commit_list(revs->commits);
>> - revs->commits = NULL;
>> - }
>> + release_revisions_commit_list(revs);
>
> IOW
>
> static void release_revisions_commit_list(struct rev_info *revs)
> {
> if (revs->commits) {
> free_commit_list(revs->commits);
> revs->commits = NULL;
> }
> }Sure, will change it.
I think the pre-image is preferrable in context, i.e. to have them all use the same early abort pattern, but not enough to argue the point :)