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
