On 10/20/2021 11:39 PM, Taylor Blau wrote:
> `git repack` invokes a handful of child processes: one to write the
> actual pack, and optionally ones to repack promisor objects and update
> the MIDX.
>
> In none of these cases do we bother to call child_process_clear(), which
> frees the memory associated with each child's arguments and environment.
>
> In order to do so, tweak each function that spawns a child process to
> have a `cleanup` label that we always visit before returning from each
> function. Then, make sure that we call child_process_clear() as a part
> of that label.
>
> Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
> ---
> builtin/repack.c | 35 ++++++++++++++++++++++++-----------
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 0b2d1e5d82..d16bab09a4 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -244,6 +244,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
> struct child_process cmd = CHILD_PROCESS_INIT;
> FILE *out;
> struct strbuf line = STRBUF_INIT;
> + int ret = 0;
nit: "ret" is short for "return" which makes me think "this will
be used as 'return ret;'" but we don't do that. Instead, we use
it as the _result_ of an inner call:
> - if (finish_command(&cmd))
> + ret = finish_command(&cmd);
> +
> +cleanup:
> + child_process_clear(&cmd);
> +
> + if (ret)
> die(_("could not finish pack-objects to repack promisor objects"));
For that reason, I would rename this to "res" or "result".
> - return finish_command(&cmd);
> + ret = finish_command(&cmd);
> +
> +cleanup:
> + child_process_clear(&cmd);
> +
> + return ret;
Here, you are taking the result of an inner call, but actually
returning it. This makes sense to be "ret".
> +cleanup:
> string_list_clear(&names, 0);
> string_list_clear(&rollback, 0);
> string_list_clear(&existing_nonkept_packs, 0);
> string_list_clear(&existing_kept_packs, 0);
> clear_pack_geometry(geometry);
> strbuf_release(&line);
> + child_process_clear(&cmd);
>
> - return 0;
> + return ret;
Also here is good.
Thanks,
-Stolee
