Re: [PATCH v3 4/4] revision: avoid hitting packfiles when commits are in commit-graph

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> 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 ...

> 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.