On Wed, Jan 18, 2023 at 5:10 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
> Change cmd_repack() to "goto cleanup" rather than "return ret" on
> error, when we returned we'd potentially skip cleaning up the
> string_lists and other data we'd allocated in this function.
This is hard to parse; the comma followed by "when" suggests you are
only changing things under a certain set of conditions rather than
explaining why you are making an unconditional change. Perhaps:
In cmd_repack() when we hit an error, replace "return ret" with "goto
cleanup" to ensure we free the necessary data structures.
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> builtin/repack.c | 13 +++++++------
> t/t6011-rev-list-with-bad-commit.sh | 1 +
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index c1402ad038f..f6493795318 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -948,7 +948,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>
> ret = start_command(&cmd);
> if (ret)
> - return ret;
> + goto cleanup;
>
> if (geometry) {
> FILE *in = xfdopen(cmd.in, "w");
> @@ -977,7 +977,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> fclose(out);
> ret = finish_command(&cmd);
> if (ret)
> - return ret;
> + goto cleanup;
>
> if (!names.nr && !po_args.quiet)
> printf_ln(_("Nothing new to pack."));
> @@ -1007,7 +1007,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> &existing_nonkept_packs,
> &existing_kept_packs);
> if (ret)
> - return ret;
> + goto cleanup;
>
> if (delete_redundant && expire_to) {
> /*
> @@ -1039,7 +1039,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> &existing_nonkept_packs,
> &existing_kept_packs);
> if (ret)
> - return ret;
> + goto cleanup;
> }
> }
>
> @@ -1115,7 +1115,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> string_list_clear(&include, 0);
>
> if (ret)
> - return ret;
> + goto cleanup;
> }
>
> reprepare_packed_git(the_repository);
> @@ -1172,10 +1172,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> write_midx_file(get_object_directory(), NULL, NULL, flags);
> }
>
> +cleanup:
> string_list_clear(&names, 1);
> string_list_clear(&existing_nonkept_packs, 0);
> string_list_clear(&existing_kept_packs, 0);
> clear_pack_geometry(geometry);
>
> - return 0;
> + return ret;
> }
> diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
> index bad02cf5b83..b2e422cf0f7 100755
> --- a/t/t6011-rev-list-with-bad-commit.sh
> +++ b/t/t6011-rev-list-with-bad-commit.sh
> @@ -2,6 +2,7 @@
>
> test_description='git rev-list should notice bad commits'
>
> +TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
>
> # Note:
> --
> 2.39.0.1225.g30a3d88132d
Code changes look fine.
