Re: [PATCH v8 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- May 21, 2014, 22:00 UTC
- Message-ID
- <20140521220035.GE12314@google.com>
- In-Reply-To
- <1400174999-26786-31-git-send-email-sahlberg@google.com>
Ronnie Sahlberg wrote:
Show 10 quoted lines
> --- 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?
[...]
Show 10 quoted lines
> @@ -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?
[...]
Show 6 quoted lines
> @@ -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.
[...]
Show 6 quoted lines
> @@ -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