Re: [PATCH 1/2] lookup_commit_in_graph(): use prepare_commit_graph() to check for graph

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 6, 2022, 21:16 UTC
Message-ID: <Yxe4xxF/yWTLAtI3@nand.local>
In-Reply-To: <Yxe1VbyYovwprPgQ@coredump.intra.peff.net>

On Tue, Sep 06, 2022 at 05:02:13PM -0400, Jeff King wrote:

> I couldn't find any other reason to avoid calling prepare_commit_graph()
> here (especially since it ends up lazy-loaded as discussed above). The
> cost of the call should not be high; after the first call, it is
> simplified down to a few integer checks.

Neither could I. Obviously f559d6d45e (revision: avoid hitting packfiles when commits are in commit-graph, 2021-08-09) is adding a call to look for a commit by object ID in the commit-graph where there wasn't one before, so there isn't anything to compare to there.

But the next-closest function `load_commit_graph_info()` calls `prepare_commit_graph()` (via `repo_find_commit_pos_in_graph()`).

And that all matches my understanding that `r->objects->commit_graph` is lazily loaded. Perhaps it should be made more difficult to access via the struct member, and instead done behind a function like `prepare_commit_graph()` (modified to return the `struct commit_graph*` if one was found).

But that's for another day :-). This is obviously correct in the meantime.

Thanks, Taylor