Re: [PATCH v6 11/27] revisions API users: add "goto cleanup" for release_revisions()

> On Wed, Apr 13, 2022 at 10:01:40PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> diff --git a/builtin/diff-files.c b/builtin/diff-files.c
>> index 70103c40952..2bfaf9ba7ae 100644
>> --- a/builtin/diff-files.c
>> +++ b/builtin/diff-files.c
>> @@ -77,8 +77,12 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
>>  
>>  	if (read_cache_preload(&rev.diffopt.pathspec) < 0) {
>>  		perror("read_cache_preload");
>> -		return -1;
>> +		result = -1;
>> +		goto cleanup;
>>  	}
>> +cleanup:
>>  	result = run_diff_files(&rev, options);
>> -	return diff_result_code(&rev.diffopt, result);
>> +	result = diff_result_code(&rev.diffopt, result);
>> +	release_revisions(&rev);
>> +	return result;
>>  }
>
> A bit late, but I happened to notice Coverity complaining about this
> code. And indeed, this patch seems pretty broken. If
> read_cache_preload() fails, we assign "-1" to result and jump to
> cleanup.
>
> But then the first thing we do in cleanup is overwrite result! That
> hides the error (depending on how run_diff_files behaves if the cache
> load failed, but one can imagine it thinks there are no files to diff).
>
> Should the cleanup label come after the call to run_diff_files()?
>
> I was also somewhat confused by the double-assignment of "result" in the
> cleanup label. But I think that is because diff_result_code() is
> massaging the current value of "result" into the right thing. But in
> that case, should the "-1" from earlier be passed to diff_result_code()?
> I think probably not (and certainly it was not before your patch). Which
> would imply that the label should go after that, like:
>
> diff --git a/builtin/diff-files.c b/builtin/diff-files.c
> index 2bfaf9ba7a..92cf6e1e92 100644
> --- a/builtin/diff-files.c
> +++ b/builtin/diff-files.c
> @@ -80,9 +80,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
>  		result = -1;
>  		goto cleanup;
>  	}
> -cleanup:
>  	result = run_diff_files(&rev, options);
>  	result = diff_result_code(&rev.diffopt, result);
> +cleanup:
>  	release_revisions(&rev);
>  	return result;
>  }
>
> -Peff