patch, 6 partsodb: track commit graphs via object source

> diff --git a/blame.c b/blame.c
> index f1c0670144..cb0b083423 100644
> --- a/blame.c
> +++ b/blame.c
> @@ -2909,9 +2909,6 @@ void setup_blame_bloom_data(struct blame_scoreboard *sb)
>  	struct blame_bloom_data *bd;
>  	struct bloom_filter_settings *bs;
>
> -	if (!sb->repo->objects->commit_graph)
> -		return;
> -
>  	bs = get_bloom_filter_settings(sb->repo);
>  	if (!bs)
>  		return;

> When filtering down revisions by paths we know to use bloom filters from
> the commit graph, if we have any. The entry point for this is in
> `check_maybe_different_in_bloom_filter()`, where we first verify that:
>
>   - We do have a commit graph.
>
>   - That the commit is contained therein by checking that we have a
>     proper generation number.
>
>   - And that the graph contains a bloom filter.
>
> The first check is somewhat redundant though: if we don't have a commit
> graph, then the second check would already tell us that we don't have a
> generation number for the specific commit.

> In theory this could be seen as a performance optimization to
> short-circuit for scenarios where there is no commit graph. But in
> practice this shouldn't matter: if there is no commit graph, then the
> commit graph data slab would also be unpopulated and thus a lookup of
> the commit should happen in constant time.

> 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.

>  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)

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

> @@ -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);

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

> ---
>  bloom.c        |  8 +++++---
>  commit-graph.c | 18 ++++++++++++------
>  commit-graph.h | 12 ++++++------
>  3 files changed, 23 insertions(+), 15 deletions(-)

> 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;

> +	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;

> commit graphs are currently stored on the object database level. This
> doesn't really make much sense conceptually, given that commit graphs
> are specific to one object source. Furthermore, with the upcoming
> pluggable object database effort, an object source's backend may not
> evene have a commit graph in the first place but store that information
> in a different format altogether.
>
> This patch series prepares for that by moving the commit graph from
> `struct object_database` into `struct odb_source`.

> Patrick Steinhardt <ps@pks.im> writes:
> 
>> commit graphs are currently stored on the object database level. This
>> doesn't really make much sense conceptually, given that commit graphs
>> are specific to one object source. Furthermore, with the upcoming
>> pluggable object database effort, an object source's backend may not
>> evene have a commit graph in the first place but store that information
>> in a different format altogether.
>>
>> This patch series prepares for that by moving the commit graph from
>> `struct object_database` into `struct odb_source`.
> 
> Hmph, I am finding the above hard to agree with at the conceptual
> level.  In some future, we may use multiple object stores in a
> single repository.  Perhaps we would be storing older parts of
> history in semi-online storage while newer parts are stored in
> readily available storage.  But the side data structure that allows
> us to quickly learn who are parents of one commit is without having
> to go to the object store in order to parse the actualy commit
> object can be stored for the entire history if we wanted to, or more
> recent part of the history but not limited to the "readily available
> storage" part.  IOW, where the boundary between the older and the
> newer parts of the history lies and which commits the commit graph
> covers should be pretty much independent.
> 
> So moving from object_database (i.e. the whole world) to individual
> odb_source (i.e. where one particular subset of the history is
> stored) feels like totally backwards to me.  Surely, a commit graph
> file may be defined over a set of packfiles and remaining loose
> object files, but it is not like an instance of the commit-graph
> file is tied to packfiles in the sense that it uses the index into
> some packfile instead of the actual object names to refer to
> commits, or anything like that (this is quite different from other
> files that are very specific to a single object store, like midx
> that is tied to the packfiles it describes).

