Re: [PATCH v3 4/4] revision: avoid hitting packfiles when commits are in commit-graph
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 5, 2021, 16:16 UTC
- Message-ID
- <xmqqy29flvd7.fsf@gitster.g>
- In-Reply-To
- <YQvFGuf3Ba/5UNXk@ncase>
Patrick Steinhardt <ps@pks.im> writes:
Show 18 quoted lines
> On Tue, Aug 03, 2021 at 02:56:49PM -0700, Junio C Hamano wrote: >> Patrick Steinhardt <ps@pks.im> writes: >> >> > 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 ...
Show 21 quoted lines
> 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.