From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Thu, 24 Mar 2022 17:01:22 GMT
Subject: Re: [PATCH v2 05/27] revision.[ch]: split freeing of revs->commit into a function
Message-ID: <220324.86h77nmgha.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqa6dgm0l8.fsf@gitster.g>


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

> Æ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 :)