> On 9/4/2025 7:12 PM, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> >> commit graphs are currently stored on the object database level. This
> >> doesn't really make much sense conceptually, given that commit graphs
> >> are specific to one object source. Furthermore, with the upcoming
> >> pluggable object database effort, an object source's backend may not
> >> evene have a commit graph in the first place but store that information
> >> in a different format altogether.
> >>
> >> This patch series prepares for that by moving the commit graph from
> >> `struct object_database` into `struct odb_source`.
> > 
> > Hmph, I am finding the above hard to agree with at the conceptual
> > level.  In some future, we may use multiple object stores in a
> > single repository.  Perhaps we would be storing older parts of
> > history in semi-online storage while newer parts are stored in
> > readily available storage.  But the side data structure that allows
> > us to quickly learn who are parents of one commit is without having
> > to go to the object store in order to parse the actualy commit
> > object can be stored for the entire history if we wanted to, or more
> > recent part of the history but not limited to the "readily available
> > storage" part.  IOW, where the boundary between the older and the
> > newer parts of the history lies and which commits the commit graph
> > covers should be pretty much independent.
> > 
> > So moving from object_database (i.e. the whole world) to individual
> > odb_source (i.e. where one particular subset of the history is
> > stored) feels like totally backwards to me.  Surely, a commit graph
> > file may be defined over a set of packfiles and remaining loose
> > object files, but it is not like an instance of the commit-graph
> > file is tied to packfiles in the sense that it uses the index into
> > some packfile instead of the actual object names to refer to
> > commits, or anything like that (this is quite different from other
> > files that are very specific to a single object store, like midx
> > that is tied to the packfiles it describes).
> 
> This is an interesting aspect to things, where the commit-graph file
> is a "structured cache" of certain commit information. It happens to
> be located within the object stores (either local or in an alternate)
> but is conceptually different in a few ways.
> 
> The biggest difference is that you can only open one commit-graph
> (or chain of commit-graphs). Having multiple files across different
> object stores will not accumulate additional context. Instead, we
> have a "first one wins" approach.
> 
> This does seem to be something that you are attempting to change
> by including the ability to load a commit-graph for each odb (and
> closing them in sequence as we close a repo).
> 
> So in this sense, the commit-graph lives at the repository level,
> not an object store level. When doing I/O to write or read a graph,
> we use a specific object store at a time.
> 
> The other direction to consider is what context we have when we
> interact with a commit-graph. We generally are parsing commits from
> a repository or loading Bloom filter data during file history walks.
> Each of these do not have a predictable nature of which object store
> will "own" the commit we are inspecting, so it wouldn't make sense
> to restrict things like odb_parse_commit() over repo_parse_commit().
> 
> With these thoughts in mind, I have these big-picture thoughts:
> 
> 1. Patches 1-5 are great. Nice cleanups.
> 
> 2. Some of Patch 6 is great, including having the I/O methods use
>    an odb_source to help focus the specific location of the files
>    being read or written. However, the movement of the struct into
>    the odb_source makes less sense and should still exist at the
>    object_database level.

> On 9/8/2025 7:17 AM, Patrick Steinhardt wrote:
> > I (probably unsurprisingly :)) don't quite agree with this.
> 
> I think I can summarize the main point you seem to be making with
> this quote:
> 
> > So I would claim that the commit graph is specifically tied to the
> > actual storage format of objects, and it's not at all obvious that it
> > would need to exist if we had a different storage format.
> 
> I think I agree in principle, if you are saying "different storage
> format" means "different commit object information" which then means
> we are talking about a completely different object type that is not
> at all compatible with current Git.

