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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Jun 15, 2026 at 01:29:46PM -0400, Jeff King wrote:
> On Mon, Jun 15, 2026 at 11:48:10AM +0200, Patrick Steinhardt wrote:
> 
> > 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.
> 
> I think it will only leak when the commit object has an "encoding"
> header. See below.
> 
> > > 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.
> 
> The first paragraph is accurate here. We'd generally just get a pointer
> to the buffer cached in the slab, because no re-encoding occurs. And in
> that case you _don't_ need to call unuse_commit_buffer(), because you
> have a read-only copy, and the slab cache will hold it forever[1].
> Calling the unuse function will be a noop.
> 
> But when we _do_ re-encode, then you get a new buffer which must be
> freed. And that is when you have to call the unuse function. And the
> reason it is "unuse" and not just "free" is that you don't necessarily
> know which you have, but that function figures it out (and frees it only
> if necessary).
> 
> So what the patch is doing is correct, but the explanation is a little
> confused. We see the leak only when re-encoding, so we'd probably want a
> test case that triggers that. Which I assume implies rewriting a commit
> that was previously generated with an encoding header.
> 
> Now back to that [1] note. Even if we didn't re-encode, we'll still hold
> onto that buffer forever. It's not a "leak" in the traditional sense
> because it's still referenced in the commit slab cache. But if you are
> going to walk over a million commits (like git-log does), you probably
> don't want to hold a million commit messages in memory at once.
> 
> For that you'd want to call free_commit_buffer() when you know you're
> totally done with it (again, like git-log does after it finishes showing
> the commit). That might be the case here in commit_tree_ext(), or it
> might happen later (I'm not familiar with the git-history code).
> 
> But note that you need to do _both_ the unuse and free calls. If we did
> re-encode, the former is needed to free the newly allocated buffer. The
> latter only drops the original buffer in the cache.

Thanks for the explanation!

Patrick