sequencer: fix memory leak if `update_squash_messages()` failed

PATCH In review Lidong Yan via GitGitGadget 4 participants 18 messages May 14, 2025 – Jun 3, 2025

lore

Share & tools

lore

Lidong Yan via GitGitGadget

author

2025-05-14 12:44 UTC #0 lore raw

PATCH 1/1 sequencer: fix memory leak if `update_squash_messages()` failed

From: Lidong Yan <[email protected]>

In sequencer.c:update_squash_messages, repo_logmsg_reencode returns either an allocated reencode string or commit buffer if no encode is needed. To free repo_logmsg_reencode result, repo_unuse_commit_buffer should be used. However, when encountering the error("unknown command..."), the absence of repo_unuse_commit_buffer results in a memory leak. I think we should add a repo_unuse_commit_buffer before return.

Signed-off-by Lidong Yan <[email protected]>

---
sequencer: fix memory leak if update_squash_messages() failed

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1964%2Fbrandb97%2Ffix-sequencer-leak-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1964/brandb97/fix-sequencer-leak-v1
Pull-Request: https://github.com/git/git/pull/1964

 sequencer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

sequencer.c +3 -1

index b5c4043757e..f288a303eaa 100644
@@ -2138,8 +2138,10 @@ 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
+	} else {
+		repo_unuse_commit_buffer(r, commit, message);
 		return error(_("unknown command: %d"), command);
+	}
 	repo_unuse_commit_buffer(r, commit, message);
 
 	if (!res)

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75

Signature · 1 line

gitgitgadget

Phillip Wood

2025-05-15 10:09 UTC #1 lore raw

Quoted 8 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>
In sequencer.c:update_squash_messages, repo_logmsg_reencode returns either an allocated reencode string or commit buffer if no encode is needed. To free repo_logmsg_reencode result, repo_unuse_commit_buffer should be used. However, when encountering the error("unknown command..."), the absence of repo_unuse_commit_buffer results in a memory leak. I think we should add a repo_unuse_commit_buffer before return.

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.

Thanks

Phillip

Quoted 37 lines

Signed-off-by Lidong Yan <[email protected]>
---
sequencer: fix memory leak if update_squash_messages() failed
In sequencer.c:update_squash_messages, repo_logmsg_reencode returns either an allocated reencode string or commit buffer if no encode is needed. To free repo_logmsg_reencode result, repo_unuse_commit_buffer should be used. However, when encountering the error("unknown command..."), the absence of repo_unuse_commit_buffer results in a memory leak. I think we should add a repo_unuse_commit_buffer before return.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1964%2Fbrandb97%2Ffix-sequencer-leak-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1964/brandb97/fix-sequencer-leak-v1
Pull-Request: https://github.com/git/git/pull/1964
  sequencer.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
sequencer.c +3 -1
index b5c4043757e..f288a303eaa 100644
@@ -2138,8 +2138,10 @@ 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
+	} else {
+		repo_unuse_commit_buffer(r, commit, message);
  		return error(_("unknown command: %d"), command);
+	}
  	repo_unuse_commit_buffer(r, commit, message);
 
  	if (!res)
base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75

Junio C Hamano

2025-05-15 16:20 UTC #2 lore raw

Quoted 5 lines · Phillip Wood

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.

sequencer.c +3 -2

index b5c4043757..269637d427 100644
@@ -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)

lidongyan

2025-05-15 16:43 UTC #3 lore raw

Quoted 21 lines · 2025年5月16日 00:20，Junio C Hamano <[email protected]> 写道：

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.

sequencer.c +3 -0

index b5c4043757..269637d427 100644
@@ -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;

sequencer.c static int update_squash_messages(struct repository *r, · line 2146

Quoted 8 lines

sequencer.c +0 -2

@@ -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)

In this case, we do not directly identify the else branch as unreachable code. However, we perform constraint solving on the conditions leading to a potential leak, which ultimately allows us to determine that the leaking path is actually unreachable.

