Re: [PATCH 3/6] commit-graph: return the prepared commit graph from `prepare_commit_graph()`

[Taylor Blau <me@ttaylorr.com>]

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