Thread (49 messages) 49 messages, 3 authors, 9h ago

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help