From: Derrick Stolee <stolee@gmail.com>
Date: Wed, 20 May 2026 16:20:36 GMT
Subject: Re: [PATCH] commit: fall back to full read when maybe_tree is NULL
Message-ID: <431a3b73-1819-4798-a0ba-b7351efe6aa1@gmail.com>
In-Reply-To: <20260519050513.GA1635924@coredump.intra.peff.net>

On 5/19/2026 1:05 AM, Jeff King wrote:
> When we load a commit object from the commit graph (rather than reading
> the object contents), we don't fill in its "maybe_tree" entry, but
> rather wait to lazy-load it. This goes back to 7b8a21dba1 (commit-graph:
> lazy-load trees for commits, 2018-04-06), and saves the work of
> instantiating tree objects that nobody cares about.
> 
> But it creates a data dependency: now the commit struct depends on the
> graph file to do that lazy load. This is a problem if we close the graph
> file; now we have a commit struct that claims to be parsed but is
> missing some of its data.


> Reported twice recently:
> 
>  - https://lore.kernel.org/git/87h5onsi0f.fsf@prevas.dk/
> 
>  - https://lore.kernel.org/git/6ae85515-9373-4c9e-90d2-5e4176590c5b@suse.com/
> 
> I don't why we suddenly got two reports. AFAICT the bug goes back to
> 2018, though it would become more prominent as use of commit graphs
> increased.

Likely, this may have changed with the switch to using geometric
maintenance instead of gc maintenance by default in Git 2.54.0. That
perhaps increased the amount of commit-graphs being present.
> +static void load_tree_from_commit_contents(struct repository *r, struct commit *commit)
> +{
> +	enum object_type type;
> +	unsigned long size;
> +	char *buf;
> +	const char *p;
> +	struct object_id tree_oid;
> +
> +	buf = odb_read_object(r->objects, &commit->object.oid, &type, &size);
> +	if (!buf)
> +		return;
> +
> +	if (type == OBJ_COMMIT &&
> +	    skip_prefix(buf, "tree ", &p) &&
> +	    !parse_oid_hex(p, &tree_oid, &p) &&
> +	    *p == '\n')
> +		set_commit_tree(commit, lookup_tree(r, &tree_oid));
> +
> +	free(buf);
> +}
> +

I like this focused parsing of the commit contents. I also briefly
considered "unparsing" the commit, but you make a good point in your
message why a focused parse here is important, especially around
munging of the parent list.

>  struct tree *repo_get_commit_tree(struct repository *r,
>  				  const struct commit *commit)
>  {
> @@ -443,7 +464,17 @@ struct tree *repo_get_commit_tree(struct repository *r,
>  	if (commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)
>  		return get_commit_tree_in_graph(r, commit);
>  
> -	return NULL;
> +	/*
> +	 * This is either a corrupt commit, or one which we partially loaded
> +	 * from a graph file but then subsequently threw away the graph data.
> +	 *
> +	 * Optimistically assume it's the latter and try to reload from
> +	 * scratch. This gives a performance penalty if it really is a corrupt
> +	 * commit, but presumably that happens rarely (and only once per
> +	 * process).
> +	 */
> +	load_tree_from_commit_contents(r, (struct commit *)commit);
> +	return commit->maybe_tree;
>  }

I agree that this is the right place to insert this logic.

> +test_expect_success 'dissociate from repo with commit graph' '
> +	git init orig &&
> +	# We are trying to make sure the dissociated repo can
> +	# find the tree of the tip commit, so the test could still
> +	# serve its purpose with an empty tree. But having actual
> +	# content future-proofs us against any kind of internal
> +	# empty-tree optimizations.
> +	echo content >orig/file &&
> +	git -C orig add . &&
> +	git -C orig commit -m foo &&
> +
> +	# We will use graph.git as our "local" source to dissociate
> +	# from.
> +	git clone --bare orig graph.git &&
> +	git -C graph.git commit-graph write --reachable &&
> +
> +	# And then finally clone orig, using graph.git to get our objects. This
> +	# must be non-bare so that we perform the checkout step, which will
> +	# need to access the tree of HEAD, which we will have originally loaded
> +	# via the commit graph.
> +	git clone --no-local --reference graph.git --dissociate orig clone
> +'
> +
Thanks for the clear extra coverage here.

-Stolee