Subject: Re: [PATCH v2 05/27] revision.[ch]: split freeing of revs->commit into a function

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 24 Mar 2022 18:01:22 +0100

On Wed, Mar 23 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <[email-masked]> 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 :)