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

On Mon, Aug 18, 2025 at 05:04:17PM -0400, Jeff King wrote:

quoted

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