Re: [PATCH v6 11/27] revisions API users: add "goto cleanup" for release_revisions()
- From
Jeff King <peff@peff.net>
- Date
- Jul 11, 2022, 18:06 UTC
- Message-ID
- <YsxmrRCSpze1csHz@coredump.intra.peff.net>
- In-Reply-To
- <patch-v6-11.27-e93791b6242-20220413T195935Z-avarab@gmail.com>
On Wed, Apr 13, 2022 at 10:01:40PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 19 quoted lines
> 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