Re: [PATCH] sequencer: fix memory leak if `update_squash_messages()` failed

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2025, 16:20 UTC
Message-ID: <xmqqldqx3ky7.fsf@gitster.g>
In-Reply-To: <935822b9-33e5-457e-95a7-64058777b52a@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 5 quoted lines

> Looking at the code, if we reaching that call to error() is a
> programming error as we should only call update_squash_messages() if
> command is TODO_FIXUP or TODO_SQUASH so I think we'd be better to
> replace error(...) with BUG(...) which calls abort() which means we
> don't care if there is a leak or not.

It is a valid way to "fix" a leak, certainly ;-).

I am curious if Lidong's tool would notice an unreachable code if only the first hunk of the attached patch is applied. The "else" clause in the second hunk would become unreachable.

diff --git c/sequencer.c w/sequencer.c
index b5c4043757..269637d427 100644
--- c/sequencer.c
+++ w/sequencer.c
@@ -2071,6 +2071,9 @@ static int update_squash_messages(struct repository *r,
 	const char *message, *body;
 	const char *encoding = get_commit_output_encoding();
 
+	if (!(command == TODO_FIXUP || command == TODO_SQUASH))
+		BUG("update_squash_messages with command %d", command);
+
 	if (ctx->current_fixup_count > 0) {
 		struct strbuf header = STRBUF_INIT;
 		char *eol;
@@ -2138,8 +2141,6 @@ static int update_squash_messages(struct repository *r,
 		strbuf_addstr(&buf, "\n\n");
 		strbuf_add_commented_lines(&buf, body, strlen(body),
 					   comment_line_str);
-	} else
-		return error(_("unknown command: %d"), command);
 	repo_unuse_commit_buffer(r, commit, message);
 
 	if (!res)