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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Oct 29, 2024 at 02:11:05PM -0700, Jonathan Tan wrote:
> When fetching, there is a step in which sought objects are first checked
> against the local repository; only objects that are not in the local
> repository are then fetched. This check first looks up the commit graph
> file, and returns "present" if the object is in there.

OK.

> However, the action of first looking up the commit graph file is not
> done everywhere in Git, especially if the type of the object at the time
> of lookup is not known. This means that in a repo corruption situation,
> a user may encounter an "object missing" error, attempt to fetch it, and
> still encounter the same error later when they reattempt their original
> action, because the object is present in the commit graph file but not in
> the object DB.

I think the type of repository corruption here may be underspecified.

You say that we have some object, say X, whose type is not known. So we
don't load the commit-graph, realize that X is missing, and then try and
fetch it. In this scenario, is X actually in the commit-graph, but not
in the object database? Further, if X is in the commit-graph, I assume
we do not look it up there because we first try and find its type, which
fails, so we assume we don't have it (despite it appearing corruptly in
the commit-graph)?

I think that matches the behavior you're describing, but I want to make
sure that I'm not thinking of something else.

> This was discovered when a lazy fetch of a missing commit completed with
> nothing actually fetched, and the writing of the commit graph file after
> every fetch then attempted to read said missing commit, triggering a
> lazy fetch of said missing commit, resulting in an infinite loop with no
> user-visible indication (until they check the list of processes running
> on their computer).

Yuck :-).

> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c | 22 +++++++++++++++++++---
>  object.h     |  2 +-
>  2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 6728a0d2f5..5a0020366b 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -57,6 +57,7 @@ static struct string_list uri_protocols = STRING_LIST_INIT_DUP;
>  #define ALTERNATE	(1U << 1)
>  #define COMMON		(1U << 6)
>  #define REACH_SCRATCH	(1U << 7)
> +#define COMPLETE_FROM_COMMIT_GRAPH	(1U << 8)
>
>  /*
>   * After sending this many "have"s if we do not get any new ACK , we
> @@ -123,15 +124,18 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
>  }
>
>  static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
> -					       int mark_tags_complete)
> +					       int mark_additional_complete_information)
>  {
>  	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_additional_complete_information)
> +			commit->object.flags |= COMPLETE_FROM_COMMIT_GRAPH;
>  		return commit;
> +	}
>
>  	while (1) {
>  		if (oid_object_info_extended(the_repository, oid, &info,
> @@ -143,7 +147,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
>
>  			if (!tag->tagged)
>  				return NULL;
> -			if (mark_tags_complete)
> +			if (mark_additional_complete_information)
>  				tag->object.flags |= COMPLETE;
>  			oid = &tag->tagged->oid;
>  		} else {
> @@ -809,6 +813,14 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
>  	save_commit_buffer = old_save_commit_buffer;
>  }
>
> +static void warn_in_commit_graph_only(const struct object_id *oid)
> +{
> +	warning(_("You are attempting to fetch %s, which is in the commit graph file but not in the object database."),
> +		oid_to_hex(oid));
> +	warning(_("This is probably due to repo corruption."));
> +	warning(_("If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object."));
> +}
> +
>  /*
>   * 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);

You discuss this a little bit in your commit message, but I wonder if we
should just die() here. I feel like we're trying to work around a
situation where the commit-graph is obviously broken because it refers
to commit objects that don't actually exist in the object store.

A few thoughts in this area:

  - What situation provokes this to be true? I could imagine there is
    some bug that we don't fully have a grasp of. But I wonder if it is
    even easier to provoke than that, say by pruning some objects out of
    the object store, then not rewriting the commit-graph, leaving some
    of the references dangling.

  - Does 'git fsck' catch this case within the commit-graph?

  - Are the other areas of the code that rely on the assumption that all
    entries in the commit-graph actually exist on disk? If so, are they
    similarly broken?

Another thought about this whole thing is that we essentially have a
code path that says: "I found this object from the commit-graph, but
don't know if I actually have it on disk, so mark it to be checked later
via has_object()".

I wonder if it would be more straightforward to replace the call to
lookup_commit_in_graph() with a direct call to has_object() in the
deref_without_lazy_fetch() function, which I think would both (a)
eliminate the need for a new flag bit to be allocated, and (b) prevent
looking up the object twice.

Thoughts?

Thanks,
Taylor