Re: [PATCH v2 06/11] odb/transaction: propagate begin errors

[Patrick Steinhardt <ps@pks.im>]

On Tue, Jul 07, 2026 at 11:14:07PM -0500, Justin Tobler wrote:
> When `odb_transaction_begin()` is invoked, the function returns the
> transaction pointer directly. There is no way for the backend to
> signal that it failed to set up its state, such as when creating the
> temporary object directory backing the transaction.
> 
> In a subsequent commit, git-receive-pack(1) starts using ODB
> transactions and needs to be able to report such failures rather
> than silently ignore them. Refactor `odb_transaction_begin()` to
> return an int error code and write the resulting transaction into an
> out parameter. Also introduce `odb_transaction_begin_or_die()` as a
> convenience for callsites that do not need to handle errors
> explicitly.
> 
> Note that `odb_transaction_begin()` now returns an error when the ODB
> already has an inflight transaction pending. ODB transaction call sites
> that may encounter an inflight transaction are updated to explicitly
> handle this case.

Yeah, this change is very much welcome and results in much saner
behaviour with less surprises. Thanks for making the change.

> diff --git a/cache-tree.c b/cache-tree.c
> index 184f7e2635..8eec1d4d52 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -474,6 +474,7 @@ static int update_one(struct cache_tree *it,
>  
>  int cache_tree_update(struct index_state *istate, int flags)
>  {
> +	int inflight = !!the_repository->objects->transaction;
>  	struct odb_transaction *transaction;
>  	int skip, i;
>  
> @@ -490,10 +491,12 @@ int cache_tree_update(struct index_state *istate, int flags)
>  
>  	trace_performance_enter();
>  	trace2_region_enter("cache_tree", "update", istate->repo);
> -	transaction = odb_transaction_begin(the_repository->objects);
> +	if (!inflight)
> +		odb_transaction_begin_or_die(the_repository->objects, &transaction);
>  	i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
>  		       "", 0, &skip, flags);
> -	odb_transaction_commit(transaction);
> +	if (!inflight)
> +		odb_transaction_commit(transaction);
>  	trace2_region_leave("cache_tree", "update", istate->repo);
>  	trace_performance_leave("cache_tree_update");
>  	if (i < 0)

Callsites like this really make me wonder why we even care to create
a transaction in the first place if we basically just commit it
immediately anyway. And while it's a bit sad that we have so many sites
where we don't really know whether we even have a transaction, I think
it's a good change that we have now annotated them clearly. A subsequent
patch series may then eventually refactor those sites so that we stop
depending on `odb->transaction` and inject the transaction via a
parameter.

> diff --git a/odb/transaction.h b/odb/transaction.h
> index d52f0533ce..36032a5365 100644
> --- a/odb/transaction.h
> +++ b/odb/transaction.h
> @@ -1,6 +1,8 @@
>  #ifndef ODB_TRANSACTION_H
>  #define ODB_TRANSACTION_H
>  
> +#include "git-compat-util.h"

We typically don't include "git-compat-util.h" in header files.

> @@ -36,11 +38,21 @@ 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. Note that it is considered an error to start a new transaction if the
> + * ODB already has an inflight transaction pending.
>   */
> -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 could make it a bit simpler to use this function by continuing to
return the transaction directly. But on the other hand this results in a
more consistent interface.

Patrick