Re: [PATCH 4/4] commit: don't lazy-fetch commits

[Jeff King <peff@peff.net>]

On Wed, Nov 30, 2022 at 12:30:49PM -0800, Jonathan Tan wrote:

> When parsing commits, fail fast when the commit is missing or
> corrupt, instead of attempting to fetch them. This is done by inlining
> repo_read_object_file() and setting the flag that prevents fetching.
> 
> This is motivated by a situation in which through a bug (not necessarily
> through Git), there was corruption in the object store of a partial
> clone. In this particular case, the problem was exposed when "git gc"
> tried to expire reflogs, which calls repo_parse_commit(), which triggers
> fetches of the missing commits.

Makes sense.

> diff --git a/commit.c b/commit.c
> index 572301b80a..17e71f5be4 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -508,6 +508,13 @@ int repo_parse_commit_internal(struct repository *r,
>  	enum object_type type;
>  	void *buffer;
>  	unsigned long size;
> +	const struct object_id *real_oid;
> +	struct object_info oi = {
> +		.typep = &type,
> +		.sizep = &size,
> +		.contentp = &buffer,
> +		.real_oidp = &real_oid,
> +	};
>  	int ret;
>  
>  	if (!item)
> @@ -516,11 +523,18 @@ int repo_parse_commit_internal(struct repository *r,
>  		return 0;
>  	if (use_commit_graph && parse_commit_in_graph(r, item))
>  		return 0;
> -	buffer = repo_read_object_file(r, &item->object.oid, &type, &size);
> -	if (!buffer)
> +
> +	/*
> +	 * Git does not support partial clones that exclude commits, so set
> +	 * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.
> +	 */
> +	if (oid_object_info_extended(r, &item->object.oid, &oi,
> +	    OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT) < 0) {
> +		die_if_corrupt(r, &item->object.oid, real_oid);
>  		return quiet_on_missing ? -1 :
>  			error("Could not read %s",
>  			     oid_to_hex(&item->object.oid));
> +	}

OK, so we know we want a commit object because we're in the
commit-parsing function, so we just ask to disable fetching.

Two devil's advocate thoughts:

  1. What if we're wrong that it's a commit? If somebody references a
     blob in a commit "parent" header, the "right" outcome is for us to
     say "oops, the type is wrong" when we try to parse it as a commit.
     But now in a partial clone, we might avoid fetching that supposed
     commit and say "you don't have this object", even though we could
     get it.

     I think I'm OK with that. Either way, the repo is corrupt, and
     we'll have informed the user of that. The fact that this bizarre
     and specific sequence of corruptions might not go as far as it can
     to deduce the root cause is probably fine.

  2. Are there other places where we'd want to do the same thing? E.g.,
     in parse_object() we might ask for an object (not knowing its type)
     only to find out that it is a commit. But we have no idea if we
     lazy-fetched it or not!

     I had somehow imagined your series would be hooking in at the level
     of the lazy-fetch code, and complaining about fetching commits. But
     that may be tricky to do, because we really don't know the type
     until after we fetch it, and selectively removing an object from
     the odb is quite hard. We'd probably have to tell the other side
     "please, don't send me any commits", which requires a protocol
     extension, and...yuck.

     By comparison, your approach is an easy win that may catch problems
     in practice (and is certainly better than the status quo).

-Peff