Patrick Steinhardt <ps@xxxxxx> writes:
> On Tue, Aug 03, 2021 at 02:56:49PM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@xxxxxx> 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 ...
> 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.
