On Fri, Nov 1, 2024 at 12:25 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> >> commit = lookup_commit_in_graph(the_repository, oid);
> >> - if (commit)
> >> + if (commit) {
> >> + if (mark_tags_complete_and_check_obj_db) {
> >> + if (!has_object(the_repository, oid, 0))
> >> + die_in_commit_graph_only(oid);
> >> + }
> >> return commit;
> >> + }
> >
> > Hmph, even when we are not doing the mark-tags-complete thing,
> > wouldn't it be a fatal error if the commit graph claims a commit
> > exists but we are missing it?
> >
> > It also makes me wonder if it would be sufficient to prevent us from
> > saying "have X" if we just pretend as if lookup_commit_in_graph()
> > returned NULL in this case.
>
> Again, sorry for the noise.
>
> I think the posted patch is better without either of these two,
> simply because the "commit graph lies" case is a repository
> corruption, and "git fsck" should catch such a corruption (and if
> not, we should make sure it does).
>
> The normal codepaths should assume a healthy working repository.
>
> As has_object() is not without cost, an extra check is warranted
> only because not checking will go into infinite recursion. If it
> does not make us fail in such an unpleasant way if we return such a
> commit when we are not doing the mark-tags-complete thing (but makes
> us fail in some other controlled way), not paying cost for an extra
> check is the right thing.
>
> Thanks.
Although the scenario I faked in t/t5330-no-lazy-fetch-with-commit-graph.sh
usually does not occur, if we are unfortunate enough to encounter this issue,
I hope it can automatically fix the problem as much as possible without
relying on me to take an extra action.
Thanks.
