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

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

[Lidong Yan via GitGitGadget]

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

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

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

[Phillip Wood]

On 14/05/2025 13:44, Lidong Yan via GitGitGadget wrote:
> 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

> 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

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

[Junio C Hamano]

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

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

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

[lidongyan]

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

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

[Lidong Yan via GitGitGadget]

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

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

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

[Junio C Hamano]

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

> 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?

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

[Phillip Wood]

On 28/05/2025 23:16, Junio C Hamano wrote:
> "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

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

[Junio C Hamano]

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.

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.

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

[Phillip Wood]

On 30/05/2025 05:29, Junio C Hamano wrote:
> 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

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

[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

-- 
gitgitgadget

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

[Lidong Yan via GitGitGadget]

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

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

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

[Junio C Hamano]

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -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".

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

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

[Lidong Yan via GitGitGadget]

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

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

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

[Junio C Hamano]

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

> 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;
>  	}

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

[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 <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

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

[Lidong Yan via GitGitGadget]

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

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

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

[Lidong Yan via GitGitGadget]

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

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

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

[Junio C Hamano]

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

> 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;
>  	}