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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 1, 2024, 17:36 UTC
Message-ID: <20241101173640.683565-1-jonathantanmy@google.com>
In-Reply-To: <xmqqikt74rs5.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 23 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> >  static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
> > -					       int mark_tags_complete)
> > +					       int mark_tags_complete_and_check_obj_db)
> >  {
> >  	enum object_type type;
> >  	struct object_info info = { .typep = &type };
> >  	struct commit *commit;
> >  
> >  	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?

If we can detect this cheaply, yes it would be ideal if every time we read a commit from the commit graph, we also check that the commit exists in the object DB. I don't think we can do it cheaply, though.

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

"have X" is controlled by the packfile negotiation part, so we would have to change that. (This part controls whether we send "want X" for a specific OID or not.)

> In any case, infinitely recursing to lazily fetch a single commit is
> definitely worth fixing.  Thanks for digging to the bottom of the
> problem and fixing it.

Thanks.