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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 31, 2024, 21:23 UTC
Message-ID: <20241031212356.545501-1-jonathantanmy@google.com>
In-Reply-To: <bzhg2a7mv2xrbahk6o5kpijx4dxmpkm4wrrjhatetowjdowout@hesucnp6cikf>

Josh Steadmon <steadmon@google.com> writes:

Show 19 quoted lines

> >  /*
> >   * Returns 1 if every object pointed to by the given remote refs is available
> >   * locally and reachable from a local ref, and 0 otherwise.
> > @@ -830,6 +842,10 @@ static int everything_local(struct fetch_pack_args *args,
> >  				      ref->name);
> >  			continue;
> >  		}
> > +		if (o->flags & COMPLETE_FROM_COMMIT_GRAPH) {
> > +			if (!has_object(the_repository, remote, 0))
> > +				warn_in_commit_graph_only(remote);
> > +		}
> 
> And now that we're checking what's local, we issue our warning if we
> have an object missing from the DB but mentioned in the commit graph.
> Seems fine, although I wonder if it makes more sense to fail earlier. It
> looks like the only place we do the
> `mark_additional_complete_information` checks is in `mark_complete()`,
> so should we just check this condition there? No strong feelings either
> way, just curious.

When we were merely warning, it was useful to mark everything then check later, so that a warning message would be printed once per object, instead of potentially multiple times. (In the infinite case that we discovered at $DAYJOB, it doesn't really matter since the message is going to be printed an infinite number of times anyway, but in the "plain" case in which the user is missing a commit and does not have automatic commit graph writing enabled, the fetch will indeed terminate.)

But since we're making this a fatal error, yes, it makes sense to fail earlier. I've made the change.