Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

["Martin Ågren" <martin.agren@gmail.com>]

On Tue, 2 Oct 2018 at 17:01, Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> diff --git a/commit-graph.c b/commit-graph.c
> index 2a24eb8b5a..7226bd6b58 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -698,6 +698,8 @@ void write_commit_graph_reachable(const char *obj_dir, int append,
>         string_list_init(&list, 1);
>         for_each_ref(add_ref_to_list, &list);
>         write_commit_graph(obj_dir, NULL, &list, append, report_progress);
> +
> +       string_list_clear(&list, 0);
>  }

Nit: The blank line adds some asymmetry, IMVHO.

>  void write_commit_graph(const char *obj_dir,
> @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,
>         compute_generation_numbers(&commits, report_progress);
>
>         graph_name = get_commit_graph_filename(obj_dir);
> -       if (safe_create_leading_directories(graph_name))
> +       if (safe_create_leading_directories(graph_name)) {
> +               UNLEAK(graph_name);
>                 die_errno(_("unable to create leading directories of %s"),
>                           graph_name);
> +       }

Do you really need this hunk? In my testing with LeakSanitizer and
valgrind, I don't need this hunk to be leak-free. Generally speaking, it
seems impossible to UNLEAK when dying, since we don't know what we have
allocated higher up in the call-stack.

Without this hunk, this patch can have my

Reviewed-by: Martin Ågren <martin.agren@gmail.com>

as I've verified the leaks before and after. With this hunk, I am
puzzled and feel uneasy, both about having to UNLEAK before dying and
about having to UNLEAK outside of builtin/.

> +       free(graph_name);
> +       free(commits.list);
>         free(oids.list);
>         oids.alloc = 0;
>         oids.nr = 0;

Both `commits` and `oids` are on the stack here, so cleaning up one more
than the other is a bit asymmetrical. Also, if we try to zero the counts
-- which seems unnecessary to me, but which is not new with this patch --
we should perhaps use `FREE_AND_NULL` too. But personally, I would just
use `free` and leave `nr` and `alloc` at whatever values they happen to
have.

Martin