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@xxxxxx>
> ---
> 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
