Re: [PATCH 1/3] log: fix memory leak if --graph is passed multiple times

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2022, 18:16 UTC
Message-ID: <xmqqh7979a8r.fsf@gitster.g>
In-Reply-To: <20220209162350.169971-1-alexhenrie24@gmail.com>

Alex Henrie <alexhenrie24@gmail.com> writes:

Show 21 quoted lines

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>  revision.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index ad4286fbdd..c03c387edd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2424,9 +2424,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  		revs->pretty_given = 1;
>  		revs->abbrev_commit = 1;
>  	} else if (!strcmp(arg, "--graph")) {
> -		revs->topo_order = 1;
> -		revs->rewrite_parents = 1;
> -		revs->graph = graph_init(revs);
> +		if (!revs->graph) {
> +			revs->topo_order = 1;
> +			revs->rewrite_parents = 1;
> +			revs->graph = graph_init(revs);
> +		}

I understand the refs->graph part but are there ways to turn off topo_order or rewrite_parents with _other_ options? I.e.

    git log --graph --another-option --graph

if --another-option flips either bits off, would make the graph code misbehave because it requires both of these bits set.

I think this is safe in the corrent code, simply because there do not seem to be a way to unset these bits once they are set, but I am not sure if that is something we want to rely on.

I think an ideal endgame should look more like

	} else if (!strcmp(arg, "--graph")) {
		revs->topo_order = 1;
		revs->rewrite_parents = 1;
+		graph_clear(revs->graph);
		revs->graph = graph_init(revs);

where graph_clear() is to release the resource held by the git_graph struct that was previously prepared (and possibly used), and becomes a no-op when given NULL (just like free(NULL) is OK).

But if we want to punt on introducing graph_clear(), perhaps

	} else if (!strcmp(arg, "--graph")) {
		revs->topo_order = 1;
		revs->rewrite_parents = 1;
		if (!revs->graph)
			revs->graph = graph_init(revs);

would be closer to the ideal endgame (and have an in-code comment to hint future developers that (1) we are aware that this is not ideal, and that (2) the right way is to clear the previously allocated one before doing another init).

Thanks.

>  	} else if (!strcmp(arg, "--encode-email-headers")) {
>  		revs->encode_email_headers = 1;
>  	} else if (!strcmp(arg, "--no-encode-email-headers")) {