Patrick Steinhardt <ps@xxxxxx> writes:
> When repacking, we assemble git-pack-objects(1) arguments both for the
> "normal" pack and for the cruft pack. This configuration gets populated
> with a bunch of `OPT_PASSTHRU` options that we end up passing to the
> child process. These options are allocated, but never free'd.
>
> Create a new `pack_objects_args_release()` function that releases the
> memory for us and call it for both sets of options.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> builtin/repack.c | 57 ++++++++++++++++++++++++++---------
> t/t5329-pack-objects-cruft.sh | 2 ++
> t/t7700-repack.sh | 1 +
> 3 files changed, 45 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 3ee8cfa732f..c31d5653f1f 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -85,17 +85,34 @@ static int repack_config(const char *var, const char *value,
> run_update_server_info = git_config_bool(var, value);
> return 0;
> }
> - if (!strcmp(var, "repack.cruftwindow"))
> + if (!strcmp(var, "repack.cruftwindow")) {
> + free(cruft_po_args->window);
> return git_config_string(&cruft_po_args->window, var, value);
Surely, as git_config_string() gives an allocated string, the
ownership rules should be that the receiving variable/struct member
is responsible for properly freeing it, which means that the current
value must be discarded before the new value is taken. Looking
good.
I wonder if any of these targets (like the cruft_po_args->window we
see here) can be updated by parsing the command line arguments (with
OPT_STRING)? It turns out that this worry is not unfounded but you
already covered it ...
> if (!cruft_po_args.window)
> - cruft_po_args.window = po_args.window;
> + cruft_po_args.window = xstrdup_or_null(po_args.window);
... here, so we are safe.
Looking good.
Thanks.