> You could store commit and object data in SQLite, in the cloud, or
> via plaintext files on disk. As long as the data is still representing
> commit objects as we format them today, the commit-graph is still a
> cache that can be used as a faster way to fill 'struct commit' objects
> in memory without navigating to that object database.
> 
> And you also mention that the commit-graph format itself could be
> more efficient. You're right! I think the way we did it within Azure
> DevOps is more efficient, because most of the commit walking algorithms
> are built working directly on the integer labels within the in-memory
> data structure instead of operating on commit structs. This allows for
> less overhead when loading the graph (it's already cached in memory)
> and when walking thousands of commits (we only translate to object IDs
> if they are important for the output). But this is all the more reason
> to keep the commit-graph structures outside of "the object store" since
> a "commit-graph database" can be implemented without being tied to an
> object store.
> 
> If you are saying "but our existing commit-graph format puts it in
> 'objects/info/commit-graph[s]'" then yes the storage of a commit-graph
> is tied to our storage of objects. But the way we interact with it in
> code is in some way a layer above that.

> There is no inherent reason why a new backend would not be able to use
> the existing commit-graph infrastructure indeed. But there are reasons
> that specific backends may not want to do so. If objects are already
> stored in a database table, then it may make way more sense to store
> additional metadata that is currently stored in the commit-graph in a
> secondary database table instead of in the commit graph.
> ...
> This is roughly what I have in my head right now. And I realize that
> this information really should be sitting in a design document. I'm
> working on that, but still need to land two more patch series before I
> want to send such a patch series to the list.

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > There is no inherent reason why a new backend would not be able to use
> > the existing commit-graph infrastructure indeed. But there are reasons
> > that specific backends may not want to do so. If objects are already
> > stored in a database table, then it may make way more sense to store
> > additional metadata that is currently stored in the commit-graph in a
> > secondary database table instead of in the commit graph.
> > ...
> > This is roughly what I have in my head right now. And I realize that
> > this information really should be sitting in a design document. I'm
> > working on that, but still need to land two more patch series before I
> > want to send such a patch series to the list.
> 
> So is everybody happy with this line of thought that makes it
> mandatory for each backend to decide and implement the commit-graph
> support if they want to?
> 
> My reading of the later part of Taylor's message[*] tells me that at
> least Taylor does not agree with that position, and I am not sure
> about this design choice, either.  Surely, each backend can have its
> own optimization, but looking at the way data from the commit-graph
> and other auxiliary data files are used to optimize real operations
> (like populating the essential fields of the commit object first
> from the graph, only to read other things lazily from the object
> database, or switching to completely different traversal machinery
> when reachability bitmap is available), we cannot say that each
> backend can store whatever side data they please and leave it at
> that.  The code paths that are supposed to be generic need to be
> aware of these side data used for optimization to some degree, so
> conceptually it is much cleaner (well, at least to my eyes, that is)
> to declare that the auxiliary data files like commit-graph and
> reachability bitmaps are defined on the objects in the repository,
> no matter what backend is used to store them.

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > There is no inherent reason why a new backend would not be able to use
> > the existing commit-graph infrastructure indeed. But there are reasons
> > that specific backends may not want to do so. If objects are already
> > stored in a database table, then it may make way more sense to store
> > additional metadata that is currently stored in the commit-graph in a
> > secondary database table instead of in the commit graph.
> > ...
> > This is roughly what I have in my head right now. And I realize that
> > this information really should be sitting in a design document. I'm
> > working on that, but still need to land two more patch series before I
> > want to send such a patch series to the list.
> 
> So is everybody happy with this line of thought that makes it
> mandatory for each backend to decide and implement the commit-graph
> support if they want to?
> 
> My reading of the later part of Taylor's message[*] tells me that at
> least Taylor does not agree with that position, and I am not sure
> about this design choice, either.  Surely, each backend can have its
> own optimization, but looking at the way data from the commit-graph
> and other auxiliary data files are used to optimize real operations
> (like populating the essential fields of the commit object first
> from the graph, only to read other things lazily from the object
> database, or switching to completely different traversal machinery
> when reachability bitmap is available), we cannot say that each
> backend can store whatever side data they please and leave it at
> that.  The code paths that are supposed to be generic need to be
> aware of these side data used for optimization to some degree, so
> conceptually it is much cleaner (well, at least to my eyes, that is)
> to declare that the auxiliary data files like commit-graph and
> reachability bitmaps are defined on the objects in the repository,
> no matter what backend is used to store them.

> On Thu, Sep 25, 2025 at 12:17:50PM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > > There is no inherent reason why a new backend would not be able to use
> > > the existing commit-graph infrastructure indeed. But there are reasons
> > > that specific backends may not want to do so. If objects are already
> > > stored in a database table, then it may make way more sense to store
> > > additional metadata that is currently stored in the commit-graph in a
> > > secondary database table instead of in the commit graph.
> > > ...
> > > This is roughly what I have in my head right now. And I realize that
> > > this information really should be sitting in a design document. I'm
> > > working on that, but still need to land two more patch series before I
> > > want to send such a patch series to the list.
> > 
> > So is everybody happy with this line of thought that makes it
> > mandatory for each backend to decide and implement the commit-graph
> > support if they want to?
> > 
> > My reading of the later part of Taylor's message[*] tells me that at
> > least Taylor does not agree with that position, and I am not sure
> > about this design choice, either.  Surely, each backend can have its
> > own optimization, but looking at the way data from the commit-graph
> > and other auxiliary data files are used to optimize real operations
> > (like populating the essential fields of the commit object first
> > from the graph, only to read other things lazily from the object
> > database, or switching to completely different traversal machinery
> > when reachability bitmap is available), we cannot say that each
> > backend can store whatever side data they please and leave it at
> > that.  The code paths that are supposed to be generic need to be
> > aware of these side data used for optimization to some degree, so
> > conceptually it is much cleaner (well, at least to my eyes, that is)
> > to declare that the auxiliary data files like commit-graph and
> > reachability bitmaps are defined on the objects in the repository,
> > no matter what backend is used to store them.
> 
> My intent here is mostly to allow us to swap out how exactly the data is
> being cached. During the Git Merge I heard from some JJ developer (I
> think) that they also have a pluggable cache, but they approach the
> issue differently: instead of making the cache a property of the object
> backend, they instead make the cache itself pluggable.
> 
> I think that's a worthwhile angle to explore. The cache would still sit
> on the repository level, and it wouldn't have to care at all whether we
> use loose objects/packfiles or any other backend. But in theory, we can
> still swap it out for a different representation as desired.
> 
> Which overall means that we can defer this to a later point in time, as
> we can make it pluggable independent from making the object database
> itself pluggable.
> 
> So I'd propose to merge the first six patches, as everyone seemed to

> agree that they improve the status quo, but drop the last patch that
> moves the commit-graph into the ODB sources.
> 
> Does that seem reasonable to everyone? If so, I don't really see a
> reason to reroll at this point. But please let me know in case I miss
> anything that needs addressing.
> 
> Thanks!
> 
> Patrick
> 

> My intent here is mostly to allow us to swap out how exactly the data is
> being cached. During the Git Merge I heard from some JJ developer (I
> think) that they also have a pluggable cache, but they approach the
> issue differently: instead of making the cache a property of the object
> backend, they instead make the cache itself pluggable.
>
> I think that's a worthwhile angle to explore. The cache would still sit
> on the repository level, and it wouldn't have to care at all whether we
> use loose objects/packfiles or any other backend. But in theory, we can
> still swap it out for a different representation as desired.

> Which overall means that we can defer this to a later point in time, as
> we can make it pluggable independent from making the object database
> itself pluggable.
>
> So I'd propose to merge the first six patches, as everyone seemed to
> agree that they improve the status quo, but drop the last patch that
> moves the commit-graph into the ODB sources.
>
> Does that seem reasonable to everyone? If so, I don't really see a
> reason to reroll at this point. But please let me know in case I miss
> anything that needs addressing.

> Patrick Steinhardt <ps@pks.im> writes:
> 
>> My intent here is mostly to allow us to swap out how exactly the data is
>> being cached. During the Git Merge I heard from some JJ developer (I
>> think) that they also have a pluggable cache, but they approach the
>> issue differently: instead of making the cache a property of the object
>> backend, they instead make the cache itself pluggable.
>>
>> I think that's a worthwhile angle to explore. The cache would still sit
>> on the repository level, and it wouldn't have to care at all whether we
>> use loose objects/packfiles or any other backend. But in theory, we can
>> still swap it out for a different representation as desired.
> 
> The idea to allow these "caches" being pluggable to the system
> independently from object store or reference store backends does
> make quite a lot of sense.  If there is only one that is plugged,
> that degenerates into the "side data like commit-graph and
> reachability bitmaps do not belong to a specific object store
> backend, but are defined over the objects known to the repository" I
> was talking about in the message you are replying to, I think.
> 
>> Which overall means that we can defer this to a later point in time, as
>> we can make it pluggable independent from making the object database
>> itself pluggable.
>>
>> So I'd propose to merge the first six patches, as everyone seemed to
>> agree that they improve the status quo, but drop the last patch that
>> moves the commit-graph into the ODB sources.
>>
>> Does that seem reasonable to everyone? If so, I don't really see a
>> reason to reroll at this point. But please let me know in case I miss
>> anything that needs addressing.
> 
> I am fine with that, but let's hear from everyone ;-).

> On Fri, Sep 05, 2025 at 02:29:50PM -0400, Derrick Stolee wrote:
> > On 9/4/2025 7:12 PM, Junio C Hamano wrote:
> > > Patrick Steinhardt <ps@pks.im> writes:
> > >
> > >> commit graphs are currently stored on the object database level. This
> > >> doesn't really make much sense conceptually, given that commit graphs
> > >> are specific to one object source. Furthermore, with the upcoming
> > >> pluggable object database effort, an object source's backend may not
> > >> evene have a commit graph in the first place but store that information
> > >> in a different format altogether.
> > >>
> > >> This patch series prepares for that by moving the commit graph from
> > >> `struct object_database` into `struct odb_source`.
> > >
> > > Hmph, I am finding the above hard to agree with at the conceptual
> > > level.  In some future, we may use multiple object stores in a
> > > single repository.  Perhaps we would be storing older parts of
> > > history in semi-online storage while newer parts are stored in
> > > readily available storage.  But the side data structure that allows
> > > us to quickly learn who are parents of one commit is without having
> > > to go to the object store in order to parse the actualy commit
> > > object can be stored for the entire history if we wanted to, or more
> > > recent part of the history but not limited to the "readily available
> > > storage" part.  IOW, where the boundary between the older and the
> > > newer parts of the history lies and which commits the commit graph
> > > covers should be pretty much independent.
> > >
> > > So moving from object_database (i.e. the whole world) to individual
> > > odb_source (i.e. where one particular subset of the history is
> > > stored) feels like totally backwards to me.  Surely, a commit graph
> > > file may be defined over a set of packfiles and remaining loose
> > > object files, but it is not like an instance of the commit-graph
> > > file is tied to packfiles in the sense that it uses the index into
> > > some packfile instead of the actual object names to refer to
> > > commits, or anything like that (this is quite different from other
> > > files that are very specific to a single object store, like midx
> > > that is tied to the packfiles it describes).
> >
> > This is an interesting aspect to things, where the commit-graph file
> > is a "structured cache" of certain commit information. It happens to
> > be located within the object stores (either local or in an alternate)
> > but is conceptually different in a few ways.
> >
> > The biggest difference is that you can only open one commit-graph
> > (or chain of commit-graphs). Having multiple files across different
> > object stores will not accumulate additional context. Instead, we
> > have a "first one wins" approach.
> >
> > This does seem to be something that you are attempting to change
> > by including the ability to load a commit-graph for each odb (and
> > closing them in sequence as we close a repo).
> >
> > So in this sense, the commit-graph lives at the repository level,
> > not an object store level. When doing I/O to write or read a graph,
> > we use a specific object store at a time.
> >
> > The other direction to consider is what context we have when we
> > interact with a commit-graph. We generally are parsing commits from
> > a repository or loading Bloom filter data during file history walks.
> > Each of these do not have a predictable nature of which object store
> > will "own" the commit we are inspecting, so it wouldn't make sense
> > to restrict things like odb_parse_commit() over repo_parse_commit().
> >
> > With these thoughts in mind, I have these big-picture thoughts:
> >
> > 1. Patches 1-5 are great. Nice cleanups.
> >
> > 2. Some of Patch 6 is great, including having the I/O methods use
> >    an odb_source to help focus the specific location of the files
> >    being read or written. However, the movement of the struct into
> >    the odb_source makes less sense and should still exist at the
> >    object_database level.
>
> I (probably unsurprisingly :)) don't quite agree with this.
>
> Let's take a step back: why does the commit-graph exist in the first
> place? It basically provides a caching mechanism to efficiently return
> information that is otherwise more expensive to obtain:
>
>   - It contains a cached representation of the graph so that we don't
>     have to parse each commit from the object database.
>
>   - It encodes generation numbers.
>
>   - It contains bloom filters.

