Re: [External] Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db

[Han Xin <hanxin.hx@bytedance.com>]

On Fri, Nov 1, 2024 at 12:25 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> 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.