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

From: Junio C Hamano <[email-masked]>
Date: Sun, 03 Apr 2022 14:49:38 -0700

Phillip Wood <[email-masked]> writes:

>>   +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 ;-).