Re: [PATCH v2 1/7] odb/transaction: add transaction finalize interface

On 26/08/09 08:38PM, Junio C Hamano wrote:
> Justin Tobler <[email protected]> writes:
> 
> > diff --git a/builtin/add.c b/builtin/add.c
> > index 60ffbede2b..501e114ed5 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -393,7 +393,7 @@ int cmd_add(int argc,
> >  	char *seen = NULL;
> >  	char *ps_matched = NULL;
> >  	struct lock_file lock_file = LOCK_INIT;
> > -	struct odb_transaction *transaction;
> > +	struct odb_transaction *transaction = NULL;
> >  
> >  	repo_config(repo, add_config, NULL);
> >  
> > @@ -610,5 +610,6 @@ int cmd_add(int argc,
> >  	free(ps_matched);
> >  	dir_clear(&dir);
> >  	clear_pathspec(&pathspec);
> > +	odb_transaction_finalize(transaction);
> >  	return exit_status;
> >  }
> 
> There is only one non-local exit between transation-begin and
> transaction-finalize, which is a call ot report_path_error()
> followed by exit(128).  Will _finalize() stay to be just freeing
> memory and nothing else?  It may be conceptually cleaner to jump to
> the bottom to make sure the clean-up sequence will always happen.

In practice, only call sites that invoke `odb_transaction_write_pack()`
(only git-receive-pack(1) for now) actually need to be concerned about
any deferred clean up outside of just freeing some memory. Conceptually
this is a bit messy though and callers shouldn't ideally have to be
aware of such specifics.

It may make sense to align the clean-up as you suggested above. I will
explore in the next version.

> The same comment applies to other codepaths to which this patch adds
> _finalize() calls.
> 
> > diff --git a/odb/transaction.c b/odb/transaction.c
> > index dab7da6a9a..9e9a982778 100644
> > --- a/odb/transaction.c
> > +++ b/odb/transaction.c
> > @@ -33,6 +33,20 @@ int odb_transaction_commit(struct odb_transaction *transaction)
> >  
> >  	ret = transaction->commit(transaction);
> >  	transaction->source->odb->transaction = NULL;
> > +
> > +	return ret;
> > +}
> > +
> > +int odb_transaction_finalize(struct odb_transaction *transaction)
> > +{
> 
> Curiously no callers added by this patch checks the return value
> of this function.  Intended or just sloppy?  If the former, perhaps
> this wants to return void instead?

In version 1 I did keep `odb_transaction_finalize()` void, but decided
to at least provide the option for callers to check for errors if they
wished. The existing callers don't, but there isn't a reason most of the
couldn't be more strict here. In the next version, similar to
`odb_transaction_begin_or_die()`, I may add an
`odb_transaction_finalize_or_die()` helper and adapt some of the
existing callers.

> The same can be said for _commit(), by the way.

There is one `odb_transaction_commit()` caller in
"builtin/receive-pack.c" that does check for errors, but ya all other
callers simply ignore them. For the same reasons mentioned above, I
opted to follow the existing behavior of ignoring temporary directory
related errors, but include error reporting as part of the interface in
case callers wanted to check. I could also add an
`odb_transaction_commit_or_die()` helper here too and adapt callers
where it is reasonable to be more strict.

-Justin