git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Taylor Blau <me@ttaylorr.com>
Date
Sep 11, 2025, 23:00 UTC
Message-ID
<aMNUi9+MZd0rtmOT@nand.local>
In-Reply-To
<20250904-b4-pks-commit-graph-via-source-v1-6-d932c2481e1a@pks.im>
On Thu, Sep 04, 2025 at 02:50:00PM +0200, Patrick Steinhardt wrote:
Show 32 quoted lines
> 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.

Show 55 quoted lines
> +	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

Previous: Patrick SteinhardtNext: Junio C Hamano
Message 12 of 26 in “odb: track commit graphs via object source”
  1. 0/6 odb: track commit graphs via object sourcePatrick Steinhardt, Sep 4, 2025
  2. 1/6 blame: drop explicit check for commit graphPatrick Steinhardt, Sep 4, 2025
  3. Taylor BlauSep 11, 2025
  4. 2/6 revision: drop explicit check for commit graphPatrick Steinhardt, Sep 4, 2025
  5. Taylor BlauSep 11, 2025
  6. 3/6 commit-graph: return the prepared commit graph from `prepare_commit_graph()`Patrick Steinhardt, Sep 4, 2025
  7. Taylor BlauSep 11, 2025
  8. 4/6 commit-graph: return commit graph from `repo_find_commit_pos_in_graph()`Patrick Steinhardt, Sep 4, 2025
  9. Taylor BlauSep 11, 2025
  10. 5/6 commit-graph: pass graphs that are to be merged as parameterPatrick Steinhardt, Sep 4, 2025
  11. 6/6 odb: move commit-graph into the object sourcesPatrick Steinhardt, Sep 4, 2025
  12. Taylor BlauSep 11, 2025
  13. Junio C HamanoSep 4, 2025
  14. Derrick StoleeSep 5, 2025
  15. Patrick SteinhardtSep 8, 2025
  16. Derrick StoleeSep 8, 2025
  17. Patrick SteinhardtSep 10, 2025
  18. Junio C HamanoSep 25, 2025
  19. Patrick SteinhardtSep 26, 2025
  20. Patrick SteinhardtOct 2, 2025
  21. Patrick SteinhardtOct 2, 2025
  22. Junio C HamanoOct 2, 2025
  23. Derrick StoleeOct 3, 2025
  24. Taylor BlauSep 11, 2025
  25. Junio C HamanoSep 4, 2025
  26. Patrick SteinhardtSep 5, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.