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

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

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.