On Mon, Jul 11 2022, Jeff King wrote:
> 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
Urgh, yes that's the obviously correct fix to bring it back to the
previous behavior, it's indeed just a misplaced "cleanup" label, sorry
about that.
