Re: [PATCH v4 13/27] revisions API users: use release_revisions() in builtin/log.c

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 3, 2022, 21:49 UTC
Message-ID: <xmqq5ynpvnv1.fsf@gitster.g>
In-Reply-To: <14c17fa9-e9ff-ac5f-dbda-4a566ed09fd3@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 22 quoted lines

>>   +static int cmd_log_deinit(int ret, struct rev_info *rev)
>> +{
>> +	release_revisions(rev);
>> +	return ret;
>> +}
>
>
>>   /*
>>    * This gives a rough estimate for how many commits we
>>    * will print out in the list.
>> @@ -558,7 +564,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
>>   	cmd_log_init(argc, argv, prefix, &rev, &opt);
>>   	if (!rev.diffopt.output_format)
>>   		rev.diffopt.output_format = DIFF_FORMAT_RAW;
>> -	return cmd_log_walk(&rev);
>> +	return cmd_log_deinit(cmd_log_walk(&rev), &rev);
>
> This is a rather unusual pattern, at first I wondered if there were
> going to be more added to the body of cmd_log_deinit() in later
> commits but there isn't so why not just call release_revisions() here
> to be consistent with the other release_revisions() call that are
> added in other patches?

It is being cute and clever by not requiring a temporary variable ret, where you would normally say

	int ret = 0; /* assume success */

	... a lot of code ...
	ret = cmd_log_walk(&rev);
        release_revisions(&rev);
	return ret;

I agree that this looks confusing; if this pattern can become majority locally in the file, I guess it would be OK---at that point we can claim that it is the (new) usual pattern ;-).