Re: [PATCH v3 4/4] revision: avoid hitting packfiles when commits are in commit-graph
From: Junio C Hamano <hidden>
Date: 2021-08-05 16:16:56
Patrick Steinhardt [off-list ref] writes:
On Tue, Aug 03, 2021 at 02:56:49PM -0700, Junio C Hamano wrote:quoted
Patrick Steinhardt [off-list ref] writes:quoted
I wonder what our stance on this is. I can definitely understand the angle that this would be a deal breaker given that we now claim commits exist which don't anymore.An optimization that produces a wrong result very fast is a useless optimization that has no place in our codebase. But don't we have some clue recorded in the commit graph file that tells us with what packfile the graph is to be used (iow, if the named packfile still exists there, the objects recorded in the graph file are to be found there) or something?Unfortunately, no. For bitmaps we have this information given that a bitmap is tied to a specific pack anyway. But for commit-graphs, the story is different given that they don't really care about the packs per se, but only about the commits.
[jc: refreshed Cc: list to limit to those in "shortlog commit-graph.[ch]"] On this subject, I'd ask those who have worked on the commit-graph for ideas. It would be a glaring flaw _if_ the data structure that is designed to be a "cache of precomputed summary that would help runtime performance" has no way to detect out-of-date cache and/or to invalidate when it goes stale, but I somehow doubt that is the case, given the caliber of folks who have worked in it. To me, it feels a lot more likely that we may be missing an existing mechanism to do so. It could be that ...
quoted hunk
We can do the following on top though:diff --git a/revision.c b/revision.c index 3527ef3f65..9e62de20ab 100644 --- a/revision.c +++ b/revision.c@@ -368,6 +368,8 @@ static struct object *get_reference(struct rev_info *revs, const char *name, object = NULL; goto out; } + } else if (!repo_has_object_file(revs->repo, oid)) { + die("bad object %s", name); } }We assert that the object exists, but `repo_has_object_file()` won't try to unpack the object header given that we request no info about the object. And because the object ID has been part of the commit-graph, we know that it's a commit. It's a bit slower compared to the version where we don't assert object existence, but still a lot faster compared to looking up the object type via the ODB.
... the above is the designed way to correctly use the commit-graph data? That is, you find an object in the commit-graph, and you make sure the object exists in the object store in some other means (because there is no mechanism for commit-graph to prevent a gc from pruning an object recorded in it away) before you consider you can use the object. Thoughts and help? Thanks.