Lidong Yan via GitGitGadget

author

2025-05-25 10:19 UTC #4 lore raw

PATCH v2 1/1 sequencer: replace error() with BUG() in update_squash_messages()

In reply to Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

In sequencer.c, caller only pass TODO_SQUASH or TODO_FIXUP to update_squash_messages(), any other command passed in should be considered as BUG. Thus I think return error('unknown command') should be replaced as BUG('unknown command').

Signed-off-by Lidong Yan <[email protected]>

---
sequencer: replace error() with BUG() in update_squash_messages()

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1964%2Fbrandb97%2Ffix-sequencer-leak-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1964/brandb97/fix-sequencer-leak-v2
Pull-Request: https://github.com/git/git/pull/1964

Range-diff vs v1:

1: a4bf7dd1cf6 ! 1: aa5ff030b37 sequencer: fix memory leak if update_squash_messages() failed
@@ Metadata
Author: Lidong Yan <[email protected]>

## Commit message ##
- sequencer: fix memory leak if update_squash_messages() failed
+ sequencer: replace error() with BUG() in update_squash_messages()

- In sequencer.c:update_squash_messages, repo_logmsg_reencode returns
- either an allocated reencode string or commit buffer if no encode is
- needed. To free repo_logmsg_reencode result, repo_unuse_commit_buffer
- should be used. However, when encountering the error("unknown command..."),
- the absence of repo_unuse_commit_buffer results in a memory leak. I
- think we should add a repo_unuse_commit_buffer before return.
+ In sequencer.c, caller only pass TODO_SQUASH or TODO_FIXUP to
+ update_squash_messages(), any other command passed in should be
+ considered as BUG. Thus I think return error('unknown command')
+ should be replaced as BUG('unknown command').

Signed-off-by Lidong Yan <[email protected]>

## sequencer.c ##
@@ sequencer.c: 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
-+ } else {
-+ repo_unuse_commit_buffer(r, commit, message);
- return error(_("unknown command: %d"), command);
-+ }
+ } else
+- return error(_("unknown command: %d"), command);
++ BUG(_("unknown command: %d"), command);
repo_unuse_commit_buffer(r, commit, message);

if (!res)

 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sequencer.c +1 -1

index b5c4043757e..3cd0dd3434e 100644
@@ -2139,7 +2139,7 @@ static int update_squash_messages(struct repository *r,
 		strbuf_add_commented_lines(&buf, body, strlen(body),
 					   comment_line_str);
 	} else
-		return error(_("unknown command: %d"), command);
+		BUG(_("unknown command: %d"), command);
 	repo_unuse_commit_buffer(r, commit, message);
 
 	if (!res)

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75

Signature · 1 line

gitgitgadget

Junio C Hamano

2025-05-28 22:16 UTC #5 lore raw

Re: [PATCH v2] sequencer: replace error() with BUG() in update_squash_messages()

Quoted 6 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>
In sequencer.c, caller only pass TODO_SQUASH or TODO_FIXUP to update_squash_messages(), any other command passed in should be considered as BUG. Thus I think return error('unknown command') should be replaced as BUG('unknown command').

