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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Jul 08, 2026 at 11:56:43AM -0500, Justin Tobler wrote:
> 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 know. But in the case where we don't want to batch we create the
transaction anyway as far as I can see, and then we commit it
immediately. So arguably, we could've just `odb_write_object()` and call
it a day.

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

Yeah, let's not worry about that too much for now then. One step at a
time :)

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

As said, I can see both arguments. And ultimately, I don't care too
much, so it's fine if this is just kept as-is.

Patrick