Re: [PATCH 3/6] odb/transaction: propagate begin errors
From: Patrick Steinhardt <hidden>
Date: 2026-06-24 11:26:53
On Tue, Jun 23, 2026 at 11:19:17PM -0500, Justin Tobler wrote:
quoted hunk ↗ jump to hunk
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...
quoted hunk ↗ jump to hunk
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