threads / patch / 63456

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

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

## tl;dr

18 messages between May 14, 2025 and Jun 3, 2025. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

Lidong Yan via GitGitGadget· May 14, 2025, 12:44 UTC · lore
From: Lidong Yan <502024330056@smail.nju.edu.cn>

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 <502024330056@smail.nju.edu.cn>
---
    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(-)
Show changes to sequencer.c +3 −1
diff --git a/sequencer.c b/sequencer.c
index b5c4043757e..f288a303eaa 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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

-- 
gitgitgadget
Phillip Wood· May 15, 2025, 10:08 UTC · re: Lidong Yan via GitGitGadget · lore

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

On 14/05/2025 13:44, Lidong Yan via GitGitGadget wrote:
Show 8 quoted lines
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
> 
> 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
Show 37 quoted lines
> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
> ---
>      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(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index b5c4043757e..f288a303eaa 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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· May 15, 2025, 16:20 UTC · re: Phillip Wood · lore

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

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.

Show changes to diff +3 −2
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)
lidongyan· May 15, 2025, 16:43 UTC · re: Junio C Hamano · lore

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

2025年5月16日 00:20，Junio C Hamano <gitster@pobox.com> 写道：
Show 29 quoted lines
> 
> 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)

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· May 25, 2025, 10:19 UTC · re: Lidong Yan via GitGitGadget · lore

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

From: Lidong Yan <502024330056@smail.nju.edu.cn>

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 <502024330056@smail.nju.edu.cn>
---
    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 <502024330056@smail.nju.edu.cn>
      
       ## 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 <502024330056@smail.nju.edu.cn>
      
       ## 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(-)
Show changes to sequencer.c +1 −1
diff --git a/sequencer.c b/sequencer.c
index b5c4043757e..3cd0dd3434e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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

-- 
gitgitgadget
Junio C Hamano· May 28, 2025, 22:16 UTC · re: Lidong Yan via GitGitGadget · lore

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

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> 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.
Show 11 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> index b5c4043757e..3cd0dd3434e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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· May 29, 2025, 13:39 UTC · re: Junio C Hamano · lore

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

On 28/05/2025 23:16, Junio C Hamano wrote:
Show 24 quoted lines
> "Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index b5c4043757e..3cd0dd3434e 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -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?

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

Best Wishes
Phillip
Junio C Hamano· May 30, 2025, 04:29 UTC · re: Phillip Wood · lore

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

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 7 quoted 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· Jun 2, 2025, 13:59 UTC · re: Junio C Hamano · lore

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

On 30/05/2025 05:29, Junio C Hamano wrote:
Show 30 quoted lines
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> 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.

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.

> 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· May 30, 2025, 01:52 UTC · re: Lidong Yan via GitGitGadget · lore

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

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
-- 
gitgitgadget
Lidong Yan via GitGitGadget· May 30, 2025, 01:52 UTC · re: Lidong Yan via GitGitGadget · lore

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

From: Lidong Yan <502024330056@smail.nju.edu.cn>

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 <502024330056@smail.nju.edu.cn>
---
 sequencer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Show changes to sequencer.c +4 −2
diff --git a/sequencer.c b/sequencer.c
index 1ee0abbd4514..93e1732504c1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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)

-- 
gitgitgadget
Junio C Hamano· May 30, 2025, 13:35 UTC · re: Lidong Yan via GitGitGadget · lore

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

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> @@ -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".

Show 13 quoted lines
>  	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· May 30, 2025, 01:52 UTC · re: Lidong Yan via GitGitGadget · lore

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

From: Lidong Yan <502024330056@smail.nju.edu.cn>

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 <502024330056@smail.nju.edu.cn>
---
 builtin/mktag.c     | 2 +-
 builtin/worktree.c  | 2 +-
 pack-bitmap-write.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 3 files +3 −3

builtin/mktag.c, builtin/worktree.c, pack-bitmap-write.c

diff --git a/builtin/mktag.c b/builtin/mktag.c
index 7ac11c46d53f..1b1dc0263e18 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -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);
 	}
 }
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 88a36ea9f867..2dceeeed8bd0 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -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));
 		}
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 7f400ee01213..56960e6ad760 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -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;
 	}
 

-- 
gitgitgadget
Junio C Hamano· May 30, 2025, 13:15 UTC · re: Lidong Yan via GitGitGadget · lore

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

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> 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.
Show 45 quoted lines
> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
> ---
>  builtin/mktag.c     | 2 +-
>  builtin/worktree.c  | 2 +-
>  pack-bitmap-write.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/mktag.c b/builtin/mktag.c
> index 7ac11c46d53f..1b1dc0263e18 100644
> --- a/builtin/mktag.c
> +++ b/builtin/mktag.c
> @@ -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);
>  	}
>  }
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 88a36ea9f867..2dceeeed8bd0 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -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));
>  		}
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 7f400ee01213..56960e6ad760 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -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· Jun 3, 2025, 02:01 UTC · re: Lidong Yan via GitGitGadget · lore

[PATCH v4 0/2] sequencer: replace error() with BUG() in update_squash_messages()

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 <502024330056@smail.nju.edu.cn>
      
     @@ 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 <502024330056@smail.nju.edu.cn>
      
-- 
gitgitgadget
Lidong Yan via GitGitGadget· Jun 3, 2025, 02:01 UTC · re: Lidong Yan via GitGitGadget · lore

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

From: Lidong Yan <502024330056@smail.nju.edu.cn>

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 <502024330056@smail.nju.edu.cn>
---
 sequencer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Show changes to sequencer.c +4 −2
diff --git a/sequencer.c b/sequencer.c
index 1ee0abbd4514..9456ca6ee97d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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)

-- 
gitgitgadget
Lidong Yan via GitGitGadget· Jun 3, 2025, 02:01 UTC · re: Lidong Yan via GitGitGadget · lore

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

From: Lidong Yan <502024330056@smail.nju.edu.cn>

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 <502024330056@smail.nju.edu.cn>
---
 builtin/mktag.c     | 2 +-
 builtin/worktree.c  | 2 +-
 pack-bitmap-write.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 3 files +3 −3

builtin/mktag.c, builtin/worktree.c, pack-bitmap-write.c

diff --git a/builtin/mktag.c b/builtin/mktag.c
index 7ac11c46d53f..1b1dc0263e18 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -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);
 	}
 }
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 88a36ea9f867..2dceeeed8bd0 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -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));
 		}
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 7f400ee01213..56960e6ad760 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -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;
 	}
 

-- 
gitgitgadget
Junio C Hamano· Jun 3, 2025, 17:40 UTC · re: Lidong Yan via GitGitGadget · lore

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

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 11 quoted lines
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> 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 <502024330056@smail.nju.edu.cn>
> ---
>  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.
Show 38 quoted lines
> diff --git a/builtin/mktag.c b/builtin/mktag.c
> index 7ac11c46d53f..1b1dc0263e18 100644
> --- a/builtin/mktag.c
> +++ b/builtin/mktag.c
> @@ -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);
>  	}
>  }
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 88a36ea9f867..2dceeeed8bd0 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -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));
>  		}
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 7f400ee01213..56960e6ad760 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -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;
>  	}

← back to recent threads