Re: [PATCH 6/6] odb: add transaction interface

> On Tue, Sep 09, 2025 at 02:11:34PM -0500, Justin Tobler wrote:
> > diff --git a/object-file.c b/object-file.c
> > index 91fddfc4984..aff6c6c6dbb 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1623,7 +1623,7 @@ int index_fd(struct index_state *istate, struct object_id *oid,
> >  		struct odb_transaction *transaction = NULL;
> >  
> >  		if (!the_repository->objects->transaction)
> > -			transaction = begin_odb_transaction(the_repository->objects);
> > +			transaction = odb_transaction_begin(the_repository->objects);
> >  
> >  		ret = index_blob_packfile_transaction(the_repository->objects->transaction,
> >  						      oid, fd,
> 
> This function is a bit of an outlier, as it is weird that we call
> `odb_transaction_begin()` instead of the specific function.
> 
> But "object-file.c" currently contains two different parts: logic that
> is related to reading and writing objects in general, and logic that
> provides the actual object source implementation. This will be split up
> eventually once we carve out the actual "files" backend, so meanwhile we
> have to live with this seemingly-unclean separation of concerns.

> > @@ -1971,7 +1971,7 @@ int read_loose_object(struct repository *repo,
> >  	return ret;
> >  }
> >  
> > -struct odb_transaction *begin_odb_transaction(struct object_database *odb)
> > +struct odb_transaction *object_file_transaction_begin(struct object_database *odb)
> >  {
> >  	if (odb->transaction)
> >  		BUG("ODB transaction already started");
> 
> I would have expected that this function now gets as input an `struct
> odb_source` instead of the whole object database. After all, the ODB
> layer is the one coordinating the sources and managing which sources to
> tap into for a specific use case. But the actual business logic to read
> or write objects should then be handled on the source level, shouldn't
> it?

> > @@ -1982,7 +1982,7 @@ struct odb_transaction *begin_odb_transaction(struct object_database *odb)
> >  	return odb->transaction;
> >  }
> >  
> > -void end_odb_transaction(struct odb_transaction *transaction)
> > +void object_file_transaction_end(struct odb_transaction *transaction)
> >  {
> >  	flush_loose_object_transaction(transaction);
> >  	flush_packfile_transaction(transaction);
> 
> Shouldn't this also be called `object_file_transaction_commit()` to
> match the ODB layer?

> > diff --git a/odb.c b/odb.c
> > index 2a92a018c42..2cd954a1040 100644
> > --- a/odb.c
> > +++ b/odb.c
> > @@ -1051,3 +1051,13 @@ void odb_clear(struct object_database *o)
> >  	hashmap_clear(&o->pack_map);
> >  	string_list_clear(&o->submodule_source_paths, 0);
> >  }
> > +
> > +struct odb_transaction *odb_transaction_begin(struct object_database *odb)
> > +{
> > +	return object_file_transaction_begin(odb);
> > +}
> 
> So with the above, I would expect that we pick the source to create the
> transaction for here and then call `object_file_transaction_begin()` on
> that source. Eventually, once we have pluggable object databases, we
> would then not call `object_file_transaction_start()` directly anymore,
> but instead we'd call e.g. `source->backend.transaction_start()`.

> > diff --git a/odb.h b/odb.h
> > index a89b2143909..c7725b3df00 100644
> > --- a/odb.h
> > +++ b/odb.h
> > @@ -185,6 +185,9 @@ struct object_database {
> >  struct object_database *odb_new(struct repository *repo);
> >  void odb_clear(struct object_database *o);
> >  
> > +struct odb_transaction *odb_transaction_begin(struct object_database *odb);
> > +void odb_transaction_commit(struct odb_transaction *transaction);
> 
> Let's add some documentation here what these functions do and why you'd
> want to use them.