> All of which makes sense with the current design of our object storage
> format, because obtaining this information can be quite expensive. But
> let's consider a different world where we for example store objects in a
> proper database:
>
>   - This database may have an efficient way to compute generation
>     numbers on the fly, either when reading an object or when writing it
>     to disk. We cannot currently store that information in the packfile
>     right now, so it needs to be stored out-of-band. But with a database
>     there is no reason why we couldn't immediately compute and store the
>     generation number on each insert.

> There's a trivial conflict with tc/last-modified that can be solved like
> this:
>
> diff --cc commit-graph.c
> index 9929c1ed87,2f20f66cfd..0000000000
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@@ -823,7 -812,12 +823,11 @@@ int corrected_commit_dates_enabled(stru
>   
>   struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r)
>   {
>  -	struct commit_graph *g;
>  +	struct commit_graph *g = prepare_commit_graph(r);
> + 
>  -	if (!prepare_commit_graph(r))
> ++	if (!g)
> + 	       return NULL;
> + 

> Patrick Steinhardt <ps@pks.im> writes:
> > diff --cc commit-graph.c
> > index 9929c1ed87,2f20f66cfd..0000000000
> > --- a/commit-graph.c
> > +++ b/commit-graph.c
> > @@@ -823,7 -812,12 +823,11 @@@ int corrected_commit_dates_enabled(stru
> >   
> >   struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r)
> >   {
> >  -	struct commit_graph *g;
> >  +	struct commit_graph *g = prepare_commit_graph(r);
> > + 
> >  -	if (!prepare_commit_graph(r))
> > ++	if (!g)
> > + 	       return NULL;
> > + 
> 
> The while (g) loop will be entirely skipped when g==NULL, and then
> the function returns NULL after iterating the loop, so there is not
> much reason to have these three lines for early-return, no?