Subject: Re: [PATCH v8 02/44] refs.c: allow passing NULL to ref_transaction_free

From: Jonathan Nieder <[email-masked]>
Date: Thu, 15 May 2014 11:15:12 -0700

Ronnie Sahlberg wrote:

> Allow ref_transaction_free to be called with NULL and as a result allow
> ref_transaction_rollback to be called for a NULL transaction.
>
> This allows us to write code that will
>   if ( (!transaction ||
>         ref_transaction_update(...))  ||
>       (ref_transaction_commit(...) && !(transaction = NULL)) {
>           ref_transaction_rollback(transaction);
>           ...
>   }
>
> In this case transaction is reset to NULL IFF ref_transaction_commit() was
> invoked and thus the rollback becomes ref_transaction_rollback(NULL) which
> is safe. IF the conditional triggered prior to ref_transaction_commit()
> then transaction is untouched and then ref_transaction_rollback(transaction)
> will rollback the failed transaction.

I still think these last two paragraphs confuse more than enlighten
here.  There's plenty of time to explain them in the patch that uses
that code.

I'd just say something like

	Allow ref_transaction_free(NULL) and hence ref_transaction_rollback(NULL)
	as no-ops.

	This makes ref_transaction_rollback easier to use and more similar to
	plain 'free'.

And maybe:

	In particular, it lets us rollback unconditionally as part of cleanup
	code after setting 'transaction = NULL' if a transaction has been
	committed or rolled back already.

Thanks,
Jonathan