{"thread":{"id":"64581","subject":"[PATCH 0/3] Some random object database related fixes","startedAt":"2025-12-05T08:20:15Z","lastAt":"2026-01-07T03:51:30Z","messageCount":26,"participants":["Patrick Steinhardt","Eric Sunshine","Justin Tobler","Toon Claes","Karthik Nayak","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"531682","messageId":"20251205-odb-related-fixes-v1-0-ef4250abb584@pks.im","threadId":"64581","inReplyTo":null,"subject":"[PATCH 0/3] Some random object database related fixes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-05T08:19:57Z","receivedAt":"2025-12-05T08:20:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series fixes some small issues I've discovered while working\non some other patch series. I've decided to split it out of these\nbecause I'm hitting the same issues in multiple series, and I don't want\nthose to become dependent on one another.\n\nThe patch series is built on top of f0ef5b6d9b with\nps/object-source-management at ac65c70663 (odb: handle recreation of\nquarantine directories, 2025-11-19) merged into it.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (3):\n builtin/repack: fix geometric repacks with promisor remotes\n builtin/gc: fix condition for whether to write commit graphs\n odb: properly close sources before freeing them\n\n builtin/gc.c | 8 +++++---\n builtin/repack.c | 5 +++--\n odb.c | 2 +-\n t/t7703-repack-geometric.sh | 26 ++++++++++++++++++++++++++\n t/t7900-maintenance.sh | 26 ++++++++++++++++++++++++++\n 5 files changed, 61 insertions(+), 6 deletions(-)\n\n\n---\nbase-commit: 2797238193944b52d12624a04a962f40b9bcad69\nchange-id: 20251205-odb-related-fixes-5f48a0993ef7\n\n"},{"id":"531683","messageId":"20251205-odb-related-fixes-v1-1-ef4250abb584@pks.im","threadId":"64581","inReplyTo":"20251205-odb-related-fixes-v1-0-ef4250abb584@pks.im","subject":"[PATCH 1/3] builtin/repack: fix geometric repacks with promisor remotes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-05T08:19:58Z","receivedAt":"2025-12-05T08:20:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When repacking a repository with promisor remotes git-repack(1) knows to\npass \"--exclude-promisor-objects\" to git-pack-objects(1). This option\nensures that the new pack will not contain any promised object that do\nnot yet exist locally.\n\nThis command line option is incompatible with \"--stdin-packs\": the\nlatter option enables the rev-walk-based machinery to figure out which\nobjects to add to the pack, whereas the former tells git-pack-objects(1)\nto merge all packs passed via stdin into one large pack. As we do not\nknow to filter those packs via the passed-in revisions it is clear that\nat the current point in time nothing sensible comes out of combining\nthese two options.\n\nBut there is one case where git-repack(1) decides to pass both options:\nwhen performing a geometric repack we always pass \"--stdin-packs\" to\nidentify the packs that should be merged. So if one performs a geometric\nrepack in a partial clone we'll end up with both options, and that\ncauses the repack to fail.\n\nFix this issue by never passing \"--exclude-promisor-objects\" when we\nhave a geometric split factor. We don't need the option anyway when\ndoing a geometric repack as we will only ever pack loose objects or\nmerge multiple packs. And neither of those cases can yield a promisor\nobject.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 5 +++--\n t/t7703-repack-geometric.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d9012141f6..4621eed3e6 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -294,9 +294,10 @@ int cmd_repack(int argc,\n \t\tstrvec_push(&cmd.args, \"--all\");\n \t\tstrvec_push(&cmd.args, \"--reflog\");\n \t\tstrvec_push(&cmd.args, \"--indexed-objects\");\n+\n+\t\tif (repo_has_promisor_remote(repo))\n+\t\t\tstrvec_push(&cmd.args, \"--exclude-promisor-objects\");\n \t}\n-\tif (repo_has_promisor_remote(repo))\n-\t\tstrvec_push(&cmd.args, \"--exclude-promisor-objects\");\n \tif (!write_midx) {\n \t\tif (write_bitmaps > 0)\n \t\t\tstrvec_push(&cmd.args, \"--write-bitmap-index\");\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 9fc1626fbf..6d2c712bff 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -445,4 +445,30 @@ test_expect_success '--geometric -l disables writing bitmaps with non-local pack\n \ttest_path_is_file member/.git/objects/pack/multi-pack-index-*.bitmap\n '\n \n+test_expect_success '--geometric works with promisor packs' '\n+\ttest_when_finished \"rm -fr remote local\" &&\n+\n+\tgit init remote &&\n+\ttest_commit -C remote first file first &&\n+\ttest_commit -C remote second file second &&\n+\tgit -C remote config set uploadpack.allowfilter 1 &&\n+\tgit -C remote config set uploadpack.allowanysha1inwant 1 &&\n+\tgit -C remote repack -Ad &&\n+\n+\tgit clone --filter=blob:none file://\"$(pwd)\"/remote local &&\n+\tgit -C local rev-list --objects --missing=print HEAD >missing-objects &&\n+\ttest_grep \"^?\" missing-objects &&\n+\n+\t# Assert that promisor packs are left alone and that we still manage to\n+\t# create new geometric packs.\n+\tls local/.git/objects/pack/*.promisor >promisors-before &&\n+\tls local/.git/objects/pack/*.pack >packs-before &&\n+\ttest_commit -C local change &&\n+\tgit -C local repack --geometric=2 &&\n+\tls local/.git/objects/pack/*.promisor >promisors-after &&\n+\tls local/.git/objects/pack/*.pack >packs-after &&\n+\t! cmp packs-before packs-after &&\n+\ttest_cmp promisors-before promisors-after\n+'\n+\n test_done\n\n-- \n2.52.0.239.gd5f0c6e74e.dirty\n\n"},{"id":"531685","messageId":"20251205-odb-related-fixes-v1-2-ef4250abb584@pks.im","threadId":"64581","inReplyTo":"20251205-odb-related-fixes-v1-0-ef4250abb584@pks.im","subject":"[PATCH 2/3] builtin/gc: fix condition for whether to write commit graphs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-05T08:19:59Z","receivedAt":"2025-12-05T08:20:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When performing auto-maintenance we check whether commit graphs need to\nbe generated by counting the number of commits that are reachable by any\nreference, but not covered by a commit graph. This search is performed\nby iterating through all references and then doing a depth-first search\nuntil we have found enough commits that are not present in the commit\ngraph.\n\nThis logic has a memory leak though:\n\n Direct leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x55555562e433 in malloc (git+0xda433)\n #1 0x555555964322 in do_xmalloc ../wrapper.c:55:8\n #2 0x5555559642e6 in xmalloc ../wrapper.c:76:9\n #3 0x55555579bf29 in commit_list_append ../commit.c:1872:35\n #4 0x55555569f160 in dfs_on_ref ../builtin/gc.c:1165:4\n #5 0x5555558c33fd in do_for_each_ref_iterator ../refs/iterator.c:431:12\n #6 0x5555558af520 in do_for_each_ref ../refs.c:1828:9\n #7 0x5555558ac317 in refs_for_each_ref ../refs.c:1833:9\n #8 0x55555569e207 in should_write_commit_graph ../builtin/gc.c:1188:11\n #9 0x55555569c915 in maintenance_is_needed ../builtin/gc.c:3492:8\n #10 0x55555569b76a in cmd_maintenance ../builtin/gc.c:3542:9\n #11 0x55555575166a in run_builtin ../git.c:506:11\n #12 0x5555557502f0 in handle_builtin ../git.c:779:9\n #13 0x555555751127 in run_argv ../git.c:862:4\n #14 0x55555575007b in cmd_main ../git.c:984:19\n #15 0x5555557523aa in main ../common-main.c:9:11\n #16 0x7ffff7a2a4d7 in __libc_start_call_main (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a4d7) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n #17 0x7ffff7a2a59a in __libc_start_main@GLIBC_2.2.5 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a59a) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n #18 0x5555555f0934 in _start (git+0x9c934)\n\nThe root cause of this memory leak is our use of `commit_list_append()`.\nThis function expects as parameters the item to append and the _tail_ of\nthe list to append. This tail will then be overwritten with the new tail\nof the list so that it can be used in subsequent calls. But we call it\nwith `commit_list_append(parent->item, &stack)`, so we end up losing\neverything but the new item.\n\nThis issue only surfaces when counting merge commits. Next to being a\nmemory leak, it also shows that we're in fact miscounting as we only\nrespect children of the last parent. All previous parents are discarded,\nso their children will be disregarded unless they are hit via another\nreference.\n\nWhile crafting a test case for the issue I was puzzled that I couldn't\nestablish the proper border at which the auto-condition would be\nfulfilled. As it turns out, there's another bug: if an object is at the\ntip of any reference we don't mark it as seen. Consequently, if it is\nreachable via any other reference, we'd count that object twice.\n\nFix both of these bugs so that we properly count objects without leaking\nany memory.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 8 +++++---\n t/t7900-maintenance.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 92c6e7b954..17ff68cbd9 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1130,8 +1130,10 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n \t\treturn 0;\n \n \tcommit = lookup_commit(the_repository, maybe_peeled);\n-\tif (!commit)\n+\tif (!commit || commit->object.flags & SEEN)\n \t\treturn 0;\n+\tcommit->object.flags |= SEEN;\n+\n \tif (repo_parse_commit(the_repository, commit) ||\n \t commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n \t\treturn 0;\n@@ -1141,7 +1143,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n \tif (data->num_not_in_graph >= data->limit)\n \t\treturn 1;\n \n-\tcommit_list_append(commit, &stack);\n+\tcommit_list_insert(commit, &stack);\n \n \twhile (!result && stack) {\n \t\tstruct commit_list *parent;\n@@ -1162,7 +1164,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n \t\t\t\tbreak;\n \t\t\t}\n \n-\t\t\tcommit_list_append(parent->item, &stack);\n+\t\t\tcommit_list_insert(parent->item, &stack);\n \t\t}\n \t}\n \ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 6b36f52df7..6f3117304f 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -206,6 +206,32 @@ test_expect_success 'commit-graph auto condition' '\n \ttest_subcommand $COMMIT_GRAPH_WRITE err &&\n \ttest_grep \"is not a valid task\" err\n\n-- \n2.52.0.239.gd5f0c6e74e.dirty\n\n"},{"id":"531684","messageId":"20251205-odb-related-fixes-v1-3-ef4250abb584@pks.im","threadId":"64581","inReplyTo":"20251205-odb-related-fixes-v1-0-ef4250abb584@pks.im","subject":"[PATCH 3/3] odb: properly close sources before freeing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-05T08:20:00Z","receivedAt":"2025-12-05T08:20:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the next commit we are about to move the packfile store into the ODB\nsource so that we have one store per source. This will lead to a memory\nleak in the following commit when reading data from a submodule via\ngit-grep(1):\n\n Direct leak of 192 byte(s) in 1 object(s) allocated from:\n #0 0x55555562e726 in calloc (git+0xda726)\n #1 0x555555964734 in xcalloc ../wrapper.c:154:8\n #2 0x555555835136 in load_multi_pack_index_one ../midx.c:135:2\n #3 0x555555834fd6 in load_multi_pack_index ../midx.c:382:6\n #4 0x5555558365b6 in prepare_multi_pack_index_one ../midx.c:716:17\n #5 0x55555586c605 in packfile_store_prepare ../packfile.c:1103:3\n #6 0x55555586c90c in packfile_store_reprepare ../packfile.c:1118:2\n #7 0x5555558546b3 in odb_reprepare ../odb.c:1106:2\n #8 0x5555558539e4 in do_oid_object_info_extended ../odb.c:715:4\n #9 0x5555558533d1 in odb_read_object_info_extended ../odb.c:862:8\n #10 0x5555558540bd in odb_read_object ../odb.c:920:6\n #11 0x55555580a330 in grep_source_load_oid ../grep.c:1934:12\n #12 0x55555580a13a in grep_source_load ../grep.c:1986:10\n #13 0x555555809103 in grep_source_is_binary ../grep.c:2014:7\n #14 0x555555807574 in grep_source_1 ../grep.c:1625:8\n #15 0x555555807322 in grep_source ../grep.c:1837:10\n #16 0x5555556a5c58 in run ../builtin/grep.c:208:10\n #17 0x55555562bb42 in void* ThreadStartFunc(void*) lsan_interceptors.cpp.o\n #18 0x7ffff7a9a979 in start_thread (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x9a979) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n #19 0x7ffff7b22d2b in __GI___clone3 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x122d2b) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n\nThe root caues of this leak is the way we set up and release the\nsubmodule:\n\n 1. We use `repo_submodule_init()` to initialize a new repository. This\n repository is stored in `repos_to_free`.\n\n 2. We now read data from the submodule repository.\n\n 3. We then call `repo_clear()` on the submodule repositories.\n\n 4. `repo_clear()` calls `odb_free()`.\n\n 5. `odb_free()` calls `odb_free_sources()` followed by `odb_close()`.\n\nThe issue here is the 5th step: we call `odb_free_sources()` _before_ we\ncall `odb_close()`. But `odb_free_sources()` already frees all sources,\nso the logic that closes them in `odb_close()` now becomes a no-op. As a\nconsequence, we never explicitly close sources at all.\n\nFix the leak by closing the store before we free the sources.\n\nSigned-off-by: Patrick Steinhardt \n---\n odb.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/odb.c b/odb.c\nindex dc8f292f3d..8e67afe185 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -1132,13 +1132,13 @@ void odb_free(struct object_database *o)\n \toidmap_clear(&o->replace_map, 1);\n \tpthread_mutex_destroy(&o->replace_mutex);\n \n+\todb_close(o);\n \todb_free_sources(o);\n \n \tfor (size_t i = 0; i < o->cached_object_nr; i++)\n \t\tfree((char *) o->cached_objects[i].value.buf);\n \tfree(o->cached_objects);\n \n-\todb_close(o);\n \tpackfile_store_free(o->packfiles);\n \tstring_list_clear(&o->submodule_source_paths, 0);\n \n\n-- \n2.52.0.239.gd5f0c6e74e.dirty\n\n"},{"id":"531734","messageId":"CAPig+cRW6tXFTqqnhH1Be33TgzT2dsdzNLFii3Now7+DNiTTvw@mail.gmail.com","threadId":"64581","inReplyTo":"20251205-odb-related-fixes-v1-3-ef4250abb584@pks.im","subject":"Re: [PATCH 3/3] odb: properly close sources before freeing them","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-12-05T23:14:22Z","receivedAt":"2025-12-05T23:14:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 5, 2025 at 6:36 AM Patrick Steinhardt wrote:\n> In the next commit we are about to move the packfile store into the ODB\n> source so that we have one store per source. This will lead to a memory\n> leak in the following commit when reading data from a submodule via\n> git-grep(1):\n> [...]\n> Signed-off-by: Patrick Steinhardt \n\nConsidering that this is patch [3/3], to what does \"In the next\ncommit...\" refer?\n"},{"id":"531742","messageId":"aTQVt4zgMbsX_6tD@pks.im","threadId":"64581","inReplyTo":"CAPig+cRW6tXFTqqnhH1Be33TgzT2dsdzNLFii3Now7+DNiTTvw@mail.gmail.com","subject":"Re: [PATCH 3/3] odb: properly close sources before freeing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-06T11:38:31Z","receivedAt":"2025-12-06T11:38:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 05, 2025 at 06:14:22PM -0500, Eric Sunshine wrote:\n> On Fri, Dec 5, 2025 at 6:36 AM Patrick Steinhardt wrote:\n> > In the next commit we are about to move the packfile store into the ODB\n> > source so that we have one store per source. This will lead to a memory\n> > leak in the following commit when reading data from a submodule via\n> > git-grep(1):\n> > [...]\n> > Signed-off-by: Patrick Steinhardt \n> \n> Considering that this is patch [3/3], to what does \"In the next\n> commit...\" refer?\n\nGood catch! I split this out of another, bigger, patch series. But as\nI've started to hit the leak in a different patch series, as well, I\ndecided to split it out into a smaller patch series.\n\nI've queued the following change locally, but will refrain from sending\nout a new version for now.\n\nThanks!\n\nPatrick\n\n1: 5c15065406 = 1: 9f813d92f3 builtin/repack: fix geometric repacks with promisor remotes\n2: 2fa3991003 = 2: 02167bfb16 builtin/gc: fix condition for whether to write commit graphs\n3: a06d0716c3 ! 3: c9ca233c29 odb: properly close sources before freeing them\n @@ Commit message\n odb: properly close sources before freeing them\n \n In the next commit we are about to move the packfile store into the ODB\n - source so that we have one store per source. This will lead to a memory\n - leak in the following commit when reading data from a submodule via\n - git-grep(1):\n + source so that we have one store per source. This can lead to a memory\n + leak when reading data from a submodule via git-grep(1):\n \n Direct leak of 192 byte(s) in 1 object(s) allocated from:\n #0 0x55555562e726 in calloc (git+0xda726)\n\n"},{"id":"531744","messageId":"CAPig+cQNKQt=kMaNYNWAPAfGej-mhLUR_BXS4J58JjVUtG7VKw@mail.gmail.com","threadId":"64581","inReplyTo":"aTQVt4zgMbsX_6tD@pks.im","subject":"Re: [PATCH 3/3] odb: properly close sources before freeing them","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-12-06T11:43:40Z","receivedAt":"2025-12-06T11:43:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Dec 6, 2025 at 6:38 AM Patrick Steinhardt wrote:\n> On Fri, Dec 05, 2025 at 06:14:22PM -0500, Eric Sunshine wrote:\n> > On Fri, Dec 5, 2025 at 6:36 AM Patrick Steinhardt wrote:\n> > > In the next commit we are about to move the packfile store into the ODB\n> > > source so that we have one store per source. This will lead to a memory\n> > > leak in the following commit when reading data from a submodule via\n> > > git-grep(1):\n> > > [...]\n> > > Signed-off-by: Patrick Steinhardt \n> >\n> > Considering that this is patch [3/3], to what does \"In the next\n> > commit...\" refer?\n>\n> Good catch! I split this out of another, bigger, patch series. But as\n> I've started to hit the leak in a different patch series, as well, I\n> decided to split it out into a smaller patch series.\n>\n> I've queued the following change locally, but will refrain from sending\n> out a new version for now.\n>\n> 3: a06d0716c3 ! 3: c9ca233c29 odb: properly close sources before freeing them\n> @@ Commit message\n> In the next commit we are about to move the packfile store into the ODB\n> - source so that we have one store per source. This will lead to a memory\n> - leak in the following commit when reading data from a submodule via\n> - git-grep(1):\n> + source so that we have one store per source. This can lead to a memory\n> + leak when reading data from a submodule via git-grep(1):\n\nI would think that you would also want to drop the \"In the next commit\nwe are about to...\" bit (considering, again, that this is patch\n[3/3]).\n"},{"id":"531752","messageId":"aTQbusI04t5tox4G@pks.im","threadId":"64581","inReplyTo":"CAPig+cQNKQt=kMaNYNWAPAfGej-mhLUR_BXS4J58JjVUtG7VKw@mail.gmail.com","subject":"Re: [PATCH 3/3] odb: properly close sources before freeing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-06T12:04:10Z","receivedAt":"2025-12-06T12:04:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Dec 06, 2025 at 06:43:40AM -0500, Eric Sunshine wrote:\n> On Sat, Dec 6, 2025 at 6:38 AM Patrick Steinhardt wrote:\n> > On Fri, Dec 05, 2025 at 06:14:22PM -0500, Eric Sunshine wrote:\n> > > On Fri, Dec 5, 2025 at 6:36 AM Patrick Steinhardt wrote:\n> > > > In the next commit we are about to move the packfile store into the ODB\n> > > > source so that we have one store per source. This will lead to a memory\n> > > > leak in the following commit when reading data from a submodule via\n> > > > git-grep(1):\n> > > > [...]\n> > > > Signed-off-by: Patrick Steinhardt \n> > >\n> > > Considering that this is patch [3/3], to what does \"In the next\n> > > commit...\" refer?\n> >\n> > Good catch! I split this out of another, bigger, patch series. But as\n> > I've started to hit the leak in a different patch series, as well, I\n> > decided to split it out into a smaller patch series.\n> >\n> > I've queued the following change locally, but will refrain from sending\n> > out a new version for now.\n> >\n> > 3: a06d0716c3 ! 3: c9ca233c29 odb: properly close sources before freeing them\n> > @@ Commit message\n> > In the next commit we are about to move the packfile store into the ODB\n> > - source so that we have one store per source. This will lead to a memory\n> > - leak in the following commit when reading data from a submodule via\n> > - git-grep(1):\n> > + source so that we have one store per source. This can lead to a memory\n> > + leak when reading data from a submodule via git-grep(1):\n> \n> I would think that you would also want to drop the \"In the next commit\n> we are about to...\" bit (considering, again, that this is patch\n> [3/3]).\n\nUgh, of course. It's the weekend, so my brain is clearly not working.\nThanks!\n\nPatrick\n"},{"id":"532001","messageId":"pva24p5jl2wjnwtdysmiqy4ljcfxtarss2cudqf5k7so36c5b3@6xkb6o2tgx5j","threadId":"64581","inReplyTo":"20251205-odb-related-fixes-v1-1-ef4250abb584@pks.im","subject":"Re: [PATCH 1/3] builtin/repack: fix geometric repacks with promisor remotes","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-12-10T19:31:44Z","receivedAt":"2025-12-10T19:31:54Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/12/05 09:19AM, Patrick Steinhardt wrote:\n> When repacking a repository with promisor remotes git-repack(1) knows to\n> pass \"--exclude-promisor-objects\" to git-pack-objects(1). This option\n> ensures that the new pack will not contain any promised object that do\n> not yet exist locally.\n> \n> This command line option is incompatible with \"--stdin-packs\": the\n> latter option enables the rev-walk-based machinery to figure out which\n> objects to add to the pack, whereas the former tells git-pack-objects(1)\n> to merge all packs passed via stdin into one large pack. As we do not\n> know to filter those packs via the passed-in revisions it is clear that\n> at the current point in time nothing sensible comes out of combining\n> these two options.\n\nIs the latter/former part here backwards? I find it a bit confusing to\nread. As I understand it, --stdin-packs expects the packfiles provided\nas input to dictate the source of objects when repacking. With\n--exclude-promisor-objects, we walk the object graph normally, but\nexclude promisor objects. Thus combining these two options would create\na conflict regarding which objects are included.\n\n> But there is one case where git-repack(1) decides to pass both options:\n> when performing a geometric repack we always pass \"--stdin-packs\" to\n> identify the packs that should be merged. So if one performs a geometric\n> repack in a partial clone we'll end up with both options, and that\n> causes the repack to fail.\n> \n> Fix this issue by never passing \"--exclude-promisor-objects\" when we\n> have a geometric split factor. We don't need the option anyway when\n> doing a geometric repack as we will only ever pack loose objects or\n> merge multiple packs. And neither of those cases can yield a promisor\n> object.\n\nI'm not sure I fully understand why --exclude-promisor-objects would not\nbe needed for geometric repacks. To clarify, do geometric repacks\nalready exclude promisor packfiles when merging? If so, then this change\nmakes sense.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/repack.c | 5 +++--\n> t/t7703-repack-geometric.sh | 26 ++++++++++++++++++++++++++\n> 2 files changed, 29 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index d9012141f6..4621eed3e6 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -294,9 +294,10 @@ int cmd_repack(int argc,\n> \t\tstrvec_push(&cmd.args, \"--all\");\n> \t\tstrvec_push(&cmd.args, \"--reflog\");\n> \t\tstrvec_push(&cmd.args, \"--indexed-objects\");\n> +\n> +\t\tif (repo_has_promisor_remote(repo))\n> +\t\t\tstrvec_push(&cmd.args, \"--exclude-promisor-objects\");\n> \t}\n> -\tif (repo_has_promisor_remote(repo))\n> -\t\tstrvec_push(&cmd.args, \"--exclude-promisor-objects\");\n\nOk, now the --exclude-promisor-objects flag is only added when there is\na promisor remote and geometric repacking is not used.\n\n> \tif (!write_midx) {\n> \t\tif (write_bitmaps > 0)\n> \t\t\tstrvec_push(&cmd.args, \"--write-bitmap-index\");\n> diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\n> index 9fc1626fbf..6d2c712bff 100755\n> --- a/t/t7703-repack-geometric.sh\n> +++ b/t/t7703-repack-geometric.sh\n> @@ -445,4 +445,30 @@ test_expect_success '--geometric -l disables writing bitmaps with non-local pack\n> \ttest_path_is_file member/.git/objects/pack/multi-pack-index-*.bitmap\n> '\n> \n> +test_expect_success '--geometric works with promisor packs' '\n> +\ttest_when_finished \"rm -fr remote local\" &&\n> +\n> +\tgit init remote &&\n> +\ttest_commit -C remote first file first &&\n> +\ttest_commit -C remote second file second &&\n> +\tgit -C remote config set uploadpack.allowfilter 1 &&\n> +\tgit -C remote config set uploadpack.allowanysha1inwant 1 &&\n> +\tgit -C remote repack -Ad &&\n> +\n> +\tgit clone --filter=blob:none file://\"$(pwd)\"/remote local &&\n> +\tgit -C local rev-list --objects --missing=print HEAD >missing-objects &&\n> +\ttest_grep \"^?\" missing-objects &&\n> +\n> +\t# Assert that promisor packs are left alone and that we still manage to\n> +\t# create new geometric packs.\n> +\tls local/.git/objects/pack/*.promisor >promisors-before &&\n> +\tls local/.git/objects/pack/*.pack >packs-before &&\n> +\ttest_commit -C local change &&\n> +\tgit -C local repack --geometric=2 &&\n> +\tls local/.git/objects/pack/*.promisor >promisors-after &&\n> +\tls local/.git/objects/pack/*.pack >packs-after &&\n> +\t! cmp packs-before packs-after &&\n> +\ttest_cmp promisors-before promisors-after\n\nOk, so it does seem to be the case that promisor packfiles are ignored\nwhen performing a geometric repack. Naive question: does this mean there\nare scenarios where a repository could accumulate many promisor\npackfiles, but never repack them?\n\n-Justin\n"},{"id":"532002","messageId":"gdyc7mdim2p32fesvcb672ssozoom4pdi7dyygacj3s66v7gd4@ydzwijirha3a","threadId":"64581","inReplyTo":"20251205-odb-related-fixes-v1-2-ef4250abb584@pks.im","subject":"Re: [PATCH 2/3] builtin/gc: fix condition for whether to write commit graphs","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-12-10T19:49:39Z","receivedAt":"2025-12-10T19:49:43Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/12/05 09:19AM, Patrick Steinhardt wrote:\n> When performing auto-maintenance we check whether commit graphs need to\n> be generated by counting the number of commits that are reachable by any\n> reference, but not covered by a commit graph. This search is performed\n> by iterating through all references and then doing a depth-first search\n> until we have found enough commits that are not present in the commit\n> graph.\n> \n> This logic has a memory leak though:\n> \n> Direct leak of 16 byte(s) in 1 object(s) allocated from:\n> #0 0x55555562e433 in malloc (git+0xda433)\n> #1 0x555555964322 in do_xmalloc ../wrapper.c:55:8\n> #2 0x5555559642e6 in xmalloc ../wrapper.c:76:9\n> #3 0x55555579bf29 in commit_list_append ../commit.c:1872:35\n> #4 0x55555569f160 in dfs_on_ref ../builtin/gc.c:1165:4\n> #5 0x5555558c33fd in do_for_each_ref_iterator ../refs/iterator.c:431:12\n> #6 0x5555558af520 in do_for_each_ref ../refs.c:1828:9\n> #7 0x5555558ac317 in refs_for_each_ref ../refs.c:1833:9\n> #8 0x55555569e207 in should_write_commit_graph ../builtin/gc.c:1188:11\n> #9 0x55555569c915 in maintenance_is_needed ../builtin/gc.c:3492:8\n> #10 0x55555569b76a in cmd_maintenance ../builtin/gc.c:3542:9\n> #11 0x55555575166a in run_builtin ../git.c:506:11\n> #12 0x5555557502f0 in handle_builtin ../git.c:779:9\n> #13 0x555555751127 in run_argv ../git.c:862:4\n> #14 0x55555575007b in cmd_main ../git.c:984:19\n> #15 0x5555557523aa in main ../common-main.c:9:11\n> #16 0x7ffff7a2a4d7 in __libc_start_call_main (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a4d7) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n> #17 0x7ffff7a2a59a in __libc_start_main@GLIBC_2.2.5 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a59a) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n> #18 0x5555555f0934 in _start (git+0x9c934)\n> \n> The root cause of this memory leak is our use of `commit_list_append()`.\n> This function expects as parameters the item to append and the _tail_ of\n> the list to append. This tail will then be overwritten with the new tail\n> of the list so that it can be used in subsequent calls. But we call it\n> with `commit_list_append(parent->item, &stack)`, so we end up losing\n> everything but the new item.\n> \n> This issue only surfaces when counting merge commits. Next to being a\n> memory leak, it also shows that we're in fact miscounting as we only\n> respect children of the last parent. All previous parents are discarded,\n> so their children will be disregarded unless they are hit via another\n> reference.\n> \n> While crafting a test case for the issue I was puzzled that I couldn't\n> establish the proper border at which the auto-condition would be\n> fulfilled. As it turns out, there's another bug: if an object is at the\n> tip of any reference we don't mark it as seen. Consequently, if it is\n> reachable via any other reference, we'd count that object twice.\n> \n> Fix both of these bugs so that we properly count objects without leaking\n> any memory.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/gc.c | 8 +++++---\n> t/t7900-maintenance.sh | 26 ++++++++++++++++++++++++++\n> 2 files changed, 31 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 92c6e7b954..17ff68cbd9 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1130,8 +1130,10 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n> \t\treturn 0;\n> \n> \tcommit = lookup_commit(the_repository, maybe_peeled);\n> -\tif (!commit)\n> +\tif (!commit || commit->object.flags & SEEN)\n> \t\treturn 0;\n> +\tcommit->object.flags |= SEEN;\n\nNow we are marking the object at the reference tip as seen so it will\nnot be counted more than once if used by other references. Makes sense.\n\n> +\n> \tif (repo_parse_commit(the_repository, commit) ||\n> \t commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n> \t\treturn 0;\n> @@ -1141,7 +1143,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n> \tif (data->num_not_in_graph >= data->limit)\n> \t\treturn 1;\n> \n> -\tcommit_list_append(commit, &stack);\n> +\tcommit_list_insert(commit, &stack);\n> \n> \twhile (!result && stack) {\n> \t\tstruct commit_list *parent;\n> @@ -1162,7 +1164,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n> \t\t\t\tbreak;\n> \t\t\t}\n> \n> -\t\t\tcommit_list_append(parent->item, &stack);\n> +\t\t\tcommit_list_insert(parent->item, &stack);\n\nWe change from commit_list_append() to commit_list_insert() so the new\nitem is added to the list without discarding the other entries. This\nfixes the memory leak and corrects the other miscounting issue. Looks\ngood.\n\n> \t\t}\n> \t}\n> \n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index 6b36f52df7..6f3117304f 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -206,6 +206,32 @@ test_expect_success 'commit-graph auto condition' '\n> \ttest_subcommand $COMMIT_GRAPH_WRITE '\n> \n> +test_expect_success 'commit-graph auto condition with merges' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit config set maintenance.auto false &&\n> +\t\tgit commit --allow-empty -m initial &&\n> +\t\tgit switch --create feature &&\n> +\t\tgit commit --allow-empty -m feature-1 &&\n> +\t\tgit commit --allow-empty -m feature-2 &&\n> +\t\tgit switch - &&\n> +\t\tgit commit --allow-empty -m main-1 &&\n> +\t\tgit commit --allow-empty -m main-2 &&\n> +\t\tgit merge feature &&\n> +\t\tgit branch -D feature &&\n\nIf we left the feature branch instead of deleting it, would that help\ntest that commits are not counted twice?\n\n> +\n> +\t\t# We have 6 commit, none of which are covered by a commit\n> +\t\t# graph. So this must be the boundary at which we start to\n> +\t\t# perform maintenance.\n> +\t\ttest_must_fail git -c maintenance.commit-graph.auto=7 \\\n> +\t\t\tmaintenance is-needed --auto --task=commit-graph &&\n> +\t\tgit -c maintenance.commit-graph.auto=6 \\\n> +\t\t\tmaintenance is-needed --auto --task=commit-graph\n> +\t)\n> +'\n\nNice fix.\n\n-Justin\n"},{"id":"532019","messageId":"aTpa0XLKPL53LaR-@pks.im","threadId":"64581","inReplyTo":"pva24p5jl2wjnwtdysmiqy4ljcfxtarss2cudqf5k7so36c5b3@6xkb6o2tgx5j","subject":"Re: [PATCH 1/3] builtin/repack: fix geometric repacks with promisor remotes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-11T05:46:57Z","receivedAt":"2025-12-11T05:47:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 10, 2025 at 01:31:44PM -0600, Justin Tobler wrote:\n> On 25/12/05 09:19AM, Patrick Steinhardt wrote:\n> > But there is one case where git-repack(1) decides to pass both options:\n> > when performing a geometric repack we always pass \"--stdin-packs\" to\n> > identify the packs that should be merged. So if one performs a geometric\n> > repack in a partial clone we'll end up with both options, and that\n> > causes the repack to fail.\n> > \n> > Fix this issue by never passing \"--exclude-promisor-objects\" when we\n> > have a geometric split factor. We don't need the option anyway when\n> > doing a geometric repack as we will only ever pack loose objects or\n> > merge multiple packs. And neither of those cases can yield a promisor\n> > object.\n> \n> I'm not sure I fully understand why --exclude-promisor-objects would not\n> be needed for geometric repacks. To clarify, do geometric repacks\n> already exclude promisor packfiles when merging? If so, then this change\n> makes sense.\n\nOkay, I had a deeper look now, and turns out my claim was completely\nwrong. We _do_ try to perform geometric repacking with promisor remotes,\nbut we don't know to handle them in any capacity:\n\n - git-pack-objects(1) just dies right away.\n\n - Even if it didn't, we would need to learn how to merge promisor\n packs.\n\nI'll drop this patch for now, thanks for prompting!\n\nPatrick\n"},{"id":"532020","messageId":"aTpbQt95JHeExceR@pks.im","threadId":"64581","inReplyTo":"gdyc7mdim2p32fesvcb672ssozoom4pdi7dyygacj3s66v7gd4@ydzwijirha3a","subject":"Re: [PATCH 2/3] builtin/gc: fix condition for whether to write commit graphs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-11T05:48:50Z","receivedAt":"2025-12-11T05:48:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 10, 2025 at 01:49:39PM -0600, Justin Tobler wrote:\n> On 25/12/05 09:19AM, Patrick Steinhardt wrote:\n> > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> > index 6b36f52df7..6f3117304f 100755\n> > --- a/t/t7900-maintenance.sh\n> > +++ b/t/t7900-maintenance.sh\n> > @@ -206,6 +206,32 @@ test_expect_success 'commit-graph auto condition' '\n> > \ttest_subcommand $COMMIT_GRAPH_WRITE > '\n> > \n> > +test_expect_success 'commit-graph auto condition with merges' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\tgit config set maintenance.auto false &&\n> > +\t\tgit commit --allow-empty -m initial &&\n> > +\t\tgit switch --create feature &&\n> > +\t\tgit commit --allow-empty -m feature-1 &&\n> > +\t\tgit commit --allow-empty -m feature-2 &&\n> > +\t\tgit switch - &&\n> > +\t\tgit commit --allow-empty -m main-1 &&\n> > +\t\tgit commit --allow-empty -m main-2 &&\n> > +\t\tgit merge feature &&\n> > +\t\tgit branch -D feature &&\n> \n> If we left the feature branch instead of deleting it, would that help\n> test that commits are not counted twice?\n\nIndeed! I couldn't make any sense of the results at the beginning of\nwriting this test, but now that I fixed the relveant bugs we can retain\nthe branch.\n\nPatrick\n"},{"id":"532023","messageId":"20251211-odb-related-fixes-v2-0-bdf875ce51fc@pks.im","threadId":"64581","inReplyTo":"20251205-odb-related-fixes-v1-0-ef4250abb584@pks.im","subject":"[PATCH v2 0/2] Some random object database related fixes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-11T07:19:57Z","receivedAt":"2025-12-11T07:20:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series fixes some small issues I've discovered while working\non some other patch series. I've decided to split it out of these\nbecause I'm hitting the same issues in multiple series, and I don't want\nthose to become dependent on one another.\n\nThe patch series is built on top of f0ef5b6d9b with\nps/object-source-management at ac65c70663 (odb: handle recreation of\nquarantine directories, 2025-11-19) merged into it.\n\nChanges in v2:\n - Drop the first commit that regards geometric repacking with promisor\n remotes. As it turns out my assertion was wrong: geometric repacks\n do and have to consider promisors, but they will fail to handle\n them. This is a bigger topic to fix though, so I'll rather want to\n move this into a separate patch series.\n - Tighten tests a bit for the commit-graph generation.\n - Stop referring to a \"subsequent\" commit that doesn't exist.\n - Link to v1: https://lore.kernel.org/r/20251205-odb-related-fixes-v1-0-ef4250abb584@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n builtin/gc: fix condition for whether to write commit graphs\n odb: properly close sources before freeing them\n\n builtin/gc.c | 8 +++++---\n odb.c | 2 +-\n t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++\n 3 files changed, 31 insertions(+), 4 deletions(-)\n\nRange-diff versus v1:\n\n1: 5c15065406 < -: ---------- builtin/repack: fix geometric repacks with promisor remotes\n2: 2fa3991003 ! 1: 1702bf6e7f builtin/gc: fix condition for whether to write commit graphs\n @@ t/t7900-maintenance.sh: test_expect_success 'commit-graph auto condition' '\n +\t\tgit commit --allow-empty -m main-1 &&\n +\t\tgit commit --allow-empty -m main-2 &&\n +\t\tgit merge feature &&\n -+\t\tgit branch -D feature &&\n +\n +\t\t# We have 6 commit, none of which are covered by a commit\n +\t\t# graph. So this must be the boundary at which we start to\n3: a06d0716c3 ! 2: 7dd4e6fabe odb: properly close sources before freeing them\n @@ Metadata\n ## Commit message ##\n odb: properly close sources before freeing them\n \n - In the next commit we are about to move the packfile store into the ODB\n - source so that we have one store per source. This will lead to a memory\n - leak in the following commit when reading data from a submodule via\n - git-grep(1):\n + It is possible to hit a memory leak when reading data from a submodule\n + via git-grep(1):\n \n Direct leak of 192 byte(s) in 1 object(s) allocated from:\n #0 0x55555562e726 in calloc (git+0xda726)\n\n---\nbase-commit: 2797238193944b52d12624a04a962f40b9bcad69\nchange-id: 20251205-odb-related-fixes-5f48a0993ef7\n\n"},{"id":"532024","messageId":"20251211-odb-related-fixes-v2-1-bdf875ce51fc@pks.im","threadId":"64581","inReplyTo":"20251211-odb-related-fixes-v2-0-bdf875ce51fc@pks.im","subject":"[PATCH v2 1/2] builtin/gc: fix condition for whether to write commit graphs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-11T07:19:58Z","receivedAt":"2025-12-11T07:20:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When performing auto-maintenance we check whether commit graphs need to\nbe generated by counting the number of commits that are reachable by any\nreference, but not covered by a commit graph. This search is performed\nby iterating through all references and then doing a depth-first search\nuntil we have found enough commits that are not present in the commit\ngraph.\n\nThis logic has a memory leak though:\n\n Direct leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x55555562e433 in malloc (git+0xda433)\n #1 0x555555964322 in do_xmalloc ../wrapper.c:55:8\n #2 0x5555559642e6 in xmalloc ../wrapper.c:76:9\n #3 0x55555579bf29 in commit_list_append ../commit.c:1872:35\n #4 0x55555569f160 in dfs_on_ref ../builtin/gc.c:1165:4\n #5 0x5555558c33fd in do_for_each_ref_iterator ../refs/iterator.c:431:12\n #6 0x5555558af520 in do_for_each_ref ../refs.c:1828:9\n #7 0x5555558ac317 in refs_for_each_ref ../refs.c:1833:9\n #8 0x55555569e207 in should_write_commit_graph ../builtin/gc.c:1188:11\n #9 0x55555569c915 in maintenance_is_needed ../builtin/gc.c:3492:8\n #10 0x55555569b76a in cmd_maintenance ../builtin/gc.c:3542:9\n #11 0x55555575166a in run_builtin ../git.c:506:11\n #12 0x5555557502f0 in handle_builtin ../git.c:779:9\n #13 0x555555751127 in run_argv ../git.c:862:4\n #14 0x55555575007b in cmd_main ../git.c:984:19\n #15 0x5555557523aa in main ../common-main.c:9:11\n #16 0x7ffff7a2a4d7 in __libc_start_call_main (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a4d7) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n #17 0x7ffff7a2a59a in __libc_start_main@GLIBC_2.2.5 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a59a) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n #18 0x5555555f0934 in _start (git+0x9c934)\n\nThe root cause of this memory leak is our use of `commit_list_append()`.\nThis function expects as parameters the item to append and the _tail_ of\nthe list to append. This tail will then be overwritten with the new tail\nof the list so that it can be used in subsequent calls. But we call it\nwith `commit_list_append(parent->item, &stack)`, so we end up losing\neverything but the new item.\n\nThis issue only surfaces when counting merge commits. Next to being a\nmemory leak, it also shows that we're in fact miscounting as we only\nrespect children of the last parent. All previous parents are discarded,\nso their children will be disregarded unless they are hit via another\nreference.\n\nWhile crafting a test case for the issue I was puzzled that I couldn't\nestablish the proper border at which the auto-condition would be\nfulfilled. As it turns out, there's another bug: if an object is at the\ntip of any reference we don't mark it as seen. Consequently, if it is\nreachable via any other reference, we'd count that object twice.\n\nFix both of these bugs so that we properly count objects without leaking\nany memory.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 8 +++++---\n t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 92c6e7b954..17ff68cbd9 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1130,8 +1130,10 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n \t\treturn 0;\n \n \tcommit = lookup_commit(the_repository, maybe_peeled);\n-\tif (!commit)\n+\tif (!commit || commit->object.flags & SEEN)\n \t\treturn 0;\n+\tcommit->object.flags |= SEEN;\n+\n \tif (repo_parse_commit(the_repository, commit) ||\n \t commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n \t\treturn 0;\n@@ -1141,7 +1143,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n \tif (data->num_not_in_graph >= data->limit)\n \t\treturn 1;\n \n-\tcommit_list_append(commit, &stack);\n+\tcommit_list_insert(commit, &stack);\n \n \twhile (!result && stack) {\n \t\tstruct commit_list *parent;\n@@ -1162,7 +1164,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n \t\t\t\tbreak;\n \t\t\t}\n \n-\t\t\tcommit_list_append(parent->item, &stack);\n+\t\t\tcommit_list_insert(parent->item, &stack);\n \t\t}\n \t}\n \ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 6b36f52df7..a2b4403595 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -206,6 +206,31 @@ test_expect_success 'commit-graph auto condition' '\n \ttest_subcommand $COMMIT_GRAPH_WRITE err &&\n \ttest_grep \"is not a valid task\" err\n\n-- \n2.52.0.270.g3f4935d65f.dirty\n\n"},{"id":"532025","messageId":"20251211-odb-related-fixes-v2-2-bdf875ce51fc@pks.im","threadId":"64581","inReplyTo":"20251211-odb-related-fixes-v2-0-bdf875ce51fc@pks.im","subject":"[PATCH v2 2/2] odb: properly close sources before freeing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-11T07:19:59Z","receivedAt":"2025-12-11T07:20:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It is possible to hit a memory leak when reading data from a submodule\nvia git-grep(1):\n\n Direct leak of 192 byte(s) in 1 object(s) allocated from:\n #0 0x55555562e726 in calloc (git+0xda726)\n #1 0x555555964734 in xcalloc ../wrapper.c:154:8\n #2 0x555555835136 in load_multi_pack_index_one ../midx.c:135:2\n #3 0x555555834fd6 in load_multi_pack_index ../midx.c:382:6\n #4 0x5555558365b6 in prepare_multi_pack_index_one ../midx.c:716:17\n #5 0x55555586c605 in packfile_store_prepare ../packfile.c:1103:3\n #6 0x55555586c90c in packfile_store_reprepare ../packfile.c:1118:2\n #7 0x5555558546b3 in odb_reprepare ../odb.c:1106:2\n #8 0x5555558539e4 in do_oid_object_info_extended ../odb.c:715:4\n #9 0x5555558533d1 in odb_read_object_info_extended ../odb.c:862:8\n #10 0x5555558540bd in odb_read_object ../odb.c:920:6\n #11 0x55555580a330 in grep_source_load_oid ../grep.c:1934:12\n #12 0x55555580a13a in grep_source_load ../grep.c:1986:10\n #13 0x555555809103 in grep_source_is_binary ../grep.c:2014:7\n #14 0x555555807574 in grep_source_1 ../grep.c:1625:8\n #15 0x555555807322 in grep_source ../grep.c:1837:10\n #16 0x5555556a5c58 in run ../builtin/grep.c:208:10\n #17 0x55555562bb42 in void* ThreadStartFunc(void*) lsan_interceptors.cpp.o\n #18 0x7ffff7a9a979 in start_thread (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x9a979) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n #19 0x7ffff7b22d2b in __GI___clone3 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x122d2b) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n\nThe root caues of this leak is the way we set up and release the\nsubmodule:\n\n 1. We use `repo_submodule_init()` to initialize a new repository. This\n repository is stored in `repos_to_free`.\n\n 2. We now read data from the submodule repository.\n\n 3. We then call `repo_clear()` on the submodule repositories.\n\n 4. `repo_clear()` calls `odb_free()`.\n\n 5. `odb_free()` calls `odb_free_sources()` followed by `odb_close()`.\n\nThe issue here is the 5th step: we call `odb_free_sources()` _before_ we\ncall `odb_close()`. But `odb_free_sources()` already frees all sources,\nso the logic that closes them in `odb_close()` now becomes a no-op. As a\nconsequence, we never explicitly close sources at all.\n\nFix the leak by closing the store before we free the sources.\n\nSigned-off-by: Patrick Steinhardt \n---\n odb.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/odb.c b/odb.c\nindex dc8f292f3d..8e67afe185 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -1132,13 +1132,13 @@ void odb_free(struct object_database *o)\n \toidmap_clear(&o->replace_map, 1);\n \tpthread_mutex_destroy(&o->replace_mutex);\n \n+\todb_close(o);\n \todb_free_sources(o);\n \n \tfor (size_t i = 0; i < o->cached_object_nr; i++)\n \t\tfree((char *) o->cached_objects[i].value.buf);\n \tfree(o->cached_objects);\n \n-\todb_close(o);\n \tpackfile_store_free(o->packfiles);\n \tstring_list_clear(&o->submodule_source_paths, 0);\n \n\n-- \n2.52.0.270.g3f4935d65f.dirty\n\n"},{"id":"532045","messageId":"87ecp1gl2z.fsf@iotcl.com","threadId":"64581","inReplyTo":"20251211-odb-related-fixes-v2-1-bdf875ce51fc@pks.im","subject":"Re: [PATCH v2 1/2] builtin/gc: fix condition for whether to write commit graphs","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-12-11T14:16:36Z","receivedAt":"2025-12-11T14:16:50Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Patrick Steinhardt writes:\n\n> The root cause of this memory leak is our use of `commit_list_append()`.\n> This function expects as parameters the item to append and the _tail_ of\n> the list to append. This tail will then be overwritten with the new tail\n> of the list so that it can be used in subsequent calls. But we call it\n> with `commit_list_append(parent->item, &stack)`, so we end up losing\n> everything but the new item.\n>\n> This issue only surfaces when counting merge commits. Next to being a\n> memory leak, it also shows that we're in fact miscounting as we only\n> respect children of the last parent. All previous parents are discarded,\n> so their children will be disregarded unless they are hit via another\n> reference.\n>\n> While crafting a test case for the issue I was puzzled that I couldn't\n> establish the proper border at which the auto-condition would be\n> fulfilled. As it turns out, there's another bug: if an object is at the\n> tip of any reference we don't mark it as seen. Consequently, if it is\n> reachable via any other reference, we'd count that object twice.\n>\n> Fix both of these bugs so that we properly count objects without leaking\n> any memory.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/gc.c | 8 +++++---\n> t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++\n> 2 files changed, 30 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 92c6e7b954..17ff68cbd9 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1130,8 +1130,10 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n> \t\treturn 0;\n> \n> \tcommit = lookup_commit(the_repository, maybe_peeled);\n> -\tif (!commit)\n> +\tif (!commit || commit->object.flags & SEEN)\n> \t\treturn 0;\n> +\tcommit->object.flags |= SEEN;\n> +\n> \tif (repo_parse_commit(the_repository, commit) ||\n> \t commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n> \t\treturn 0;\n> @@ -1141,7 +1143,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n> \tif (data->num_not_in_graph >= data->limit)\n> \t\treturn 1;\n> \n> -\tcommit_list_append(commit, &stack);\n> +\tcommit_list_insert(commit, &stack);\n\ncommit_list_insert() prepends the commit to the beginning of the list,\nwhile commit_list_append() appends it at the end. Because the list is\nonly used for counting, we don't care about the order. So this fix looks\ngood to me.\n\nI also approve the other changes in this series.\n\n-- \nCheers,\nToon\n"},{"id":"532078","messageId":"eerya3xfsrf3nb5onk6b2nefaj2ghsu3v3rhtijijht7r77rxt@chqmu6gmhq3f","threadId":"64581","inReplyTo":"20251211-odb-related-fixes-v2-0-bdf875ce51fc@pks.im","subject":"Re: [PATCH v2 0/2] Some random object database related fixes","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-12-12T15:01:11Z","receivedAt":"2025-12-12T15:01:17Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/12/11 08:19AM, Patrick Steinhardt wrote:\n> Changes in v2:\n> - Drop the first commit that regards geometric repacking with promisor\n> remotes. As it turns out my assertion was wrong: geometric repacks\n> do and have to consider promisors, but they will fail to handle\n> them. This is a bigger topic to fix though, so I'll rather want to\n> move this into a separate patch series.\n> - Tighten tests a bit for the commit-graph generation.\n> - Stop referring to a \"subsequent\" commit that doesn't exist.\n> - Link to v1: https://lore.kernel.org/r/20251205-odb-related-fixes-v1-0-ef4250abb584@pks.im\n\nThe change is this version look good to me.\n\n-Justin\n"},{"id":"533133","messageId":"CAOLa=ZSZ9PKCi=vQY8WKhwAHVZT-keA5XOXBMVrB4ZW+u2uNhg@mail.gmail.com","threadId":"64581","inReplyTo":"20251211-odb-related-fixes-v2-1-bdf875ce51fc@pks.im","subject":"Re: [PATCH v2 1/2] builtin/gc: fix condition for whether to write commit graphs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-01-06T11:27:29Z","receivedAt":"2026-01-06T11:27:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> When performing auto-maintenance we check whether commit graphs need to\n> be generated by counting the number of commits that are reachable by any\n> reference, but not covered by a commit graph. This search is performed\n> by iterating through all references and then doing a depth-first search\n> until we have found enough commits that are not present in the commit\n> graph.\n>\n> This logic has a memory leak though:\n>\n> Direct leak of 16 byte(s) in 1 object(s) allocated from:\n> #0 0x55555562e433 in malloc (git+0xda433)\n> #1 0x555555964322 in do_xmalloc ../wrapper.c:55:8\n> #2 0x5555559642e6 in xmalloc ../wrapper.c:76:9\n> #3 0x55555579bf29 in commit_list_append ../commit.c:1872:35\n> #4 0x55555569f160 in dfs_on_ref ../builtin/gc.c:1165:4\n> #5 0x5555558c33fd in do_for_each_ref_iterator ../refs/iterator.c:431:12\n> #6 0x5555558af520 in do_for_each_ref ../refs.c:1828:9\n> #7 0x5555558ac317 in refs_for_each_ref ../refs.c:1833:9\n> #8 0x55555569e207 in should_write_commit_graph ../builtin/gc.c:1188:11\n> #9 0x55555569c915 in maintenance_is_needed ../builtin/gc.c:3492:8\n> #10 0x55555569b76a in cmd_maintenance ../builtin/gc.c:3542:9\n> #11 0x55555575166a in run_builtin ../git.c:506:11\n> #12 0x5555557502f0 in handle_builtin ../git.c:779:9\n> #13 0x555555751127 in run_argv ../git.c:862:4\n> #14 0x55555575007b in cmd_main ../git.c:984:19\n> #15 0x5555557523aa in main ../common-main.c:9:11\n> #16 0x7ffff7a2a4d7 in __libc_start_call_main (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a4d7) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n> #17 0x7ffff7a2a59a in __libc_start_main@GLIBC_2.2.5 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a59a) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n> #18 0x5555555f0934 in _start (git+0x9c934)\n>\n> The root cause of this memory leak is our use of `commit_list_append()`.\n> This function expects as parameters the item to append and the _tail_ of\n> the list to append. This tail will then be overwritten with the new tail\n> of the list so that it can be used in subsequent calls. But we call it\n> with `commit_list_append(parent->item, &stack)`, so we end up losing\n> everything but the new item.\n>\n> This issue only surfaces when counting merge commits. Next to being a\n> memory leak, it also shows that we're in fact miscounting as we only\n> respect children of the last parent. All previous parents are discarded,\n> so their children will be disregarded unless they are hit via another\n> reference.\n\nYikes. So we never go down the path of the first N-1 parents? Does that\ninversely mean, commit-graph generation would be slower now in\nrepositories with lots of merges, since it is fixed to follow all paths\ncorrectly?\n\n>\n> While crafting a test case for the issue I was puzzled that I couldn't\n> establish the proper border at which the auto-condition would be\n> fulfilled. As it turns out, there's another bug: if an object is at the\n> tip of any reference we don't mark it as seen. Consequently, if it is\n> reachable via any other reference, we'd count that object twice.\n>\n\nSo if an object is at the tip of N references, we'd count it N times\nright?\n\n\n> Fix both of these bugs so that we properly count objects without leaking\n> any memory.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/gc.c | 8 +++++---\n> t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++\n> 2 files changed, 30 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 92c6e7b954..17ff68cbd9 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1130,8 +1130,10 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n> \t\treturn 0;\n>\n> \tcommit = lookup_commit(the_repository, maybe_peeled);\n> -\tif (!commit)\n> +\tif (!commit || commit->object.flags & SEEN)\n> \t\treturn 0;\n> +\tcommit->object.flags |= SEEN;\n> +\n\nMakes sense.\n\n> \tif (repo_parse_commit(the_repository, commit) ||\n> \t commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n> \t\treturn 0;\n> @@ -1141,7 +1143,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n> \tif (data->num_not_in_graph >= data->limit)\n> \t\treturn 1;\n>\n> -\tcommit_list_append(commit, &stack);\n> +\tcommit_list_insert(commit, &stack);\n>\n> \twhile (!result && stack) {\n> \t\tstruct commit_list *parent;\n> @@ -1162,7 +1164,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n> \t\t\t\tbreak;\n> \t\t\t}\n>\n> -\t\t\tcommit_list_append(parent->item, &stack);\n> +\t\t\tcommit_list_insert(parent->item, &stack);\n> \t\t}\n> \t}\n>\n\n'append' expects the tail of the list, switching to 'insert' adds the\ncommit to the top of the list.\n\nAlso adding it to the top of the list actually does a DFS like the\nfunction name suggests. While adding it to the tail would be BFS.\n\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index 6b36f52df7..a2b4403595 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -206,6 +206,31 @@ test_expect_success 'commit-graph auto condition' '\n> \ttest_subcommand $COMMIT_GRAPH_WRITE '\n>\n> +test_expect_success 'commit-graph auto condition with merges' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit config set maintenance.auto false &&\n> +\t\tgit commit --allow-empty -m initial &&\n> +\t\tgit switch --create feature &&\n> +\t\tgit commit --allow-empty -m feature-1 &&\n> +\t\tgit commit --allow-empty -m feature-2 &&\n> +\t\tgit switch - &&\n> +\t\tgit commit --allow-empty -m main-1 &&\n> +\t\tgit commit --allow-empty -m main-2 &&\n> +\t\tgit merge feature &&\n> +\n> +\t\t# We have 6 commit, none of which are covered by a commit\n> +\t\t# graph. So this must be the boundary at which we start to\n> +\t\t# perform maintenance.\n> +\t\ttest_must_fail git -c maintenance.commit-graph.auto=7 \\\n> +\t\t\tmaintenance is-needed --auto --task=commit-graph &&\n> +\t\tgit -c maintenance.commit-graph.auto=6 \\\n> +\t\t\tmaintenance is-needed --auto --task=commit-graph\n> +\t)\n> +'\n> +\n\nThis doesn't test the fix around double-counting tip objects, no?\n\n> test_expect_success 'run --task=bogus' '\n> \ttest_must_fail git maintenance run --task=bogus 2>err &&\n> \ttest_grep \"is not a valid task\" err\n>\n> --\n> 2.52.0.270.g3f4935d65f.dirty\n"},{"id":"533134","messageId":"CAOLa=ZTdLgsUcii0hunbh3t-zz4QU5weWXmqQ6KcjT8fWK_b5g@mail.gmail.com","threadId":"64581","inReplyTo":"20251211-odb-related-fixes-v2-0-bdf875ce51fc@pks.im","subject":"Re: [PATCH v2 0/2] Some random object database related fixes","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-01-06T11:29:46Z","receivedAt":"2026-01-06T11:29:48Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> this patch series fixes some small issues I've discovered while working\n> on some other patch series. I've decided to split it out of these\n> because I'm hitting the same issues in multiple series, and I don't want\n> those to become dependent on one another.\n>\n> The patch series is built on top of f0ef5b6d9b with\n> ps/object-source-management at ac65c70663 (odb: handle recreation of\n> quarantine directories, 2025-11-19) merged into it.\n>\n> Changes in v2:\n> - Drop the first commit that regards geometric repacking with promisor\n> remotes. As it turns out my assertion was wrong: geometric repacks\n> do and have to consider promisors, but they will fail to handle\n> them. This is a bigger topic to fix though, so I'll rather want to\n> move this into a separate patch series.\n> - Tighten tests a bit for the commit-graph generation.\n> - Stop referring to a \"subsequent\" commit that doesn't exist.\n> - Link to v1: https://lore.kernel.org/r/20251205-odb-related-fixes-v1-0-ef4250abb584@pks.im\n>\n> Thanks!\n>\n> Patrick\n>\n\nI think we are missing a test case in 1/2 but the series looks close to\ndone. Thanks\n\n- Karthik\n"},{"id":"533138","messageId":"aVz4G-gRf-mA2N56@pks.im","threadId":"64581","inReplyTo":"CAOLa=ZSZ9PKCi=vQY8WKhwAHVZT-keA5XOXBMVrB4ZW+u2uNhg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] builtin/gc: fix condition for whether to write commit graphs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-06T11:55:07Z","receivedAt":"2026-01-06T11:55:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 06, 2026 at 03:27:29AM -0800, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > When performing auto-maintenance we check whether commit graphs need to\n> > be generated by counting the number of commits that are reachable by any\n> > reference, but not covered by a commit graph. This search is performed\n> > by iterating through all references and then doing a depth-first search\n> > until we have found enough commits that are not present in the commit\n> > graph.\n> >\n> > This logic has a memory leak though:\n> >\n> > Direct leak of 16 byte(s) in 1 object(s) allocated from:\n> > #0 0x55555562e433 in malloc (git+0xda433)\n> > #1 0x555555964322 in do_xmalloc ../wrapper.c:55:8\n> > #2 0x5555559642e6 in xmalloc ../wrapper.c:76:9\n> > #3 0x55555579bf29 in commit_list_append ../commit.c:1872:35\n> > #4 0x55555569f160 in dfs_on_ref ../builtin/gc.c:1165:4\n> > #5 0x5555558c33fd in do_for_each_ref_iterator ../refs/iterator.c:431:12\n> > #6 0x5555558af520 in do_for_each_ref ../refs.c:1828:9\n> > #7 0x5555558ac317 in refs_for_each_ref ../refs.c:1833:9\n> > #8 0x55555569e207 in should_write_commit_graph ../builtin/gc.c:1188:11\n> > #9 0x55555569c915 in maintenance_is_needed ../builtin/gc.c:3492:8\n> > #10 0x55555569b76a in cmd_maintenance ../builtin/gc.c:3542:9\n> > #11 0x55555575166a in run_builtin ../git.c:506:11\n> > #12 0x5555557502f0 in handle_builtin ../git.c:779:9\n> > #13 0x555555751127 in run_argv ../git.c:862:4\n> > #14 0x55555575007b in cmd_main ../git.c:984:19\n> > #15 0x5555557523aa in main ../common-main.c:9:11\n> > #16 0x7ffff7a2a4d7 in __libc_start_call_main (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a4d7) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n> > #17 0x7ffff7a2a59a in __libc_start_main@GLIBC_2.2.5 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a59a) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n> > #18 0x5555555f0934 in _start (git+0x9c934)\n> >\n> > The root cause of this memory leak is our use of `commit_list_append()`.\n> > This function expects as parameters the item to append and the _tail_ of\n> > the list to append. This tail will then be overwritten with the new tail\n> > of the list so that it can be used in subsequent calls. But we call it\n> > with `commit_list_append(parent->item, &stack)`, so we end up losing\n> > everything but the new item.\n> >\n> > This issue only surfaces when counting merge commits. Next to being a\n> > memory leak, it also shows that we're in fact miscounting as we only\n> > respect children of the last parent. All previous parents are discarded,\n> > so their children will be disregarded unless they are hit via another\n> > reference.\n> \n> Yikes. So we never go down the path of the first N-1 parents? Does that\n> inversely mean, commit-graph generation would be slower now in\n> repositories with lots of merges, since it is fixed to follow all paths\n> correctly?\n\nYeah, this was quite broken indeed. I don't think that the fixed walk\nshould result in a significant slowdown:\n\n - We stop walking the parent chain whenever we see a commit that is\n covered by the commit-graph.\n\n - And our walk of commits that are not covered by the commit-graph is\n bounded by \"maintenance.commit-graph.auto\", which defaults to 100\n commits.\n\nSo in practice, the cost should be negligible.\n\nThere's going to be some exceptions thoulgh. Most importantly, the\ncomplexity of the computation scales directly with the number of refs as\nwe use `refs_for_each_ref()`. So if you have a gazillion refs I'd expect\nthe performance impact to become noticeable. But that's already been the\ncase before this commit.\n\nWe may want to revisit this in the future if we ever notice that this\ndoes become an issue.\n\n> > While crafting a test case for the issue I was puzzled that I couldn't\n> > establish the proper border at which the auto-condition would be\n> > fulfilled. As it turns out, there's another bug: if an object is at the\n> > tip of any reference we don't mark it as seen. Consequently, if it is\n> > reachable via any other reference, we'd count that object twice.\n> >\n> \n> So if an object is at the tip of N references, we'd count it N times\n> right?\n\nYeah, exactly. Let me rephrase that slightly.\n\n> > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> > index 6b36f52df7..a2b4403595 100755\n> > --- a/t/t7900-maintenance.sh\n> > +++ b/t/t7900-maintenance.sh\n> > @@ -206,6 +206,31 @@ test_expect_success 'commit-graph auto condition' '\n> > \ttest_subcommand $COMMIT_GRAPH_WRITE > '\n> >\n> > +test_expect_success 'commit-graph auto condition with merges' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\tgit config set maintenance.auto false &&\n> > +\t\tgit commit --allow-empty -m initial &&\n> > +\t\tgit switch --create feature &&\n> > +\t\tgit commit --allow-empty -m feature-1 &&\n> > +\t\tgit commit --allow-empty -m feature-2 &&\n> > +\t\tgit switch - &&\n> > +\t\tgit commit --allow-empty -m main-1 &&\n> > +\t\tgit commit --allow-empty -m main-2 &&\n> > +\t\tgit merge feature &&\n> > +\n> > +\t\t# We have 6 commit, none of which are covered by a commit\n> > +\t\t# graph. So this must be the boundary at which we start to\n> > +\t\t# perform maintenance.\n> > +\t\ttest_must_fail git -c maintenance.commit-graph.auto=7 \\\n> > +\t\t\tmaintenance is-needed --auto --task=commit-graph &&\n> > +\t\tgit -c maintenance.commit-graph.auto=6 \\\n> > +\t\t\tmaintenance is-needed --auto --task=commit-graph\n> > +\t)\n> > +'\n> > +\n> \n> This doesn't test the fix around double-counting tip objects, no?\n\nHm, true. I initially used `test_commit()` here, which uncovered the\ndouble-counting as it creates lightweight tags by default. Let me revert\nback to use that function so that we exercise this again.\n\nThanks!\n\nPatrick\n"},{"id":"533140","messageId":"20260106-odb-related-fixes-v3-0-7ac157207b20@pks.im","threadId":"64581","inReplyTo":"20251205-odb-related-fixes-v1-0-ef4250abb584@pks.im","subject":"[PATCH v3 0/2] Some random object database related fixes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-06T12:58:48Z","receivedAt":"2026-01-06T12:59:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series fixes some small issues I've discovered while working\non some other patch series. I've decided to split it out of these\nbecause I'm hitting the same issues in multiple series, and I don't want\nthose to become dependent on one another.\n\nThe patch series is built on top of f0ef5b6d9b with\nps/object-source-management at ac65c70663 (odb: handle recreation of\nquarantine directories, 2025-11-19) merged into it.\n\nChanges in v3:\n - Use `test_commit ()` so that we the same object at multiple tips.\n - Slightly reword the commit message.\n - Link to v2: https://lore.kernel.org/r/20251211-odb-related-fixes-v2-0-bdf875ce51fc@pks.im\n\nChanges in v2:\n - Drop the first commit that regards geometric repacking with promisor\n remotes. As it turns out my assertion was wrong: geometric repacks\n do and have to consider promisors, but they will fail to handle\n them. This is a bigger topic to fix though, so I'll rather want to\n move this into a separate patch series.\n - Tighten tests a bit for the commit-graph generation.\n - Stop referring to a \"subsequent\" commit that doesn't exist.\n - Link to v1: https://lore.kernel.org/r/20251205-odb-related-fixes-v1-0-ef4250abb584@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n builtin/gc: fix condition for whether to write commit graphs\n odb: properly close sources before freeing them\n\n builtin/gc.c | 8 +++++---\n odb.c | 2 +-\n t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++\n 3 files changed, 31 insertions(+), 4 deletions(-)\n\nRange-diff versus v2:\n\n1: 564b26fa6b ! 1: 3ef6ea3560 builtin/gc: fix condition for whether to write commit graphs\n @@ Commit message\n establish the proper border at which the auto-condition would be\n fulfilled. As it turns out, there's another bug: if an object is at the\n tip of any reference we don't mark it as seen. Consequently, if it is\n - reachable via any other reference, we'd count that object twice.\n + the tip of or reachable via another ref, we'd count that object multiple\n + times.\n \n Fix both of these bugs so that we properly count objects without leaking\n any memory.\n @@ t/t7900-maintenance.sh: test_expect_success 'commit-graph auto condition' '\n +\t(\n +\t\tcd repo &&\n +\t\tgit config set maintenance.auto false &&\n -+\t\tgit commit --allow-empty -m initial &&\n ++\t\ttest_commit initial &&\n +\t\tgit switch --create feature &&\n -+\t\tgit commit --allow-empty -m feature-1 &&\n -+\t\tgit commit --allow-empty -m feature-2 &&\n ++\t\ttest_commit feature-1 &&\n ++\t\ttest_commit feature-2 &&\n +\t\tgit switch - &&\n -+\t\tgit commit --allow-empty -m main-1 &&\n -+\t\tgit commit --allow-empty -m main-2 &&\n ++\t\ttest_commit main-1 &&\n ++\t\ttest_commit main-2 &&\n +\t\tgit merge feature &&\n +\n -+\t\t# We have 6 commit, none of which are covered by a commit\n ++\t\t# We have 6 commits, none of which are covered by a commit\n +\t\t# graph. So this must be the boundary at which we start to\n +\t\t# perform maintenance.\n +\t\ttest_must_fail git -c maintenance.commit-graph.auto=7 \\\n2: 20bb4741eb = 2: 55cad3ea0f odb: properly close sources before freeing them\n\n---\nbase-commit: 2797238193944b52d12624a04a962f40b9bcad69\nchange-id: 20251205-odb-related-fixes-5f48a0993ef7\n\n"},{"id":"533141","messageId":"20260106-odb-related-fixes-v3-1-7ac157207b20@pks.im","threadId":"64581","inReplyTo":"20260106-odb-related-fixes-v3-0-7ac157207b20@pks.im","subject":"[PATCH v3 1/2] builtin/gc: fix condition for whether to write commit graphs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-06T12:58:49Z","receivedAt":"2026-01-06T12:59:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When performing auto-maintenance we check whether commit graphs need to\nbe generated by counting the number of commits that are reachable by any\nreference, but not covered by a commit graph. This search is performed\nby iterating through all references and then doing a depth-first search\nuntil we have found enough commits that are not present in the commit\ngraph.\n\nThis logic has a memory leak though:\n\n Direct leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x55555562e433 in malloc (git+0xda433)\n #1 0x555555964322 in do_xmalloc ../wrapper.c:55:8\n #2 0x5555559642e6 in xmalloc ../wrapper.c:76:9\n #3 0x55555579bf29 in commit_list_append ../commit.c:1872:35\n #4 0x55555569f160 in dfs_on_ref ../builtin/gc.c:1165:4\n #5 0x5555558c33fd in do_for_each_ref_iterator ../refs/iterator.c:431:12\n #6 0x5555558af520 in do_for_each_ref ../refs.c:1828:9\n #7 0x5555558ac317 in refs_for_each_ref ../refs.c:1833:9\n #8 0x55555569e207 in should_write_commit_graph ../builtin/gc.c:1188:11\n #9 0x55555569c915 in maintenance_is_needed ../builtin/gc.c:3492:8\n #10 0x55555569b76a in cmd_maintenance ../builtin/gc.c:3542:9\n #11 0x55555575166a in run_builtin ../git.c:506:11\n #12 0x5555557502f0 in handle_builtin ../git.c:779:9\n #13 0x555555751127 in run_argv ../git.c:862:4\n #14 0x55555575007b in cmd_main ../git.c:984:19\n #15 0x5555557523aa in main ../common-main.c:9:11\n #16 0x7ffff7a2a4d7 in __libc_start_call_main (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a4d7) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n #17 0x7ffff7a2a59a in __libc_start_main@GLIBC_2.2.5 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a59a) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n #18 0x5555555f0934 in _start (git+0x9c934)\n\nThe root cause of this memory leak is our use of `commit_list_append()`.\nThis function expects as parameters the item to append and the _tail_ of\nthe list to append. This tail will then be overwritten with the new tail\nof the list so that it can be used in subsequent calls. But we call it\nwith `commit_list_append(parent->item, &stack)`, so we end up losing\neverything but the new item.\n\nThis issue only surfaces when counting merge commits. Next to being a\nmemory leak, it also shows that we're in fact miscounting as we only\nrespect children of the last parent. All previous parents are discarded,\nso their children will be disregarded unless they are hit via another\nreference.\n\nWhile crafting a test case for the issue I was puzzled that I couldn't\nestablish the proper border at which the auto-condition would be\nfulfilled. As it turns out, there's another bug: if an object is at the\ntip of any reference we don't mark it as seen. Consequently, if it is\nthe tip of or reachable via another ref, we'd count that object multiple\ntimes.\n\nFix both of these bugs so that we properly count objects without leaking\nany memory.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 8 +++++---\n t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 92c6e7b954..17ff68cbd9 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1130,8 +1130,10 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n \t\treturn 0;\n \n \tcommit = lookup_commit(the_repository, maybe_peeled);\n-\tif (!commit)\n+\tif (!commit || commit->object.flags & SEEN)\n \t\treturn 0;\n+\tcommit->object.flags |= SEEN;\n+\n \tif (repo_parse_commit(the_repository, commit) ||\n \t commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n \t\treturn 0;\n@@ -1141,7 +1143,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n \tif (data->num_not_in_graph >= data->limit)\n \t\treturn 1;\n \n-\tcommit_list_append(commit, &stack);\n+\tcommit_list_insert(commit, &stack);\n \n \twhile (!result && stack) {\n \t\tstruct commit_list *parent;\n@@ -1162,7 +1164,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data)\n \t\t\t\tbreak;\n \t\t\t}\n \n-\t\t\tcommit_list_append(parent->item, &stack);\n+\t\t\tcommit_list_insert(parent->item, &stack);\n \t\t}\n \t}\n \ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 6b36f52df7..7cc0ce57f8 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -206,6 +206,31 @@ test_expect_success 'commit-graph auto condition' '\n \ttest_subcommand $COMMIT_GRAPH_WRITE err &&\n \ttest_grep \"is not a valid task\" err\n\n-- \n2.52.0.508.g883dcfc63e.dirty\n\n"},{"id":"533142","messageId":"20260106-odb-related-fixes-v3-2-7ac157207b20@pks.im","threadId":"64581","inReplyTo":"20260106-odb-related-fixes-v3-0-7ac157207b20@pks.im","subject":"[PATCH v3 2/2] odb: properly close sources before freeing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-06T12:58:50Z","receivedAt":"2026-01-06T12:59:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It is possible to hit a memory leak when reading data from a submodule\nvia git-grep(1):\n\n Direct leak of 192 byte(s) in 1 object(s) allocated from:\n #0 0x55555562e726 in calloc (git+0xda726)\n #1 0x555555964734 in xcalloc ../wrapper.c:154:8\n #2 0x555555835136 in load_multi_pack_index_one ../midx.c:135:2\n #3 0x555555834fd6 in load_multi_pack_index ../midx.c:382:6\n #4 0x5555558365b6 in prepare_multi_pack_index_one ../midx.c:716:17\n #5 0x55555586c605 in packfile_store_prepare ../packfile.c:1103:3\n #6 0x55555586c90c in packfile_store_reprepare ../packfile.c:1118:2\n #7 0x5555558546b3 in odb_reprepare ../odb.c:1106:2\n #8 0x5555558539e4 in do_oid_object_info_extended ../odb.c:715:4\n #9 0x5555558533d1 in odb_read_object_info_extended ../odb.c:862:8\n #10 0x5555558540bd in odb_read_object ../odb.c:920:6\n #11 0x55555580a330 in grep_source_load_oid ../grep.c:1934:12\n #12 0x55555580a13a in grep_source_load ../grep.c:1986:10\n #13 0x555555809103 in grep_source_is_binary ../grep.c:2014:7\n #14 0x555555807574 in grep_source_1 ../grep.c:1625:8\n #15 0x555555807322 in grep_source ../grep.c:1837:10\n #16 0x5555556a5c58 in run ../builtin/grep.c:208:10\n #17 0x55555562bb42 in void* ThreadStartFunc(void*) lsan_interceptors.cpp.o\n #18 0x7ffff7a9a979 in start_thread (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x9a979) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n #19 0x7ffff7b22d2b in __GI___clone3 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x122d2b) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n\nThe root caues of this leak is the way we set up and release the\nsubmodule:\n\n 1. We use `repo_submodule_init()` to initialize a new repository. This\n repository is stored in `repos_to_free`.\n\n 2. We now read data from the submodule repository.\n\n 3. We then call `repo_clear()` on the submodule repositories.\n\n 4. `repo_clear()` calls `odb_free()`.\n\n 5. `odb_free()` calls `odb_free_sources()` followed by `odb_close()`.\n\nThe issue here is the 5th step: we call `odb_free_sources()` _before_ we\ncall `odb_close()`. But `odb_free_sources()` already frees all sources,\nso the logic that closes them in `odb_close()` now becomes a no-op. As a\nconsequence, we never explicitly close sources at all.\n\nFix the leak by closing the store before we free the sources.\n\nSigned-off-by: Patrick Steinhardt \n---\n odb.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/odb.c b/odb.c\nindex dc8f292f3d..8e67afe185 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -1132,13 +1132,13 @@ void odb_free(struct object_database *o)\n \toidmap_clear(&o->replace_map, 1);\n \tpthread_mutex_destroy(&o->replace_mutex);\n \n+\todb_close(o);\n \todb_free_sources(o);\n \n \tfor (size_t i = 0; i < o->cached_object_nr; i++)\n \t\tfree((char *) o->cached_objects[i].value.buf);\n \tfree(o->cached_objects);\n \n-\todb_close(o);\n \tpackfile_store_free(o->packfiles);\n \tstring_list_clear(&o->submodule_source_paths, 0);\n \n\n-- \n2.52.0.508.g883dcfc63e.dirty\n\n"},{"id":"533152","messageId":"CAOLa=ZT8_vij=2TU3GNZSST0N8Oj1CmaOd0ZzBcp32N8Aze0WQ@mail.gmail.com","threadId":"64581","inReplyTo":"20260106-odb-related-fixes-v3-0-7ac157207b20@pks.im","subject":"Re: [PATCH v3 0/2] Some random object database related fixes","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-01-06T16:30:16Z","receivedAt":"2026-01-06T16:30:20Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> this patch series fixes some small issues I've discovered while working\n> on some other patch series. I've decided to split it out of these\n> because I'm hitting the same issues in multiple series, and I don't want\n> those to become dependent on one another.\n>\n> The patch series is built on top of f0ef5b6d9b with\n> ps/object-source-management at ac65c70663 (odb: handle recreation of\n> quarantine directories, 2025-11-19) merged into it.\n>\n> Changes in v3:\n> - Use `test_commit ()` so that we the same object at multiple tips.\n> - Slightly reword the commit message.\n> - Link to v2: https://lore.kernel.org/r/20251211-odb-related-fixes-v2-0-bdf875ce51fc@pks.im\n>\n> Changes in v2:\n> - Drop the first commit that regards geometric repacking with promisor\n> remotes. As it turns out my assertion was wrong: geometric repacks\n> do and have to consider promisors, but they will fail to handle\n> them. This is a bigger topic to fix though, so I'll rather want to\n> move this into a separate patch series.\n> - Tighten tests a bit for the commit-graph generation.\n> - Stop referring to a \"subsequent\" commit that doesn't exist.\n> - Link to v1: https://lore.kernel.org/r/20251205-odb-related-fixes-v1-0-ef4250abb584@pks.im\n>\n> Thanks!\n>\n> Patrick\n>\n> ---\n> Patrick Steinhardt (2):\n> builtin/gc: fix condition for whether to write commit graphs\n> odb: properly close sources before freeing them\n>\n> builtin/gc.c | 8 +++++---\n> odb.c | 2 +-\n> t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++\n> 3 files changed, 31 insertions(+), 4 deletions(-)\n>\n> Range-diff versus v2:\n>\n> 1: 564b26fa6b ! 1: 3ef6ea3560 builtin/gc: fix condition for whether to write commit graphs\n> @@ Commit message\n> establish the proper border at which the auto-condition would be\n> fulfilled. As it turns out, there's another bug: if an object is at the\n> tip of any reference we don't mark it as seen. Consequently, if it is\n> - reachable via any other reference, we'd count that object twice.\n> + the tip of or reachable via another ref, we'd count that object multiple\n> + times.\n>\n> Fix both of these bugs so that we properly count objects without leaking\n> any memory.\n> @@ t/t7900-maintenance.sh: test_expect_success 'commit-graph auto condition' '\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit config set maintenance.auto false &&\n> -+\t\tgit commit --allow-empty -m initial &&\n> ++\t\ttest_commit initial &&\n> +\t\tgit switch --create feature &&\n> -+\t\tgit commit --allow-empty -m feature-1 &&\n> -+\t\tgit commit --allow-empty -m feature-2 &&\n> ++\t\ttest_commit feature-1 &&\n> ++\t\ttest_commit feature-2 &&\n> +\t\tgit switch - &&\n> -+\t\tgit commit --allow-empty -m main-1 &&\n> -+\t\tgit commit --allow-empty -m main-2 &&\n> ++\t\ttest_commit main-1 &&\n> ++\t\ttest_commit main-2 &&\n> +\t\tgit merge feature &&\n> +\n> -+\t\t# We have 6 commit, none of which are covered by a commit\n> ++\t\t# We have 6 commits, none of which are covered by a commit\n> +\t\t# graph. So this must be the boundary at which we start to\n> +\t\t# perform maintenance.\n> +\t\ttest_must_fail git -c maintenance.commit-graph.auto=7 \\\n> 2: 20bb4741eb = 2: 55cad3ea0f odb: properly close sources before freeing them\n>\n> ---\n> base-commit: 2797238193944b52d12624a04a962f40b9bcad69\n> change-id: 20251205-odb-related-fixes-5f48a0993ef7\n\nThe changes in this version looks good to me! :)\n"},{"id":"533153","messageId":"CAOLa=ZSawYRJ5_P=YQuG1zCPb=9hJS52O_JiAdbHCjb2wbohQg@mail.gmail.com","threadId":"64581","inReplyTo":"aVz4G-gRf-mA2N56@pks.im","subject":"Re: [PATCH v2 1/2] builtin/gc: fix condition for whether to write commit graphs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-01-06T16:33:33Z","receivedAt":"2026-01-06T16:33:36Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Tue, Jan 06, 2026 at 03:27:29AM -0800, Karthik Nayak wrote:\n>> Patrick Steinhardt writes:\n>>\n>> > When performing auto-maintenance we check whether commit graphs need to\n>> > be generated by counting the number of commits that are reachable by any\n>> > reference, but not covered by a commit graph. This search is performed\n>> > by iterating through all references and then doing a depth-first search\n>> > until we have found enough commits that are not present in the commit\n>> > graph.\n>> >\n>> > This logic has a memory leak though:\n>> >\n>> > Direct leak of 16 byte(s) in 1 object(s) allocated from:\n>> > #0 0x55555562e433 in malloc (git+0xda433)\n>> > #1 0x555555964322 in do_xmalloc ../wrapper.c:55:8\n>> > #2 0x5555559642e6 in xmalloc ../wrapper.c:76:9\n>> > #3 0x55555579bf29 in commit_list_append ../commit.c:1872:35\n>> > #4 0x55555569f160 in dfs_on_ref ../builtin/gc.c:1165:4\n>> > #5 0x5555558c33fd in do_for_each_ref_iterator ../refs/iterator.c:431:12\n>> > #6 0x5555558af520 in do_for_each_ref ../refs.c:1828:9\n>> > #7 0x5555558ac317 in refs_for_each_ref ../refs.c:1833:9\n>> > #8 0x55555569e207 in should_write_commit_graph ../builtin/gc.c:1188:11\n>> > #9 0x55555569c915 in maintenance_is_needed ../builtin/gc.c:3492:8\n>> > #10 0x55555569b76a in cmd_maintenance ../builtin/gc.c:3542:9\n>> > #11 0x55555575166a in run_builtin ../git.c:506:11\n>> > #12 0x5555557502f0 in handle_builtin ../git.c:779:9\n>> > #13 0x555555751127 in run_argv ../git.c:862:4\n>> > #14 0x55555575007b in cmd_main ../git.c:984:19\n>> > #15 0x5555557523aa in main ../common-main.c:9:11\n>> > #16 0x7ffff7a2a4d7 in __libc_start_call_main (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a4d7) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n>> > #17 0x7ffff7a2a59a in __libc_start_main@GLIBC_2.2.5 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a59a) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)\n>> > #18 0x5555555f0934 in _start (git+0x9c934)\n>> >\n>> > The root cause of this memory leak is our use of `commit_list_append()`.\n>> > This function expects as parameters the item to append and the _tail_ of\n>> > the list to append. This tail will then be overwritten with the new tail\n>> > of the list so that it can be used in subsequent calls. But we call it\n>> > with `commit_list_append(parent->item, &stack)`, so we end up losing\n>> > everything but the new item.\n>> >\n>> > This issue only surfaces when counting merge commits. Next to being a\n>> > memory leak, it also shows that we're in fact miscounting as we only\n>> > respect children of the last parent. All previous parents are discarded,\n>> > so their children will be disregarded unless they are hit via another\n>> > reference.\n>>\n>> Yikes. So we never go down the path of the first N-1 parents? Does that\n>> inversely mean, commit-graph generation would be slower now in\n>> repositories with lots of merges, since it is fixed to follow all paths\n>> correctly?\n>\n> Yeah, this was quite broken indeed. I don't think that the fixed walk\n> should result in a significant slowdown:\n>\n> - We stop walking the parent chain whenever we see a commit that is\n> covered by the commit-graph.\n>\n\nYup that holds true.\n\n> - And our walk of commits that are not covered by the commit-graph is\n> bounded by \"maintenance.commit-graph.auto\", which defaults to 100\n> commits.\n\nAh, I didn't know this. Okay so there is a sensible cap here.\n\n> So in practice, the cost should be negligible.\n>\n> There's going to be some exceptions thoulgh. Most importantly, the\n> complexity of the computation scales directly with the number of refs as\n> we use `refs_for_each_ref()`. So if you have a gazillion refs I'd expect\n> the performance impact to become noticeable. But that's already been the\n> case before this commit.\n>\n> We may want to revisit this in the future if we ever notice that this\n> does become an issue.\n>\n\nFair enough. Either ways, your fixes were _required_. So glad that we\ngot it fixed.\n\n[snip]\n"},{"id":"533178","messageId":"xmqqo6n6hygx.fsf@gitster.g","threadId":"64581","inReplyTo":"CAOLa=ZT8_vij=2TU3GNZSST0N8Oj1CmaOd0ZzBcp32N8Aze0WQ@mail.gmail.com","subject":"Re: [PATCH v3 0/2] Some random object database related fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-07T03:51:26Z","receivedAt":"2026-01-07T03:51:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Patrick Steinhardt writes:\n>\n>> base-commit: 2797238193944b52d12624a04a962f40b9bcad69\n>> change-id: 20251205-odb-related-fixes-5f48a0993ef7\n>\n> The changes in this version looks good to me! :)\n\nThanks, both. Let's mark it for 'next'.\n"}]}