On Thu, Sep 04, 2025 at 02:49:57PM +0200, Patrick Steinhardt wrote:
> When making use of commit graphs, one needs to first prepare them by
> calling `prepare_commit_graph()`. Once that function was called and the
> commit graph was prepared successfully, the caller is now expected to
> access the graph directly via `struct object_database::commit_graph`.
>
> In a subsequent change, we're going to move the commit graph pointer
> from `struct object_database` into `struct odb_source`. With this
> change, semantics will change so that we use the commit graph of the
> first source that has one. Consequently, all callers that currently
> deference the `commit_graph` pointer would now have to loop around the
> list of sources to find the commit graph.
> This would become quite unwieldy. So instead of shifting the burden onto
> such callers, adapt `prepare_commit_graph()` to return the prepared
> commit graph, if any. Like this, callers are expected to call that
> function and then use the returned commit graph.
Hmmph. I see what you're saying, though I'm not sure I agree with the
implication here. Presumably "r->objects->commit_graph" could be
rewritten as "repo_commit_graph(r->objects->sources)" or similar. But
I'm OK with this approach, too.
> int generation_numbers_enabled(struct repository *r)
> {
> uint32_t first_generation;
> struct commit_graph *g;
> - if (!prepare_commit_graph(r))
> - return 0;
>
> - g = r->objects->commit_graph;
> -
> - if (!g->num_commits)
> - return 0;
> + g = prepare_commit_graph(r);
> + if (!g || !g->num_commits)
Makes sense; this isn't an exact translation, since the conditional now
also checks for the NULL-ness of "g" first. But that's necessary, since
if the (now-removed) earlier call to prepare_commit_graph() succeeded,
we know that "g" is non-NULL here.
Since that function is now responsible for handing us the commit_graph
itself, checking for success means that we have to see if "g" is
non-NULL first before doing something with it.
> @@ -799,12 +796,9 @@ int generation_numbers_enabled(struct repository *r)
> int corrected_commit_dates_enabled(struct repository *r)
> {
> struct commit_graph *g;
> - if (!prepare_commit_graph(r))
> - return 0;
>
> - g = r->objects->commit_graph;
> -
> - if (!g->num_commits)
> + g = prepare_commit_graph(r);
> + if (!g || !g->num_commits)
Same here.
> @@ -1012,23 +1006,26 @@ static int find_commit_pos_in_graph(struct commit *item, struct commit_graph *g,
> int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,
> uint32_t *pos)
> {
> - if (!prepare_commit_graph(r))
> + struct commit_graph *g = prepare_commit_graph(r);
> + if (!g)
> return 0;
> - return find_commit_pos_in_graph(c, r->objects->commit_graph, pos);
> + return find_commit_pos_in_graph(c, g, pos);
These and other changes may have read a little bit cleaner if there were
a preparatory commit which introduced "g" as a variable on the stack,
since that would change:
struct commit_graph *g;
if (!prepare_commit_graph(r))
return 0;
g = the_repository->objects->commit_graph;
into:
struct commit_graph *g = prepare_commit_graph(r);
if (!g)
return 0;
, without affecting the rest of the function, keeping the diff at least
easier to read (or smaller) by eliminating the "r->objects->commit_graph"
to "g" change.
Not a big deal at all, just a thought that I had while reviewing.
> @@ -2519,6 +2518,7 @@ int write_commit_graph(struct odb_source *source,
> int replace = 0;
> struct bloom_filter_settings bloom_settings = DEFAULT_BLOOM_FILTER_SETTINGS;
> struct topo_level_slab topo_levels;
> + struct commit_graph *g;
>
> prepare_repo_settings(r);
> if (!r->settings.core_commit_graph) {
> @@ -2547,23 +2547,13 @@ int write_commit_graph(struct odb_source *source,
> init_topo_level_slab(&topo_levels);
> ctx.topo_levels = &topo_levels;
>
> - prepare_commit_graph(ctx.r);
> - if (ctx.r->objects->commit_graph) {
> - struct commit_graph *g = ctx.r->objects->commit_graph;
> -
> - while (g) {
> - g->topo_levels = &topo_levels;
> - g = g->base_graph;
> - }
> - }
> + g = prepare_commit_graph(ctx.r);
> + for (struct commit_graph *chain = g; chain; chain = chain->base_graph)
> + g->topo_levels = &topo_levels;
Makes sense.
The rest looks all good.
Thanks,
Taylor
