From: Derrick Stolee <stolee@gmail•com>
To: Jeff King <peff@peff•net>, git@vger•kernel.org
Cc: Rasmus Villemoes <ravi@prevas•dk>, Daniel Mach <daniel.mach@suse•com>
Subject: Re: [PATCH] commit: fall back to full read when maybe_tree is NULL
Date: Wed, 20 May 2026 12:20:36 -0400 [thread overview]
Message-ID: <431a3b73-1819-4798-a0ba-b7351efe6aa1@gmail.com> (raw)
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
prev parent reply other threads:[~2026-05-20 16:20 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-19 5:05 [PATCH] commit: fall back to full read when maybe_tree is NULL Jeff King
2026-05-19 5:56 ` Junio C Hamano
2026-05-19 6:15 ` Jeff King
2026-05-20 16:22 ` Derrick Stolee
2026-05-19 6:25 ` Rasmus Villemoes
2026-05-20 16:20 ` Derrick Stolee [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=431a3b73-1819-4798-a0ba-b7351efe6aa1@gmail.com \
--to=stolee@gmail$(echo .)com \
--cc=daniel.mach@suse$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
--cc=ravi@prevas$(echo .)dk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox