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

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