Re: [PATCH v2 06/11] odb/transaction: propagate begin errors

[Justin Tobler <jltobler@gmail.com>]

On 26/07/08 08:41AM, Patrick Steinhardt wrote:
> On Tue, Jul 07, 2026 at 11:14:07PM -0500, Justin Tobler wrote:
> > @@ -490,10 +491,12 @@ int cache_tree_update(struct index_state *istate, int flags)
> >  
> >  	trace_performance_enter();
> >  	trace2_region_enter("cache_tree", "update", istate->repo);
> > -	transaction = odb_transaction_begin(the_repository->objects);
> > +	if (!inflight)
> > +		odb_transaction_begin_or_die(the_repository->objects, &transaction);
> >  	i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
> >  		       "", 0, &skip, flags);
> > -	odb_transaction_commit(transaction);
> > +	if (!inflight)
> > +		odb_transaction_commit(transaction);
> >  	trace2_region_leave("cache_tree", "update", istate->repo);
> >  	trace_performance_leave("cache_tree_update");
> >  	if (i < 0)
> 
> Callsites like this really make me wonder why we even care to create
> a transaction in the first place if we basically just commit it
> immediately anyway. And while it's a bit sad that we have so many sites
> where we don't really know whether we even have a transaction, I think
> it's a good change that we have now annotated them clearly. A subsequent
> patch series may then eventually refactor those sites so that we stop
> depending on `odb->transaction` and inject the transaction via a
> parameter.

Call sites like the one mentioned above are using ODB transactions as an
optimization to batch the full fsyncs in bulk. In cases where the is not
already a transaction, they start one to take advantage of it.

I fully agree though that an ODB transaction should ideally be started
at a higher layer and wired down to these call sites. I have a couple of
patches in my tree that start to tackle this which I plan to send in
another series. :)

> > diff --git a/odb/transaction.h b/odb/transaction.h
> > index d52f0533ce..36032a5365 100644
> > --- a/odb/transaction.h
> > +++ b/odb/transaction.h
> > @@ -1,6 +1,8 @@
> >  #ifndef ODB_TRANSACTION_H
> >  #define ODB_TRANSACTION_H
> >  
> > +#include "git-compat-util.h"
> 
> We typically don't include "git-compat-util.h" in header files.

Will remove. Thanks

> > @@ -36,11 +38,21 @@ struct odb_transaction {
> >  };
> >  
> >  /*
> > - * Starts an ODB transaction. Subsequent objects are written to the transaction
> > - * and not committed until odb_transaction_commit() is invoked on the
> > - * transaction. If the ODB already has a pending transaction, NULL is returned.
> > + * Starts an ODB transaction and returns it via `out`. Subsequent objects are
> > + * written to the transaction and not committed until odb_transaction_commit()
> > + * is invoked on the transaction. Returns 0 on success and a negative value on
> > + * error. Note that it is considered an error to start a new transaction if the
> > + * ODB already has an inflight transaction pending.
> >   */
> > -struct odb_transaction *odb_transaction_begin(struct object_database *odb);
> > +int odb_transaction_begin(struct object_database *odb,
> > +			  struct odb_transaction **out);
> > +
> > +static inline void odb_transaction_begin_or_die(struct object_database *odb,
> > +						struct odb_transaction **out)
> > +{
> > +	if (odb_transaction_begin(odb, out))
> > +		die(_("failed to start ODB transaction"));
> > +}
> 
> We could make it a bit simpler to use this function by continuing to
> return the transaction directly. But on the other hand this results in a
> more consistent interface.

Ya, I was a bit back and forth about this myself. I ultimately landed on
keeping a more consistent interface though. Happy to change if others
feel differently though.

-Justin