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

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

Jonathan Tan <jonathantanmy@google.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>> 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.
>
> Just checking...by "the posted patch is better without either
> of these two", do you mean that we should not use has_object()
> here?

No, "these two" refers to two changes I hinted at in my message,
i.e. (1) regardless of mark_tags_complete_and_check_obj_db shouldn't
we check with has_object() and die? and (2) if we commit=NULL and
keep going, would it be sufficient to fix it?