Re: [PATCH] fetch: in partial clone, check presence of targets

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 20, 2018, 22:10 UTC
Message-ID: <20180920221038.74193-1-jonathantanmy@google.com>
In-Reply-To: <xmqqh8ijzyep.fsf@gitster-ct.c.googlers.com>

Show 26 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > +	if (repository_format_partial_clone) {
> > +		/*
> > +		 * For the purposes of the connectivity check,
> > +		 * check_connected() considers all objects promised by
> > +		 * promisor objects as existing, which means that the
> > +		 * connectivity check would pass even if a target object
> > +		 * in rm is merely promised and not present. When
> > +		 * fetching objects, we need all of them to be present
> > +		 * (in addition to having correct connectivity), so
> > +		 * check them directly.
> > +		 */
> > +		struct ref *r;
> > +		for (r = rm; r; r = r->next) {
> > +			if (!has_object_file(&r->old_oid))
> > +				return -1;
> > +		}
> 
> Because check_connected() lies in the presense of "promisor", we can
> defeat it this way, which makes sense.  
> 
> I wonder if it makes sense to do this check unconditionally, even
> when we are in a fully functioning clone.  The check is quite cheap
> and can reject a ref_map that has an object we do not know about,
> without check_connected() having to ask the rev-list.

It makes sense to me from a runtime point of view - the usual case, for me, is when we're missing at least one object that we're fetching, and doing the cheap check even allows us to skip the expensive check.

The hard part for me lies in how to communicate to future readers of the code that they cannot remove this section to simplify the code. We would need a more complicated comment, something like this:

 /*
  * Check if all wanted objects are present.
  *
  * If the local repository is a partial clone, check_connected() is not
  * sufficient - it would pass even if a wanted object is merely
  * promised and not present. This is because, for the purposes of the
  * connectivity check, check_connected() considers all objects promised
  * by promisor objects as existing.
  *
  * If the local repository is not a partial clone, check_connected() is
  * sufficient, but this loop allows us to avoid the expensive
  * check_connected() in the usual case that at least one wanted object
  * is missing.
  */