From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 21 May 2014 22:00:35 GMT
Subject: Re: [PATCH v8 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
Message-ID: <20140521220035.GE12314@google.com>
In-Reply-To: <1400174999-26786-31-git-send-email-sahlberg@google.com>

Ronnie Sahlberg wrote:

> --- a/refs.c
> +++ b/refs.c
> @@ -3308,6 +3308,12 @@ struct ref_update {
>  	const char refname[FLEX_ARRAY];
>  };
>  
> +enum ref_transaction_status {
> +	REF_TRANSACTION_OPEN   = 0,
> +	REF_TRANSACTION_CLOSED = 1,
> +	REF_TRANSACTION_ERROR  = 2,

What is the difference between _TRANSACTION_CLOSED and
_TRANSACTION_ERROR?

[...]
> @@ -3340,6 +3347,11 @@ void ref_transaction_free(struct ref_transaction *transaction)
>  
>  void ref_transaction_rollback(struct ref_transaction *transaction)
>  {
> +	if (!transaction)
> +		return;
> +
> +	transaction->status = REF_TRANSACTION_ERROR;
> +
>  	ref_transaction_free(transaction);

Once the transaction is freed, transaction->status is not reachable any
more so no one can tell that you've set it to _ERROR.  What is the
intended effect?

[...]
> @@ -3366,6 +3378,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
>  	if (have_old && !old_sha1)
>  		die("BUG: have_old is true but old_sha1 is NULL");
>  
> +	if (transaction->status != REF_TRANSACTION_OPEN)
> +		die("BUG: update on transaction that is not open");

Ok.

[...]
> @@ -3538,6 +3564,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  	clear_loose_ref_cache(&ref_cache);
>  
>  cleanup:
> +	transaction->status = ret ? REF_TRANSACTION_ERROR
> +	  : REF_TRANSACTION_CLOSED;

Nit: odd use of whitespace.

Overall thoughts: I like the idea of enforcing the API more strictly
("after an error, the only permitted operations are...").  The details
leave me a little confused because I don't think anything is
distinguishing between _CLOSED and _ERROR.  Maybe the enum only needs
two states.

Thanks,
Jonathan