Re: [PATCH v2 07/11] odb/transaction: propagate commit errors

[Justin Tobler <jltobler@gmail.com>]

On 26/07/08 08:41AM, Patrick Steinhardt wrote:
> On Tue, Jul 07, 2026 at 11:14:08PM -0500, Justin Tobler wrote:
> > diff --git a/odb/transaction.c b/odb/transaction.c
> > index df4275151b..51af2c9a61 100644
> > --- a/odb/transaction.c
> > +++ b/odb/transaction.c
> > @@ -16,19 +16,26 @@ int odb_transaction_begin(struct object_database *odb,
> >  	return ret;
> >  }
> >  
> > -void odb_transaction_commit(struct odb_transaction *transaction)
> > +int odb_transaction_commit(struct odb_transaction *transaction)
> >  {
> > +	int ret;
> > +
> >  	if (!transaction)
> > -		return;
> > +		return 0;
> >  
> >  	/*
> >  	 * Ensure the transaction ending matches the pending transaction.
> >  	 */
> >  	ASSERT(transaction == transaction->source->odb->transaction);
> >  
> > -	transaction->commit(transaction);
> > +	ret = transaction->commit(transaction);
> > +	if (ret)
> > +		return ret;
> > +
> >  	transaction->source->odb->transaction = NULL;
> >  	free(transaction);
> > +
> > +	return 0;
> >  }
> 
> Doesn't this cause a leak now?

Good call. Ya, if odb_transaction_commit() fails, we don't free the
transaction. In the next version I'll go ahead and clear the transaction
if we fail.

> I think this interface here is doing the same mistake that our reference
> transactions did, where we automatically released the transaction on
> commit. That caused multiple lifetime issues with references all over
> the place.

Ya, it probaby makes sense to introduce a separate
`odb_transaction_release()` function to make this explicit and update
callers accordingly. I have another series I working on that introduces
`odb_transaction_abort()`. This might be a good place to add it in too.

-Justin