Yup. The only caller has

	else if (is_fixup(command)) {
	     if (update_squash_messages(r, command, ...) {

and is_fixup() is confusingly [*] defined to return true only when the command is one of these two values.

[Footnote]

* And a similarly named is_fixup_flag() only accepts TODO_FIXUP and never TODO_SQUASH. Both of these helpers probably need to be renamed.

sequencer.c static int update_squash_messages(struct repository *r, · line 2143

Quoted 11 lines

sequencer.c +1 -1

index b5c4043757e..3cd0dd3434e 100644
@@ -2139,7 +2139,7 @@ static int update_squash_messages(struct repository *r,
 		strbuf_add_commented_lines(&buf, body, strlen(body),
 					   comment_line_str);
 	} else
-		return error(_("unknown command: %d"), command);
+		BUG(_("unknown command: %d"), command);
 	repo_unuse_commit_buffer(r, commit, message);

BUG() is not end-user facing but programmer facing, and we do not use _("...") in them. I see a few existing violators that need to be corrected.

OK. Or

	if (!is_fixup(command))
		BUG("not a FIXUP or SQUASH %d", command);

at the very beginning of the function?

Phillip Wood

2025-05-29 13:39 UTC #6 lore raw

Re: [PATCH v2] sequencer: replace error() with BUG() in update_squash_messages()

Quoted 18 lines · Junio C Hamano

sequencer.c static int update_squash_messages(struct repository *r, · line 2143
Quoted 11 lines · Lidong Yan via GitGitGadget
sequencer.c +1 -1
index b5c4043757e..3cd0dd3434e 100644
@@ -2139,7 +2139,7 @@ static int update_squash_messages(struct repository *r,
  		strbuf_add_commented_lines(&buf, body, strlen(body),
  					   comment_line_str);
  	} else
-		return error(_("unknown command: %d"), command);
+		BUG(_("unknown command: %d"), command);
  	repo_unuse_commit_buffer(r, commit, message);
BUG() is not end-user facing but programmer facing, and we do not use _("...") in them. I see a few existing violators that need to be corrected.

Quoted 6 lines

OK. Or
	if (!is_fixup(command))
		BUG("not a FIXUP or SQUASH %d", command);
at the very beginning of the function?

Asserting the precondition at the start of the function sounds like a good idea

Best Wishes

Phillip

Junio C Hamano

2025-05-30 04:29 UTC #7 lore raw

Re: [PATCH v2] sequencer: replace error() with BUG() in update_squash_messages()

Quoted 7 lines · Phillip Wood

Quoted 4 lines
OK. Or
	if (!is_fixup(command))
		BUG("not a FIXUP or SQUASH %d", command);
at the very beginning of the function?
Asserting the precondition at the start of the function sounds like a good idea

I am of two minds.

I do not know if this is better for longer-term maintainability or not. Such a message at the beginning of the function declares that the current implementation does not want to receive a command that is not either of these two. Such a message tells the next developer who adds another caller to the function to make sure not to pass other commands, which is good.

But I do not know what it tells to the next developer who wants to add support for another command. After opening that initial gate a bit wider, perhaps to

	if (!is_filxup(command) && !is_my_new_command(command))
		BUT("not a FIXUP or SQUASH or MYCOMMAND %d", command);

would they make sure to handle their new command correctly in the if/elseif cascade, from which we are removing the "error()" with such a change, and would our reviewers notice if they forget to do so? I dunno.

In any case, such a future change will have to be done with a good understanding of the entirety of this small function anyway, so I guess either way is OK.

Thanks.

Phillip Wood

2025-06-02 13:59 UTC #8 lore raw

Re: [PATCH v2] sequencer: replace error() with BUG() in update_squash_messages()

Quoted 23 lines · Junio C Hamano

Quoted 7 lines · Phillip Wood
Quoted 4 lines
OK. Or
	if (!is_fixup(command))
		BUG("not a FIXUP or SQUASH %d", command);
at the very beginning of the function?
Asserting the precondition at the start of the function sounds like a good idea
I am of two minds.
I do not know if this is better for longer-term maintainability or not. Such a message at the beginning of the function declares that the current implementation does not want to receive a command that is not either of these two. Such a message tells the next developer who adds another caller to the function to make sure not to pass other commands, which is good.
But I do not know what it tells to the next developer who wants to add support for another command. After opening that initial gate a bit wider, perhaps to

Quoted 7 lines

	if (!is_filxup(command) && !is_my_new_command(command))
		BUT("not a FIXUP or SQUASH or MYCOMMAND %d", command);
would they make sure to handle their new command correctly in the if/elseif cascade, from which we are removing the "error()" with such a change, and would our reviewers notice if they forget to do so? I dunno.

I guess it cuts both ways. As you say it is a small function and I think someone adding support for a new command would need to do more that just change the precondition to get their code to work unless they're calling this when they don't need to.

Quoted 3 lines

In any case, such a future change will have to be done with a good understanding of the entirety of this small function anyway, so I guess either way is OK.

Agreed

Phillip

Lidong Yan via GitGitGadget

author

2025-05-30 01:52 UTC #9 lore raw

v3 cover sequencer: replace error() with BUG() in update_squash_messages()

In reply to Lidong Yan via GitGitGadget

Lidong Yan (2):
sequencer: replace error() with BUG() in update_squash_messages() BUG(): remove leading underscore of the format string

 builtin/mktag.c     | 2 +-
 builtin/worktree.c  | 2 +-
 pack-bitmap-write.c | 2 +-
 sequencer.c         | 6 ++++--
 4 files changed, 7 insertions(+), 5 deletions(-)

base-commit: fcfe60668e05ffde2610bfef9045797618c145ac
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1964%2Fbrandb97%2Ffix-sequencer-leak-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1964/brandb97/fix-sequencer-leak-v3
Pull-Request: https://github.com/git/git/pull/1964

Range-diff vs v2:

1: aa5ff030b37 ! 1: b812f973d18 sequencer: replace error() with BUG() in update_squash_messages()

     @@ Commit message
      
       ## sequencer.c ##
      @@ sequencer.c: static int update_squash_messages(struct repository *r,
     + 	const char *message, *body;
     + 	const char *encoding = get_commit_output_encoding();
     + 
     ++	if (!is_fixup(command))
     ++		BUG("unknown command: %d", command);
     ++
     + 	if (ctx->current_fixup_count > 0) {
     + 		struct strbuf header = STRBUF_INIT;
     + 		char *eol;
     +@@ sequencer.c: 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
     +-	} else
      -		return error(_("unknown command: %d"), command);
     -+		BUG(_("unknown command: %d"), command);
     ++	}
       	repo_unuse_commit_buffer(r, commit, message);
       
       	if (!res)

-: ----------- > 2: e1f84c111f6 BUG(): remove leading underscore of the format string

Signature · 1 line

gitgitgadget

Lidong Yan via GitGitGadget

author

2025-05-30 01:52 UTC #10 lore raw

PATCH v3 1/2 sequencer: replace error() with BUG() in update_squash_messages()

From: Lidong Yan <[email protected]>

Signed-off-by Lidong Yan <[email protected]>

---
 sequencer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

sequencer.c +4 -2

index 1ee0abbd4514..93e1732504c1 100644
@@ -2067,6 +2067,9 @@ static int update_squash_messages(struct repository *r,
 	const char *message, *body;
 	const char *encoding = get_commit_output_encoding();
 
+	if (!is_fixup(command))
+		BUG("unknown command: %d", command);
+
 	if (ctx->current_fixup_count > 0) {
 		struct strbuf header = STRBUF_INIT;
 		char *eol;

@@ -2134,8 +2137,7 @@ 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)

Signature · 1 line

gitgitgadget

Junio C Hamano

2025-05-30 13:35 UTC #11 lore raw

Re: [PATCH v3 1/2] sequencer: replace error() with BUG() in update_squash_messages()

sequencer.c static int update_squash_messages(struct repository *r, · line 2071

Quoted 6 lines · Lidong Yan via GitGitGadget

sequencer.c +2 -0

@@ -2067,6 +2067,9 @@ static int update_squash_messages(struct repository *r,
 	const char *message, *body;
 	const char *encoding = get_commit_output_encoding();
 
+	if (!is_fixup(command))
+		BUG("unknown command: %d", command);

This is not necessarily unknown. It may be a known one like TODO_PICK but the reason why we are rejecting it is because it is not either FIXUP or SQUASH, so we should say so. BUG() is a message to our developers, so a clear message that tells them that they are not supposed to pass anything but FIXUP/SQUASH is far better than saying "unknown".

Quoted 13 lines

sequencer.c +1 -2

 	if (ctx->current_fixup_count > 0) {
 		struct strbuf header = STRBUF_INIT;
 		char *eol;

@@ -2134,8 +2137,7 @@ 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)

Lidong Yan via GitGitGadget

author

2025-05-30 01:52 UTC #12 lore raw

PATCH v3 2/2 BUG(): remove leading underscore of the format string

In reply to Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

BUG() is not end-user facing but programmer facing, and we do not use _("...") in them. I searched all BUG(_ pattern and replace them with BUG(

Signed-off-by Lidong Yan <[email protected]>

---
 builtin/mktag.c     | 2 +-
 builtin/worktree.c  | 2 +-
 pack-bitmap-write.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

builtin/mktag.c +1 -1

index 7ac11c46d53f..1b1dc0263e18 100644
@@ -41,7 +41,7 @@ static int mktag_fsck_error_func(struct fsck_options *o UNUSED,
 		fprintf_ln(stderr, _("error: tag input does not pass fsck: %s"), message);
 		return 1;
 	default:
-		BUG(_("%d (FSCK_IGNORE?) should never trigger this callback"),
+		BUG("%d (FSCK_IGNORE?) should never trigger this callback",
 		    msg_type);
 	}
 }

builtin/worktree.c +1 -1

index 88a36ea9f867..2dceeeed8bd0 100644
@@ -621,7 +621,7 @@ static void print_preparing_worktree_line(int detach,
 		else {
 			struct commit *commit = lookup_commit_reference_by_name(branch);
 			if (!commit)
-				BUG(_("unreachable: invalid reference: %s"), branch);
+				BUG("unreachable: invalid reference: %s", branch);
 			fprintf_ln(stderr, _("Preparing worktree (detached HEAD %s)"),
 				  repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV));
 		}

pack-bitmap-write.c +1 -1

index 7f400ee01213..56960e6ad760 100644
@@ -1087,7 +1087,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
 					 oid_access);
 
 		if (commit_pos < 0)
-			BUG(_("trying to write commit not in index"));
+			BUG("trying to write commit not in index");
 		stored->commit_pos = commit_pos + base_objects;
 	}

Signature · 1 line

gitgitgadget

Junio C Hamano

2025-05-30 13:15 UTC #13 lore raw

Re: [PATCH v3 2/2] BUG(): remove leading underscore of the format string

Quoted 5 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>
BUG() is not end-user facing but programmer facing, and we do not use _("...") in them. I searched all BUG(_ pattern and replace them with BUG(

The first sentence is good. We'd write the second sentence more like

	Replace all occurrences of BUG(_("..."), ...) with
	BUG("...", ...).

I.e. you instruct somebody sitting in front of the keyboard who is modifying the code for you what to do in imperative mood.

The changes themselves look good to me.

Thanks.

Quoted 45 lines

Signed-off-by Lidong Yan <[email protected]>

---
 builtin/mktag.c     | 2 +-
 builtin/worktree.c  | 2 +-
 pack-bitmap-write.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

builtin/mktag.c +1 -1

index 7ac11c46d53f..1b1dc0263e18 100644
@@ -41,7 +41,7 @@ static int mktag_fsck_error_func(struct fsck_options *o UNUSED,
 		fprintf_ln(stderr, _("error: tag input does not pass fsck: %s"), message);
 		return 1;
 	default:
-		BUG(_("%d (FSCK_IGNORE?) should never trigger this callback"),
+		BUG("%d (FSCK_IGNORE?) should never trigger this callback",
 		    msg_type);
 	}
 }

builtin/worktree.c +1 -1

index 88a36ea9f867..2dceeeed8bd0 100644
@@ -621,7 +621,7 @@ static void print_preparing_worktree_line(int detach,
 		else {
 			struct commit *commit = lookup_commit_reference_by_name(branch);
 			if (!commit)
-				BUG(_("unreachable: invalid reference: %s"), branch);
+				BUG("unreachable: invalid reference: %s", branch);
 			fprintf_ln(stderr, _("Preparing worktree (detached HEAD %s)"),
 				  repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV));
 		}

pack-bitmap-write.c +1 -1

index 7f400ee01213..56960e6ad760 100644
@@ -1087,7 +1087,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
 					 oid_access);
 
 		if (commit_pos < 0)
-			BUG(_("trying to write commit not in index"));
+			BUG("trying to write commit not in index");
 		stored->commit_pos = commit_pos + base_objects;
 	}

Lidong Yan via GitGitGadget

author

2025-06-03 02:01 UTC #14 lore raw

v4 cover sequencer: replace error() with BUG() in update_squash_messages()

In reply to Lidong Yan via GitGitGadget

Lidong Yan (2):
sequencer: replace error() with BUG() in update_squash_messages() BUG(): remove leading underscore of the format string

 builtin/mktag.c     | 2 +-
 builtin/worktree.c  | 2 +-
 pack-bitmap-write.c | 2 +-
 sequencer.c         | 6 ++++--
 4 files changed, 7 insertions(+), 5 deletions(-)

base-commit: fcfe60668e05ffde2610bfef9045797618c145ac
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1964%2Fbrandb97%2Ffix-sequencer-leak-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1964/brandb97/fix-sequencer-leak-v4
Pull-Request: https://github.com/git/git/pull/1964

Range-diff vs v3:

1: b812f973d18 ! 1: f2d2cfd6a87 sequencer: replace error() with BUG() in update_squash_messages()
@@ Commit message

In sequencer.c, caller only pass TODO_SQUASH or TODO_FIXUP to update_squash_messages(), any other command passed in should be
- considered as BUG. Thus I think return error('unknown command')
- should be replaced as BUG('unknown command').
+ considered as BUG. Replace return error('unknown command')
+ with BUG('not a FIXUP or SQUASH').

Signed-off-by Lidong Yan <[email protected]>

@@ sequencer.c: static int update_squash_messages(struct repository *r,
const char *encoding = get_commit_output_encoding();

+ if (!is_fixup(command))
-+ BUG("unknown command: %d", command);
++ BUG("not a FIXUP or SQUASH %d", command);
+
if (ctx->current_fixup_count > 0) {
struct strbuf header = STRBUF_INIT;
2: e1f84c111f6 ! 2: 9d69c19273b BUG(): remove leading underscore of the format string
@@ Commit message
BUG(): remove leading underscore of the format string

BUG() is not end-user facing but programmer facing, and we do not
- use _("...") in them. I searched all BUG(_ pattern and replace
- them with BUG(
+ use _("...") in them. Replace all BUG(_("...")) with BUG("...")

Signed-off-by Lidong Yan <[email protected]>

Signature · 1 line

gitgitgadget

Lidong Yan via GitGitGadget

author

2025-06-03 02:01 UTC #15 lore raw

PATCH v4 1/2 sequencer: replace error() with BUG() in update_squash_messages()

From: Lidong Yan <[email protected]>

In sequencer.c, caller only pass TODO_SQUASH or TODO_FIXUP to update_squash_messages(), any other command passed in should be considered as BUG. Replace return error('unknown command') with BUG('not a FIXUP or SQUASH').

Signed-off-by Lidong Yan <[email protected]>

---
 sequencer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

sequencer.c +4 -2

index 1ee0abbd4514..9456ca6ee97d 100644
@@ -2067,6 +2067,9 @@ static int update_squash_messages(struct repository *r,
 	const char *message, *body;
 	const char *encoding = get_commit_output_encoding();
 
+	if (!is_fixup(command))
+		BUG("not a FIXUP or SQUASH %d", command);
+
 	if (ctx->current_fixup_count > 0) {
 		struct strbuf header = STRBUF_INIT;
 		char *eol;

@@ -2134,8 +2137,7 @@ 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)

Signature · 1 line

gitgitgadget

Lidong Yan via GitGitGadget

author

2025-06-03 02:01 UTC #16 lore raw

PATCH v4 2/2 BUG(): remove leading underscore of the format string

In reply to Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

BUG() is not end-user facing but programmer facing, and we do not use _("...") in them. Replace all BUG(_("...")) with BUG("...")

Signed-off-by Lidong Yan <[email protected]>

---
 builtin/mktag.c     | 2 +-
 builtin/worktree.c  | 2 +-
 pack-bitmap-write.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

builtin/mktag.c +1 -1

index 7ac11c46d53f..1b1dc0263e18 100644
@@ -41,7 +41,7 @@ static int mktag_fsck_error_func(struct fsck_options *o UNUSED,
 		fprintf_ln(stderr, _("error: tag input does not pass fsck: %s"), message);
 		return 1;
 	default:
-		BUG(_("%d (FSCK_IGNORE?) should never trigger this callback"),
+		BUG("%d (FSCK_IGNORE?) should never trigger this callback",
 		    msg_type);
 	}
 }

builtin/worktree.c +1 -1

index 88a36ea9f867..2dceeeed8bd0 100644
@@ -621,7 +621,7 @@ static void print_preparing_worktree_line(int detach,
 		else {
 			struct commit *commit = lookup_commit_reference_by_name(branch);
 			if (!commit)
-				BUG(_("unreachable: invalid reference: %s"), branch);
+				BUG("unreachable: invalid reference: %s", branch);
 			fprintf_ln(stderr, _("Preparing worktree (detached HEAD %s)"),
 				  repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV));
 		}

pack-bitmap-write.c +1 -1

index 7f400ee01213..56960e6ad760 100644
@@ -1087,7 +1087,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
 					 oid_access);
 
 		if (commit_pos < 0)
-			BUG(_("trying to write commit not in index"));
+			BUG("trying to write commit not in index");
 		stored->commit_pos = commit_pos + base_objects;
 	}

Signature · 1 line

gitgitgadget

Junio C Hamano

2025-06-03 17:40 UTC #17 lore raw

Re: [PATCH v4 2/2] BUG(): remove leading underscore of the format string

Quoted 11 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>
BUG() is not end-user facing but programmer facing, and we do not use _("...") in them. Replace all BUG(_("...")) with BUG("...")
Signed-off-by Lidong Yan <[email protected]>
---
 builtin/mktag.c     | 2 +-
 builtin/worktree.c  | 2 +-
 pack-bitmap-write.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

This is unrelated to the sequencer code clarification change, so I'll queue [1/2] and [2/2] as separate topics.

Thanks.

Quoted 38 lines

builtin/mktag.c +1 -1

index 7ac11c46d53f..1b1dc0263e18 100644
@@ -41,7 +41,7 @@ static int mktag_fsck_error_func(struct fsck_options *o UNUSED,
 		fprintf_ln(stderr, _("error: tag input does not pass fsck: %s"), message);
 		return 1;
 	default:
-		BUG(_("%d (FSCK_IGNORE?) should never trigger this callback"),
+		BUG("%d (FSCK_IGNORE?) should never trigger this callback",
 		    msg_type);
 	}
 }

builtin/worktree.c +1 -1

index 88a36ea9f867..2dceeeed8bd0 100644
@@ -621,7 +621,7 @@ static void print_preparing_worktree_line(int detach,
 		else {
 			struct commit *commit = lookup_commit_reference_by_name(branch);
 			if (!commit)
-				BUG(_("unreachable: invalid reference: %s"), branch);
+				BUG("unreachable: invalid reference: %s", branch);
 			fprintf_ln(stderr, _("Preparing worktree (detached HEAD %s)"),
 				  repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV));
 		}

pack-bitmap-write.c +1 -1

index 7f400ee01213..56960e6ad760 100644
@@ -1087,7 +1087,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
 					 oid_access);
 
 		if (commit_pos < 0)
-			BUG(_("trying to write commit not in index"));
+			BUG("trying to write commit not in index");
 		stored->commit_pos = commit_pos + base_objects;
 	}