Re: [PATCH 6/6] odb/transaction: make `write_object_stream()` pluggable

[Justin Tobler <jltobler@gmail.com>]

On 26/03/31 09:48AM, Patrick Steinhardt wrote:
> On Mon, Mar 30, 2026 at 10:38:35PM -0500, Justin Tobler wrote:
> > How an ODB transaction handles writing objects is expected to vary
> > between implementations. Introduce a new `write_object_stream()`
> > callback in `struct odb_transaction` to make this function pluggable.
> > Wire up `index_blob_packfile_transaction()` for use with `struct
> > odb_transaction_files` accordingly.
> > 
> > Signed-off-by: Justin Tobler <jltobler@gmail.com>
> > ---
> >  object-file.c     |  9 +++++----
> >  odb/transaction.c |  7 +++++++
> >  odb/transaction.h | 25 ++++++++++++++++++++++---
> >  3 files changed, 34 insertions(+), 7 deletions(-)
> > 
> > diff --git a/object-file.c b/object-file.c
> > index 4c797d6498..b1c97faef3 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1680,10 +1680,10 @@ int index_fd(struct index_state *istate, struct object_id *oid,
> >  				.data = &data,
> >  			};
> >  
> > -			ret = index_blob_packfile_transaction(odb->transaction,
> > -							      &in_stream,
> > -							      xsize_t(st->st_size),
> > -							      oid);
> > +			ret = odb_transaction_write_object_stream(odb->transaction,
> > +								  &in_stream,
> > +								  xsize_t(st->st_size),
> > +								  oid);
> >  			odb_transaction_commit(transaction);
> >  		} else {
> >  			if (hash_blob_stream(the_repository->hash_algo, oid, fd,
> > @@ -2146,6 +2146,7 @@ struct odb_transaction *odb_transaction_files_begin(struct odb_source *source)
> >  	transaction = xcalloc(1, sizeof(*transaction));
> >  	transaction->base.source = source;
> >  	transaction->base.commit = odb_transaction_files_commit;
> > +	transaction->base.write_object_stream = index_blob_packfile_transaction;
> >  
> >  	return &transaction->base;
> >  }
> 
> I was originally expecting the upcast to `odb_transaction_files` in
> `index_blob_packfile_transaction()` to go away in this last step, but
> that of course doesn't make much sense as it now _becomes_ the
> implementation of `write_object_stream()`.
> 
> But should we rename to `odb_transaction_files_write_object_stream()`?

I initially held off from changing the name because in a followup
series I plan to migrate `odb_source_write_object_stream()` into this
same function and was going to rename at that time. It doesn't really
hurt to rename it now though. Will do. 

> > diff --git a/odb/transaction.h b/odb/transaction.h
> > index a56e392f21..584e8de36e 100644
> > --- a/odb/transaction.h
> > +++ b/odb/transaction.h
> > @@ -12,14 +12,24 @@
> >   *
> >   * Each ODB source is expected to implement its own transaction handling.
> >   */
> > -struct odb_transaction;
> > -typedef void (*odb_transaction_commit_fn)(struct odb_transaction *transaction);
> >  struct odb_transaction {
> >  	/* The ODB source the transaction is opened against. */
> >  	struct odb_source *source;
> >  
> >  	/* The ODB source specific callback invoked to commit a transaction. */
> > -	odb_transaction_commit_fn commit;
> > +	void (*commit)(struct odb_transaction *transaction);
> > +
> > +	/*
> > +	 * This callback is expected to write the given object stream into
> > +	 * the ODB transaction.
> 
> Should we note that for now, the expectation is to always write a blob?

I plan to drop this restriction in a future series, but it certainly
makes sense to document for now. Will update.

Thanks,
-Justin