{"thread":{"id":"65808","subject":"[PATCH] builtin/history: unuse the commit buffer after use","startedAt":"2026-06-14T14:16:18Z","lastAt":"2026-09-15T08:29:54Z","messageCount":17,"participants":["Kaartic Sivaraam","Patrick Steinhardt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"545488","messageId":"20260614141600.620272-1-kaartic.sivaraam@gmail.com","threadId":"65808","inReplyTo":null,"subject":"[PATCH] builtin/history: unuse the commit buffer after use","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2026-06-14T14:15:40Z","receivedAt":"2026-06-14T14:16:18Z","isPatch":true,"body":"While running `git history reword` using a Git built with `SANITIZE` flag set\nto `address,leak`, we could observe the following leak being reported:\n\n-- 8< --\n\n=================================================================\n==7156==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 1813 byte(s) in 1 object(s) allocated from:\n    #0 0x79a3d1d2b60f in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:67\n    #1 0x612e2bb8c2e9 in do_xmalloc /path/to/git/wrapper.c:55\n    #2 0x612e2bb8c3f7 in do_xmallocz /path/to/git/wrapper.c:89\n    #3 0x612e2bb8c48f in xmallocz_gently /path/to/git/wrapper.c:102\n    #4 0x612e2b8dc28e in unpack_compressed_entry /path/to/git/packfile.c:1744\n    #5 0x612e2b8dd12a in unpack_entry /path/to/git/packfile.c:1897\n    #6 0x612e2b8daae2 in cache_or_unpack_entry /path/to/git/packfile.c:1535\n    #7 0x612e2b8db1f6 in packed_object_info_with_index_pos /path/to/git/packfile.c:1617\n    #8 0x612e2b8dc1c4 in packed_object_info /path/to/git/packfile.c:1732\n    #9 0x612e2b8def05 in packfile_store_read_object_info /path/to/git/packfile.c:2228\n    #10 0x612e2b889cb0 in odb_source_files_read_object_info odb/source-files.c:58\n    #11 0x612e2b8805e9 in odb_source_read_object_info odb/source.h:326\n    #12 0x612e2b885cf0 in do_oid_object_info_extended /path/to/git/odb.c:572\n    #13 0x612e2b886fe4 in odb_read_object_info_extended /path/to/git/odb.c:710\n    #14 0x612e2b887584 in odb_read_object /path/to/git/odb.c:756\n    #15 0x612e2b68d6e4 in repo_get_commit_buffer /path/to/git/commit.c:399\n    #16 0x612e2b91a7d4 in repo_logmsg_reencode /path/to/git/pretty.c:716\n    #17 0x612e2b3de7da in commit_tree_ext builtin/history.c:127\n    #18 0x612e2b3dee9f in commit_tree_with_edited_message builtin/history.c:183\n    #19 0x612e2b3e2c4d in cmd_history_reword builtin/history.c:717\n    #20 0x612e2b3e53b6 in cmd_history builtin/history.c:998\n    #21 0x612e2b27ae97 in run_builtin /path/to/git/git.c:506\n    #22 0x612e2b27b9ae in handle_builtin /path/to/git/git.c:782\n    #23 0x612e2b27c240 in run_argv /path/to/git/git.c:865\n    #24 0x612e2b27cd94 in cmd_main /path/to/git/git.c:986\n    #25 0x612e2b5c4267 in main /path/to/git/common-main.c:9\n    #26 0x79a3d182a600 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:59\n    #27 0x79a3d182a717 in __libc_start_main_impl ../csu/libc-start.c:360\n    #28 0x612e2b276124 in _start (/path/to.local/bin/git+0x211124) (BuildId: 8da3d640a944e21b895fc4802d7942b1505be663)\n\nSUMMARY: AddressSanitizer: 1813 byte(s) leaked in 1 allocation(s).\n\n-- >8 --\n\nA deeper investigation on this reveals the following as the root cause.\n\nAs part of rewording a commit in `git history`, we get the commit message\nbuffer in the `commit_tree_ext` function. This in turn obtains the buffer\nfrom `repo_logmsg_reencode`. Given how `commit_tree_ext` is invoking the\nfunction with the last two parameters as NULL, we are clearly not expecting\na reencode to happen. In this case, the buffer that we receive from\n`repo_logmsg_reencode` ends up always being obtained from a call to\n`repo_get_commit_buffer`.\n\nThis buffer is expected to be released with an accompanying call to\n`repo_unuse_commit_buffer` which takes care of freeing it. This call\nis missing in the `commit_tree_ext` flow thus resulting in the leak.\n\nFix this by ensuring we call `repo_unuse_commit_buffer` on the\noriginal_message buffer.\n\nSigned-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n---\nI must mention that I also noticed the following comment in `commit_tree_ext`:\n\n»       /* We retain authorship of the original commit. */\n»       original_message = repo_logmsg_reencode(repo, commit_with_message, NULL, NULL);\n\n... but I'm not quite sure why we don't unuse the buffer after its purpose is\ndone. Kindly englighten me in case I missed something.\n\n\n builtin/history.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/history.c b/builtin/history.c\nindex 091465a59e..0e9259b5d7 100644\n--- a/builtin/history.c\n+++ b/builtin/history.c\n@@ -154,6 +154,7 @@ static int commit_tree_ext(struct repository *repo,\n \tfree_commit_extra_headers(original_extra_headers);\n \tstrbuf_release(&commit_message);\n \tfree(original_author);\n+\trepo_unuse_commit_buffer(repo, commit_with_message, original_message);\n \treturn ret;\n }\n \n-- \n2.55.0.rc0.738.g0c8ab3ebcc.dirty\n\n"},{"id":"545539","messageId":"ai_KWo9o1Fhc6OFs@pks.im","threadId":"65808","inReplyTo":"20260614141600.620272-1-kaartic.sivaraam@gmail.com","subject":"Re: [PATCH] builtin/history: unuse the commit buffer after use","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-15T09:48:10Z","receivedAt":"2026-06-15T09:48:17Z","isPatch":true,"body":"On Sun, Jun 14, 2026 at 02:15:40PM +0000, Kaartic Sivaraam wrote:\n> While running `git history reword` using a Git built with `SANITIZE` flag set\n> to `address,leak`, we could observe the following leak being reported:\n\nHuh, curious. That seems to hint that we're missing test coverage for\nthis specific scenario, as our test suite doesn't detect this leak.\n\n[snip]\n> A deeper investigation on this reveals the following as the root cause.\n> \n> As part of rewording a commit in `git history`, we get the commit message\n> buffer in the `commit_tree_ext` function. This in turn obtains the buffer\n> from `repo_logmsg_reencode`. Given how `commit_tree_ext` is invoking the\n> function with the last two parameters as NULL, we are clearly not expecting\n> a reencode to happen. In this case, the buffer that we receive from\n> `repo_logmsg_reencode` ends up always being obtained from a call to\n> `repo_get_commit_buffer`.\n> \n> This buffer is expected to be released with an accompanying call to\n> `repo_unuse_commit_buffer` which takes care of freeing it. This call\n> is missing in the `commit_tree_ext` flow thus resulting in the leak.\n\nSo this doesn't really read specific at all, and I would have expected\nus to hit this leak. Puzzling.\n\n> Fix this by ensuring we call `repo_unuse_commit_buffer` on the\n> original_message buffer.\n> \n> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> ---\n> I must mention that I also noticed the following comment in `commit_tree_ext`:\n> \n> »       /* We retain authorship of the original commit. */\n> »       original_message = repo_logmsg_reencode(repo, commit_with_message, NULL, NULL);\n> \n> ... but I'm not quite sure why we don't unuse the buffer after its purpose is\n> done. Kindly englighten me in case I missed something.\n\nDid you maybe confuse \"authorship\" with \"ownership\" while reading the\ncomment? The comment only mentions that we retain the original \"Author\"\ncommit metadata, it doesn't refer to ownership of the underlying\nobjects.\n\n> diff --git a/builtin/history.c b/builtin/history.c\n> index 091465a59e..0e9259b5d7 100644\n> --- a/builtin/history.c\n> +++ b/builtin/history.c\n> @@ -154,6 +154,7 @@ static int commit_tree_ext(struct repository *repo,\n>  \tfree_commit_extra_headers(original_extra_headers);\n>  \tstrbuf_release(&commit_message);\n>  \tfree(original_author);\n> +\trepo_unuse_commit_buffer(repo, commit_with_message, original_message);\n>  \treturn ret;\n>  }\n\nYup, this makes sense to me.\n\nThanks!\n\nPatrick\n"},{"id":"545605","messageId":"20260615172946.GD91269@coredump.intra.peff.net","threadId":"65808","inReplyTo":"ai_KWo9o1Fhc6OFs@pks.im","subject":"Re: [PATCH] builtin/history: unuse the commit buffer after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-15T17:29:46Z","receivedAt":"2026-06-15T17:29:48Z","isPatch":true,"body":"On Mon, Jun 15, 2026 at 11:48:10AM +0200, Patrick Steinhardt wrote:\n\n> On Sun, Jun 14, 2026 at 02:15:40PM +0000, Kaartic Sivaraam wrote:\n> > While running `git history reword` using a Git built with `SANITIZE` flag set\n> > to `address,leak`, we could observe the following leak being reported:\n> \n> Huh, curious. That seems to hint that we're missing test coverage for\n> this specific scenario, as our test suite doesn't detect this leak.\n\nI think it will only leak when the commit object has an \"encoding\"\nheader. See below.\n\n> > As part of rewording a commit in `git history`, we get the commit message\n> > buffer in the `commit_tree_ext` function. This in turn obtains the buffer\n> > from `repo_logmsg_reencode`. Given how `commit_tree_ext` is invoking the\n> > function with the last two parameters as NULL, we are clearly not expecting\n> > a reencode to happen. In this case, the buffer that we receive from\n> > `repo_logmsg_reencode` ends up always being obtained from a call to\n> > `repo_get_commit_buffer`.\n> > \n> > This buffer is expected to be released with an accompanying call to\n> > `repo_unuse_commit_buffer` which takes care of freeing it. This call\n> > is missing in the `commit_tree_ext` flow thus resulting in the leak.\n> \n> So this doesn't really read specific at all, and I would have expected\n> us to hit this leak. Puzzling.\n\nThe first paragraph is accurate here. We'd generally just get a pointer\nto the buffer cached in the slab, because no re-encoding occurs. And in\nthat case you _don't_ need to call unuse_commit_buffer(), because you\nhave a read-only copy, and the slab cache will hold it forever[1].\nCalling the unuse function will be a noop.\n\nBut when we _do_ re-encode, then you get a new buffer which must be\nfreed. And that is when you have to call the unuse function. And the\nreason it is \"unuse\" and not just \"free\" is that you don't necessarily\nknow which you have, but that function figures it out (and frees it only\nif necessary).\n\nSo what the patch is doing is correct, but the explanation is a little\nconfused. We see the leak only when re-encoding, so we'd probably want a\ntest case that triggers that. Which I assume implies rewriting a commit\nthat was previously generated with an encoding header.\n\nNow back to that [1] note. Even if we didn't re-encode, we'll still hold\nonto that buffer forever. It's not a \"leak\" in the traditional sense\nbecause it's still referenced in the commit slab cache. But if you are\ngoing to walk over a million commits (like git-log does), you probably\ndon't want to hold a million commit messages in memory at once.\n\nFor that you'd want to call free_commit_buffer() when you know you're\ntotally done with it (again, like git-log does after it finishes showing\nthe commit). That might be the case here in commit_tree_ext(), or it\nmight happen later (I'm not familiar with the git-history code).\n\nBut note that you need to do _both_ the unuse and free calls. If we did\nre-encode, the former is needed to free the newly allocated buffer. The\nlatter only drops the original buffer in the cache.\n\n-Peff\n"},{"id":"545625","messageId":"ajDjB67HvbdgjYSa@pks.im","threadId":"65808","inReplyTo":"20260615172946.GD91269@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/history: unuse the commit buffer after use","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-16T05:45:43Z","receivedAt":"2026-06-16T05:45:51Z","isPatch":true,"body":"On Mon, Jun 15, 2026 at 01:29:46PM -0400, Jeff King wrote:\n> On Mon, Jun 15, 2026 at 11:48:10AM +0200, Patrick Steinhardt wrote:\n> \n> > On Sun, Jun 14, 2026 at 02:15:40PM +0000, Kaartic Sivaraam wrote:\n> > > While running `git history reword` using a Git built with `SANITIZE` flag set\n> > > to `address,leak`, we could observe the following leak being reported:\n> > \n> > Huh, curious. That seems to hint that we're missing test coverage for\n> > this specific scenario, as our test suite doesn't detect this leak.\n> \n> I think it will only leak when the commit object has an \"encoding\"\n> header. See below.\n> \n> > > As part of rewording a commit in `git history`, we get the commit message\n> > > buffer in the `commit_tree_ext` function. This in turn obtains the buffer\n> > > from `repo_logmsg_reencode`. Given how `commit_tree_ext` is invoking the\n> > > function with the last two parameters as NULL, we are clearly not expecting\n> > > a reencode to happen. In this case, the buffer that we receive from\n> > > `repo_logmsg_reencode` ends up always being obtained from a call to\n> > > `repo_get_commit_buffer`.\n> > > \n> > > This buffer is expected to be released with an accompanying call to\n> > > `repo_unuse_commit_buffer` which takes care of freeing it. This call\n> > > is missing in the `commit_tree_ext` flow thus resulting in the leak.\n> > \n> > So this doesn't really read specific at all, and I would have expected\n> > us to hit this leak. Puzzling.\n> \n> The first paragraph is accurate here. We'd generally just get a pointer\n> to the buffer cached in the slab, because no re-encoding occurs. And in\n> that case you _don't_ need to call unuse_commit_buffer(), because you\n> have a read-only copy, and the slab cache will hold it forever[1].\n> Calling the unuse function will be a noop.\n> \n> But when we _do_ re-encode, then you get a new buffer which must be\n> freed. And that is when you have to call the unuse function. And the\n> reason it is \"unuse\" and not just \"free\" is that you don't necessarily\n> know which you have, but that function figures it out (and frees it only\n> if necessary).\n> \n> So what the patch is doing is correct, but the explanation is a little\n> confused. We see the leak only when re-encoding, so we'd probably want a\n> test case that triggers that. Which I assume implies rewriting a commit\n> that was previously generated with an encoding header.\n> \n> Now back to that [1] note. Even if we didn't re-encode, we'll still hold\n> onto that buffer forever. It's not a \"leak\" in the traditional sense\n> because it's still referenced in the commit slab cache. But if you are\n> going to walk over a million commits (like git-log does), you probably\n> don't want to hold a million commit messages in memory at once.\n> \n> For that you'd want to call free_commit_buffer() when you know you're\n> totally done with it (again, like git-log does after it finishes showing\n> the commit). That might be the case here in commit_tree_ext(), or it\n> might happen later (I'm not familiar with the git-history code).\n> \n> But note that you need to do _both_ the unuse and free calls. If we did\n> re-encode, the former is needed to free the newly allocated buffer. The\n> latter only drops the original buffer in the cache.\n\nThanks for the explanation!\n\nPatrick\n"},{"id":"546725","messageId":"94b0bed5-c86a-4291-b958-52f09faebd29@gmail.com","threadId":"65808","inReplyTo":"ai_KWo9o1Fhc6OFs@pks.im","subject":"Re: [PATCH] builtin/history: unuse the commit buffer after use","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2026-06-30T03:43:28Z","receivedAt":"2026-06-30T03:43:34Z","isPatch":true,"body":"Hi Patrick,\n\nOn 15/06/26 15:18, Patrick Steinhardt wrote:\n> Huh, curious. That seems to hint that we're missing test coverage for\n> this specific scenario, as our test suite doesn't detect this leak.\n>\n\nIndeed. The tricky thing is (as mentioned in another thread), this is \nhappening only when we get a commit not cached in the commit slab. Once \nwe get an idea on how certain commits get cached in the commit slab \nwhile others don't, we can write a test case that would catch this leak.\n\n> \n> So this doesn't really read specific at all, and I would have expected\n> us to hit this leak. Puzzling.\n>\n\nYeah. My bad with the commit message. The leak is not happening always. \nThat is a reason we may not have caught this in the test suite.\n\n>> I must mention that I also noticed the following comment in `commit_tree_ext`:\n>>\n>> »       /* We retain authorship of the original commit. */\n>> »       original_message = repo_logmsg_reencode(repo, commit_with_message, NULL, NULL);\n>>\n>> ... but I'm not quite sure why we don't unuse the buffer after its purpose is\n>> done. Kindly englighten me in case I missed something.\n> \n> Did you maybe confuse \"authorship\" with \"ownership\" while reading the\n> comment? The comment only mentions that we retain the original \"Author\"\n> commit metadata, it doesn't refer to ownership of the underlying\n> objects.\n>\n\nGot it. Thank you for the correction!\n\n--\nSivaraam\n\nPS: Sorry about the delay in response. Was stuck with some personal work.\n\n"},{"id":"546726","messageId":"317d0f7b-469f-4456-8808-506e17de264d@gmail.com","threadId":"65808","inReplyTo":"20260615172946.GD91269@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/history: unuse the commit buffer after use","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2026-06-30T03:45:49Z","receivedAt":"2026-06-30T03:45:53Z","isPatch":true,"body":"Hi Peff,\n\nOn 15/06/26 22:59, Jeff King wrote:\n> On Mon, Jun 15, 2026 at 11:48:10AM +0200, Patrick Steinhardt wrote:\n>> Huh, curious. That seems to hint that we're missing test coverage for\n>> this specific scenario, as our test suite doesn't detect this leak.\n> \n> I think it will only leak when the commit object has an \"encoding\"\n> header. See below.\n> \n\nI'm quite sure this is not about the commit with the encoding header. \nMore below.\n\n> The first paragraph is accurate here. We'd generally just get a pointer\n> to the buffer cached in the slab, because no re-encoding occurs. And in\n> that case you _don't_ need to call unuse_commit_buffer(), because you\n> have a read-only copy, and the slab cache will hold it forever[1].\n> Calling the unuse function will be a noop.\n> \n> But when we _do_ re-encode, then you get a new buffer which must be\n> freed. And that is when you have to call the unuse function. And the\n> reason it is \"unuse\" and not just \"free\" is that you don't necessarily\n> know which you have, but that function figures it out (and frees it only\n> if necessary).\n> \n> So what the patch is doing is correct, but the explanation is a little\n> confused. We see the leak only when re-encoding, so we'd probably want a\n> test case that triggers that. Which I assume implies rewriting a commit\n> that was previously generated with an encoding header.\n> \n\nThank you very much for these insights! It has been helpful but on \nfurther digging I think this is not about reencoding. On testing and \ndigging further, the leak appears to be happening when the commit that \nis being reworded we get is a freshly allocated buffer from \nrepo_get_commit_buffer. I'm still trying to figure out how specific \ncommits get cached in the slab while other commits don't. I'll update \nthis thread shortly once I get an idea about the same.\n\nMeanwhile, if anyone knows offhand about this, kindly chime in.\n\n> Now back to that [1] note. Even if we didn't re-encode, we'll still hold\n> onto that buffer forever. It's not a \"leak\" in the traditional sense\n> because it's still referenced in the commit slab cache. But if you are\n> going to walk over a million commits (like git-log does), you probably\n> don't want to hold a million commit messages in memory at once.\n> \n> For that you'd want to call free_commit_buffer() when you know you're\n> totally done with it (again, like git-log does after it finishes showing\n> the commit). That might be the case here in commit_tree_ext(), or it\n> might happen later (I'm not familiar with the git-history code).\n>\n> But note that you need to do _both_ the unuse and free calls. If we did\n> re-encode, the former is needed to free the newly allocated buffer. The\n> latter only drops the original buffer in the cache.\n>\n\n From my understanding, I think we may not need free_commit_buffer for \nthe following reasons:\n\n- The leak was only being reported when the commit did not come\n   from the commit slab.\n- We are not going to be reading too many commit objects into memory in\n   this code path. Hence freeing the commit in the slab isn't strictly\n   necessary.\n\nKindly correct me if I missed something, though.\n\nTo conclude, I think the change that the patch proposes if fine but the \ncommit message definitely needs updation.\n\n--\nSivaraam\n\n"},{"id":"546728","messageId":"20260630052608.GA2495216@coredump.intra.peff.net","threadId":"65808","inReplyTo":"317d0f7b-469f-4456-8808-506e17de264d@gmail.com","subject":"Re: [PATCH] builtin/history: unuse the commit buffer after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-30T05:26:08Z","receivedAt":"2026-06-30T05:26:10Z","isPatch":true,"body":"On Tue, Jun 30, 2026 at 09:15:49AM +0530, Kaartic Sivaraam wrote:\n\n> > So what the patch is doing is correct, but the explanation is a little\n> > confused. We see the leak only when re-encoding, so we'd probably want a\n> > test case that triggers that. Which I assume implies rewriting a commit\n> > that was previously generated with an encoding header.\n> > \n> \n> Thank you very much for these insights! It has been helpful but on further\n> digging I think this is not about reencoding. On testing and digging\n> further, the leak appears to be happening when the commit that is being\n> reworded we get is a freshly allocated buffer from repo_get_commit_buffer.\n> I'm still trying to figure out how specific commits get cached in the slab\n> while other commits don't. I'll update this thread shortly once I get an\n> idea about the same.\n> \n> Meanwhile, if anyone knows offhand about this, kindly chime in.\n\nThe most likely cause of an uncached commit buffer is that we loaded the\ncommit from the commit-graph (and thus never opened the object in the\nfirst place, so there was nothing to cache).\n\nI suppose it is also possible that we might create a commit struct and\nthen before ever calling parse_commit() on it, ask to see the commit\nmsg. And thus we never looked at either the commit graph nor the object\nitself.\n\nWe'd also refuse to cache if save_commit_buffer is disabled (since\nthat's the point of that flag). If that were the case I'd expect it to\nbe set consistently within a given program.\n\nSo my guess is probably the commit graph, but it could be that the\ncommand tries to format unparsed commits (which is _not_ a bug or error,\nbut would trigger the situation where we hand back a freshly allocated\nbuffer).\n\nI suspect the leak could _also_ be caused by re-encoding, but yeah, the\nroot cause is \"repo_logmsg_reencode gave us an allocated string\", and\nthat can happen when re-encoding is necessary, or when we did not have a\ncached buffer in the first place.\n\n> > But note that you need to do _both_ the unuse and free calls. If we did\n> > re-encode, the former is needed to free the newly allocated buffer. The\n> > latter only drops the original buffer in the cache.\n> \n> From my understanding, I think we may not need free_commit_buffer for the\n> following reasons:\n> \n> - The leak was only being reported when the commit did not come\n>   from the commit slab.\n> - We are not going to be reading too many commit objects into memory in\n>   this code path. Hence freeing the commit in the slab isn't strictly\n>   necessary.\n> \n> Kindly correct me if I missed something, though.\n\nIt depends on the definition of \"too many\" here. ;) Probably nobody will\nnotice if you keep a dozen in memory, but they may if the command\nhappens to get thousands of commits as input. That's not likely, but if\nthere's an easy moment where the program can say \"ah, I am done with\ncommit X, let's free any resources associated with it\" then I think it\nis worth doing.\n\nIf there isn't such an easy moment, it is probably not that big a deal\nto leave it as-is. It's a rare case when somebody would notice at all,\nand if you think over the implications above, a repo with an up-to-date\ncommit graph won't have very many cached entries in the first place.\n\n> To conclude, I think the change that the patch proposes if fine but the\n> commit message definitely needs updation.\n\nYep. Whether we should be calling free_commit_buffer() or not is really\nan orthogonal question to the leak. If we want to do something about it,\nit would be a separate patch anyway.\n\n-Peff\n"},{"id":"546730","messageId":"20260630053825.GC2495216@coredump.intra.peff.net","threadId":"65808","inReplyTo":"94b0bed5-c86a-4291-b958-52f09faebd29@gmail.com","subject":"Re: [PATCH] builtin/history: unuse the commit buffer after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-30T05:38:25Z","receivedAt":"2026-06-30T05:38:26Z","isPatch":true,"body":"On Tue, Jun 30, 2026 at 09:13:28AM +0530, Kaartic Sivaraam wrote:\n\n> On 15/06/26 15:18, Patrick Steinhardt wrote:\n> > Huh, curious. That seems to hint that we're missing test coverage for\n> > this specific scenario, as our test suite doesn't detect this leak.\n> > \n> \n> Indeed. The tricky thing is (as mentioned in another thread), this is\n> happening only when we get a commit not cached in the commit slab. Once we\n> get an idea on how certain commits get cached in the commit slab while\n> others don't, we can write a test case that would catch this leak.\n\nTry:\n\n  make SANITIZE=leak\n  cd t\n  GIT_TEST_COMMIT_GRAPH=1 ./t3451-history-reword.sh -v -i\n\nThat shows off the leak. It's possible we should be running the LSan\ntests in CI with more feature flags enabled, but I suspect it's just as\nlikely to miss a case as to add one (i.e., there might be a leak when we\n_don't_ use the commit graph). We could do both, but the combinatorics\nget expensive.\n\nYou could add a specific test that builds a commit graph and runs a\nhistory-reword. That would show off the fix, but it's so specific that I\nfind it a bit unlikely that it would catch a useful regression in the\nfuture.\n\nSo I dunno. I would be content with showing the commands above in the\ncommit message.\n\n-Peff\n"},{"id":"546732","messageId":"20260630055026.GE2495216@coredump.intra.peff.net","threadId":"65808","inReplyTo":"20260630053825.GC2495216@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/history: unuse the commit buffer after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-30T05:50:26Z","receivedAt":"2026-06-30T05:50:27Z","isPatch":true,"body":"On Tue, Jun 30, 2026 at 01:38:25AM -0400, Jeff King wrote:\n\n> Try:\n> \n>   make SANITIZE=leak\n>   cd t\n>   GIT_TEST_COMMIT_GRAPH=1 ./t3451-history-reword.sh -v -i\n\nOf course that made me curious how a full run of the test suite would\nreact to that flag. We get a few failures of t345x tests, but they all\nlook like the same leak discussed here.\n\nt4014 also fails, but with a unique leak. I don't think it's the same\nthing; it looks like we may leak the slab from init_topo_walk().\n\n-Peff\n"},{"id":"546736","messageId":"20260630064405.GF2495216@coredump.intra.peff.net","threadId":"65808","inReplyTo":"20260630055026.GE2495216@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/history: unuse the commit buffer after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-30T06:44:05Z","receivedAt":"2026-06-30T06:44:07Z","isPatch":true,"body":"On Tue, Jun 30, 2026 at 01:50:26AM -0400, Jeff King wrote:\n\n> t4014 also fails, but with a unique leak. I don't think it's the same\n> thing; it looks like we may leak the slab from init_topo_walk().\n\nFixed over in\nhttps://lore.kernel.org/git/20260630063944.GA3733670@coredump.intra.peff.net/.\n\nI think it can be handled independently of your fix here. You might also\nrun afoul of the prove TAP-output thing fixed in that thread.\n\n-Peff\n"},{"id":"552435","messageId":"20260910114052.325683-1-kaartic.sivaraam@gmail.com","threadId":"65808","inReplyTo":"20260614141600.620272-1-kaartic.sivaraam@gmail.com","subject":"[PATCH v2] builtin/history: unuse the commit buffer after use","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2026-09-10T11:39:51Z","receivedAt":"2026-09-10T11:41:15Z","isPatch":true,"body":"While running `git history reword` on a commit with `SANITIZE` flag set\nto `address,leak`, we could observe the following leak being reported:\n\n-- 8< --\n\n=================================================================\n==122337==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 263 byte(s) in 1 object(s) allocated from:\n    #0 0x7002c14fd9c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69\n    #1 0x5cdd008ec077 in do_xmalloc /me/git/wrapper.c:55\n    #2 0x5cdd008ec185 in do_xmallocz /me/git/wrapper.c:89\n    #3 0x5cdd008ec1fa in xmallocz /me/git/wrapper.c:97\n    #4 0x5cdd005b99d8 in unpack_loose_rest /me/git/object-file.c:216\n    #5 0x5cdd005e45f4 in read_object_info_from_path odb/source-loose.c:174\n    #6 0x5cdd005e4ba0 in odb_source_loose_read_object_info odb/source-loose.c:235\n    #7 0x5cdd005d9f83 in odb_source_read_object_info odb/source.h:413\n    #8 0x5cdd005daaed in odb_source_files_read_object_info odb/source-files.c:93\n    #9 0x5cdd005d1c8c in odb_source_read_object_info odb/source.h:413\n    #10 0x5cdd005d5bdd in do_oid_object_info_extended /me/git/odb.c:592\n    #11 0x5cdd005d7080 in odb_read_object_info_extended /me/git/odb.c:747\n    #12 0x5cdd005d75d8 in odb_read_object /me/git/odb.c:793\n    #13 0x5cdd003d9af7 in repo_get_commit_buffer /me/git/commit.c:399\n    #14 0x5cdd006739ed in repo_logmsg_reencode /me/git/pretty.c:716\n    #15 0x5cdd0012287a in commit_tree_ext builtin/history.c:134\n    #16 0x5cdd00122f33 in commit_tree_with_edited_message builtin/history.c:190\n    #17 0x5cdd00126e44 in cmd_history_reword builtin/history.c:748\n    #18 0x5cdd0012b051 in cmd_history builtin/history.c:1209\n    #19 0x5cdcfffb8faf in run_builtin /me/git/git.c:510\n    #20 0x5cdcfffb9ac6 in handle_builtin /me/git/git.c:786\n    #21 0x5cdcfffba358 in run_argv /me/git/git.c:869\n    #22 0x5cdcfffbaea9 in cmd_main /me/git/git.c:990\n    #23 0x5cdd0030f27f in main /me/git/common-main.c:9\n    #24 0x7002c102a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n    #25 0x7002c102a28a in __libc_start_main_impl ../csu/libc-start.c:360\n    #26 0x5cdcfffb4134 in _start (/home/sivaraam/.local/bin/git+0x217134) (BuildId: 549c1036ab1f9f4fd55546e5bf31c7bd81b008fd)\n\n-- >8 --\n\nA deeper investigation on this reveals the following as the root cause.\n\nAs part of rewording a commit in `git history`, we get the commit message\nbuffer in the `commit_tree_ext` function. This in turn obtains the buffer\nfrom `repo_logmsg_reencode`. In this case, the buffer that we receive from\n`repo_logmsg_reencode` ends up always being obtained from a call to\n`repo_get_commit_buffer`. The buffer that `repo_get_commit_buffer` ends\nup to be one that is not cached in the commit slab but a fresh buffer\nthat is returned from `odb_read_object`. This could be confirmed\nconfirmed by the stacktrace in the leak. A plausible reason for us\nreceiving an uncached buffer might be because the commit comes from the\ncommit-graph.\n\nIn any case, this uncached buffer is expected to be released with an\naccompanying call to `repo_unuse_commit_buffer` which takes care of\nfree-ing it. This call is missing in the `commit_tree_ext` flow\nthus resulting in the leak.\n\nFix this by ensuring we call `repo_unuse_commit_buffer` on the\noriginal_message buffer.\n\nFor those who are curious, the following is a minimal way to\nreproduce the leak. I'm including this here as the leak does\nnot happen when we get a cached commit obtained from the commit\nslab:\n\n-- 8< --\n$ git init scratch\nInitialized empty Git repository in /me/test-repos/scratch/.git/\n$ cd scratch/\n$ touch one && git add one && git commit -m \"Commit one\"\n[main (root-commit) 2182f9c] Commit one\n 1 file changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 one\n$ touch two && git add two && git commit -m \"Commit two\"\n[main 5550f33] Commit two\n 1 file changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 two\n$ git commit-graph write --reachable\n$ git history reword HEAD --dry-run\nupdate refs/heads/main eaded0872b14b3937605c77c0042429ca1e3bbe1 fd19e3776c75b8da9555c7c616ce0df9db7c6641\n\n=================================================================\n==122337==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 263 byte(s) in 1 object(s) allocated from:\n\n... snip ...\n\nSUMMARY: AddressSanitizer: 263 byte(s) leaked in 1 allocation(s).\n-- >8 --\n\nThis leak could also be triggered in our test suite if we run\nt3451-history-reword.sh as follows:\n\n-- 8< --\n$ make SANITIZE=leak\n$ cd t\n$ GIT_TEST_COMMIT_GRAPH=1 ./t3451-history-reword.sh -v -i\n-- >8 --\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n---\nChanges since v2:\n\nJust updated the commit message to clarify the root cause\nmore clearly. I haven't added an explicit test case as\nit wasn't clear if it is really worth it as Peff points out.\n\nThank you, Peff, for your help with this!\n\nOn a tangent, I noticed that the leak is only triggereable\nin the test suite, when we use `make SANITIZE=leak` and not\nwhen we use `make SANITIZE=address,leak`. It seems we\nintentionally disable leak detection in Asan via\nthe following line in t/test-lib.sh:\n\n   prepend_var ASAN_OPTIONS : detect_leaks=0\n\nI noticed the comment above saying the following\n\n   # If we were built with ASAN, it may complain about leaks\n   # of program-lifetime variables. Disable it by default to lower\n   # the noise level.\n\nI wonder if it has become stale now as we are fine with the test\nsuite reporting leaks when we build with `make SANITIZE=leak`.\n\nWould it be worth while to avoid turning off detect_leaks while\nusing Asan?\n\n builtin/history.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/history.c b/builtin/history.c\nindex 091465a59e..0e9259b5d7 100644\n--- a/builtin/history.c\n+++ b/builtin/history.c\n@@ -154,6 +154,7 @@ static int commit_tree_ext(struct repository *repo,\n \tfree_commit_extra_headers(original_extra_headers);\n \tstrbuf_release(&commit_message);\n \tfree(original_author);\n+\trepo_unuse_commit_buffer(repo, commit_with_message, original_message);\n \treturn ret;\n }\n \n-- \n2.55.0.806.gd4f651056d\n\n"},{"id":"552442","messageId":"xmqq4ifxgree.fsf@gitster.g","threadId":"65808","inReplyTo":"20260910114052.325683-1-kaartic.sivaraam@gmail.com","subject":"Re: [PATCH v2] builtin/history: unuse the commit buffer after use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-10T13:36:09Z","receivedAt":"2026-09-10T13:36:11Z","isPatch":true,"body":"Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:\n\n> While running `git history reword` on a commit with `SANITIZE` flag set\n> to `address,leak`, we could observe the following leak being reported:\n>\n> -- 8< --\n>\n> =================================================================\n> ==122337==ERROR: LeakSanitizer: detected memory leaks\n>\n> Direct leak of 263 byte(s) in 1 object(s) allocated from:\n>     #0 0x7002c14fd9c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69\n>     #1 0x5cdd008ec077 in do_xmalloc /me/git/wrapper.c:55\n>     #2 0x5cdd008ec185 in do_xmallocz /me/git/wrapper.c:89\n>     #3 0x5cdd008ec1fa in xmallocz /me/git/wrapper.c:97\n>     #4 0x5cdd005b99d8 in unpack_loose_rest /me/git/object-file.c:216\n>     #5 0x5cdd005e45f4 in read_object_info_from_path odb/source-loose.c:174\n>     #6 0x5cdd005e4ba0 in odb_source_loose_read_object_info odb/source-loose.c:235\n>     #7 0x5cdd005d9f83 in odb_source_read_object_info odb/source.h:413\n>     #8 0x5cdd005daaed in odb_source_files_read_object_info odb/source-files.c:93\n>     #9 0x5cdd005d1c8c in odb_source_read_object_info odb/source.h:413\n>     #10 0x5cdd005d5bdd in do_oid_object_info_extended /me/git/odb.c:592\n>     #11 0x5cdd005d7080 in odb_read_object_info_extended /me/git/odb.c:747\n>     #12 0x5cdd005d75d8 in odb_read_object /me/git/odb.c:793\n>     #13 0x5cdd003d9af7 in repo_get_commit_buffer /me/git/commit.c:399\n>     #14 0x5cdd006739ed in repo_logmsg_reencode /me/git/pretty.c:716\n>     #15 0x5cdd0012287a in commit_tree_ext builtin/history.c:134\n>     #16 0x5cdd00122f33 in commit_tree_with_edited_message builtin/history.c:190\n>     #17 0x5cdd00126e44 in cmd_history_reword builtin/history.c:748\n>     #18 0x5cdd0012b051 in cmd_history builtin/history.c:1209\n>     #19 0x5cdcfffb8faf in run_builtin /me/git/git.c:510\n>     #20 0x5cdcfffb9ac6 in handle_builtin /me/git/git.c:786\n>     #21 0x5cdcfffba358 in run_argv /me/git/git.c:869\n>     #22 0x5cdcfffbaea9 in cmd_main /me/git/git.c:990\n>     #23 0x5cdd0030f27f in main /me/git/common-main.c:9\n>     #24 0x7002c102a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n>     #25 0x7002c102a28a in __libc_start_main_impl ../csu/libc-start.c:360\n>     #26 0x5cdcfffb4134 in _start (/home/sivaraam/.local/bin/git+0x217134) (BuildId: 549c1036ab1f9f4fd55546e5bf31c7bd81b008fd)\n>\n> -- >8 --\n>\n> A deeper investigation on this reveals the following as the root cause.\n\nI am not sure if you are going to explain the root cause in such a\nway that is understandable by human readers, you would want to scare\nthem away with a stack trace.\n\n> As part of rewording a commit in `git history`, we get the commit message\n> buffer in the `commit_tree_ext` function. This in turn obtains the buffer\n> from `repo_logmsg_reencode`. In this case, the buffer that we receive from\n> `repo_logmsg_reencode` ends up always being obtained from a call to\n> `repo_get_commit_buffer`. The buffer that `repo_get_commit_buffer` ends\n> up to be one that is not cached in the commit slab but a fresh buffer\n> that is returned from `odb_read_object`. This could be confirmed\n> confirmed by the stacktrace in the leak. A plausible reason for us\n> receiving an uncached buffer might be because the commit comes from the\n> commit-graph.\n>\n> In any case, this uncached buffer is expected to be released with an\n> accompanying call to `repo_unuse_commit_buffer` which takes care of\n> free-ing it. This call is missing in the `commit_tree_ext` flow\n> thus resulting in the leak.\n>\n> Fix this by ensuring we call `repo_unuse_commit_buffer` on the\n> original_message buffer.\n>\n> For those who are curious, the following is a minimal way to\n> reproduce the leak. I'm including this here as the leak does\n> not happen when we get a cached commit obtained from the commit\n> slab:\n\n> -- 8< --\n> $ git init scratch\n> Initialized empty Git repository in /me/test-repos/scratch/.git/\n> $ cd scratch/\n> $ touch one && git add one && git commit -m \"Commit one\"\n> [main (root-commit) 2182f9c] Commit one\n>  1 file changed, 0 insertions(+), 0 deletions(-)\n>  create mode 100644 one\n> $ touch two && git add two && git commit -m \"Commit two\"\n> [main 5550f33] Commit two\n>  1 file changed, 0 insertions(+), 0 deletions(-)\n>  create mode 100644 two\n> $ git commit-graph write --reachable\n> $ git history reword HEAD --dry-run\n> update refs/heads/main eaded0872b14b3937605c77c0042429ca1e3bbe1 fd19e3776c75b8da9555c7c616ce0df9db7c6641\n>\n> =================================================================\n> ==122337==ERROR: LeakSanitizer: detected memory leaks\n>\n> Direct leak of 263 byte(s) in 1 object(s) allocated from:\n>\n> ... snip ...\n>\n> SUMMARY: AddressSanitizer: 263 byte(s) leaked in 1 allocation(s).\n> -- >8 --\n>\n> This leak could also be triggered in our test suite if we run\n> t3451-history-reword.sh as follows:\n>\n> -- 8< --\n> $ make SANITIZE=leak\n> $ cd t\n> $ GIT_TEST_COMMIT_GRAPH=1 ./t3451-history-reword.sh -v -i\n> -- >8 --\n\nPlease do not abuse scissors line when you do not mean \"discard all\nof the above and exclude it from the resulting commit log message\".\n\n>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> ---\n> Changes since v2:\n>\n> Just updated the commit message to clarify the root cause\n> more clearly. I haven't added an explicit test case as\n> it wasn't clear if it is really worth it as Peff points out.\n>\n> Thank you, Peff, for your help with this!\n>\n> On a tangent, I noticed that the leak is only triggereable\n> in the test suite, when we use `make SANITIZE=leak` and not\n> when we use `make SANITIZE=address,leak`. It seems we\n> intentionally disable leak detection in Asan via\n> the following line in t/test-lib.sh:\n>\n>    prepend_var ASAN_OPTIONS : detect_leaks=0\n>\n> I noticed the comment above saying the following\n>\n>    # If we were built with ASAN, it may complain about leaks\n>    # of program-lifetime variables. Disable it by default to lower\n>    # the noise level.\n>\n> I wonder if it has become stale now as we are fine with the test\n> suite reporting leaks when we build with `make SANITIZE=leak`.\n>\n> Would it be worth while to avoid turning off detect_leaks while\n> using Asan?\n>\n>  builtin/history.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/history.c b/builtin/history.c\n> index 091465a59e..0e9259b5d7 100644\n> --- a/builtin/history.c\n> +++ b/builtin/history.c\n> @@ -154,6 +154,7 @@ static int commit_tree_ext(struct repository *repo,\n>  \tfree_commit_extra_headers(original_extra_headers);\n>  \tstrbuf_release(&commit_message);\n>  \tfree(original_author);\n> +\trepo_unuse_commit_buffer(repo, commit_with_message, original_message);\n>  \treturn ret;\n>  }\n"},{"id":"552447","messageId":"20260910150021.348548-1-kaartic.sivaraam@gmail.com","threadId":"65808","inReplyTo":"xmqq4ifxgree.fsf@gitster.g","subject":"[PATCH v3] builtin/history: unuse the commit buffer after use","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2026-09-10T15:00:15Z","receivedAt":"2026-09-10T15:00:32Z","isPatch":true,"body":"While running `git history reword` on a commit with `SANITIZE` flag set\nto `address,leak`, we could observe a leak being reported (trace near\nthe end).\n\nThe root cause is as follows.\n\nAs part of rewording a commit, `commit_tree_ext` obtains the commit\nmessage buffer from `repo_logmsg_reencode`. As we ask for no output\nencoding, that function hands back the buffer from\n`repo_get_commit_buffer` verbatim.\n\n`repo_get_commit_buffer` returns the buffer cached in the commit slab\nif there is one, and otherwise reads the object afresh via\n`odb_read_object`. As the stacktrace below shows, we take the latter\npath here. The buffer is uncached because the commit was parsed from\nthe commit-graph: such a parse is answered from the graph file alone,\nso it never reads the object and never caches a buffer.\n\nA buffer obtained this way is expected to be released with an\naccompanying call to `repo_unuse_commit_buffer`, which takes care of\nfreeing it. This call is missing in the `commit_tree_ext` flow, thus\nresulting in the leak.\n\nFix this by ensuring we call `repo_unuse_commit_buffer` on the\noriginal_message buffer.\n\nUsing `repo_unuse_commit_buffer` is the correct way to release the\nbuffer since it frees only buffers the slab doesn't own. Plain free\nwould double-free on the cached path.\n\nFor those who are curious, the following is a minimal way to reproduce\nthe leak. Note the `git commit-graph --write` step, without which the\nbuffer comes from the commit slab and nothing leaks:\n\n  $ git init scratch\n  Initialized empty Git repository in /me/test-repos/scratch/.git/\n  $ cd scratch/\n  $ touch one && git add one && git commit -m \"Commit one\"\n  [main (root-commit) 2182f9c] Commit one\n   1 file changed, 0 insertions(+), 0 deletions(-)\n   create mode 100644 one\n  $ touch two && git add two && git commit -m \"Commit two\"\n  [main 5550f33] Commit two\n   1 file changed, 0 insertions(+), 0 deletions(-)\n   create mode 100644 two\n  $ git commit-graph write --reachable\n  $ git history reword HEAD --dry-run\n  update refs/heads/main eaded0872b14b3937605c77c0042429ca1e3bbe1\n   fd19e3776c75b8da9555c7c616ce0df9db7c6641\n\nThis leak could also be triggered in our test suite if we run\nt3451-history-reword.sh as follows:\n\n  $ make SANITIZE=leak\n  $ cd t\n  $ GIT_TEST_COMMIT_GRAPH=1 ./t3451-history-reword.sh -v -i\n\n=== Memory leak strack trace ===\n\n==122337==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 263 byte(s) in 1 object(s) allocated from:\n    #0 0x7002c14fd9c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69\n    #1 0x5cdd008ec077 in do_xmalloc /me/git/wrapper.c:55\n    #2 0x5cdd008ec185 in do_xmallocz /me/git/wrapper.c:89\n    #3 0x5cdd008ec1fa in xmallocz /me/git/wrapper.c:97\n    #4 0x5cdd005b99d8 in unpack_loose_rest /me/git/object-file.c:216\n    #5 0x5cdd005e45f4 in read_object_info_from_path odb/source-loose.c:174\n    #6 0x5cdd005e4ba0 in odb_source_loose_read_object_info odb/source-loose.c:235\n    #7 0x5cdd005d9f83 in odb_source_read_object_info odb/source.h:413\n    #8 0x5cdd005daaed in odb_source_files_read_object_info odb/source-files.c:93\n    #9 0x5cdd005d1c8c in odb_source_read_object_info odb/source.h:413\n    #10 0x5cdd005d5bdd in do_oid_object_info_extended /me/git/odb.c:592\n    #11 0x5cdd005d7080 in odb_read_object_info_extended /me/git/odb.c:747\n    #12 0x5cdd005d75d8 in odb_read_object /me/git/odb.c:793\n    #13 0x5cdd003d9af7 in repo_get_commit_buffer /me/git/commit.c:399\n    #14 0x5cdd006739ed in repo_logmsg_reencode /me/git/pretty.c:716\n    #15 0x5cdd0012287a in commit_tree_ext builtin/history.c:134\n    #16 0x5cdd00122f33 in commit_tree_with_edited_message builtin/history.c:190\n    #17 0x5cdd00126e44 in cmd_history_reword builtin/history.c:748\n    #18 0x5cdd0012b051 in cmd_history builtin/history.c:1209\n    #19 0x5cdcfffb8faf in run_builtin /me/git/git.c:510\n    #20 0x5cdcfffb9ac6 in handle_builtin /me/git/git.c:786\n    #21 0x5cdcfffba358 in run_argv /me/git/git.c:869\n    #22 0x5cdcfffbaea9 in cmd_main /me/git/git.c:990\n    #23 0x5cdd0030f27f in main /me/git/common-main.c:9\n    #24 0x7002c102a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n    #25 0x7002c102a28a in __libc_start_main_impl ../csu/libc-start.c:360\n    #26 0x5cdcfffb4134 in _start (/home/sivaraam/.local/bin/git+0x217134)\n    (BuildId: 549c1036ab1f9f4fd55546e5bf31c7bd81b008fd)\n\nSUMMARY: AddressSanitizer: 263 byte(s) leaked in 1 allocation(s).\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n---\nChanges since v2:\n\n- Tried to improve the commit message to make it more readable (hopefully).\n\n builtin/history.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/history.c b/builtin/history.c\nindex 091465a59e..0e9259b5d7 100644\n--- a/builtin/history.c\n+++ b/builtin/history.c\n@@ -154,6 +154,7 @@ static int commit_tree_ext(struct repository *repo,\n \tfree_commit_extra_headers(original_extra_headers);\n \tstrbuf_release(&commit_message);\n \tfree(original_author);\n+\trepo_unuse_commit_buffer(repo, commit_with_message, original_message);\n \treturn ret;\n }\n \n-- \n2.55.0.806.gb8242b093d\n\n"},{"id":"552462","messageId":"20260910160254.GB251185@coredump.intra.peff.net","threadId":"65808","inReplyTo":"20260910150021.348548-1-kaartic.sivaraam@gmail.com","subject":"Re: [PATCH v3] builtin/history: unuse the commit buffer after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-10T16:02:54Z","receivedAt":"2026-09-10T16:03:02Z","isPatch":true,"body":"On Thu, Sep 10, 2026 at 08:30:15PM +0530, Kaartic Sivaraam wrote:\n\n> Changes since v2:\n> \n> - Tried to improve the commit message to make it more readable (hopefully).\n\nThanks, the patch looks good and I think the commit message is accurate.\n\nI probably would have written something much shorter, like:\n\n  Every call to repo_logmsg_reencode() must be paired with a call to\n  repo_unuse_commit_buffer(), or we may leak an allocated buffer. We\n  have such a leak in \"git history\", which we can fix by adding an unuse\n  call.\n\n  The leak-checking tests don't detect this because we only allocate a\n  fresh buffer sometimes: when the message is reencoded, or when we had\n  to load it fresh from the odb (e.g., because the commit was parsed\n  from the commit graph rather than the object contents). But you can\n  see it by running:\n\n    make SANITIZE=leak\n    cd t\n    GIT_TEST_COMMIT_GRAPH=1 ./t3451-history-reword.sh\n\nI'm not suggesting a v3 with this wording, as I think there are\ndiminishing returns to polishing commit messages forever. Mostly just\nfood for thought for future patches. :)\n\n-Peff\n"},{"id":"552463","messageId":"xmqqv78df57r.fsf@gitster.g","threadId":"65808","inReplyTo":"20260910160254.GB251185@coredump.intra.peff.net","subject":"Re: [PATCH v3] builtin/history: unuse the commit buffer after use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-10T16:20:40Z","receivedAt":"2026-09-10T16:20:43Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> I'm not suggesting a v3 with this wording, as I think there are\n> diminishing returns to polishing commit messages forever. Mostly just\n> food for thought for future patches. :)\n\nI obviously agree.  I wonder what my recent favorite prompt given to\na nearby LLM, \"State the same thing with 1/N number of words\"\nfollowed by the proposed commit log message that I found too long to\nread, would produce for the v3 message.\n\nI'd use N=8~10 for this one ;-)\n"},{"id":"552467","messageId":"20d607c2-2380-4d52-86b2-25016574abdb@gmail.com","threadId":"65808","inReplyTo":"20260910160254.GB251185@coredump.intra.peff.net","subject":"Re: [PATCH v3] builtin/history: unuse the commit buffer after use","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2026-09-10T16:37:08Z","receivedAt":"2026-09-10T16:37:14Z","isPatch":true,"body":"On 9/10/26 21:32, Jeff King wrote:\n> On Thu, Sep 10, 2026 at 08:30:15PM +0530, Kaartic Sivaraam wrote:\n> \n> I'm not suggesting a v3 with this wording, as I think there are\n> diminishing returns to polishing commit messages forever. Mostly just\n> food for thought for future patches. :)\n> \n\nThat was very helpful, thanks! I suppose I've lost touch with writing \npatches for this community. I tried to beef it up hoping that more \ncontext / clarity is always useful. OTOH, I definitely agree that fewer \nwords to convey the same is always better. Will try to improve in the \npatches to come :-)\n\nThat said, I don't mind sending a v4 with your proposed message as it is \na strict improvement, though.\n\n-- \nSivaraam\n\n"},{"id":"552744","messageId":"20260915082943.117985-1-kaartic.sivaraam@gmail.com","threadId":"65808","inReplyTo":"20260910150021.348548-1-kaartic.sivaraam@gmail.com","subject":"[PATCH v4] builtin/history: unuse the commit buffer after use","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2026-09-15T08:29:36Z","receivedAt":"2026-09-15T08:29:54Z","isPatch":true,"body":"Every call to repo_logmsg_reencode() must be paired with a call to\nrepo_unuse_commit_buffer(), or we may leak an allocated buffer. We\nhave such a leak in \"git history\", which we can fix by adding an unuse\ncall.\n\nThe leak-checking tests don't detect this because we only allocate a\nfresh buffer sometimes: when the message is reencoded, or when we had\nto load it fresh from the odb (e.g., because the commit was parsed\nfrom the commit graph rather than the object contents). But you can\nsee it by running:\n\n  make SANITIZE=leak\n  cd t\n  GIT_TEST_COMMIT_GRAPH=1 ./t3451-history-reword.sh\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n---\nChanges since v3:\n\n- Replaced the long commit message with the concise one\n  suggested by Peff.\n\n builtin/history.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/history.c b/builtin/history.c\nindex 091465a59e..0e9259b5d7 100644\n--- a/builtin/history.c\n+++ b/builtin/history.c\n@@ -154,6 +154,7 @@ static int commit_tree_ext(struct repository *repo,\n \tfree_commit_extra_headers(original_extra_headers);\n \tstrbuf_release(&commit_message);\n \tfree(original_author);\n+\trepo_unuse_commit_buffer(repo, commit_with_message, original_message);\n \treturn ret;\n }\n \n-- \n2.55.0.801.g894675e619\n\n"}]}