Junio C Hamano <gitster@xxxxxxxxx> writes:
> 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.
Just checking...by "the posted patch is better without either
of these two", do you mean that we should not use has_object()
here? I included it here in as narrow a scope as possible (when
"mark_tags_complete_and_check_obj_db" is true) precisely because not
checking will go into infinite recursion, as you said. (And indeed I did
not expand the scope because I agree that the normal codepaths should
assume a healthy working repository.)
