Re: [PATCH] commit-graph: fix start_delayed_progress() leak

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 4, 2025, 07:42 UTC
Message-ID: <aD_46Qxh9oVj-P3U@pks.im>
In-Reply-To: <pull.1986.git.git.1749006675784.gitgitgadget@gmail.com>

On Wed, Jun 04, 2025 at 03:11:15AM +0000, Lidong Yan via GitGitGadget wrote:

Show 7 quoted lines

> From: Lidong Yan <502024330056@smail.nju.edu.cn>
> 
> In commit-graph.c:graph_write(), if read_one_commit() failed,
> progress allocated in start_delayed_progress() will leak. Add
> stop_progress() before goto cleanup.
> 
> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>

Nit: it might make sense to send multiple patches that are related, like
your memory leak fixes, in the same patch series. That makes it a bit
easier for reviewers to group together related reviews.

Show 9 quoted lines

> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index a783a86e797..ee48980248f 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -311,6 +311,7 @@ static int graph_write(int argc, const char **argv, const char *prefix,
>  		while (strbuf_getline(&buf, stdin) != EOF) {
>  			if (read_one_commit(&commits, progress, buf.buf)) {
>  				result = 1;
> +				stop_progress(&progress);

This function calls `stop_progress_msg()`, which knows to exit in case `*progress` is a NULL pointer. We thus don't have to guard this line with `if (progress)`. So the patch looks good to me, thanks!

Patrick