Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes:
> 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?
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?
