{"thread":{"id":"65656","subject":"[PATCH] commit: fall back to full read when maybe_tree is NULL","startedAt":"2026-05-19T05:05:15Z","lastAt":"2026-05-20T16:22:04Z","messageCount":6,"participants":["Jeff King","Junio C Hamano","Rasmus Villemoes","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"543585","messageId":"20260519050513.GA1635924@coredump.intra.peff.net","threadId":"65656","inReplyTo":null,"subject":"[PATCH] commit: fall back to full read when maybe_tree is NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-19T05:05:13Z","receivedAt":"2026-05-19T05:05:15Z","isPatch":true,"body":"When we load a commit object from the commit graph (rather than reading\nthe object contents), we don't fill in its \"maybe_tree\" entry, but\nrather wait to lazy-load it. This goes back to 7b8a21dba1 (commit-graph:\nlazy-load trees for commits, 2018-04-06), and saves the work of\ninstantiating tree objects that nobody cares about.\n\nBut it creates a data dependency: now the commit struct depends on the\ngraph file to do that lazy load. This is a problem if we close the graph\nfile; now we have a commit struct that claims to be parsed but is\nmissing some of its data.\n\nIt's rare for this to be a problem in practice, because we don't tend to\nclose the graph files at all, and if we do we don't tend to look at\ntheir commits afterward. But there is one case that is easy to trigger:\ngit-clone's --dissociate option will close the object database before\nrunning the dissociate repack, and then afterwards still try to check\nout the working tree. This will yield an error like:\n\n  fatal: unable to parse commit b29edc0babef41810f7b1c9ee1d74058f22e4080\n  warning: Clone succeeded, but checkout failed.\n\nWhat happens is that we expect repo_get_commit_tree() to lazy-load the\ntree, but commit_graph_position() returns COMMIT_NOT_FROM_GRAPH because\nthe position slab has gone away (and even if it hadn't, we don't have\nthe graph file itself available anymore).\n\nLet's try harder to find the tree in repo_get_commit_tree() by actually\nopening the commit object and parsing the tree line. This is extra work,\nbut no more than we'd have to go to if we hadn't done the initial graph\nload in the first place.\n\nIt does mean that a corrupt commit (e.g., one that points to a non-tree\nobject for which we couldn't instantiate a struct) will repeatedly load\nthe object from disk, once for each call to repo_get_commit_tree(). But\nsuch corruptions should be rare, and we don't tend to perform such calls\nrepeatedly (usually we'd abort the operation upon seeing corruption).\n\nIt also means we have to reimplement a bit of the commit parsing. We\ncan't just use parse_commit_buffer() here, because it expects an\nunparsed struct and wants to load everything, including parent links.\nBut we don't know if the parent list has been munged during traversal,\nso it's not safe for us to touch it. Fortunately, it's quite easy to\nload just the tree, as it is always the first line of the commit object.\n\nThere is an alternative approach which I considered but rejected:\n\"complete\" each graph-loaded commit struct when we close the graph file\nby looking up and instantiating their trees at close time. This is the\nmost elegant solution in some sense, as it resolves the data dependency\nat the moment it goes away. And it avoids ever opening the commit\nobjects at all, which can be more efficient.\n\nBut not always. The resolving effort scales with the number of\ngraph-loaded commits, even though we may only later access one or a few.\nSo the tradeoff depends on how many were loaded in total versus how many\nwill be later accessed.\n\nAnd in most cases, we will not access any at all! Programs which close\nthe object database before exiting will then do a bunch of work for no\nreason. This could be mitigated by requiring a separate function to\nresolve the graph structs before closing the file. But now each close\ncall has to consider whether to call that resolving function. So we'd\nfix this case in git-clone, but we don't know what other cases (if any)\nare lurking.\n\nMoreover, this strategy does nothing if we lose access to the graph file\nunexpectedly (e.g., due to a system error). I'm not entirely sure this\nis possible now (we mmap it, so I'd guess any error would turn into\nSIGBUS anyway). But it feels like making the lazy-load more robust\n(which this patch does) is the best way to handle a wide variety of\npossible failure modes.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nReported twice recently:\n\n - https://lore.kernel.org/git/87h5onsi0f.fsf@prevas.dk/\n\n - https://lore.kernel.org/git/6ae85515-9373-4c9e-90d2-5e4176590c5b@suse.com/\n\nI don't why we suddenly got two reports. AFAICT the bug goes back to\n2018, though it would become more prominent as use of commit graphs\nincreased.\n\n commit.c                   | 33 ++++++++++++++++++++++++++++++++-\n t/t5604-clone-reference.sh | 23 +++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex 4385ae4329..cfc87ad185 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -434,6 +434,27 @@ static inline void set_commit_tree(struct commit *c, struct tree *t)\n \tc->maybe_tree = t;\n }\n \n+static void load_tree_from_commit_contents(struct repository *r, struct commit *commit)\n+{\n+\tenum object_type type;\n+\tunsigned long size;\n+\tchar *buf;\n+\tconst char *p;\n+\tstruct object_id tree_oid;\n+\n+\tbuf = odb_read_object(r->objects, &commit->object.oid, &type, &size);\n+\tif (!buf)\n+\t\treturn;\n+\n+\tif (type == OBJ_COMMIT &&\n+\t    skip_prefix(buf, \"tree \", &p) &&\n+\t    !parse_oid_hex(p, &tree_oid, &p) &&\n+\t    *p == '\\n')\n+\t\tset_commit_tree(commit, lookup_tree(r, &tree_oid));\n+\n+\tfree(buf);\n+}\n+\n struct tree *repo_get_commit_tree(struct repository *r,\n \t\t\t\t  const struct commit *commit)\n {\n@@ -443,7 +464,17 @@ struct tree *repo_get_commit_tree(struct repository *r,\n \tif (commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n \t\treturn get_commit_tree_in_graph(r, commit);\n \n-\treturn NULL;\n+\t/*\n+\t * This is either a corrupt commit, or one which we partially loaded\n+\t * from a graph file but then subsequently threw away the graph data.\n+\t *\n+\t * Optimistically assume it's the latter and try to reload from\n+\t * scratch. This gives a performance penalty if it really is a corrupt\n+\t * commit, but presumably that happens rarely (and only once per\n+\t * process).\n+\t */\n+\tload_tree_from_commit_contents(r, (struct commit *)commit);\n+\treturn commit->maybe_tree;\n }\n \n struct object_id *get_commit_tree_oid(const struct commit *commit)\ndiff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh\nindex 470bfb610c..c232ab8c15 100755\n--- a/t/t5604-clone-reference.sh\n+++ b/t/t5604-clone-reference.sh\n@@ -360,4 +360,27 @@ test_expect_success SYMLINKS 'clone repo with symlinked objects directory' '\n \tgrep \"is a symlink, refusing to clone with --local\" err\n '\n \n+test_expect_success 'dissociate from repo with commit graph' '\n+\tgit init orig &&\n+\t# We are trying to make sure the dissociated repo can\n+\t# find the tree of the tip commit, so the test could still\n+\t# serve its purpose with an empty tree. But having actual\n+\t# content future-proofs us against any kind of internal\n+\t# empty-tree optimizations.\n+\techo content >orig/file &&\n+\tgit -C orig add . &&\n+\tgit -C orig commit -m foo &&\n+\n+\t# We will use graph.git as our \"local\" source to dissociate\n+\t# from.\n+\tgit clone --bare orig graph.git &&\n+\tgit -C graph.git commit-graph write --reachable &&\n+\n+\t# And then finally clone orig, using graph.git to get our objects. This\n+\t# must be non-bare so that we perform the checkout step, which will\n+\t# need to access the tree of HEAD, which we will have originally loaded\n+\t# via the commit graph.\n+\tgit clone --no-local --reference graph.git --dissociate orig clone\n+'\n+\n test_done\n-- \n2.54.0.524.g198262df96\n"},{"id":"543588","messageId":"xmqqcxys7xi4.fsf@gitster.g","threadId":"65656","inReplyTo":"20260519050513.GA1635924@coredump.intra.peff.net","subject":"Re: [PATCH] commit: fall back to full read when maybe_tree is NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-19T05:56:51Z","receivedAt":"2026-05-19T05:56:53Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> It also means we have to reimplement a bit of the commit parsing. We\n> can't just use parse_commit_buffer() here, because it expects an\n> unparsed struct and wants to load everything, including parent links.\n> But we don't know if the parent list has been munged during traversal,\n> so it's not safe for us to touch it. Fortunately, it's quite easy to\n> load just the tree, as it is always the first line of the commit object.\n\nI was hoping that existing code to parse out the tree in\nparse_commit_buffer() will become a call into this new helper\nfunction, so that we avoid duplicating the logic.\n\n> Moreover, this strategy does nothing if we lose access to the graph file\n> unexpectedly (e.g., due to a system error).\n\nOr simultaneous repack may lose the file from the filesystem,\nperhaps?\n\n> +static void load_tree_from_commit_contents(struct repository *r, struct commit *commit)\n> +{\n> +\tenum object_type type;\n> +\tunsigned long size;\n> +\tchar *buf;\n> +\tconst char *p;\n> +\tstruct object_id tree_oid;\n> +\n> +\tbuf = odb_read_object(r->objects, &commit->object.oid, &type, &size);\n> +\tif (!buf)\n> +\t\treturn;\n> +\n> +\tif (type == OBJ_COMMIT &&\n> +\t    skip_prefix(buf, \"tree \", &p) &&\n> +\t    !parse_oid_hex(p, &tree_oid, &p) &&\n> +\t    *p == '\\n')\n> +\t\tset_commit_tree(commit, lookup_tree(r, &tree_oid));\n> +\n> +\tfree(buf);\n> +}\n\nLooks quite straight-forward.  Don't you need to pay attention to\nr->hash_algo and call parse_oid_hex_algop() instead?\n\nOr are we pretty much sure that \"r\" is always \"the_repository\" here,\nin which case parse_oid_hex() that uses \"the_hash_algo\" would be\nsufficient?\n\nThanks.\n"},{"id":"543590","messageId":"20260519061534.GA1709881@coredump.intra.peff.net","threadId":"65656","inReplyTo":"xmqqcxys7xi4.fsf@gitster.g","subject":"Re: [PATCH] commit: fall back to full read when maybe_tree is NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-19T06:15:34Z","receivedAt":"2026-05-19T06:15:35Z","isPatch":true,"body":"On Tue, May 19, 2026 at 02:56:51PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > It also means we have to reimplement a bit of the commit parsing. We\n> > can't just use parse_commit_buffer() here, because it expects an\n> > unparsed struct and wants to load everything, including parent links.\n> > But we don't know if the parent list has been munged during traversal,\n> > so it's not safe for us to touch it. Fortunately, it's quite easy to\n> > load just the tree, as it is always the first line of the commit object.\n> \n> I was hoping that existing code to parse out the tree in\n> parse_commit_buffer() will become a call into this new helper\n> function, so that we avoid duplicating the logic.\n\nYeah, I would like to have shared more code, but I think the amount that\ncan actually be shared gets overwhelmed by boilerplate. In particular,\nparse_commit_buffer() wants to keep advancing the pointer afterwards,\nsince it actually reads the other lines.\n\n> > Moreover, this strategy does nothing if we lose access to the graph file\n> > unexpectedly (e.g., due to a system error).\n> \n> Or simultaneous repack may lose the file from the filesystem,\n> perhaps?\n\nI don't think so, because our mmap would hold onto the contents until\nthe process ends. You'd really need some case where we actually drop the\nmmap. I could see us doing that if we found that it was corrupted or\nsomething, but I don't think that happens currently. We close it only\nfor odb_close(), or when writing a new graph file (and so it would only\naffect the \"commit-graph write\" process itself).\n\n> Looks quite straight-forward.  Don't you need to pay attention to\n> r->hash_algo and call parse_oid_hex_algop() instead?\n> \n> Or are we pretty much sure that \"r\" is always \"the_repository\" here,\n> in which case parse_oid_hex() that uses \"the_hash_algo\" would be\n> sufficient?\n\nNo, I didn't even think about it, since the use of the_hash_algo is\nhidden behind the function. We definitely should use the hash algo from\n\"r\", since we have access to it. I'm not even sure if you can have repos\nof two different hashes loaded in the same process at this point, but\ncertainly it is the correct long-term direction.\n\nHere's a re-roll with the one-line fixup:\n\n    diff --git a/commit.c b/commit.c\n    index cfc87ad185..499a9602ad 100644\n    --- a/commit.c\n    +++ b/commit.c\n    @@ -448,7 +448,7 @@ static void load_tree_from_commit_contents(struct repository *r, struct commit *\n     \n     \tif (type == OBJ_COMMIT &&\n     \t    skip_prefix(buf, \"tree \", &p) &&\n    -\t    !parse_oid_hex(p, &tree_oid, &p) &&\n    +\t    !parse_oid_hex_algop(p, &tree_oid, &p, r->hash_algo) &&\n     \t    *p == '\\n')\n     \t\tset_commit_tree(commit, lookup_tree(r, &tree_oid));\n     \n\n-- >8 --\nSubject: commit: fall back to full read when maybe_tree is NULL\n\nWhen we load a commit object from the commit graph (rather than reading\nthe object contents), we don't fill in its \"maybe_tree\" entry, but\nrather wait to lazy-load it. This goes back to 7b8a21dba1 (commit-graph:\nlazy-load trees for commits, 2018-04-06), and saves the work of\ninstantiating tree objects that nobody cares about.\n\nBut it creates a data dependency: now the commit struct depends on the\ngraph file to do that lazy load. This is a problem if we close the graph\nfile; now we have a commit struct that claims to be parsed but is\nmissing some of its data.\n\nIt's rare for this to be a problem in practice, because we don't tend to\nclose the graph files at all, and if we do we don't tend to look at\ntheir commits afterward. But there is one case that is easy to trigger:\ngit-clone's --dissociate option will close the object database before\nrunning the dissociate repack, and then afterwards still try to check\nout the working tree. This will yield an error like:\n\n  fatal: unable to parse commit b29edc0babef41810f7b1c9ee1d74058f22e4080\n  warning: Clone succeeded, but checkout failed.\n\nWhat happens is that we expect repo_get_commit_tree() to lazy-load the\ntree, but commit_graph_position() returns COMMIT_NOT_FROM_GRAPH because\nthe position slab has gone away (and even if it hadn't, we don't have\nthe graph file itself available anymore).\n\nLet's try harder to find the tree in repo_get_commit_tree() by actually\nopening the commit object and parsing the tree line. This is extra work,\nbut no more than we'd have to go to if we hadn't done the initial graph\nload in the first place.\n\nIt does mean that a corrupt commit (e.g., one that points to a non-tree\nobject for which we couldn't instantiate a struct) will repeatedly load\nthe object from disk, once for each call to repo_get_commit_tree(). But\nsuch corruptions should be rare, and we don't tend to perform such calls\nrepeatedly (usually we'd abort the operation upon seeing corruption).\n\nIt also means we have to reimplement a bit of the commit parsing. We\ncan't just use parse_commit_buffer() here, because it expects an\nunparsed struct and wants to load everything, including parent links.\nBut we don't know if the parent list has been munged during traversal,\nso it's not safe for us to touch it. Fortunately, it's quite easy to\nload just the tree, as it is always the first line of the commit object.\n\nThere is an alternative approach which I considered but rejected:\n\"complete\" each graph-loaded commit struct when we close the graph file\nby looking up and instantiating their trees at close time. This is the\nmost elegant solution in some sense, as it resolves the data dependency\nat the moment it goes away. And it avoids ever opening the commit\nobjects at all, which can be more efficient.\n\nBut not always. The resolving effort scales with the number of\ngraph-loaded commits, even though we may only later access one or a few.\nSo the tradeoff depends on how many were loaded in total versus how many\nwill be later accessed.\n\nAnd in most cases, we will not access any at all! Programs which close\nthe object database before exiting will then do a bunch of work for no\nreason. This could be mitigated by requiring a separate function to\nresolve the graph structs before closing the file. But now each close\ncall has to consider whether to call that resolving function. So we'd\nfix this case in git-clone, but we don't know what other cases (if any)\nare lurking.\n\nMoreover, this strategy does nothing if we lose access to the graph file\nunexpectedly (e.g., due to a system error). I'm not entirely sure this\nis possible now (we mmap it, so I'd guess any error would turn into\nSIGBUS anyway). But it feels like making the lazy-load more robust\n(which this patch does) is the best way to handle a wide variety of\npossible failure modes.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c                   | 33 ++++++++++++++++++++++++++++++++-\n t/t5604-clone-reference.sh | 23 +++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex 4385ae4329..499a9602ad 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -434,6 +434,27 @@ static inline void set_commit_tree(struct commit *c, struct tree *t)\n \tc->maybe_tree = t;\n }\n \n+static void load_tree_from_commit_contents(struct repository *r, struct commit *commit)\n+{\n+\tenum object_type type;\n+\tunsigned long size;\n+\tchar *buf;\n+\tconst char *p;\n+\tstruct object_id tree_oid;\n+\n+\tbuf = odb_read_object(r->objects, &commit->object.oid, &type, &size);\n+\tif (!buf)\n+\t\treturn;\n+\n+\tif (type == OBJ_COMMIT &&\n+\t    skip_prefix(buf, \"tree \", &p) &&\n+\t    !parse_oid_hex_algop(p, &tree_oid, &p, r->hash_algo) &&\n+\t    *p == '\\n')\n+\t\tset_commit_tree(commit, lookup_tree(r, &tree_oid));\n+\n+\tfree(buf);\n+}\n+\n struct tree *repo_get_commit_tree(struct repository *r,\n \t\t\t\t  const struct commit *commit)\n {\n@@ -443,7 +464,17 @@ struct tree *repo_get_commit_tree(struct repository *r,\n \tif (commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n \t\treturn get_commit_tree_in_graph(r, commit);\n \n-\treturn NULL;\n+\t/*\n+\t * This is either a corrupt commit, or one which we partially loaded\n+\t * from a graph file but then subsequently threw away the graph data.\n+\t *\n+\t * Optimistically assume it's the latter and try to reload from\n+\t * scratch. This gives a performance penalty if it really is a corrupt\n+\t * commit, but presumably that happens rarely (and only once per\n+\t * process).\n+\t */\n+\tload_tree_from_commit_contents(r, (struct commit *)commit);\n+\treturn commit->maybe_tree;\n }\n \n struct object_id *get_commit_tree_oid(const struct commit *commit)\ndiff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh\nindex 470bfb610c..c232ab8c15 100755\n--- a/t/t5604-clone-reference.sh\n+++ b/t/t5604-clone-reference.sh\n@@ -360,4 +360,27 @@ test_expect_success SYMLINKS 'clone repo with symlinked objects directory' '\n \tgrep \"is a symlink, refusing to clone with --local\" err\n '\n \n+test_expect_success 'dissociate from repo with commit graph' '\n+\tgit init orig &&\n+\t# We are trying to make sure the dissociated repo can\n+\t# find the tree of the tip commit, so the test could still\n+\t# serve its purpose with an empty tree. But having actual\n+\t# content future-proofs us against any kind of internal\n+\t# empty-tree optimizations.\n+\techo content >orig/file &&\n+\tgit -C orig add . &&\n+\tgit -C orig commit -m foo &&\n+\n+\t# We will use graph.git as our \"local\" source to dissociate\n+\t# from.\n+\tgit clone --bare orig graph.git &&\n+\tgit -C graph.git commit-graph write --reachable &&\n+\n+\t# And then finally clone orig, using graph.git to get our objects. This\n+\t# must be non-bare so that we perform the checkout step, which will\n+\t# need to access the tree of HEAD, which we will have originally loaded\n+\t# via the commit graph.\n+\tgit clone --no-local --reference graph.git --dissociate orig clone\n+'\n+\n test_done\n-- \n2.54.0.547.gb3b6f86dd6\n\n"},{"id":"543593","messageId":"87o6ibex0u.fsf@prevas.dk","threadId":"65656","inReplyTo":"20260519050513.GA1635924@coredump.intra.peff.net","subject":"Re: [PATCH] commit: fall back to full read when maybe_tree is NULL","fromName":"Rasmus Villemoes","fromEmail":"ravi@prevas.dk","sentAt":"2026-05-19T06:25:21Z","receivedAt":"2026-05-19T06:25:28Z","isPatch":true,"body":"On Tue, May 19 2026, Jeff King <peff@peff.net> wrote:\n\n> When we load a commit object from the commit graph (rather than reading\n> the object contents), we don't fill in its \"maybe_tree\" entry, but\n> rather wait to lazy-load it. This goes back to 7b8a21dba1 (commit-graph:\n> lazy-load trees for commits, 2018-04-06), and saves the work of\n> instantiating tree objects that nobody cares about.\n>\n> But it creates a data dependency: now the commit struct depends on the\n> graph file to do that lazy load. This is a problem if we close the graph\n> file; now we have a commit struct that claims to be parsed but is\n> missing some of its data.\n>\n> It's rare for this to be a problem in practice, because we don't tend to\n> close the graph files at all, and if we do we don't tend to look at\n> their commits afterward. But there is one case that is easy to trigger:\n> git-clone's --dissociate option will close the object database before\n> running the dissociate repack, and then afterwards still try to check\n> out the working tree. This will yield an error like:\n>\n>   fatal: unable to parse commit b29edc0babef41810f7b1c9ee1d74058f22e4080\n>   warning: Clone succeeded, but checkout failed.\n>\n> What happens is that we expect repo_get_commit_tree() to lazy-load the\n> tree, but commit_graph_position() returns COMMIT_NOT_FROM_GRAPH because\n> the position slab has gone away (and even if it hadn't, we don't have\n> the graph file itself available anymore).\n>\n> Let's try harder to find the tree in repo_get_commit_tree() by actually\n> opening the commit object and parsing the tree line. This is extra work,\n> but no more than we'd have to go to if we hadn't done the initial graph\n> load in the first place.\n\nI can confirm that this, applied on top of v2.54.0, fixes the problem\nfor the instance I had.\n\nTested-by: Rasmus Villemoes <ravi@prevas.dk>\n\nThanks,\nRasmus\n"},{"id":"543744","messageId":"431a3b73-1819-4798-a0ba-b7351efe6aa1@gmail.com","threadId":"65656","inReplyTo":"20260519050513.GA1635924@coredump.intra.peff.net","subject":"Re: [PATCH] commit: fall back to full read when maybe_tree is NULL","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-05-20T16:20:36Z","receivedAt":"2026-05-20T16:20:39Z","isPatch":true,"body":"On 5/19/2026 1:05 AM, Jeff King wrote:\n> When we load a commit object from the commit graph (rather than reading\n> the object contents), we don't fill in its \"maybe_tree\" entry, but\n> rather wait to lazy-load it. This goes back to 7b8a21dba1 (commit-graph:\n> lazy-load trees for commits, 2018-04-06), and saves the work of\n> instantiating tree objects that nobody cares about.\n> \n> But it creates a data dependency: now the commit struct depends on the\n> graph file to do that lazy load. This is a problem if we close the graph\n> file; now we have a commit struct that claims to be parsed but is\n> missing some of its data.\n\n\n> Reported twice recently:\n> \n>  - https://lore.kernel.org/git/87h5onsi0f.fsf@prevas.dk/\n> \n>  - https://lore.kernel.org/git/6ae85515-9373-4c9e-90d2-5e4176590c5b@suse.com/\n> \n> I don't why we suddenly got two reports. AFAICT the bug goes back to\n> 2018, though it would become more prominent as use of commit graphs\n> increased.\n\nLikely, this may have changed with the switch to using geometric\nmaintenance instead of gc maintenance by default in Git 2.54.0. That\nperhaps increased the amount of commit-graphs being present.\n> +static void load_tree_from_commit_contents(struct repository *r, struct commit *commit)\n> +{\n> +\tenum object_type type;\n> +\tunsigned long size;\n> +\tchar *buf;\n> +\tconst char *p;\n> +\tstruct object_id tree_oid;\n> +\n> +\tbuf = odb_read_object(r->objects, &commit->object.oid, &type, &size);\n> +\tif (!buf)\n> +\t\treturn;\n> +\n> +\tif (type == OBJ_COMMIT &&\n> +\t    skip_prefix(buf, \"tree \", &p) &&\n> +\t    !parse_oid_hex(p, &tree_oid, &p) &&\n> +\t    *p == '\\n')\n> +\t\tset_commit_tree(commit, lookup_tree(r, &tree_oid));\n> +\n> +\tfree(buf);\n> +}\n> +\n\nI like this focused parsing of the commit contents. I also briefly\nconsidered \"unparsing\" the commit, but you make a good point in your\nmessage why a focused parse here is important, especially around\nmunging of the parent list.\n\n>  struct tree *repo_get_commit_tree(struct repository *r,\n>  \t\t\t\t  const struct commit *commit)\n>  {\n> @@ -443,7 +464,17 @@ struct tree *repo_get_commit_tree(struct repository *r,\n>  \tif (commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n>  \t\treturn get_commit_tree_in_graph(r, commit);\n>  \n> -\treturn NULL;\n> +\t/*\n> +\t * This is either a corrupt commit, or one which we partially loaded\n> +\t * from a graph file but then subsequently threw away the graph data.\n> +\t *\n> +\t * Optimistically assume it's the latter and try to reload from\n> +\t * scratch. This gives a performance penalty if it really is a corrupt\n> +\t * commit, but presumably that happens rarely (and only once per\n> +\t * process).\n> +\t */\n> +\tload_tree_from_commit_contents(r, (struct commit *)commit);\n> +\treturn commit->maybe_tree;\n>  }\n\nI agree that this is the right place to insert this logic.\n\n> +test_expect_success 'dissociate from repo with commit graph' '\n> +\tgit init orig &&\n> +\t# We are trying to make sure the dissociated repo can\n> +\t# find the tree of the tip commit, so the test could still\n> +\t# serve its purpose with an empty tree. But having actual\n> +\t# content future-proofs us against any kind of internal\n> +\t# empty-tree optimizations.\n> +\techo content >orig/file &&\n> +\tgit -C orig add . &&\n> +\tgit -C orig commit -m foo &&\n> +\n> +\t# We will use graph.git as our \"local\" source to dissociate\n> +\t# from.\n> +\tgit clone --bare orig graph.git &&\n> +\tgit -C graph.git commit-graph write --reachable &&\n> +\n> +\t# And then finally clone orig, using graph.git to get our objects. This\n> +\t# must be non-bare so that we perform the checkout step, which will\n> +\t# need to access the tree of HEAD, which we will have originally loaded\n> +\t# via the commit graph.\n> +\tgit clone --no-local --reference graph.git --dissociate orig clone\n> +'\n> +\nThanks for the clear extra coverage here.\n\n-Stolee\n\n"},{"id":"543745","messageId":"478ff417-d5d0-458f-b5cd-472373eed7b2@gmail.com","threadId":"65656","inReplyTo":"20260519061534.GA1709881@coredump.intra.peff.net","subject":"Re: [PATCH] commit: fall back to full read when maybe_tree is NULL","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-05-20T16:22:02Z","receivedAt":"2026-05-20T16:22:04Z","isPatch":true,"body":"On 5/19/2026 2:15 AM, Jeff King wrote:\n> On Tue, May 19, 2026 at 02:56:51PM +0900, Junio C Hamano wrote:\n\n>> Looks quite straight-forward.  Don't you need to pay attention to\n>> r->hash_algo and call parse_oid_hex_algop() instead?\n>>\n>> Or are we pretty much sure that \"r\" is always \"the_repository\" here,\n>> in which case parse_oid_hex() that uses \"the_hash_algo\" would be\n>> sufficient?\n> \n> No, I didn't even think about it, since the use of the_hash_algo is\n> hidden behind the function. We definitely should use the hash algo from\n> \"r\", since we have access to it. I'm not even sure if you can have repos\n> of two different hashes loaded in the same process at this point, but\n> certainly it is the correct long-term direction.\n> \n> Here's a re-roll with the one-line fixup:\n> \n>     diff --git a/commit.c b/commit.c\n>     index cfc87ad185..499a9602ad 100644\n>     --- a/commit.c\n>     +++ b/commit.c\n>     @@ -448,7 +448,7 @@ static void load_tree_from_commit_contents(struct repository *r, struct commit *\n>      \n>      \tif (type == OBJ_COMMIT &&\n>      \t    skip_prefix(buf, \"tree \", &p) &&\n>     -\t    !parse_oid_hex(p, &tree_oid, &p) &&\n>     +\t    !parse_oid_hex_algop(p, &tree_oid, &p, r->hash_algo) &&\n>      \t    *p == '\\n')\n>      \t\tset_commit_tree(commit, lookup_tree(r, &tree_oid));\n>      \n\nI figured that this was already tested via the test variable that\nruns the test with SHA256, but the multi-repo case is an interesting\none that I'm sure would catch us at some point in the future.\n\nI'm happy with the re-roll here.\n\nThanks,\n-Stolee\n\n\n"}]}