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

[Justin Tobler <jltobler@gmail.com>]

On 26/07/08 08:32PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > 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.
> >
> > Signed-off-by: Justin Tobler <jltobler@gmail.com>
> > ---
> > ...
> > diff --git a/odb/transaction.c b/odb/transaction.c
> > index b16e07aebf..a5fba7f908 100644
> > --- a/odb/transaction.c
> > +++ b/odb/transaction.c
> > @@ -1,15 +1,20 @@
> >  #include "git-compat-util.h"
> > +#include "gettext.h"
> >  #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)
> >  {
> > +	int ret;
> > +
> >  	if (odb->transaction)
> > -		return NULL;
> > +		return error(_("object database transaction already pending"));
> >  
> > -	odb_source_begin_transaction(odb->sources, &odb->transaction);
> > +	ret = odb_source_begin_transaction(odb->sources, out);
> > +	odb->transaction = *out;
> 
> Can odb_source_begin_transaction() ever fail?  If so, and when it
> fails, would *out be left untouched?  

In this patch there it not yet a way for it to fail, but it can return
an error later in the series. When an error is encountered though, *out
_is_ left untouched.

> I am wondering if we want
> 
> 	if (!(ret = odb_source_begin_transaction(odb->sources, out)))
>         	odb->transaction = *out;
> 
> or something like that.

I think it is a good idea to for `odb_transaction_begin()` to ensure the
repository transaction is only set on success though. Will update in the
next version. Thanks

-Justin