Re: [PATCH 5/5] describe: pass commit to describe_commit()

From: Jeff King <peff@peff.net>
Date: Aug 19, 2025, 17:02 UTC
Message-ID: <20250819170255.GB1050577@coredump.intra.peff.net>
In-Reply-To: <aKQwRaX94uwTwiQP@pks.im>

On Tue, Aug 19, 2025 at 10:05:25AM +0200, Patrick Steinhardt wrote:

Show 14 quoted lines

> On Mon, Aug 18, 2025 at 05:04:17PM -0400, Jeff King wrote:
> > There's a call in describe_commit() to lookup_commit_reference(), but we
> > don't check the return value. If it returns NULL, we'll segfault as we
> > immediately dereference the result.
> > 
> > In practice this can never happen, since all callers pass an oid which
> > came from a "struct commit" already. So we can make this more obvious
> > by just taking that commit struct in the first place.
> 
> I was wondering a bit about commit-graphs. We had the case in the past
> where it was possible to look up commits via the graph even though they
> don't exist in the ODB. So we might actually end up with a missing
> object if `GIT_COMMIT_GRAPH_PARANOIA=false`, which is the default value.
> But that might be fine? No idea without digging further.

I don't think existence matters here. Any call to parse_object() will call lookup_object() and find the same commit struct we already had, and then exit immediately because its "parsed" flag is set. So we'd never get a NULL return from lookup_commit_reference().

> In any case, the refactoring makes sense regardless from my point of
> view.

But yeah. Even if I am wrong above, this would fix it. ;)

-Peff