Re: [PATCH 6/6] odb: move commit-graph into the object sources

[Taylor Blau <me@ttaylorr.com>]

On Thu, Sep 04, 2025 at 02:50:00PM +0200, Patrick Steinhardt wrote:
> Commit graphs are inherently tied to one specific object source.
> Furthermore, with the upcoming pluggable object sources, it is not even
> guaranteed that an object source may even have a commit graph as these
> are specific to the actual on-disk data format.
>
> Prepare for this future by moving the commit-graph pointer from `struct
> object_database` to `struct odb_source`. Eventually, this will allow us
> to make commit graphs an implementation detail of an object source's
> backend.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  commit-graph.c | 65 +++++++++++++++++++++++++++++++++++++++++-----------------
>  commit-graph.h |  2 +-
>  odb.c          |  9 ++++----
>  odb.h          |  6 +++---
>  packfile.c     |  3 +--
>  5 files changed, 56 insertions(+), 29 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 0e25b14076..9929c1ed87 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -721,11 +721,15 @@ static struct commit_graph *load_commit_graph_chain(struct odb_source *source)
>
>  struct commit_graph *read_commit_graph_one(struct odb_source *source)
>  {
> -	struct commit_graph *g = load_commit_graph_v1(source);
> +	struct commit_graph *g;
> +
> +	if (source->commit_graph_attempted)
> +		return NULL;

I wonder if some callers will find this surprising. The current analog
of this function is commit-graph.c::prepare_commit_graph(), which does:

    if (r->objects->commit_graph_attempted)
        return r->objects->commit_graph;

, but this function returns NULL on the second (and subsequent) calls,
even if the object source in question has a commit graph that was loaded
in an earlier step.

> +	source->commit_graph_attempted = true;
>
> +	g = load_commit_graph_v1(source);
>  	if (!g)
>  		g = load_commit_graph_chain(source);
> -
>  	return g;
>  }
>
> @@ -737,6 +741,7 @@ struct commit_graph *read_commit_graph_one(struct odb_source *source)
>   */
>  static struct commit_graph *prepare_commit_graph(struct repository *r)
>  {
> +	bool all_attempted = true;
>  	struct odb_source *source;
>
>  	/*
> @@ -749,9 +754,19 @@ static struct commit_graph *prepare_commit_graph(struct repository *r)
>  	if (!r->gitdir || r->commit_graph_disabled)
>  		return NULL;
>
> -	if (r->objects->commit_graph_attempted)
> -		return r->objects->commit_graph;
> -	r->objects->commit_graph_attempted = 1;
> +	odb_prepare_alternates(r->objects);
> +	for (source = r->objects->sources; source; source = source->next) {
> +		all_attempted &= source->commit_graph_attempted;
> +		if (source->commit_graph)
> +			return source->commit_graph;
> +	}
> +
> +	/*
> +	 * There is no point in re-trying to load commit graphs if we already
> +	 * tried loading all of them beforehand.
> +	 */
> +	if (all_attempted)
> +		return NULL;
>
>  	prepare_repo_settings(r);
>
> @@ -768,14 +783,16 @@ static struct commit_graph *prepare_commit_graph(struct repository *r)
>  	if (!commit_graph_compatible(r))
>  		return NULL;
>
> -	odb_prepare_alternates(r->objects);
>  	for (source = r->objects->sources; source; source = source->next) {
> -		r->objects->commit_graph = read_commit_graph_one(source);
> -		if (r->objects->commit_graph)
> -			break;
> +		if (source->commit_graph_attempted)
> +			continue;
> +
> +		source->commit_graph = read_commit_graph_one(source);
> +		if (source->commit_graph)
> +			return source->commit_graph;

Is this what callers expect? The pre-image here returned the
commit-graph belonging to the current repository's object database, not
any of its alternate(s).

I'm not opposed to the idea that perhaps loading commit-graphs from
alternates would be useful, but I am uncomfortable making that change in
this series.

Thanks,
Taylor