On Wed, Apr 13, 2022 at 10:01:40PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

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