Re: [PATCH] builtin/history: unuse the commit buffer after use

[Patrick Steinhardt <ps@pks.im>]

On Sun, Jun 14, 2026 at 02:15:40PM +0000, Kaartic Sivaraam wrote:
> While running `git history reword` using a Git built with `SANITIZE` flag set
> to `address,leak`, we could observe the following leak being reported:

Huh, curious. That seems to hint that we're missing test coverage for
this specific scenario, as our test suite doesn't detect this leak.

[snip]
> A deeper investigation on this reveals the following as the root cause.
> 
> As part of rewording a commit in `git history`, we get the commit message
> buffer in the `commit_tree_ext` function. This in turn obtains the buffer
> from `repo_logmsg_reencode`. Given how `commit_tree_ext` is invoking the
> function with the last two parameters as NULL, we are clearly not expecting
> a reencode to happen. In this case, the buffer that we receive from
> `repo_logmsg_reencode` ends up always being obtained from a call to
> `repo_get_commit_buffer`.
> 
> This buffer is expected to be released with an accompanying call to
> `repo_unuse_commit_buffer` which takes care of freeing it. This call
> is missing in the `commit_tree_ext` flow thus resulting in the leak.

So this doesn't really read specific at all, and I would have expected
us to hit this leak. Puzzling.

> Fix this by ensuring we call `repo_unuse_commit_buffer` on the
> original_message buffer.
> 
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> ---
> I must mention that I also noticed the following comment in `commit_tree_ext`:
> 
> »       /* We retain authorship of the original commit. */
> »       original_message = repo_logmsg_reencode(repo, commit_with_message, NULL, NULL);
> 
> ... but I'm not quite sure why we don't unuse the buffer after its purpose is
> done. Kindly englighten me in case I missed something.

Did you maybe confuse "authorship" with "ownership" while reading the
comment? The comment only mentions that we retain the original "Author"
commit metadata, it doesn't refer to ownership of the underlying
objects.

> diff --git a/builtin/history.c b/builtin/history.c
> index 091465a59e..0e9259b5d7 100644
> --- a/builtin/history.c
> +++ b/builtin/history.c
> @@ -154,6 +154,7 @@ static int commit_tree_ext(struct repository *repo,
>  	free_commit_extra_headers(original_extra_headers);
>  	strbuf_release(&commit_message);
>  	free(original_author);
> +	repo_unuse_commit_buffer(repo, commit_with_message, original_message);
>  	return ret;
>  }

Yup, this makes sense to me.

Thanks!

Patrick