{"thread":{"id":"60686","subject":"[PATCH] commit-graph: retain commit slab when closing NULL commit_graph","startedAt":"2024-01-05T05:41:48Z","lastAt":"2024-01-11T18:35:34Z","messageCount":7,"participants":["Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"486338","messageId":"20240105054142.GA2035092@coredump.intra.peff.net","threadId":"60686","inReplyTo":null,"subject":"[PATCH] commit-graph: retain commit slab when closing NULL commit_graph","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-05T05:41:42Z","receivedAt":"2024-01-05T05:41:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This fixes a regression introduced in ac6d45d11f (commit-graph: move\nslab-clearing to close_commit_graph(), 2023-10-03), in which running:\n\n  git -c fetch.writeCommitGraph=true fetch --recurse-submodules\n\nmultiple times in a freshly cloned repository causes a segfault. What\nhappens in the second (and subsequent) runs is this:\n\n  1. We make a \"struct commit\" for any ref tips which we're storing\n     (even if we already have them, they still go into FETCH_HEAD).\n\n     Because the first run will have created a commit graph, we'll find\n     those commits in the graph.\n\n     The commit struct is therefore created with a NULL \"maybe_tree\"\n     entry, because we can load its oid from the graph later. But to do\n     that we need to remember that we got the commit from the graph,\n     which is recorded in a global commit_graph_data_slab object.\n\n  2. Because we're using --recurse-submodules, we'll try to fetch each\n     of the possible submodules. That implies creating a separate\n     \"struct repository\" in-process for each submodule, which will\n     require a later call to repo_clear().\n\n     The call to repo_clear() calls raw_object_store_clear(), which in\n     turn calls close_object_store(), which in turn calls\n     close_commit_graph(). And the latter frees the commit graph data\n     slab.\n\n  3. Later, when trying to write out a new commit graph, we'll ask for\n     their tree oid via get_commit_tree_oid(), which will see that the\n     object is parsed but with a NULL maybe_tree field. We'd then\n     usually pull it from the graph file, but because the slab was\n     cleared, we don't realize that we can do so! We end up returning\n     NULL and segfaulting.\n\n     (It seems questionable that we'd write a graph entry for such a\n     commit anyway, since we know we already have one. I didn't\n     double-check, but that may simply be another side effect of having\n     cleared the slab).\n\nThe bug is in step (2) above. We should not be clearing the slab when\ncleaning up the submodule repository structs. Prior to ac6d45d11f, we\ndid not do so because it was done inside a helper function that returned\nearly when it saw NULL. So the behavior change from that commit is that\nwe'll now _always_ clear the slab via repo_clear(), even if the\nrepository being closed did not have a commit graph (and thus would have\na NULL commit_graph struct).\n\nThe most immediate fix is to add in a NULL check in close_commit_graph(),\nmaking it a true noop when passed in an object_store with a NULL\ncommit_graph (it's OK to just return early, since the rest of its code\nis already a noop when passed NULL). That restores the pre-ac6d45d11f\nbehavior. And that's what this patch does, along with a test that\nexercises it (we already have a test that uses submodules along with\nfetch.writeCommitGraph, but the bug only triggers when there is a\nsubsequent fetch and when that fetch uses --recurse-submodules).\n\nSo that fixes the regression in the least-risky way possible.\n\nI do think there's some fragility here that we might want to follow up\non. We have a global commit_graph_data_slab that contains graph\npositions, and our global commit structs depend on the that slab\nremaining valid. But close_commit_graph() is just about closing _one_\nobject store's graph. So it's dangerous to call that function and clear\nthe slab without also throwing away any \"struct commit\" we might have\nparsed that depends on it.\n\nWhich at first glance seems like a bug we could already trigger. In the\nsituation described here, there is no commit graph in the submodule\nrepository, so our commit graph is NULL (in fact, in our test script\nthere is no submodule repo at all, so we immediately return from\nrepo_init() and call repo_clear() only to free up memory). But what\nwould happen if there was one? Wouldn't we see a non-NULL commit_graph\nentry, and then clear the global slab anyway?\n\nThe answer is \"no\", but for very bizarre reasons. Remember that\nrepo_clear() calls raw_object_store_clear(), which then calls\nclose_object_store() and thus close_commit_graph(). But before it does\nso, raw_object_store_clear() does something else: it frees the commit\ngraph and sets it to NULL! So by this code path we'll _never_ see a\nnon-NULL commit_graph struct, and thus never clear the slab.\n\nSo it happens to work out. But it still seems questionable to me that we\nwould clear a global slab (which might still be in use) when closing the\ncommit graph. This clearing comes from 957ba814bf (commit-graph: when\nclosing the graph, also release the slab, 2021-09-08), and was fixing a\ncase where we really did need it to be closed (and in that case we\npresumably call close_object_store() more directly).\n\nSo I suspect there may still be a bug waiting to happen there, as any\nobject loaded before the call to close_object_store() may be stranded\nwith a bogus maybe_tree entry (and thus looking at it after the call\nmight cause an error). But I'm not sure how to trigger it, nor what the\nfix should look like (you probably would need to \"unparse\" any objects\npulled from the graph). And so this patch punts on that for now in favor\nof fixing the recent regression in the most direct way, which should not\nhave any other fallouts.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI prepared this on top of jk/commit-graph-leak-fixes, which is the\nbranch in v2.43.0 that introduced the regression.\n\n commit-graph.c   | 3 +++\n t/t5510-fetch.sh | 3 ++-\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex e4d09da090..f26503295a 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -727,6 +727,9 @@ struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r)\n \n void close_commit_graph(struct raw_object_store *o)\n {\n+\tif (!o->commit_graph)\n+\t\treturn;\n+\n \tclear_commit_graph_data_slab(&commit_graph_data_slab);\n \tfree_commit_graph(o->commit_graph);\n \to->commit_graph = NULL;\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 19c36b57f4..bcf524d549 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -802,7 +802,8 @@ test_expect_success 'fetch.writeCommitGraph with submodules' '\n \t\tcd super-clone &&\n \t\trm -rf .git/objects/info &&\n \t\tgit -c fetch.writeCommitGraph=true fetch origin &&\n-\t\ttest_path_is_file .git/objects/info/commit-graphs/commit-graph-chain\n+\t\ttest_path_is_file .git/objects/info/commit-graphs/commit-graph-chain &&\n+\t\tgit -c fetch.writeCommitGraph=true fetch --recurse-submodules origin\n \t)\n '\n \n-- \n2.43.0.514.g7147b80757\n"},{"id":"486348","messageId":"ZZg3YIEDCjbbGyVX@nand.local","threadId":"60686","inReplyTo":"20240105054142.GA2035092@coredump.intra.peff.net","subject":"Re: [PATCH] commit-graph: retain commit slab when closing NULL commit_graph","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-05T17:07:44Z","receivedAt":"2024-01-05T17:07:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jan 05, 2024 at 12:41:42AM -0500, Jeff King wrote:\n> This fixes a regression introduced in ac6d45d11f (commit-graph: move\n> slab-clearing to close_commit_graph(), 2023-10-03), in which running:\n>\n>   git -c fetch.writeCommitGraph=true fetch --recurse-submodules\n>\n> multiple times in a freshly cloned repository causes a segfault. What\n> happens in the second (and subsequent) runs is this:\n>\n>   1. We make a \"struct commit\" for any ref tips which we're storing\n>      (even if we already have them, they still go into FETCH_HEAD).\n>\n>      Because the first run will have created a commit graph, we'll find\n>      those commits in the graph.\n>\n>      The commit struct is therefore created with a NULL \"maybe_tree\"\n>      entry, because we can load its oid from the graph later. But to do\n>      that we need to remember that we got the commit from the graph,\n>      which is recorded in a global commit_graph_data_slab object.\n>\n>   2. Because we're using --recurse-submodules, we'll try to fetch each\n>      of the possible submodules. That implies creating a separate\n>      \"struct repository\" in-process for each submodule, which will\n>      require a later call to repo_clear().\n>\n>      The call to repo_clear() calls raw_object_store_clear(), which in\n>      turn calls close_object_store(), which in turn calls\n>      close_commit_graph(). And the latter frees the commit graph data\n>      slab.\n>\n>   3. Later, when trying to write out a new commit graph, we'll ask for\n>      their tree oid via get_commit_tree_oid(), which will see that the\n>      object is parsed but with a NULL maybe_tree field. We'd then\n>      usually pull it from the graph file, but because the slab was\n>      cleared, we don't realize that we can do so! We end up returning\n>      NULL and segfaulting.\n>\n>      (It seems questionable that we'd write a graph entry for such a\n>      commit anyway, since we know we already have one. I didn't\n>      double-check, but that may simply be another side effect of having\n>      cleared the slab).\n\nYeah, I agree that we should not be re-writing a commit-graph entry that\nalready exists in an earlier (non-removed) layer of the commit-graph.\nI haven't thought through all of the details here, but that sounds like\na potentially dangerous thing to be doing.\n\n> So that fixes the regression in the least-risky way possible.\n\nThanks for the detailed explanation. I think that the fix presented here\nis a reasonable approach, and is worthwhile to pick up.\n\n> I do think there's some fragility here that we might want to follow up\n> on. We have a global commit_graph_data_slab that contains graph\n> positions, and our global commit structs depend on the that slab\n> remaining valid. But close_commit_graph() is just about closing _one_\n> object store's graph. So it's dangerous to call that function and clear\n> the slab without also throwing away any \"struct commit\" we might have\n> parsed that depends on it.\n\nI definitely agree that there seems like a high risk of another\npotentially-dangerous bug happening as a result of this area.\n\nOne thing that sticks out to me is that we have a scope mismatch\nbetween the commit structs, the raw_object_store, and the commit slabs.\nSlabs are tied to the lifetime of the raw_object_store, but the commit\nobjects are tied to the global lifetime of the process. I wonder if it\nwould make sense to have a slab per object-store, and require that you\npass both the commit *and* the object-store you're looking it up in as\narguments to any slab-related functions.\n\nI dunno. I have not put a ton of thought into that ^^ approach either,\nso let me know if it seems obviously bogus to you or if this is worth\nlooking into further.\n\nThanks,\nTaylor\n"},{"id":"486355","messageId":"xmqqedevo8ps.fsf@gitster.g","threadId":"60686","inReplyTo":"20240105054142.GA2035092@coredump.intra.peff.net","subject":"Re: [PATCH] commit-graph: retain commit slab when closing NULL commit_graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-05T19:56:15Z","receivedAt":"2024-01-05T19:56:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This fixes a regression introduced in ac6d45d11f (commit-graph: move\n> slab-clearing to close_commit_graph(), 2023-10-03), in which running:\n> ...\n> So it happens to work out. But it still seems questionable to me that we\n> would clear a global slab (which might still be in use) when closing the\n> commit graph. This clearing comes from 957ba814bf (commit-graph: when\n> closing the graph, also release the slab, 2021-09-08), and was fixing a\n> case where we really did need it to be closed (and in that case we\n> presumably call close_object_store() more directly).\n\nWow, that is nasty.\n\nBut anyway, thanks for your usual \"3 pages of explanation for 2\nlines of change\".  The patch does look the best fix in the meantime.\n"},{"id":"486519","messageId":"20240110113914.GE16674@coredump.intra.peff.net","threadId":"60686","inReplyTo":"ZZg3YIEDCjbbGyVX@nand.local","subject":"Re: [PATCH] commit-graph: retain commit slab when closing NULL commit_graph","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-10T11:39:14Z","receivedAt":"2024-01-10T11:39:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 05, 2024 at 12:07:44PM -0500, Taylor Blau wrote:\n\n> > I do think there's some fragility here that we might want to follow up\n> > on. We have a global commit_graph_data_slab that contains graph\n> > positions, and our global commit structs depend on the that slab\n> > remaining valid. But close_commit_graph() is just about closing _one_\n> > object store's graph. So it's dangerous to call that function and clear\n> > the slab without also throwing away any \"struct commit\" we might have\n> > parsed that depends on it.\n> \n> I definitely agree that there seems like a high risk of another\n> potentially-dangerous bug happening as a result of this area.\n> \n> One thing that sticks out to me is that we have a scope mismatch\n> between the commit structs, the raw_object_store, and the commit slabs.\n> Slabs are tied to the lifetime of the raw_object_store, but the commit\n> objects are tied to the global lifetime of the process. I wonder if it\n> would make sense to have a slab per object-store, and require that you\n> pass both the commit *and* the object-store you're looking it up in as\n> arguments to any slab-related functions.\n\nRight, that scope mismatch is the crux of the issue. I had imagined that\nif you're really closing the object database you might want to just\nthrow away all of the object structs. But we don't have a way to do that\n(and in general I'd be slightly scared if we did, since lots of code\nlikes to think of object pointers as immutable).\n\nI don't know if your solution works, though. If I \"partially\" load a\ncommit (i.e., its \"parsed\" flag is 1, but maybe_tree is still NULL, not\nhaving been loaded from the graph yet), then that commit object depends\non the slab. If we later ask the commit about its tree, then it needs to\nrefer to the slab to see if it came from the graph. Right now that\nhappens in the global slab. If we did it in a per-object-store slab,\nwe'd need each commit to know which object store it came from!\n\nOf course we do have a \"struct repository\" argument in\nrepo_get_commit_tree(). So...maybe that's enough? It still doesn't get\nus all the way there, though. In this case the culprit was submodules,\nand we really would be closing an alternate repo. But we have actual\ncalls to close_commit_graph() sprinkled around. Any code which closes\nany commit-graph and then still accesses a \"struct commit\" is\npotentially buggy.\n\nI think a simpler solution would be that upon clearing the slab, we\neither \"finish\" each commit (filling in the maybe_tree field) or\n\"unparse\" it (unsetting the parsed flag, and then doing a regular and/or\ngraph lookup if it is accessed again later).\n\nIt should be easy-ish to iterate through the slab and look at the\ncommits that are mentioned in it. Though maybe not? Each commit knows\nits slab-id, but I'm not sure if we have a master list of commits to go\nthe other way.\n\nSo yet another alternative is: don't clear the slab. Mark the entries as\n\"this came from a graph, but we don't know the correct graph position\nanymore\" (presumably with a new sentinel value for data->graph_pos). And\nthen functions like repo_get_commit_tree() would know that they need to\ntry harder (e.g., doing the unparse then and going back to the object\nstore to parse again).\n\nHmm. Actually, maybe COMMIT_NOT_FROM_GRAPH already functions that way.\nIf we see a commit that is parsed but has a NULL maybe_tree, then if we\nsee COMMIT_NOT_FROM_GRAPH we assume it's just a corrupt commit and\nreturn NULL. But we could just try harder to re-parse it then. Like:\n\ndiff --git a/commit.c b/commit.c\nindex ef679a0b93..909898cf7f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -423,6 +423,24 @@ 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+\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.\n+\t *\n+\t * This will throw away the parents list, which is potentially sketchy.\n+\t * A better version of this would just unset commit->object.parsed\n+\t * and then do a minimal version of parse_commit() that _just_ loads\n+\t * the tree data (and/or graph position if available).\n+\t *\n+\t * Naturally we'd need to drop the \"const\" from our commit above, too.\n+\t */\n+\tunparse_commit(r, &commit->object.oid);\n+\trepo_parse_commit(r, commit);\n+\n \treturn NULL;\n }\n \nI dunno. I do feel like this is a lurking maintenance headache, and\nmight even be a triggerable bug. But without knowing of a way that it\nhappens in the current code base, it feels like it would be easy to make\nthings worse rather than better.\n\nI'm content to leave it for now and let this discussion serve as a\nreference if somebody does manage to run into it in practice.\n\n-Peff\n"},{"id":"486540","messageId":"xmqq34v5dtz9.fsf@gitster.g","threadId":"60686","inReplyTo":"20240110113914.GE16674@coredump.intra.peff.net","subject":"Re: [PATCH] commit-graph: retain commit slab when closing NULL commit_graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T16:38:18Z","receivedAt":"2024-01-10T16:38:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think a simpler solution would be that upon clearing the slab, we\n> either \"finish\" each commit (filling in the maybe_tree field) or\n> \"unparse\" it (unsetting the parsed flag, and then doing a regular and/or\n> graph lookup if it is accessed again later).\n\nWow, that's clever.\n\n> It should be easy-ish to iterate through the slab and look at the\n> commits that are mentioned in it. Though maybe not? Each commit knows\n> its slab-id, but I'm not sure if we have a master list of commits to go\n> the other way.\n\nWe have table of all in-core objects, don't we?\n\n> +\t * This will throw away the parents list, which is potentially sketchy.\n> +\t * A better version of this would just unset commit->object.parsed\n> +\t * and then do a minimal version of parse_commit() that _just_ loads\n> +\t * the tree data (and/or graph position if available).\n\nYeah, it is a concern that we may be working with an in-core commit\nobject whose parent list has already been rewritten during revision\ntraversal.  Well thought out.\n\n> +\t *\n> +\t * Naturally we'd need to drop the \"const\" from our commit above, too.\n> +\t */\n> +\tunparse_commit(r, &commit->object.oid);\n> +\trepo_parse_commit(r, commit);\n> +\n>  \treturn NULL;\n>  }\n>  \n> I dunno. I do feel like this is a lurking maintenance headache, and\n> might even be a triggerable bug. But without knowing of a way that it\n> happens in the current code base, it feels like it would be easy to make\n> things worse rather than better.\n\nUnfortunately I share the feeling X-<.\n\nThanks.\n"},{"id":"486606","messageId":"20240111075313.GF48154@coredump.intra.peff.net","threadId":"60686","inReplyTo":"xmqq34v5dtz9.fsf@gitster.g","subject":"Re: [PATCH] commit-graph: retain commit slab when closing NULL commit_graph","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-11T07:53:13Z","receivedAt":"2024-01-11T07:53:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 10, 2024 at 08:38:18AM -0800, Junio C Hamano wrote:\n\n> > It should be easy-ish to iterate through the slab and look at the\n> > commits that are mentioned in it. Though maybe not? Each commit knows\n> > its slab-id, but I'm not sure if we have a master list of commits to go\n> > the other way.\n> \n> We have table of all in-core objects, don't we?\n\nOh, duh. Yes, we could iterate over obj_hash. I do think the \"on demand\"\nversion I showed later in the message is better, though, as the work\nboth scales with the number of affected commits (rather than the total\nnumber of objects) and can be done lazily (so callers that are not buggy\npay no price at all).\n\n> > +\t * This will throw away the parents list, which is potentially sketchy.\n> > +\t * A better version of this would just unset commit->object.parsed\n> > +\t * and then do a minimal version of parse_commit() that _just_ loads\n> > +\t * the tree data (and/or graph position if available).\n> \n> Yeah, it is a concern that we may be working with an in-core commit\n> object whose parent list has already been rewritten during revision\n> traversal.  Well thought out.\n\nHmm. Looking at my \"a better version\" sentence, I guess we know that\neither:\n\n  1. The object really is corrupt (i.e., we could not load the tree).\n\n  2. It came from a graph in the first place.\n\nSo what if we just tried harder to look it up in the graph file (rather\nthan the slab) when we see COMMIT_NOT_FROM_GRAPH? And indeed, we even\nhave a function to do this already!\n\nSo something like this almost works:\n\ndiff --git a/commit.c b/commit.c\nindex b3223478bc..096a3d18d0 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -418,10 +418,12 @@ static inline void set_commit_tree(struct commit *c, struct tree *t)\n struct tree *repo_get_commit_tree(struct repository *r,\n \t\t\t\t  const struct commit *commit)\n {\n+\tuint32_t pos;\n+\n \tif (commit->maybe_tree || !commit->object.parsed)\n \t\treturn commit->maybe_tree;\n \n-\tif (commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n+\tif (repo_find_commit_pos_in_graph(r, commit, &pos))\n \t\treturn get_commit_tree_in_graph(r, commit);\n \n \treturn NULL;\n\nbut:\n\n  1. It doesn't update the slab, so get_commit_tree_in_graph() then\n     complains immediately, because it uses only\n     commit_graph_position(). I guess we'd need to do:\n\n       commit_graph_data_at(commit)->graph_pos = pos;\n\n     somewhere. That does make the recently-found segfault go away.\n     But...\n\n  2. I'm not sure if other spots would want similar treatment. We store\n     generation numbers in the slab, too. I think we'll figure things\n     out when they're not available, so there's no segfault problem. But\n     it's maybe a potential performance issue. Likewise, it is probably\n     a bug that the graph-writing code is looking at this commit at all\n     (since we know it is already in a graph file). So we might be\n     papering over that bug (that is, even though the segfault is gone,\n     we are perhaps still writing a duplicate graph entry).\n\n> > I dunno. I do feel like this is a lurking maintenance headache, and\n> > might even be a triggerable bug. But without knowing of a way that it\n> > happens in the current code base, it feels like it would be easy to make\n> > things worse rather than better.\n> \n> Unfortunately I share the feeling X-<.\n\nI somehow sniped myself into thinking about it more, but that has only\nreinforced my feeling that I'm afraid to touch it. ;)\n\n-Peff\n"},{"id":"486641","messageId":"xmqqedenhg5v.fsf@gitster.g","threadId":"60686","inReplyTo":"20240111075313.GF48154@coredump.intra.peff.net","subject":"Re: [PATCH] commit-graph: retain commit slab when closing NULL commit_graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-11T18:35:24Z","receivedAt":"2024-01-11T18:35:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jan 10, 2024 at 08:38:18AM -0800, Junio C Hamano wrote:\n>\n>> > It should be easy-ish to iterate through the slab and look at the\n>> > commits that are mentioned in it. Though maybe not? Each commit knows\n>> > its slab-id, but I'm not sure if we have a master list of commits to go\n>> > the other way.\n>> \n>> We have table of all in-core objects, don't we?\n>\n> Oh, duh. Yes, we could iterate over obj_hash. I do think the \"on demand\"\n> version I showed later in the message is better, though, as the work\n> both scales with the number of affected commits (rather than the total\n> number of objects) and can be done lazily (so callers that are not buggy\n> pay no price at all).\n\nYeah, it is far more desirable than scanning obj_hash if we can do\nthe right thing on lazily.\n\n> So what if we just tried harder to look it up in the graph file (rather\n> than the slab) when we see COMMIT_NOT_FROM_GRAPH? And indeed, we even\n> have a function to do this already!\n\n;-)\n\n> but:\n> ...\n> I somehow sniped myself into thinking about it more, but that has only\n> reinforced my feeling that I'm afraid to touch it. ;)\n\nThanks for a nice summary.\n"}]}