On Tue, 2 Oct 2018 at 17:01, Derrick Stolee via GitGitGadget
<gitgitgadget@xxxxxxxxx> 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@xxxxxxxxx>
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
