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

From: Jonathan Nieder <[email-masked]>
Date: Tue, 13 May 2014 15:44:22 -0700

Ronnie Sahlberg wrote:

> Allow ref_transaction_free to be called with NULL and in extension allow
> ref_transaction_rollback to be called for a NULL transaction.

In extension = as a result?

Makes sense.  It lets someone do the usual

	struct ref_transaction *transaction;
	int ret = 0;

	if (something_fails()) {
		ret = -1;
		goto cleanup;
	}
	...

 cleanup:
	ref_transaction_free(transaction);
	return ret;

just like you can already do with free().

> This allows us to write code that will
>
>   if ( (!transaction ||
>         ref_transaction_update(...))  ||
>       (ref_transaction_commit(...) && !(transaction = NULL)) {
>           ref_transaction_rollback(transaction);
>           ...
>   }

Somewhere in the whitespace and parentheses I'm lost.

Is the idea that when ref_transaction_commit fails it will have
freed the transaction so we need not to roll back to prevent a
double free?  I think it would be simpler for the caller to
unconditionally set transaction to NULL after calling
ref_transaction_commit in such a case to avoid use-after-free.

Even better if it is the caller's responsibility to free
the transaction.  At any rate, it doesn't seem related to this
patch.

> --- a/refs.c
> +++ b/refs.c
> @@ -3303,6 +3303,9 @@ static void ref_transaction_free(struct ref_transaction *transaction)
>  {
>  	int i;
>  
> +	if (!transaction)
> +		return;

Except for the unclear commit message,
Reviewed-by: Jonathan Nieder <[email-masked]>