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

[Patrick Steinhardt <ps@pks.im>]

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.

Patrick