Re: [PATCH 3/6] odb/transaction: propagate begin errors

[Justin Tobler <jltobler@gmail.com>]

On 26/06/24 01:26PM, Patrick Steinhardt wrote:
> On Tue, Jun 23, 2026 at 11:19:17PM -0500, Justin Tobler wrote:
> > diff --git a/odb/transaction.c b/odb/transaction.c
> > index b16e07aebf..d3de01db50 100644
> > --- a/odb/transaction.c
> > +++ b/odb/transaction.c
> > @@ -2,14 +2,20 @@
> >  #include "odb/source.h"
> >  #include "odb/transaction.h"
> >  
> > -struct odb_transaction *odb_transaction_begin(struct object_database *odb)
> > +int odb_transaction_begin(struct object_database *odb,
> > +			  struct odb_transaction **out)
> >  {
> > -	if (odb->transaction)
> > -		return NULL;
> > +	int ret;
> >  
> > -	odb_source_begin_transaction(odb->sources, &odb->transaction);
> > +	if (odb->transaction) {
> > +		*out = NULL;
> > +		return 0;
> > +	}
> 
> Hm. So we may return successful, but not set the `out` pointer to a
> transaction. And...
> 
> > diff --git a/odb/transaction.h b/odb/transaction.h
> > index f4c1ebfaaa..cd6d50f2e5 100644
> > --- a/odb/transaction.h
> > +++ b/odb/transaction.h
> > @@ -33,11 +35,20 @@ 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. If the ODB already has a pending transaction, `out` is set to NULL.
> >   */
> > -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 don't special-case that here, either. So a caller may invoke the
> function, not die, but it might still not have a valid transaction. That
> feels wrong to me.

Ya, the original idea for a NULL transaction to signal that there is
already a transaction in flight and nested ODB transaction users to use
that to determine whether they needed to start a new transaction or not.
I completely agree thought that this is rather ackward.

Instead we could just make the callers that are worried about
potentially nested transactions handle this explicitly. I'll do that in
the next version.

-Justin